The PR branch HEAD was 2808c33ba at the time of this review club meeting.
Notes
RPC methods can take parameters in three ways:
Using positional parameters, e.g. bitcoin-cli getblock "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"
Using named parameters, e.g. bitcoin-cli named getblock blockhash="00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"
Using an options object, e.g. bitcoin-cli -named bumpfee "66975ce3ea2b0815d677eaac1f1822276943cf7361d3eb920ad3cc278b473609" options='{"fee_rate": 10}'
For an end-user (especially through bitcoin-cli), the options notation can be quite verbose, especially when they just want to specify a single parameter.
Some endpoints, such as send, allow passing parameters such as conf_target either as a options field or a named/positional parameter, but this comes with code overhead and needs to be implemented by every RPC method.
To simplify the interface, #26485 allows any options parameter to also be passed as a named parameter.
Why do some RPCs use an options parameter? Do we still need it? If so, what for? If not, can we remove it?
Which function is responsible for checking if an options field is passed as a named parameter? What other approaches can you think of to achieve the same goal this PR is trying to achieve?
The documentation for send lists conf_target both as a named argument (#2) as well as a field in options. When looking at the code, however, it seems like conf_target is defined only once. How is this possible?
Why does RPCHelpMan::GetArgNames()now return a std::vector<std::pair<std::string, bool>> instead of a std::vector<std::string>? What does the bool represent?
<stickies-v> welcome everyone! Today we're looking at #26485, authored by ryanofsky. The notes and questions are available on https://bitcoincore.reviews/26485
<stickies-v> pablomartin: well, that's just the cli syntax. on the RPC side (which is the only thing this PR is touching), we're indeed passing named arguments as a dict/object, but that's nothing new - we already had named arguments
<stickies-v> Which function is responsible for checking if an `options` field is passed as a named parameter? What other approaches can you think of to achieve the same goal this PR is trying to achieve?
<stickies-v> The documentation for `send` lists `conf_target` both as a named argument (#2) as well as a field in `options`. When looking at the code, however, it seems like `conf_target` is defined only once. How is this possible?
<stickies-v> and for RPCs with a lot of parameters, such as e.g. the `send` family, it's quite cumbersome to provide a whole bunch of null/default values for every single RPC call.
<stickies-v> so my point is if you run `bitcoin-cli help send`, the help shows definitions for e.g. `conf_target` twice: once as a named parameter, and once as an `options` field. but in the code, we only seem to be defining it once
<stickies-v> Why does `RPCHelpMan::GetArgNames()` now return a `std::vector<std::pair<std::string, bool>>` instead of a `std::vector<std::string>`? What does the `bool` represent?
<stickies-v> pablomartin: oh yeah, absolutely, the end-user decides whether they pass positional or named args. but adding the `bool` as a `pair` item allows us to let the developer specify per-method how they want this behaviour to work, as opposed to for example automatically enabling it for all `OBJ` parameters, or for all `OBJ` parameters named `options`
<stickies-v> LarryRuane: yeah exactly. kinda like how even though when accessing a vector element it's safer and generally recommended to use `v.at(i)`, you'll often see `v[i]` used in our codebase but (typically/hopefully) only if we've ensured that `i` definitely is in range, because then it's just a bit faster
<stickies-v> i think (but didn't name the variable) that `fr` just stands for `find_result` or something (not a huge fan of 1-2 letter named variables...)
<stickies-v> we just want to check if we've already processed the key/value pair earlier, and if so throw an error, to avoid people passing the same parameter as option/positional/named parameter