wallet: Make RPC help compile-time static #19250

maflcko · 2020-06-11T14:24:55Z

Currently calling help on a wallet RPC method will either return help: unknown command: getnewaddress or the actual help. This runtime dependency of the help is a bug that complicates any tool that relies on documentation. Also, the code that enables the bug is overly complicated and confusing.

The fix is split into two commits:

First, a commit that can be reviewed with the --color-moved=dimmed-zebra option and tested with the included test.
Second, a commit that removes the complicated and confusing code.

maflcko · 2020-06-11T14:33:27Z

This bugfix has been split out from #18531

achow101 · 2020-06-11T15:32:08Z

ACK fae7923

laanwj · 2020-06-11T16:31:24Z

I don't think calling this 'move-only', while strictly correct, is what we usually mean with that. It implies that whole functions are moved keeping the functionality the same hence 'move-only'.

ACK otherwise. The help doesn't depend on the wallet being loaded, or what wallet is selected, so moving this upfront makes sense.

maflcko · 2020-06-11T16:40:00Z

Force pushed to removed "move-only bugfix" from the pull request title and commit subject line. No other changes:

$ git range-diff  bitcoin-core/master  fae7923a0b fadf6bd04f
1:  fafcc0d8f5 ! 1:  fad889cbf0 wallet move-only bugfix: Make RPC help compile-time static
    @@ Metadata
     Author: MarcoFalke <falke.marco@gmail.com>
     
      ## Commit message ##
    -    wallet move-only bugfix: Make RPC help compile-time static
    +    wallet: Make RPC help compile-time static
     
      ## src/wallet/rpcdump.cpp ##
     @@ src/wallet/rpcdump.cpp: static void RescanWallet(CWallet& wallet, const WalletRescanReserver& reserver,
2:  fae7923a0b = 2:  fadf6bd04f refactor: Remove unused request.fHelp

promag

Tested ACK fadf6bd.

I've pushed f11f864 that I think you could cherry pick here.

Only change is that now you can call help on invalid wallets, like src/bitcoin-cli -rpcwallet=404 -regtest help listunspent and it doesn't error.

promag · 2020-06-12T09:44:25Z

src/wallet/rpcwallet.cpp

        return wallets[0];
    }

-    if (request.fHelp) return nullptr;
-
    if (!HasWallets()) {


if (if wallets.size() == 0) and ditch HasWallets.

This has one ACK and two stale ACKs, so I'd prefer to keep it as is for now. Maybe @ryanofsky can combine this into the other vpwallets cleaunup?

I can open a PR but it's too damn height. No rush really.

I am happy to review if you open a pull request

achow101 · 2020-06-12T14:54:51Z

re-ACK fadf6bd

fadf6bd refactor: Remove unused request.fHelp (MarcoFalke) fad889c wallet: Make RPC help compile-time static (MarcoFalke) Pull request description: Currently calling `help` on a wallet RPC method will either return `help: unknown command: getnewaddress` or the actual help. This runtime dependency of the help is a bug that complicates any tool that relies on documentation. Also, the code that enables the bug is overly complicated and confusing. The fix is split into two commits: * First, a commit that can be reviewed with the `--color-moved=dimmed-zebra` option and tested with the included test. * Second, a commit that removes the complicated and confusing code. ACKs for top commit: achow101: re-ACK fadf6bd promag: Tested ACK fadf6bd. Tree-SHA512: 65d4ff400467f57cb8415c30ce30f814dc76c5c157308b7a7409c59ac9db629e65dfba31cd9c389cfe60a008d3d87787ea0a0e0f2671fd65fd190543c915493d

ccf1f6e refactor: Drop ::HasWallets() (João Barbosa) Pull request description: Minor follow-up of #19250. The global `HasWallets()` is used only once and at the call site there's already a way to know if any wallet is loaded. ACKs for top commit: MarcoFalke: ACK ccf1f6e hebasto: ACK ccf1f6e, I have reviewed the changes and they look OK, I agree they can be merged. Tree-SHA512: fb902c045cbd331eaf71716c04734520f2ce7f2b317db510c4ce140162bbc683327b5a40ac860f6cde5add37e069065274d39dfa147fac2091eedec505f2f7eb

ccf1f6e refactor: Drop ::HasWallets() (João Barbosa) Pull request description: Minor follow-up of bitcoin#19250. The global `HasWallets()` is used only once and at the call site there's already a way to know if any wallet is loaded. ACKs for top commit: MarcoFalke: ACK ccf1f6e hebasto: ACK ccf1f6e, I have reviewed the changes and they look OK, I agree they can be merged. Tree-SHA512: fb902c045cbd331eaf71716c04734520f2ce7f2b317db510c4ce140162bbc683327b5a40ac860f6cde5add37e069065274d39dfa147fac2091eedec505f2f7eb

…ones (server) fa7592b rpc: Update server to use new RPCHelpMan (MarcoFalke) aaaaad5 rpc: Add option to hide RPCArg (MarcoFalke) fa9708f rpc: Assert that passed arg names are equal to hardcoded ones (MarcoFalke) faaeb2b rpc: Add CRPCCommand constructor which takes RPCHelpMan (MarcoFalke) fa8ec00 rpc: Check that left section is not multiline (MarcoFalke) Pull request description: This is split out from #18531 to just touch the RPC methods in server. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue #18607 * The changes itself fixed bug #19250 ACKs for top commit: laanwj: ACK fa7592b ryanofsky: Code review ACK fa7592b. Looks great! Just some hidden arg and Check() and comment cleanups since last review Tree-SHA512: e64b6a212f4a3aeedeee47557559bde104d5fd40cdc1746b27eb2f3d4c8885d5e6e4dd287595ea11cdbc6a939654fe103cae765fd505875444d851f0abb11308

…ommand ones (server) fa7592b rpc: Update server to use new RPCHelpMan (MarcoFalke) aaaaad5 rpc: Add option to hide RPCArg (MarcoFalke) fa9708f rpc: Assert that passed arg names are equal to hardcoded ones (MarcoFalke) faaeb2b rpc: Add CRPCCommand constructor which takes RPCHelpMan (MarcoFalke) fa8ec00 rpc: Check that left section is not multiline (MarcoFalke) Pull request description: This is split out from bitcoin#18531 to just touch the RPC methods in server. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue bitcoin#18607 * The changes itself fixed bug bitcoin#19250 ACKs for top commit: laanwj: ACK fa7592b ryanofsky: Code review ACK fa7592b. Looks great! Just some hidden arg and Check() and comment cleanups since last review Tree-SHA512: e64b6a212f4a3aeedeee47557559bde104d5fd40cdc1746b27eb2f3d4c8885d5e6e4dd287595ea11cdbc6a939654fe103cae765fd505875444d851f0abb11308

…ones (misc) fa77de2 rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) (MarcoFalke) fa50bdc rpc: Limit echo to 10 args (MarcoFalke) fa89ca9 refactor: Use C++11 range based for loops to simplify rpc code (MarcoFalke) fa459bd rpc: Treat all args after a hidden arg as hidden as well (MarcoFalke) Pull request description: This is split out from #18531 to just touch the RPC methods in misc. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue #18607 * The changes itself fixed bug #19250 ACKs for top commit: laanwj: Code review ACK fa77de2 fjahr: tested ACK fa77de2 theStack: ACK fa77de2 ryanofsky: Code review ACK fa77de2. Pretty straightfoward changes Tree-SHA512: badae1606518c0b55ce2c0bb9025d14f05556532375eb20fd6f3bfadae1e5e6568860bff8599d037e655bf1d23f1f464ca17f4db10a6ab3d502b6e9e61c7b3d3

…ommand ones (misc) fa77de2 rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) (MarcoFalke) fa50bdc rpc: Limit echo to 10 args (MarcoFalke) fa89ca9 refactor: Use C++11 range based for loops to simplify rpc code (MarcoFalke) fa459bd rpc: Treat all args after a hidden arg as hidden as well (MarcoFalke) Pull request description: This is split out from bitcoin#18531 to just touch the RPC methods in misc. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue bitcoin#18607 * The changes itself fixed bug bitcoin#19250 ACKs for top commit: laanwj: Code review ACK fa77de2 fjahr: tested ACK fa77de2 theStack: ACK bitcoin@fa77de2 ryanofsky: Code review ACK fa77de2. Pretty straightfoward changes Tree-SHA512: badae1606518c0b55ce2c0bb9025d14f05556532375eb20fd6f3bfadae1e5e6568860bff8599d037e655bf1d23f1f464ca17f4db10a6ab3d502b6e9e61c7b3d3

…ommand ones (server) fa7592b rpc: Update server to use new RPCHelpMan (MarcoFalke) aaaaad5 rpc: Add option to hide RPCArg (MarcoFalke) fa9708f rpc: Assert that passed arg names are equal to hardcoded ones (MarcoFalke) faaeb2b rpc: Add CRPCCommand constructor which takes RPCHelpMan (MarcoFalke) fa8ec00 rpc: Check that left section is not multiline (MarcoFalke) Pull request description: This is split out from bitcoin#18531 to just touch the RPC methods in server. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue bitcoin#18607 * The changes itself fixed bug bitcoin#19250 ACKs for top commit: laanwj: ACK fa7592b ryanofsky: Code review ACK fa7592b. Looks great! Just some hidden arg and Check() and comment cleanups since last review Tree-SHA512: e64b6a212f4a3aeedeee47557559bde104d5fd40cdc1746b27eb2f3d4c8885d5e6e4dd287595ea11cdbc6a939654fe103cae765fd505875444d851f0abb11308

…ommand ones (mining,zmq,rpcdump) fa3d9ce rpc: Assert that RPCArg names are equal to CRPCCommand ones (rpcdump) (MarcoFalke) fa32c1d rpc: Assert that RPCArg names are equal to CRPCCommand ones (zmq) (MarcoFalke) faaa46d rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining) (MarcoFalke) fa93bc1 rpc: Remove unused return type from appendCommand (MarcoFalke) Pull request description: This is split out from bitcoin#18531 to just touch the RPC methods in misc. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue bitcoin#18607 * The changes itself fixed bug bitcoin#19250 ACKs for top commit: fjahr: tested ACK fa3d9ce promag: Code review ACK fa3d9ce. Tree-SHA512: 068ade4b55cc195868d53b7f9a27151d45b440857bb069e261a49d102a49a38fdba5d68868516a1d66a54a73ba34681362f934ded7349e894042bde873b75719

…(blockchain,rawtransaction) fa6bb0c Assert that RPCArg names are equal to CRPCCommand ones (rawtransaction) (MarcoFalke) fa80c81 Assert that RPCArg names are equal to CRPCCommand ones (blockchain) (MarcoFalke) Pull request description: This is split out from #18531 to just touch some RPC methods. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue #18607 * The changes itself fixed bug #19250 ACKs for top commit: fjahr: utACK fa6bb0c tryphe: utACK fa6bb0c. Reducing data duplication is nice. Code changes are minimal and concise. Tree-SHA512: deb0edc3f999baf055526eaa199b98c500635e12502dece7aa3cad5319db330eb5ee7459a5c8f040a83671a7f20c560c19a2026fb76c8416f138aa332727cbce

…d ones (blockchain,rawtransaction) fa6bb0c Assert that RPCArg names are equal to CRPCCommand ones (rawtransaction) (MarcoFalke) fa80c81 Assert that RPCArg names are equal to CRPCCommand ones (blockchain) (MarcoFalke) Pull request description: This is split out from bitcoin#18531 to just touch some RPC methods. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue bitcoin#18607 * The changes itself fixed bug bitcoin#19250 ACKs for top commit: fjahr: utACK fa6bb0c tryphe: utACK fa6bb0c. Reducing data duplication is nice. Code changes are minimal and concise. Tree-SHA512: deb0edc3f999baf055526eaa199b98c500635e12502dece7aa3cad5319db330eb5ee7459a5c8f040a83671a7f20c560c19a2026fb76c8416f138aa332727cbce

…(net, rpcwallet) fa14f57 Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) (MarcoFalke) Pull request description: This is the last part split out from #18531 to just touch some RPC methods. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue #18607 * The changes itself fixed bug #19250 ACKs for top commit: fjahr: tACK fa14f57 ryanofsky: Code review ACK fa14f57. Just straightforward replacements except code moved in `addnode`, and displatching updated in `bumpfee_helper` Tree-SHA512: e07af150f1d95a88e558256ce197a6b7dc6cd722a6d6c13c75d944c49c2e2441f8b8237e9f94b03db69fa18f9bda627b0781d5e1da70bf5415e09b38728a8cb1

…d ones (net, rpcwallet) fa14f57 Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) (MarcoFalke) Pull request description: This is the last part split out from bitcoin#18531 to just touch some RPC methods. Description from the main pr: ### Motivation RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here. ### Changes The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`. ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue bitcoin#18607 * The changes itself fixed bug bitcoin#19250 ACKs for top commit: fjahr: tACK fa14f57 ryanofsky: Code review ACK fa14f57. Just straightforward replacements except code moved in `addnode`, and displatching updated in `bumpfee_helper` Tree-SHA512: e07af150f1d95a88e558256ce197a6b7dc6cd722a6d6c13c75d944c49c2e2441f8b8237e9f94b03db69fa18f9bda627b0781d5e1da70bf5415e09b38728a8cb1

faaf9c5 remove CRPCCommand constructor that takes rpcfn_type function pointer (MarcoFalke) fa19bb2 remove dead rpc code (MarcoFalke) Pull request description: Remove the CRPCCommand arguments, now that they are asserted to be equal and thus redundant ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue #18607 * The changes itself fixed bug #19250 ACKs for top commit: fjahr: tested ACK faaf9c5 promag: Tested ACK faaf9c5. ryanofsky: Code review ACK faaf9c5. Two obviously good simplifications. Tree-SHA512: 5de3b440f7b2ed2c3e86655d4f0e2e5df9c67e8ce3c7817d5ea5311d1a38690f2f3e28fab41aad6936be9fc884326d037e5f19e85d4d2fe281474dada13911ee

faaf9c5 remove CRPCCommand constructor that takes rpcfn_type function pointer (MarcoFalke) fa19bb2 remove dead rpc code (MarcoFalke) Pull request description: Remove the CRPCCommand arguments, now that they are asserted to be equal and thus redundant ### Future work > Here or follow up, makes sense to also assert type of returned UniValue? Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including: * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table * Auto-formatting and sanity checking the RPCExamples with RPCMan * Checking passed-in json in self-check. Removing redundant checks * Checking returned json against documentation to avoid regressions or false documentation * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static ### Bugs found * The assert identified issue bitcoin#18607 * The changes itself fixed bug bitcoin#19250 ACKs for top commit: fjahr: tested ACK faaf9c5 promag: Tested ACK faaf9c5. ryanofsky: Code review ACK faaf9c5. Two obviously good simplifications. Tree-SHA512: 5de3b440f7b2ed2c3e86655d4f0e2e5df9c67e8ce3c7817d5ea5311d1a38690f2f3e28fab41aad6936be9fc884326d037e5f19e85d4d2fe281474dada13911ee

Summary: > Currently calling help on a wallet RPC method will either return help: unknown command: getnewaddress or the actual help. This runtime dependency of the help is a bug that complicates any tool that relies on documentation. Also, the code that enables the bug is overly complicated and confusing. > > The fix is split into two commits: > > - First, a commit that can be reviewed with the --color-moved=dimmed-zebra option and tested with the included test. > - Second, a commit that removes the complicated and confusing code. This is a backport of [[bitcoin/bitcoin#19250 | core#19250]] [1/2] bitcoin/bitcoin@fad889c Depends on D9452 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9455

Summary: This concludes backport of [[bitcoin/bitcoin#19250 | core#19250]] [2/2] bitcoin/bitcoin@fadf6bd Depends on D9455 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9456

Summary: > Minor follow-up of [[bitcoin/bitcoin#19250 | core#19250]]. The global HasWallets() is used only once and at the call site there's already a way to know if any wallet is loaded. This is a backport of [[bitcoin/bitcoin#19261 | core#19261]] Depends on D9456 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9457

Updated RPCs for Bitcoin PRs bitcoin#19100 and bitcoin#19250.

maflcko mentioned this pull request Jun 11, 2020

rpc: remove deprecated CRPCCommand constructor #18531

Merged

maflcko added this to the 0.21.0 milestone Jun 11, 2020

DrahtBot added RPC/REST/ZMQ Tests Wallet labels Jun 11, 2020

maflcko removed the Tests label Jun 11, 2020

maflcko changed the title ~~wallet: [move-only bugfix] Make RPC help compile-time static~~ wallet: Make RPC help compile-time static Jun 11, 2020

MarcoFalke added 2 commits June 11, 2020 12:38

wallet: Make RPC help compile-time static

fad889c

refactor: Remove unused request.fHelp

fadf6bd

maflcko force-pushed the 2006-rpcWalletHelp branch from fae7923 to fadf6bd Compare June 11, 2020 16:39

promag reviewed Jun 12, 2020

View reviewed changes

maflcko merged commit 19e9192 into bitcoin:master Jun 12, 2020

maflcko deleted the 2006-rpcWalletHelp branch June 12, 2020 20:47

promag mentioned this pull request Jun 12, 2020

refactor: Drop ::HasWallets() #19261

Merged

ryanofsky mentioned this pull request Jun 19, 2020

refactor: remove ::vpwallets and related global variables #19101

Merged

maflcko mentioned this pull request Jun 26, 2020

rpc: Assert that RPCArg names are equal to CRPCCommand ones (server) #19386

Merged

maflcko mentioned this pull request Jul 15, 2020

rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) #19528

Merged

maflcko mentioned this pull request Aug 14, 2020

rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining,zmq,rpcdump) #19717

Merged

maflcko mentioned this pull request Aug 31, 2020

Assert that RPCArg names are equal to CRPCCommand ones (blockchain,rawtransaction) #19849

Merged

maflcko mentioned this pull request Sep 22, 2020

Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) #19994

Merged

cryptapus pushed a commit to cryptapus/bitcoin that referenced this pull request May 3, 2021

Merge branch 'bitcoin' into auxpow

72b476a

Updated RPCs for Bitcoin PRs bitcoin#19100 and bitcoin#19250.

cryptapus pushed a commit to cryptapus/bitcoin that referenced this pull request May 3, 2021

Merge branch 'auxpow'

d481525

Updated RPCs for Bitcoin PRs bitcoin#19100 and bitcoin#19250.

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: Make RPC help compile-time static #19250

wallet: Make RPC help compile-time static #19250

Uh oh!

maflcko commented Jun 11, 2020 •

edited

Loading

Uh oh!

maflcko commented Jun 11, 2020

Uh oh!

achow101 commented Jun 11, 2020

Uh oh!

laanwj commented Jun 11, 2020

Uh oh!

maflcko commented Jun 11, 2020

Uh oh!

promag left a comment

Uh oh!

promag Jun 12, 2020

Uh oh!

maflcko Jun 12, 2020

Uh oh!

promag Jun 12, 2020

Uh oh!

maflcko Jun 12, 2020

Uh oh!

promag Jun 12, 2020

Uh oh!

achow101 commented Jun 12, 2020

Uh oh!

Uh oh!

wallet: Make RPC help compile-time static #19250

wallet: Make RPC help compile-time static #19250

Uh oh!

Conversation

maflcko commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 11, 2020

Uh oh!

achow101 commented Jun 11, 2020

Uh oh!

laanwj commented Jun 11, 2020

Uh oh!

maflcko commented Jun 11, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

promag Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

promag Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 12, 2020

Uh oh!

Uh oh!

maflcko commented Jun 11, 2020 •

edited

Loading