RPC: Accept options as named-only parameters #26485

pull ryanofsky wants to merge 4 commits into bitcoin:master from ryanofsky:pr/nonly changing 25 files +452 −185
  1. ryanofsky commented at 6:41 pm on November 10, 2022: contributor

    Allow RPC methods which take an options parameter (importmulti, listunspent, fundrawtransaction, bumpfee, send, sendall, walletcreatefundedpsbt, simulaterawtransaction), to accept the options as named parameters, without the need for nested JSON objects.

    This makes it possible to make calls like:

    0src/bitcoin-cli -named bumpfee txid fee_rate=10
    

    instead of

    0src/bitcoin-cli -named bumpfee txid options='{"fee_rate": 10}'
    

    RPC help is also updated to show options as top level named arguments instead of as nested objects.

     0@@ -15,16 +15,17 @@
     1 
     2 Arguments:
     3 1. txid                           (string, required) The txid to be bumped
     4-2. options                        (json object, optional)
     5+2. options                        (json object, optional) Options object that can be used to pass named arguments, listed below.
     6+                 
     7+Named Arguments:
     8-     {
     9-       "conf_target": n,          (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks
    10+conf_target                       (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks
    11 
    12-       "fee_rate": amount,        (numeric or string, optional, default=not set, fall back to wallet fee estimation) 
    13+fee_rate                          (numeric or string, optional, default=not set, fall back to wallet fee estimation) 
    14                                   Specify a fee rate in sat/vB instead of relying on the built-in fee estimator.
    15                                   Must be at least 1.000 sat/vB higher than the current transaction fee rate.
    16                                   WARNING: before version 0.21, fee_rate was in BTC/kvB. As of 0.21, fee_rate is in sat/vB.
    17                                   
    18-       "replaceable": bool,       (boolean, optional, default=true) Whether the new transaction should still be
    19+replaceable                       (boolean, optional, default=true) Whether the new transaction should still be
    20                                   marked bip-125 replaceable. If true, the sequence numbers in the transaction will
    21                                   be left unchanged from the original. If false, any input sequence numbers in the
    22                                   original transaction that were less than 0xfffffffe will be increased to 0xfffffffe
    23@@ -32,11 +33,10 @@
    24                                   still be replaceable in practice, for example if it has unconfirmed ancestors which
    25                                   are replaceable).
    26                                   
    27-       "estimate_mode": "str",    (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
    28+estimate_mode                     (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
    29                                   "unset"
    30                                   "economical"
    31                                   "conservative"
    32-     }
    33 
    34 Result:
    35 {                    (json object)
    

    Review suggestion: To understand this PR, it is probably easiest to review the commits in reverse order because the last commit shows the external API changes, the middle commit shows the internal API changes, and the first commit contains the low-level implementation.

  2. ryanofsky commented at 6:49 pm on November 10, 2022: contributor

    ~This is a draft because it needs more unit tests and is not tested very much yet in general. Also probably should have release notes and updates to generated documentation.~

    UPDATE: These things are completed, so no longer in draft status

  3. luke-jr commented at 6:49 pm on November 10, 2022: member
    Concept ACK. Another approach might be #17356. Not sure which is better (17356 is probably more readable code?)
  4. DrahtBot added the label RPC/REST/ZMQ on Nov 10, 2022
  5. DrahtBot commented at 0:12 am on November 12, 2022: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK achow101
    Concept ACK luke-jr, aureleoules, TheCharlatan
    Stale ACK ajtowns

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #bitcoin-core/gui/733 (Deniability - a tool to automatically improve coin ownership privacy by denavila)
    • #27792 (wallet: Deniability API (Unilateral Transaction Meta-Privacy) by denavila)
    • #27788 (rpc: Be able to access RPC parameters by name by achow101)
    • #27101 (Support JSON-RPC 2.0 when requested by client by pinheadmz)
    • #25243 (test: autogenerate bash completion by suhailsaqan)
    • #24963 (RPC/Wallet: Convert walletprocesspsbt to use options parameter by luke-jr)
    • #21283 (Implement BIP 370 PSBTv2 by achow101)

    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.

  6. DrahtBot added the label Needs rebase on Nov 14, 2022
  7. ryanofsky commented at 10:02 pm on November 14, 2022: contributor

    @MarcoFalke you may have thoughts on RPCHelpMan changes in this PR. I didn’t make any changes to generated documentation yet, but I’m thinking adding a new “Named arguments” section between the existing “Arguments” and “Result” sections:

    For RPC methods that take explicit options arguments (existing RPC methods like like importmulti, listunspent, etc), the documentation lines for those arguments could look like:

    03. options (json object, optional) Options object that can be used to pass named arguments
    
  8. luke-jr commented at 10:07 pm on November 14, 2022: member
    Concept ACK on a simple RPC help for “options”, but I wonder if there’s a better way to avoid duplicating all positional argument docs… Unless your thought was to have only non-positional ones under the Named arguments list?
  9. ryanofsky commented at 9:23 pm on November 15, 2022: contributor

    Concept ACK on a simple RPC help for “options”, but I wonder if there’s a better way to avoid duplicating all positional argument docs… Unless your thought was to have only non-positional ones under the Named arguments list?

    Right, no duplication. Arguments that can be named and positional shown with names and numbers in “Arguments” section. Arguments that can only be passed by name shown without numbers in the “Named arguments”

  10. ryanofsky force-pushed on Nov 16, 2022
  11. DrahtBot removed the label Needs rebase on Nov 16, 2022
  12. ryanofsky force-pushed on Nov 16, 2022
  13. DrahtBot added the label Needs rebase on Nov 30, 2022
  14. ryanofsky force-pushed on Dec 9, 2022
  15. ryanofsky force-pushed on Dec 9, 2022
  16. ryanofsky commented at 8:46 pm on December 9, 2022: contributor
    Rebased e1b5be5d5a3d3a0ea5a53fa1acd47128efa75bb0 -> fae439b117bb90b6288b2e9b807e8219439cd726 (pr/nonly.3 -> pr/nonly.4, compare) after base PR #19762 merge. Also added unit tests and implemented changes to generated documentation discussed above. Rebased fae439b117bb90b6288b2e9b807e8219439cd726 -> cea5e37d4ec34d62c413c27775b7bafeaebff6c4 (pr/nonly.4 -> pr/nonly.5, compare) due to conflict with #26601.
  17. 903803 approved
  18. DrahtBot removed the label Needs rebase on Dec 9, 2022
  19. bitcoin deleted a comment on Dec 10, 2022
  20. ryanofsky marked this as ready for review on Dec 10, 2022
  21. DrahtBot added the label Needs rebase on Dec 13, 2022
  22. aureleoules commented at 11:23 am on December 15, 2022: member
    Concept ACK
  23. ryanofsky renamed this:
    RPC: Add support for name-only parameters
    RPC: Accept options as named-only parameters
    on Dec 16, 2022
  24. ryanofsky force-pushed on Dec 17, 2022
  25. ryanofsky force-pushed on Dec 17, 2022
  26. ryanofsky commented at 0:06 am on December 17, 2022: contributor
    Updated cea5e37d4ec34d62c413c27775b7bafeaebff6c4 -> fb39d5a14a8a4ed22cf6cad523a51cf709f95650 (pr/nonly.5 -> pr/nonly.6, compare) adding release notes, internal documentation, and making a few simplifications Rebased fb39d5a14a8a4ed22cf6cad523a51cf709f95650 -> ec11f06e472eb7e1f5654ab8537feff0f17065a5 (pr/nonly.6 -> pr/nonly.7, compare) due to conflict with #26628 Rebased ec11f06e472eb7e1f5654ab8537feff0f17065a5 -> 4224077ac28cf4a40eeacb5e96d7008ca6cc6b10 (pr/nonly.7 -> pr/nonly.8, compare) due to conflicts with #21576 and #26661 Rebased 4224077ac28cf4a40eeacb5e96d7008ca6cc6b10 -> 0db4df75e70f021e63cbc5283779d81f96b894d6 (pr/nonly.8 -> pr/nonly.9, compare) due to conflicts with #26706 and #26039 Rebased 0db4df75e70f021e63cbc5283779d81f96b894d6 -> d936e991a63db798c9ac68238c5c48d42cdd65c7 (pr/nonly.9 -> pr/nonly.10, compare) due to conflict with #26919
  27. DrahtBot removed the label Needs rebase on Dec 17, 2022
  28. DrahtBot added the label Needs rebase on Dec 20, 2022
  29. ryanofsky force-pushed on Jan 6, 2023
  30. DrahtBot removed the label Needs rebase on Jan 6, 2023
  31. DrahtBot added the label Needs rebase on Jan 18, 2023
  32. ryanofsky force-pushed on Jan 20, 2023
  33. DrahtBot removed the label Needs rebase on Jan 21, 2023
  34. DrahtBot added the label Needs rebase on Jan 23, 2023
  35. ryanofsky force-pushed on Feb 10, 2023
  36. DrahtBot removed the label Needs rebase on Feb 10, 2023
  37. DrahtBot added the label Needs rebase on Feb 16, 2023
  38. ryanofsky force-pushed on Feb 17, 2023
  39. ryanofsky commented at 7:42 pm on February 17, 2023: contributor
    Rebased d936e991a63db798c9ac68238c5c48d42cdd65c7 -> 7fd1401d8116323adfa2a87bbcf6ea41437cd0fa (pr/nonly.10 -> pr/nonly.11, compare) due to conflict with #25344
  40. DrahtBot removed the label Needs rebase on Feb 17, 2023
  41. TheCharlatan commented at 12:12 pm on February 21, 2023: contributor

    Concept ACK

    This has been annoying me for years. I think there is one more options field that could be converted to OBJ_NAMED_PARAMS in the scanblocks call.

    There is also the template_request argument in getblocktemplate and the solving_data argument in fundrawtransaction that could be converted as well, but they contain a bunch of array arguments, so I’m not sold if this would actually increase ergonomics.

    The tests now all seem to be using the named arguments. Are there tests ensuring the current option arguments can still be read?

  42. DrahtBot added the label Needs rebase on Mar 27, 2023
  43. ryanofsky force-pushed on Mar 27, 2023
  44. ryanofsky commented at 7:23 pm on March 27, 2023: contributor
    Rebased 7fd1401d8116323adfa2a87bbcf6ea41437cd0fa -> 927a910cbe6c96d60c6a8b05baad179ef3f61f1c (pr/nonly.11 -> pr/nonly.12, compare) due to conflict with #26642.
  45. DrahtBot removed the label Needs rebase on Mar 27, 2023
  46. ajtowns commented at 4:42 am on April 14, 2023: contributor

    There are a two cases where the same name is valid both as a positional argument and in the options object: send and sendall both have conf_target, estimate_mode and fee_rate as named/positional parameters as well as options members (fee_rate explicitly, the others via FundTxDoc). Both those RPCs are marked as “EXPERIMENTAL”, so perhaps it would be good to change their API now?

    I believe:

     0--- a/src/rpc/util.cpp
     1+++ b/src/rpc/util.cpp
     2@@ -493,6 +493,14 @@ RPCHelpMan::RPCHelpMan(std::string name, std::string description, std::vector<RP
     3         for (const std::string& name : names) {
     4             CHECK_NONFATAL(named_args.insert(name).second);
     5         }
     6+        if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) {
     7+            for (const auto& inner : arg.m_inner) {
     8+                std::vector<std::string> inner_names = SplitString(inner.m_names, '|');
     9+                for (const std::string& inner_name : inner_names) {
    10+                    CHECK_NONFATAL(named_args.insert(inner_name).second);
    11+                }
    12+            }
    13+        }
    14         // Default value type should match argument type only when defined
    15         if (arg.m_fallback.index() == 2) {
    16             const RPCArg::Type type = arg.m_type;
    

    would then let you catch these sorts duplicates as soon as you run bitcoind after introducing them.

  47. ajtowns commented at 7:47 am on April 14, 2023: contributor

    Approach ACK

    (just for the record)

  48. ryanofsky commented at 8:43 pm on April 18, 2023: contributor

    re: #26485 (comment) @ajtowns I like the safety check you suggested, and agree it could catch some programming errors. But I also think what send and sendall functions are trying to do to provide compatibility is reasonable, and adding the check would limit what they are able to do.

    send and sendall currently accept conf_target, estimate_mode and fee_rate values as positional parameters (1), or as named parameters (2), or as options (3), and forbidding duplicate option/parameter names would require dropping support for either for (1) or (3). Dropping support for (1) would make these methods not backwards compatible and less convenient to call and dropping support for (3) would make them less consistent with fundrawtransaction and walletcreatefundedpsbt which take these values as options.

    I do think it could make sense to add a check for duplicate options/named parameter fields in the future, but I don’t see a way of doing it now without breaking things, and the having the extra safety check for programming errors doesn’t seem critical here.

    But if you do have a more specific suggestion for how to change the send/sendall functions, I’d definitely be curious. Maybe there is another way out of this tradeoff that I’m not thinking of.

  49. ajtowns commented at 4:20 pm on April 19, 2023: contributor

    They’re both experimental RPCs, so the fact that dropping support for them as a positional param (1) is a backwards incompatibility doesn’t seem terribly important? That we can do positional+named params easily now also seems like it doesn’t make things very inconvenient?

    Could add an RPCArgOptions{.also_positional=true} to mark the options members that are intentionally duplicating positional arguments.

  50. ryanofsky referenced this in commit 284173937d on Apr 19, 2023
  51. ryanofsky commented at 8:09 pm on April 19, 2023: contributor

    Thanks, I added the check and flag you suggested in a new commit.


    Added 1 commits 927a910cbe6c96d60c6a8b05baad179ef3f61f1c -> 284173937d156e1c0f291abe0b82274468d242e8 (pr/nonly.12 -> pr/nonly.13, compare)

  52. DrahtBot added the label CI failed on Apr 19, 2023
  53. in src/rpc/util.cpp:500 in 284173937d outdated
    492@@ -493,6 +493,14 @@ RPCHelpMan::RPCHelpMan(std::string name, std::string description, std::vector<RP
    493         for (const std::string& name : names) {
    494             CHECK_NONFATAL(named_args.insert(name).second);
    495         }
    496+        if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) {
    497+            for (const auto& inner : arg.m_inner) {
    498+                std::vector<std::string> inner_names = SplitString(inner.m_names, '|');
    499+                for (const std::string& inner_name : inner_names) {
    500+                    CHECK_NONFATAL(named_args.insert(inner_name).second || inner.m_opts.also_positional);
    


    ajtowns commented at 4:37 am on April 20, 2023:

    I think this should be:

    0if (!inner.m_opts.also_positional) {
    1    CHECK_NONFATAL(named_args.insert(inner_name).second);
    2}
    

    Otherwise a duplicate positional parameter after the OBJ_NAMED_PARAMS entry will give an error.

    EDIT: or just if (inner.m_opts.also_positional) continue; prior to inner_names = SplitString.


    ryanofsky commented at 5:53 pm on April 20, 2023:

    re: #26485 (review)

    Otherwise a duplicate positional parameter after the OBJ_NAMED_PARAMS entry will give an error.

    Agree that’s not desirable. The reason I wrote it the other way though was that I was trying to prevent the code from allowing two also_positional = true parameters from having the same name. I updated the check to just do the right thing in both cases and added test coverage for both.

  54. ryanofsky referenced this in commit eaee226a17 on Apr 20, 2023
  55. ryanofsky force-pushed on Apr 20, 2023
  56. ryanofsky commented at 6:06 pm on April 20, 2023: contributor
    Updated 284173937d156e1c0f291abe0b82274468d242e8 -> eaee226a17546b93245ca1435e4b468c368d9e86 (pr/nonly.13 -> pr/nonly.14, compare) making check for duplicate parameter names less strict and adding more test coverage for the check
  57. ajtowns commented at 10:16 am on April 26, 2023: contributor

    ACK eaee226a17546b93245ca1435e4b468c368d9e86

    Failing test looks like it was fixed in #27340 already.

  58. DrahtBot added the label Needs rebase on May 1, 2023
  59. RPC: Add add OBJ_NAMED_PARAMS type
    OBJ_NAMED_PARAMS type works the same as OBJ type except it registers the object
    keys to be accepted as top-level named-only RPC parameters. Generated
    documentation also lists the object keys seperately in a new "Named arguments"
    section of help text.
    
    Named-only RPC parameters have the same semantics as python keyword-only
    arguments (https://peps.python.org/pep-3102/). They are always required to be
    passed by name, so they don't affect interpretation of positional arguments,
    and aren't affected when positional arguments are added or removed.
    
    The new OBJ_NAMED_PARAMS type is used in the next commit to make it easier to
    pass options values to various RPC methods.
    
    Co-authored-by: Andrew Chow <github@achow101.com>
    702b56d2a8
  60. RPC: Allow RPC methods accepting options to take named parameters
    Co-authored-by: Andrew Chow <github@achow101.com>
    96233146dd
  61. test: Update python tests to use named parameters instead of options objects 95d7de0964
  62. rpc: Add check for unintended option/parameter name clashes
    Also add flag to allow RPC methods that intendionally accept options and
    parameters with the same name bypass the check.
    
    Check and flag were suggested by ajtowns
    https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1507916357
    
    Co-authored-by: Anthony Towns <aj@erisian.com.au>
    2cd28e9fef
  63. ryanofsky referenced this in commit 58948235d8 on May 3, 2023
  64. ryanofsky referenced this in commit 2808c33bae on May 3, 2023
  65. ryanofsky force-pushed on May 3, 2023
  66. DrahtBot removed the label Needs rebase on May 3, 2023
  67. DrahtBot removed the label CI failed on May 3, 2023
  68. in src/rpc/server.cpp:424 in 2808c33bae outdated
    416@@ -417,7 +417,9 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
    417     // "args" parameter, if present.
    418     int hole = 0;
    419     int initial_hole_size = 0;
    420-    for (const std::string &argNamePattern: argNames) {
    421+    const std::string* initial_param = nullptr;
    422+    UniValue options{UniValue::VOBJ};
    423+    for (const auto& [argNamePattern, named_only]: argNames) {
    424         std::vector<std::string> vargNames = SplitString(argNamePattern, '|');
    425         auto fr = argsIn.end();
    


    ismaelsadeeq commented at 8:46 pm on May 15, 2023:

    Concept ACK Nit: As mentioned in review club, it would be nice to use a more descriptive variable name for the fr variable, considering your change is within the transformNamedArguments method. like find_result or a similar alternative.

    0        auto find_result = argsIn.end();
    
  69. achow101 commented at 0:54 am on May 31, 2023: member

    ~ACK 2808c33bae95a0d2516a6e9a550032af142a04de~

    I find translateNamedArguments somewhat difficult to read, but it seems like there isn’t really much that can be done to make it easier to comprehend without completely overhauling it. I’ve ended up doing that in #27788.

    Edit: It looks like bitcoin-cli is not converting the types.

  70. DrahtBot requested review from ajtowns on May 31, 2023
  71. ajtowns commented at 2:28 am on May 31, 2023: contributor

    reACK 2808c33bae95a0d2516a6e9a550032af142a04de

    (I wouldn’t expect bitcoin-cli to convert the types)

  72. DrahtBot removed review request from ajtowns on May 31, 2023
  73. achow101 referenced this in commit e76651a945 on May 31, 2023
  74. achow101 commented at 3:02 am on May 31, 2023: member

    (I wouldn’t expect bitcoin-cli to convert the types)

    Then this ends up being fairly useless, e.g.:

    0$ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
    1error code: -3
    2error message:
    3JSON value of type null is not of expected type string
    
  75. ajtowns commented at 3:40 am on May 31, 2023: contributor

    (I wouldn’t expect bitcoin-cli to convert the types)

    Then this ends up being fairly useless, e.g.:

    0$ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
    1error code: -3
    2error message:
    3JSON value of type null is not of expected type string
    

    Do you mean JSON value of type string for field change_position is not of expected type number ? (If not, I can’t duplicate your error…)

    Yeah, that does suck, and I don’t see an obvious fix – presumably we’d want named-only parameters to be treated as json rather than a string (unless overridden), but if it’s not overridden, then bitcoin-cli has no way of knowing if change_position here is a positional param or a named one…

  76. achow101 commented at 3:49 am on May 31, 2023: member

    Do you mean JSON value of type string for field change_position is not of expected type number ? (If not, I can’t duplicate your error…)

    Oops, yes, I don’t think I was on the HEAD commit.

    Yeah, that does suck, and I don’t see an obvious fix – presumably we’d want named-only parameters to be treated as json rather than a string (unless overridden), but if it’s not overridden, then bitcoin-cli has no way of knowing if change_position here is a positional param or a named one…

    The obvious fix is to update the table src/rpc/client.cpp to contain all of the named-only parameters as we do for all the other parameters. It’s just a bit tedious. They could just use the same position index as the existing options to avoid converting any other positionals.

    But also we should probably figure out a better way to deal with the conversions in general.

  77. ryanofsky commented at 11:51 am on May 31, 2023: contributor

    But also we should probably figure out a better way to deal with the conversions in general.

    I think it was an unfortunate decision to hardcode parameter information into the client, and could think of various alternatives.

    One alternative could be to avoid the need to process arguments on the client side at all and and add a new exec RPC method that takes an args array of strings and an optional stdin string parameter to figure out on the server side where there is full type information what RPC method to call and how to convert the string args to JSON. An exec method could also do more interpretation like #20273 if that is desirable. The bitcoin client could have a new -exec or -dumb option to invoke the exec RPC instead of trying to convert arguments to JSON itself. Or -exec could just be the default client behavior.

    Another alternative could be to have the client download parameter information from the server instead of hardcoding it, maybe through a schema specification like https://open-rpc.org/getting-started. Probably the client would need to cache this information to avoid a performance penalty, and it could be complicated deciding when to invalidate the cache. Though in the most common case when the client is using a cookie file, the server could just write an rpc-schema.json file next to the cookie and the client could just use that without downloading anything.

  78. achow101 commented at 3:41 pm on May 31, 2023: member

    One alternative could be to avoid the need to process arguments on the client side at all and and add a new exec RPC method that takes an args array of strings and an optional stdin string parameter to figure out on the server side where there is full type information what RPC method to call and how to convert the string args to JSON. An exec method could also do more interpretation like #20273 if that is desirable. The bitcoin client could have a new -exec or -dumb option to invoke the exec RPC instead of trying to convert arguments to JSON itself. Or -exec could just be the default client behavior.

    Could we just have the server automatically convert the arguments as needed? This can still be done before the RPC execution since we have type information in the RPCHelpMan.

  79. ryanofsky commented at 5:36 pm on May 31, 2023: contributor

    Could we just have the server automatically convert the arguments as needed? This can still be done before the RPC execution since we have type information in the RPCHelpMan.

    Oh you’re right. That seems like a better idea, simpler than either of my suggestions. And I think it could allow getting rid of the vRPCConvertParams table entirely?

    I think the only possible downside of having the server automatically convert unexpected string parameters to JSON numbers/bools/objects would be if it reduced type checking for other RPC clients that are more strongly typed. For example if the python client were allowed to pass "true" instead of True or "3" instead of 3, it could make python code more fragile and less safe. But the looser type checking could be controlled with an header to avoid applying it to all clients.

    (This idea actually seems like an mirror version of #15448, which changed the client to pass unparseable JSON arguments as strings instead of raising an error. This suggestion would change the server to parse unexpected string arguments as JSON instead of raising an error.)

  80. achow101 commented at 5:47 pm on May 31, 2023: member
    @ryanofsky Are you planning on making any further changes here?
  81. ryanofsky commented at 6:49 pm on May 31, 2023: contributor

    @ryanofsky Are you planning on making any further changes here?

    I’m not working on anything. The only suggestion I saw was to rename the fr variable, but that is a preexisting variable so would prefer not to change that here.

  82. achow101 commented at 7:05 pm on May 31, 2023: member

    Ah okay, I wasn’t sure if you were going to add the named-only parameters to the conversion table, but based on the above discussion, it sounds like we’re going to move towards eliminating the table in a followup.

    ACK 2808c33bae95a0d2516a6e9a550032af142a04de

  83. ryanofsky referenced this in commit 5559ad2c69 on May 31, 2023
  84. ryanofsky force-pushed on May 31, 2023
  85. ryanofsky commented at 9:21 pm on May 31, 2023: contributor

    I wasn’t sure if you were going to add the named-only parameters to the conversion table

    Sorry, I just missed the part of your earlier comment where you suggested to this. I didn’t realize the conversion table could handle parameters not passed by position. Should be fixed now though.

    Updated 2808c33bae95a0d2516a6e9a550032af142a04de -> 5559ad2c69ef82c18843b9214e5ba3974834a1ae (pr/nonly.15 -> pr/nonly.16, compare) with rpc/client.cpp entries

  86. DrahtBot added the label CI failed on Jun 1, 2023
  87. achow101 commented at 4:52 am on June 1, 2023: member

    It looks like the conversion table is missing the arguments that take amounts; these also need to be converted. The test is also failing as the named only arguments are not being output in the dump_all_command_conversions, the regex can’t handle negatives, and psbt has mismatched conversions.

    The following diff should fix those issues:

      0diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
      1index be4d777169..7bd66ebbd2 100644
      2--- a/src/rpc/client.cpp
      3+++ b/src/rpc/client.cpp
      4@@ -101,7 +101,10 @@ static const CRPCConvertParam vRPCConvertParams[] =
      5     { "listunspent", 2, "addresses" },
      6     { "listunspent", 3, "include_unsafe" },
      7     { "listunspent", 4, "query_options" },
      8+    { "listunspent", -1, "minimumAmount" },
      9+    { "listunspent", -1, "maximumAmount" },
     10     { "listunspent", -1, "maximumCount" },
     11+    { "listunspent", -1, "minimumSumAmount" },
     12     { "listunspent", -1, "include_immature_coinbase" },
     13     { "getblock", 1, "verbosity" },
     14     { "getblock", 1, "verbose" },
     15@@ -134,6 +137,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
     16     { "fundrawtransaction", -1, "changePosition"},
     17     { "fundrawtransaction", -1, "includeWatching"},
     18     { "fundrawtransaction", -1, "lockUnspents"},
     19+    { "fundrawtransaction", -1, "fee_rate"},
     20+    { "fundrawtransaction", -1, "feeRate"},
     21     { "fundrawtransaction", -1, "subtractFeeFromOutputs"},
     22     { "fundrawtransaction", -1, "input_weights"},
     23     { "fundrawtransaction", -1, "conf_target"},
     24@@ -151,6 +156,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
     25     { "walletcreatefundedpsbt", -1, "changePosition"},
     26     { "walletcreatefundedpsbt", -1, "includeWatching"},
     27     { "walletcreatefundedpsbt", -1, "lockUnspents"},
     28+    { "walletcreatefundedpsbt", -1, "fee_rate"},
     29+    { "walletcreatefundedpsbt", -1, "feeRate"},
     30     { "walletcreatefundedpsbt", -1, "subtractFeeFromOutputs"},
     31     { "walletcreatefundedpsbt", -1, "conf_target"},
     32     { "walletcreatefundedpsbt", -1, "replaceable"},
     33@@ -186,6 +193,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     34     { "send", -1, "maxconf"},
     35     { "send", -1, "add_to_wallet"},
     36     { "send", -1, "change_position"},
     37+    { "send", -1, "fee_rate"},
     38     { "send", -1, "include_watching"},
     39     { "send", -1, "inputs"},
     40     { "send", -1, "locktime"},
     41@@ -200,6 +208,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     42     { "sendall", 3, "fee_rate"},
     43     { "sendall", 4, "options" },
     44     { "sendall", -1, "add_to_wallet"},
     45+    { "sendall", -1, "fee_rate"},
     46     { "sendall", -1, "include_watching"},
     47     { "sendall", -1, "inputs"},
     48     { "sendall", -1, "locktime"},
     49@@ -244,10 +253,12 @@ static const CRPCConvertParam vRPCConvertParams[] =
     50     { "gettxspendingprevout", 0, "outputs" },
     51     { "bumpfee", 1, "options" },
     52     { "bumpfee", -1, "conf_target"},
     53+    { "bumpfee", -1, "fee_rate"},
     54     { "bumpfee", -1, "replaceable"},
     55     { "bumpfee", -1, "outputs"},
     56     { "psbtbumpfee", 1, "options" },
     57     { "psbtbumpfee", -1, "conf_target"},
     58+    { "psbtbumpfee", -1, "fee_rate"},
     59     { "psbtbumpfee", -1, "replaceable"},
     60     { "psbtbumpfee", -1, "outputs"},
     61     { "logging", 0, "include" },
     62diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
     63index c35706ce24..2a1061d49f 100644
     64--- a/src/rpc/util.cpp
     65+++ b/src/rpc/util.cpp
     66@@ -706,17 +706,30 @@ std::string RPCHelpMan::ToString() const
     67 UniValue RPCHelpMan::GetArgMap() const
     68 {
     69     UniValue arr{UniValue::VARR};
     70+
     71+    auto push_back_arg_info = [&arr](const std::string& rpc_name, int pos, const std::string& arg_name, const RPCArg::Type& type) {
     72+        UniValue map{UniValue::VARR};
     73+        map.push_back(rpc_name);
     74+        map.push_back(pos);
     75+        map.push_back(arg_name);
     76+        map.push_back(type == RPCArg::Type::STR ||
     77+                      type == RPCArg::Type::STR_HEX);
     78+        arr.push_back(map);
     79+    };
     80+
     81     for (int i{0}; i < int(m_args.size()); ++i) {
     82         const auto& arg = m_args.at(i);
     83         std::vector<std::string> arg_names = SplitString(arg.m_names, '|');
     84         for (const auto& arg_name : arg_names) {
     85-            UniValue map{UniValue::VARR};
     86-            map.push_back(m_name);
     87-            map.push_back(i);
     88-            map.push_back(arg_name);
     89-            map.push_back(arg.m_type == RPCArg::Type::STR ||
     90-                          arg.m_type == RPCArg::Type::STR_HEX);
     91-            arr.push_back(map);
     92+            push_back_arg_info(m_name, i, arg_name, arg.m_type);
     93+            if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) {
     94+                for (const auto& inner : arg.m_inner) {
     95+                    std::vector<std::string> inner_names = SplitString(inner.m_names, '|');
     96+                    for (const std::string& inner_name : inner_names) {
     97+                        push_back_arg_info(m_name, -1, inner_name, inner.m_type);
     98+                    }
     99+                }
    100+            }
    101         }
    102     }
    103     return arr;
    104diff --git a/test/functional/rpc_help.py b/test/functional/rpc_help.py
    105index 7acc3cbbd5..dbd4b3047e 100755
    106--- a/test/functional/rpc_help.py
    107+++ b/test/functional/rpc_help.py
    108@@ -32,7 +32,7 @@ def process_mapping(fname):
    109                 if line.startswith('};'):
    110                     in_rpcs = False
    111                 elif '{' in line and '"' in line:
    112-                    m = re.search(r'{ *("[^"]*"), *([0-9]+) *, *("[^"]*") *},', line)
    113+                    m = re.search(r'{ *("[^"]*"), *(-?[0-9]+) *, *("[^"]*") *},', line)
    114                     assert m, 'No match to table expression: %s' % line
    115                     name = parse_string(m.group(1))
    116                     idx = int(m.group(2))
    117@@ -85,8 +85,8 @@ class HelpRpcTest(BitcoinTestFramework):
    118 
    119         for argname, convert in converts_by_argname.items():
    120             if all(convert) != any(convert):
    121-                # Only allow dummy to fail consistency check
    122-                assert argname == 'dummy', ('WARNING: conversion mismatch for argument named %s (%s)' % (argname, list(zip(all_methods_by_argname[argname], converts_by_argname[argname]))))
    123+                # Only allow dummy and psbt to fail consistency check
    124+                assert argname in ['dummy', "psbt"], ('WARNING: conversion mismatch for argument named %s (%s)' % (argname, list(zip(all_methods_by_argname[argname], converts_by_argname[argname]))))
    125 
    126     def test_categories(self):
    127         node = self.nodes[0]
    
  88. ASISBusiness approved
  89. ryanofsky commented at 2:22 pm on June 1, 2023: contributor

    It looks like the conversion table is missing the arguments that take amounts; these also need to be converted.

    Thank you! Will apply patch. I’m surprised amounts need to be converted, though. I thought AmountFromValue handled both numbers and strings so strings would be fine.

  90. achow101 commented at 2:57 pm on June 1, 2023: member

    I’m surprised amounts need to be converted, though

    It actually may just be the test that checks the conversion table that requires it since dump_all_command_conversions marks amounts as needed to be converted. However converting amounts is also consistent with all of the other RPCs that accept amounts as arguments, even prior to that test being added.

  91. ryanofsky force-pushed on Jun 1, 2023
  92. ryanofsky commented at 4:15 pm on June 1, 2023: contributor
    Updated 5559ad2c69ef82c18843b9214e5ba3974834a1ae -> 2cd28e9fef5dd743bcd70025196ee311fcfdcae4 (pr/nonly.16 -> pr/nonly.17, compare) with suggested test and conversion table fixes. I also changed the code to use actual position of the options parameters as originally suggested, instead of -1 values.
  93. achow101 commented at 7:09 pm on June 1, 2023: member

    ACK 2cd28e9fef5dd743bcd70025196ee311fcfdcae4

    Only changes since last are the suggested conversion table changes

  94. DrahtBot requested review from ajtowns on Jun 1, 2023
  95. achow101 merged this on Jun 1, 2023
  96. achow101 closed this on Jun 1, 2023

  97. sidhujag referenced this in commit ba2a7a9621 on Jun 1, 2023
  98. janus referenced this in commit 4d0fd0392f on Sep 11, 2023
  99. bitcoin locked this on May 31, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me