[RPC] Give RPC commands more information about the RPC request #8788

jonasschnelli · 2016-09-22T08:04:32Z

Alternative solution for #8775.
Looks more invasive than it is (mostly search & replace).

This PR does replace the RPC call signature (UniValue &params, bool fHelp) with (JSONRequest &request) which allows the registered RPC calls to get more information about the request.

The JSONRequest object contains the HTTP base auth username as well as the path of the called HTTP endpoint.

This PR would allow wallet distinction based on HTTP auth username and or URL endpoints.

jonasschnelli · 2016-09-22T08:08:59Z

For the multiwallet support, each wallet RPC call could do something like CWallet *pwallet = CWallets::getWalletFromRequest(request) instead of accessing pwalletMain.

laanwj · 2016-09-22T12:24:15Z

Concept ACK, the code churn is a bit unfortunate but passing a context structure into RPC methods had to be done at some point...
(and it's better to face this once than hacking in something like a thread local pointer...)

jonasschnelli · 2016-09-22T12:35:13Z

Yes. It's larger then I initially though. But once we did the "large" migration to a structure/object-passing we will be flexible for further changes.
Most changed lines are just a params. to request.params. which should be easy to verify/review.

maflcko · 2016-09-22T22:52:34Z

src/rpc/blockchain.cpp

 {
-    if (fHelp || params.size() < 1 || params.size() > 2) {
+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2) {


nit: You can put something like

UniValue params = request.params;

in the first line of the method to make the diff smaller and less verbose?

I have considered this. But if we would have done this from the beginning, wouldn't it be without the extra UniValue params delegation?
I think this PRs solution is cleaner.

We have to do this once and therefore probably accept a larger diff.

No strong opinion on this. Indeed, doing const UniValue &params = request.params; at the beginning of each function would reduce diff, on the other hand, I don't think prefixing request. everywhere is bad. It even makes the code a little bit more readable. "What params? The request params".
I'm fine with keeping it like this

laanwj · 2016-09-25T09:46:55Z

This reminds me, I should have called that class JSONRPCRequest not JSONRequest. It's a JSON-RPC server not a JSON server.

If you agree I think it would make sense to rename it as a first commit (it only appears in 5 places) before it becomes an argument to every RPC call.

Edit: GAH, JSONRPCRequest is already a function name in protocol.h. This could explain why I made the decision to name it like that. Ok... never mind the above.

laanwj · 2016-09-29T16:53:34Z

@jonasschnelli I'd suggest the following to get rid of the current JSONRPCRequest: laanwj@ae6db2a, then rename the actual request object.

This is more consistent with `JSONRPCReplyObj`.

jonasschnelli · 2016-10-19T12:46:38Z

Rebased and added laanwj@ae6db2a as first commit, renamed JSONRequest to JSONRPCRequest.

laanwj · 2016-10-19T13:16:20Z

utACK e7156ad

laanwj · 2016-10-19T13:22:26Z

src/rest.cpp

@@ -286,6 +286,7 @@ static bool rest_chaininfo(HTTPRequest* req, const std::string& strURIPart)
    switch (rf) {
    case RF_JSON: {
        JSONRPCRequest jsonRequest;
+        jsonRequest.params = UniValue(UniValue::VARR);


This is kind of ugly, but it already was ugly, as we're not supposed to call into RPC from the REST interface (or use any RPC stuff at all from REST). At some point these functions should be moved to a shared compilation unit.
Out of scope for this pull ofcourse.

…equest e7156ad [RPC] pass HTTP basic authentication username to the JSONRequest object (Jonas Schnelli) 69d1c25 [RPC] Give RPC commands more information about the RPC request (Jonas Schnelli) 23c32a9 rpc: Change JSONRPCRequest to JSONRPCRequestObj (Wladimir J. van der Laan)

Network active toggle was already based on "[RPC] Give RPC commands more information about the RPC request" We need to use the old UniValue style until that one is backported

* Merge bitcoin#8996: Network activity toggle 19f46f1 Qt: New network_disabled icon (Luke Dashjr) 54cf997 RPC/Net: Use boolean consistently for networkactive, and remove from getinfo (Luke Dashjr) b2b33d9 Overhaul network activity toggle (Jonas Schnelli) 32efa79 Qt: Add GUI feedback and control of network activity state. (Jon Lund Steffensen) e38993b RPC: Add "togglenetwork" method to toggle network activity temporarily (Jon Lund Steffensen) 7c9a98a Allow network activity to be temporarily suspended. (Jon Lund Steffensen) * Revert on-click behavior of network status icon to showing peers list Stay with the way Dash handled clicking on the status icon * Add theme support for network disabled icon * Merge bitcoin#8874: Multiple Selection for peer and ban tables 1077577 Fix auto-deselection of peers (Andrew Chow) addfdeb Multiple Selection for peer and ban tables (Andrew Chow) * Merge bitcoin#9190: qt: Plug many memory leaks ed998ea qt: Avoid OpenSSL certstore-related memory leak (Wladimir J. van der Laan) 5204598 qt: Avoid shutdownwindow-related memory leak (Wladimir J. van der Laan) e4f126a qt: Avoid splash-screen related memory leak (Wladimir J. van der Laan) 693384e qt: Prevent thread/memory leak on exiting RPCConsole (Wladimir J. van der Laan) 47db075 qt: Plug many memory leaks (Wladimir J. van der Laan) * Merge bitcoin#9218: qt: Show progress overlay when clicking spinner icon 042f9fa qt: Show progress overlay when clicking spinner icon (Wladimir J. van der Laan) 827d9a3 qt: Replace NetworkToggleStatusBarControl with generic ClickableLabel (Wladimir J. van der Laan) * Merge bitcoin#9266: Bugfix: Qt/RPCConsole: Put column enum in the right places df17fe0 Bugfix: Qt/RPCConsole: Put column enum in the right places (Luke Dashjr) * Merge bitcoin#9255: qt: layoutAboutToChange signal is called layoutAboutToBeChanged f36349e qt: Remove on_toggleNetworkActiveButton_clicked from RPCConsole (Wladimir J. van der Laan) 297cc20 qt: layoutAboutToChange signal is called layoutAboutToBeChanged (Wladimir J. van der Laan) * Use UniValue until bitcoin PR bitcoin#8788 is backported Network active toggle was already based on "[RPC] Give RPC commands more information about the RPC request" We need to use the old UniValue style until that one is backported * Merge bitcoin#8906: [qt] sync-overlay: Don't show progress twice fafeec3 [qt] sync-overlay: Don't show progress twice (MarcoFalke) * Merge bitcoin#8985: Use pindexBestHeader instead of setBlockIndexCandidates for NotifyHeaderTip() 3154d6e [Qt] use NotifyHeaderTip's height and date for the progress update (Jonas Schnelli) 0a261b6 Use pindexBestHeader instead of setBlockIndexCandidates for NotifyHeaderTip() (Jonas Schnelli) * Merge bitcoin#9280: [Qt] Show ModalOverlay by pressing the progress bar, allow hiding 89a3723 [Qt] Show ModalOverlay by pressing the progress bar, disabled show() in sync mode (Jonas Schnelli) * Merge bitcoin#9461: [Qt] Improve progress display during headers-sync and peer-finding 40ec7c7 [Qt] Improve progress display during headers-sync and peer-finding (Jonas Schnelli) * Merge bitcoin#9588: qt: Use nPowTargetSpacing constant fa4d478 qt: Use nPowTargetSpacing constant (MarcoFalke) * Hide modal overlay forever when syncing has catched up Don't allow to open it again by clicking on the progress bar and spinner icon. Currently the overlay does not show meaningful information about masternode sync and it gives the impression of being stuck after the block chain sync is done. * Don't include chainparams.h in sendcoinsdialog.cpp This was just a remainder of a backported PR which meant to change some calculation in this file which does not apply to Dash. * Also check for fNetworkActive in ConnectNode * Merge bitcoin#9528: [qt] Rename formateNiceTimeOffset(qint64) to formatNiceTimeOffset(qint64) 988d300 [qt] Rename formateNiceTimeOffset(qint64) to formatNiceTimeOffset(qint64) (practicalswift) * Merge bitcoin#11237: qt: Fixing division by zero in time remaining c8d38ab Refactor tipUpdate as per style guide (MeshCollider) 3b69a08 Fix division by zero in time remaining (MeshCollider) Pull request description: Fixes bitcoin#10291, bitcoin#11265 progressDelta may be 0 (or even negative according to 11265), this checks for that and prints unknown if it is, because we cannot calculate an estimate for the time remaining (would be infinite or negative). Tree-SHA512: bc5708e5ed6e4670d008219558c5fbb25709bd99a32c98ec39bb74f94a0b7fa058f3d03389ccdd39e6723e6b5b48e34b13ceee7c051c2db631e51d8ec3e1d68c

…e RPC request e7156ad [RPC] pass HTTP basic authentication username to the JSONRequest object (Jonas Schnelli) 69d1c25 [RPC] Give RPC commands more information about the RPC request (Jonas Schnelli) 23c32a9 rpc: Change JSONRPCRequest to JSONRPCRequestObj (Wladimir J. van der Laan)

jonasschnelli added Refactoring RPC/REST/ZMQ labels Sep 22, 2016

jonasschnelli force-pushed the 2016/09/rpc_container branch from 0692086 to ff5b791 Compare September 22, 2016 09:04

maflcko reviewed Sep 22, 2016

View reviewed changes

laanwj and others added 2 commits October 19, 2016 14:30

rpc: Change JSONRPCRequest to JSONRPCRequestObj

23c32a9

This is more consistent with `JSONRPCReplyObj`.

[RPC] Give RPC commands more information about the RPC request

69d1c25

jonasschnelli force-pushed the 2016/09/rpc_container branch from ff5b791 to 69d1c25 Compare October 19, 2016 12:46

[RPC] pass HTTP basic authentication username to the JSONRequest object

e7156ad

laanwj reviewed Oct 19, 2016

View reviewed changes

laanwj merged commit e7156ad into bitcoin:master Oct 19, 2016

This was referenced Oct 19, 2016

Add importmulti RPC call #7551

Merged

Locked memory manager #8753

Merged

maflcko mentioned this pull request Oct 19, 2016

RPC refactoring: Access wallet using new GetWalletForJSONRPCRequest #8775

Merged

mrbandrews mentioned this pull request Oct 21, 2016

[RPC] Simplified bumpfee command. #8456

Merged

ryanofsky mentioned this pull request Jun 15, 2017

[wallet] [tests] Add listwallets RPC, include wallet name in getwalletinfo and add multiwallet test #10604

Merged

dagurval mentioned this pull request Feb 13, 2018

[RPC] Give RPC commands more information about the RPC request bitcoinxt/bitcoinxt#333

Merged

CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 24, 2019

Head Fix on bitcoin#8788 merge

2976bb4

Fuzzbawls mentioned this pull request Jul 4, 2020

[Tracking] Remove wallet 'account' API PIVX-Project/PIVX#1706

Closed

11 tasks

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[RPC] Give RPC commands more information about the RPC request #8788

[RPC] Give RPC commands more information about the RPC request #8788

Uh oh!

jonasschnelli commented Sep 22, 2016

Uh oh!

jonasschnelli commented Sep 22, 2016

Uh oh!

laanwj commented Sep 22, 2016 •

edited

Loading

Uh oh!

jonasschnelli commented Sep 22, 2016

Uh oh!

maflcko Sep 22, 2016

Uh oh!

jonasschnelli Sep 23, 2016

Uh oh!

laanwj Sep 25, 2016

Uh oh!

laanwj commented Sep 25, 2016 •

edited

Loading

Uh oh!

laanwj commented Sep 29, 2016

Uh oh!

jonasschnelli commented Oct 19, 2016

Uh oh!

laanwj commented Oct 19, 2016

Uh oh!

laanwj Oct 19, 2016

Uh oh!

Uh oh!

[RPC] Give RPC commands more information about the RPC request #8788

[RPC] Give RPC commands more information about the RPC request #8788

Uh oh!

Conversation

jonasschnelli commented Sep 22, 2016

Uh oh!

jonasschnelli commented Sep 22, 2016

Uh oh!

laanwj commented Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Sep 22, 2016

Uh oh!

maflcko Sep 22, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Sep 23, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj Sep 25, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 29, 2016

Uh oh!

jonasschnelli commented Oct 19, 2016

Uh oh!

laanwj commented Oct 19, 2016

Uh oh!

laanwj Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented Sep 22, 2016 •

edited

Loading

laanwj commented Sep 25, 2016 •

edited

Loading