Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPCHelpMan: Pass through Result and Examples #14987

Merged
merged 1 commit into from Jan 29, 2019

Conversation

Projects
None yet
6 participants
@MarcoFalke
Copy link
Member

commented Dec 17, 2018

Passing the rpc result and rpc examples through RPCHelpMan makes it clear in what order they appear in the stringified version. Future improvements could then autoformat or autogenerate them.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-rpcResultFlat branch 2 times, most recently Dec 17, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

Resulting diff for faa1522
diff --git a/abandontransaction b/abandontransaction
index 4ce0ab0..4727471 100644
--- a/abandontransaction
+++ b/abandontransaction
@@ -9,8 +9,6 @@ It has no effect on transactions which are already abandoned.
 Arguments:
 1. txid    (string, required) The transaction id
 
-Result:
-
 Examples:
 > bitcoin-cli abandontransaction "1075db55d416d3ca199f55b6084e2115b9345e16c5cf302fc80e9d5fbf5d48d"
 > curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "abandontransaction", "params": ["1075db55d416d3ca199f55b6084e2115b9345e16c5cf302fc80e9d5fbf5d48d"] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/
diff --git a/estimaterawfee b/estimaterawfee
index 300ff13..8284dcf 100644
--- a/estimaterawfee
+++ b/estimaterawfee
@@ -39,5 +39,5 @@ Result:
 
 Results are returned for any horizon which tracks blocks up to the confirmation target.
 
-Example:
+Examples:
 > bitcoin-cli estimaterawfee 6 0.9
diff --git a/estimatesmartfee b/estimatesmartfee
index 3cad235..b787d02 100644
--- a/estimatesmartfee
+++ b/estimatesmartfee
@@ -29,5 +29,5 @@ fee estimation is able to return based on how long it has been running.
 An error is returned if not enough transactions and blocks
 have been observed to make an estimate for any number of blocks.
 
-Example:
+Examples:
 > bitcoin-cli estimatesmartfee 6
diff --git a/getrawmempool b/getrawmempool
index e995089..7050878 100644
--- a/getrawmempool
+++ b/getrawmempool
@@ -7,7 +7,7 @@ Hint: use getmempoolentry to fetch a specific transaction from the mempool.
 Arguments:
 1. verbose    (boolean, optional, default=false) True for a json object, false for array of transaction ids
 
-Result: (for verbose = false):
+Result (for verbose = false):
 [                     (json array of string)
   "transactionid"     (string) The transaction id
   ,...
diff --git a/importaddress b/importaddress
index e0f1865..9c15eea 100644
--- a/importaddress
+++ b/importaddress
@@ -2,12 +2,6 @@ importaddress "address" ( "label" rescan p2sh )
 
 Adds an address or script (in hex) that can be watched as if it were in your wallet but cannot be used to spend. Requires a new wallet backup.
 
-Arguments:
-1. address    (string, required) The Bitcoin address (or hex-encoded script)
-2. label      (string, optional, default="") An optional label
-3. rescan     (boolean, optional, default=true) Rescan the wallet for transactions
-4. p2sh       (boolean, optional, default=false) Add the P2SH version of the script as well
-
 Note: This call can take over an hour to complete if rescan is true, during that time, other rpc calls
 may report that the imported address exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.
 If you have the full public key, you should call importpubkey instead of this.
@@ -15,6 +9,12 @@ If you have the full public key, you should call importpubkey instead of this.
 Note: If you import a non-standard raw script in hex form, outputs sending to it will be treated
 as change, and not show up in many RPCs.
 
+Arguments:
+1. address    (string, required) The Bitcoin address (or hex-encoded script)
+2. label      (string, optional, default="") An optional label
+3. rescan     (boolean, optional, default=true) Rescan the wallet for transactions
+4. p2sh       (boolean, optional, default=false) Add the P2SH version of the script as well
+
 Examples:
 
 Import an address with rescan
diff --git a/importmulti b/importmulti
index 51a6f51..a6afbb2 100644
--- a/importmulti
+++ b/importmulti
@@ -4,6 +4,9 @@ Import addresses/scripts (with private or public keys, redeem script (P2SH)), op
 If an address/script is imported without all of the private keys required to spend from that address, it will be watchonly. The 'watchonly' option must be set to true in this case or a warning will be returned.
 Conversely, if all the private keys are provided and the address/script is spendable, the watchonly option must be set to false, or a warning will be returned.
 
+Note: This call can take over an hour to complete if rescan is true, during that time, other rpc calls
+may report that the imported keys, addresses or scripts exists but related transactions are still missing.
+
 Arguments:
 1. requests                                                         (json array, required) Data to be imported
      [
@@ -36,12 +39,11 @@ Arguments:
        "rescan": bool,                                              (boolean, optional, default=true) Stating if should rescan the blockchain after all imports
      }
 
-Note: This call can take over an hour to complete if rescan is true, during that time, other rpc calls
-may report that the imported keys, addresses or scripts exists but related transactions are still missing.
+Result:
+
+Response is an array with the same size as the input that has the execution result :
+  [{"success": true}, {"success": true, "warnings": ["Ignoring irrelevant private key"]}, {"success": false, "error": {"code": -1, "message": "Internal Server Error"}}, ...]
 
 Examples:
 > bitcoin-cli importmulti '[{ "scriptPubKey": { "address": "<my address>" }, "timestamp":1455191478 }, { "scriptPubKey": { "address": "<my 2nd address>" }, "label": "example 2", "timestamp": 1455191480 }]'
 > bitcoin-cli importmulti '[{ "scriptPubKey": { "address": "<my address>" }, "timestamp":1455191478 }]' '{ "rescan": false}'
-
-Response is an array with the same size as the input that has the execution result :
-  [{"success": true}, {"success": true, "warnings": ["Ignoring irrelevant private key"]}, {"success": false, "error": {"code": -1, "message": "Internal Server Error"}}, ...]
diff --git a/importprivkey b/importprivkey
index d083a48..01793fd 100644
--- a/importprivkey
+++ b/importprivkey
@@ -3,14 +3,14 @@ importprivkey "privkey" ( "label" rescan )
 Adds a private key (as returned by dumpprivkey) to your wallet. Requires a new wallet backup.
 Hint: use importmulti to import more than one private key.
 
+Note: This call can take over an hour to complete if rescan is true, during that time, other rpc calls
+may report that the imported key exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.
+
 Arguments:
 1. privkey    (string, required) The private key (see dumpprivkey)
 2. label      (string, optional, default=current label if address exists, otherwise "") An optional label
 3. rescan     (boolean, optional, default=true) Rescan the wallet for transactions
 
-Note: This call can take over an hour to complete if rescan is true, during that time, other rpc calls
-may report that the imported key exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.
-
 Examples:
 
 Dump a private key
diff --git a/importpubkey b/importpubkey
index 3c79e07..54354e1 100644
--- a/importpubkey
+++ b/importpubkey
@@ -2,14 +2,14 @@ importpubkey "pubkey" ( "label" rescan )
 
 Adds a public key (in hex) that can be watched as if it were in your wallet but cannot be used to spend. Requires a new wallet backup.
 
+Note: This call can take over an hour to complete if rescan is true, during that time, other rpc calls
+may report that the imported pubkey exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.
+
 Arguments:
 1. pubkey    (string, required) The hex-encoded public key
 2. label     (string, optional, default="") An optional label
 3. rescan    (boolean, optional, default=true) Rescan the wallet for transactions
 
-Note: This call can take over an hour to complete if rescan is true, during that time, other rpc calls
-may report that the imported pubkey exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.
-
 Examples:
 
 Import a public key with rescan
diff --git a/invalidateblock b/invalidateblock
index bb1513e..6d22805 100644
--- a/invalidateblock
+++ b/invalidateblock
@@ -5,8 +5,6 @@ Permanently marks a block as invalid, as if it violated a consensus rule.
 Arguments:
 1. blockhash    (string, required) the hash of the block to mark as invalid
 
-Result:
-
 Examples:
 > bitcoin-cli invalidateblock "blockhash"
 > curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "invalidateblock", "params": ["blockhash"] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/
diff --git a/listunspent b/listunspent
index b068fc6..0960a1c 100644
--- a/listunspent
+++ b/listunspent
@@ -22,7 +22,7 @@ Arguments:
        "minimumSumAmount": amount,    (numeric or string, optional, default=unlimited) Minimum sum value of all UTXOs in BTC
      }
 
-Result
+Result:
 [                   (array of json object)
   {
     "txid" : "txid",          (string) the transaction id 
@@ -43,7 +43,7 @@ Result
   ,...
 ]
 
-Examples
+Examples:
 > bitcoin-cli listunspent 
 > bitcoin-cli listunspent 6 9999999 "[\"1PGFqEzfmQch1gKD3ra4k18PNj3tTUUSqg\",\"1LtvqCaApEdUGFkpKMM4MstjcaL4dKg8SP\"]"
 > curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "listunspent", "params": [6, 9999999 "[\"1PGFqEzfmQch1gKD3ra4k18PNj3tTUUSqg\",\"1LtvqCaApEdUGFkpKMM4MstjcaL4dKg8SP\"]"] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/
diff --git a/listwalletdir b/listwalletdir
index 54ea06a..9a3c26e 100644
--- a/listwalletdir
+++ b/listwalletdir
@@ -1,5 +1,7 @@
 listwalletdir
 Returns a list of wallets in the wallet directory.
+
+Result:
 {
   "wallets" : [                (json array of objects)
     {
diff --git a/preciousblock b/preciousblock
index 786992d..2135eee 100644
--- a/preciousblock
+++ b/preciousblock
@@ -9,8 +9,6 @@ The effects of preciousblock are not retained across restarts.
 Arguments:
 1. blockhash    (string, required) the hash of the block to mark as precious
 
-Result:
-
 Examples:
 > bitcoin-cli preciousblock "blockhash"
 > curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "preciousblock", "params": ["blockhash"] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/
diff --git a/reconsiderblock b/reconsiderblock
index f3ee35b..c1dc5bd 100644
--- a/reconsiderblock
+++ b/reconsiderblock
@@ -6,8 +6,6 @@ This can be used to undo the effects of invalidateblock.
 Arguments:
 1. blockhash    (string, required) the hash of the block to reconsider
 
-Result:
-
 Examples:
 > bitcoin-cli reconsiderblock "blockhash"
 > curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "reconsiderblock", "params": ["blockhash"] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/
diff --git a/resendwallettransactions b/resendwallettransactions
index d349b7e..865257b 100644
--- a/resendwallettransactions
+++ b/resendwallettransactions
@@ -2,5 +2,7 @@ resendwallettransactions
 Immediately re-broadcast unconfirmed wallet transactions to all peers.
 Intended only for testing; the wallet code periodically re-broadcasts
 automatically.
+
+Result:
 Returns an RPC error if -walletbroadcast is set to false.
 Returns array of transaction ids that were re-broadcast.
diff --git a/settxfee b/settxfee
index 579448a..bb72205 100644
--- a/settxfee
+++ b/settxfee
@@ -5,7 +5,7 @@ Set the transaction fee per kB for this wallet. Overrides the global -paytxfee c
 Arguments:
 1. amount    (numeric or string, required) The transaction fee in BTC/kB
 
-Result
+Result:
 true|false        (boolean) Returns true if successful
 
 Examples:
diff --git a/submitblock b/submitblock
index f0602ae..9141363 100644
--- a/submitblock
+++ b/submitblock
@@ -7,8 +7,6 @@ Arguments:
 1. hexdata    (string, required) the hex-encoded block data to submit
 2. dummy      (string, optional, default=ignored) dummy value, for compatibility with BIP22. This value is ignored.
 
-Result:
-
 Examples:
 > bitcoin-cli submitblock "mydata"
 > curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "submitblock", "params": ["mydata"] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/
diff --git a/walletpassphrase b/walletpassphrase
index 8cf7ba3..40002fd 100644
--- a/walletpassphrase
+++ b/walletpassphrase
@@ -3,14 +3,14 @@ walletpassphrase "passphrase" timeout
 Stores the wallet decryption key in memory for 'timeout' seconds.
 This is needed prior to performing transactions related to private keys such as sending bitcoins
 
-Arguments:
-1. passphrase    (string, required) The wallet passphrase
-2. timeout       (numeric, required) The time to keep the decryption key in seconds; capped at 100000000 (~3 years).
-
 Note:
 Issuing the walletpassphrase command while the wallet is already unlocked will set a new unlock
 time that overrides the old one.
 
+Arguments:
+1. passphrase    (string, required) The wallet passphrase
+2. timeout       (numeric, required) The time to keep the decryption key in seconds; capped at 100000000 (~3 years).
+
 Examples:
 
 Unlock the wallet for 60 seconds
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15279 (wallet: Clarify rescanblockchain doc by MarcoFalke)
  • #15272 (doc: correct logging return type and RPC example by fanquake)
  • #15245 (remove deprecated mentions of signrawtransaction from fundraw help by instagibbs)
  • #15226 (Allow creating blank (empty) wallets (alternative) by achow101)
  • #15157 (rpc: Bumpfee units change, satoshis to BTC by benthecarman)
  • #15006 (Add option to create an encrypted wallet by achow101)
  • #14918 (RPCHelpMan: Check default values are given at compile-time by MarcoFalke)
  • #14898 (nextpagepointer flag for listtransactions by hosseinamin)
  • #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
  • #14459 (docs: Consistent type names in RPC help descriptions by ch4ot1c)
  • #13756 (wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof)
  • #13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)
  • #11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Show resolved Hide resolved src/rpc/mining.cpp
Show resolved Hide resolved src/rpc/util.h Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-rpcResultFlat branch Dec 21, 2018

@MarcoFalke MarcoFalke changed the title [rcp] RPCHelpMan: Require documentation of return value at compile time RPCHelpMan: Pass through Result and Examples Dec 21, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-rpcResultFlat branch Dec 21, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-rpcResultFlat branch Dec 24, 2018

@DrahtBot DrahtBot removed the Needs rebase label Dec 24, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-rpcResultFlat branch 2 times, most recently Jan 15, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

utACK faa90fe9d5e688fdd311a819721f6e911cc441a5

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-rpcResultFlat branch to fab0585 Jan 16, 2019

@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels Jan 16, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-rpcResultFlat branch from fab0585 to fa4686b Jan 25, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 25, 2019

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

A bunch of the changes add redundant "Result:" headers when there were conditional results, eg for getblockheader:

+Result:
+
 Result (for verbose = true):

Could be worth doing:

struct RPCResult {
     std::string m_cond;
     std::string m_result;
     explicit RPCResult(std::string result) : m_result{result} { }
     explicit RPCResult(std::string cond, std::string result) : m_cond{cond}, m_result{result} { }
}
struct RPCResults {
    std::vector<RPCResult> m_results;
    RPCResults(RPCResult result) : m_results{{result}} { }
    RPCResults(std::initializer_list<RPCResult> results) : m_results{results} { }
    std::string ToDescription() const;
}

and changing RPCHelpMan() to take RPCResults instead. Then you can supply either RPCResult("foo") for unconditional results, or { RPCResult{"for verbose=true", "...results..."}, RPCResult{"for verbose=false"}, "...other results..."} } for conditional results, or RPCResults{} if you're not giving any result examples.

Patch at ajtowns@1ca7fb3 (feel free to include it as a fixup if you like it)

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-rpcResultFlat branch from 1ca7fb3 to fa5a2ae Jan 25, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-rpcResultFlat branch from fa5a2ae to faa1522 Jan 25, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

Thanks. I stole your changes and squashed them with some minor fixups. Hope you don't mind.

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

ACK faa1522

Help differences checked with:

mkdir BEFORE AFTER
git checkout faa1522e^ && make && make check && ./bitcoind -regtest
for a in $(./bitcoin-cli -regtest help | grep '^[^=]' | sed 's/ .*//') estimaterawfee setmocktime echo echojson signrawtransaction invalidateblock reconsiderblock waitfornewblock waitforblock waitforblockheight syncwithvalidationinterfacequeue resendwallettransactions; do ./bitcoin-cli -regtest help $a > BEFORE/$a.txt; done
git checkout faa1522e && make && make check && ./bitcoind -regtest
for a in $(./bitcoin-cli -regtest help | grep '^[^=]' | sed 's/ .*//') estimaterawfee setmocktime echo echojson signrawtransaction invalidateblock reconsiderblock waitfornewblock waitforblock waitforblockheight syncwithvalidationinterfacequeue resendwallettransactions; do ./bitcoin-cli -regtest help $a > AFTER/$a.txt; done
git diff --color-moved=dimmed_zebra --no-index BEFORE/ AFTER/

(there isn't a way to just list all the RPC commands including hidden ones?)

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jan 29, 2019

Merge bitcoin#14987: RPCHelpMan: Pass through Result and Examples
faa1522 RPCHelpMan: Pass through Result and Examples (MarcoFalke)

Pull request description:

  Passing the rpc result and rpc examples through `RPCHelpMan` makes it clear in what order they appear in the stringified version. Future improvements could then autoformat or autogenerate them.

Tree-SHA512: b32a5c178cc80f50a7e9b93a38e2b26d5994188ecafe9e61bbc599941b44b9b0e4e4be6413d4464fac6e8e73661a191a77d34917f2e6293de19fb59519dd4487
@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

@ajtowns I modified CRPCTable::help to print even hidden ones to get the diff in #14987 (comment)

Anyway, this has two ACKs, three upvotes (which I count as Concept ACK) and only conflicts with prs that have review and are mine, so I am going to merge this.

Sorry for the merge conflicts :(

@MarcoFalke MarcoFalke merged commit faa1522 into bitcoin:master Jan 29, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1812-rpcResultFlat branch Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.