rpc: Add RPC help for getblock verbosity level 3

kiminuo commented at 6:39 am on October 20, 2021: contributor

This is a follow-up PR to #22918 which addresses review comments (first commit). The second commit adds missing RPC help for verbosity level 3.

fanquake added the label RPC/REST/ZMQ on Oct 20, 2021

in src/rpc/blockchain.cpp:1011 in 27163807c2 outdated

1006+                                        {RPCResult::Type::OBJ, "scriptPubKey", "",
1007+                                        {
1008+                                            {RPCResult::Type::STR, "asm", "The asm"},
1009+                                            {RPCResult::Type::STR, "hex", "The hex"},
1010+                                            {RPCResult::Type::STR, "address", /* optional */ true, "The Bitcoin address (only if a well-defined address exists)"},
1011+                                            {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"},

kiminuo commented at 6:42 am on October 20, 2021:

I actually like very much: https://github.com/bitcoin/bitcoin/blob/0ccf9b2e5594581deef2f60174c3651a57f93b64/src/rpc/rawtransaction.cpp#L547

Moving https://github.com/bitcoin/bitcoin/blob/0ccf9b2e5594581deef2f60174c3651a57f93b64/src/rpc/rawtransaction.cpp#L526-L534 to rpc/util.h would make it available for other RPC calls and the help output would be more informative.

WDYT? Is it good for this PR or another one?

laanwj commented at 12:38 pm on November 23, 2021:

SGTM, it cannot hurt to enumerate them if a function to provide that information is available already anyway.

in src/core_write.cpp:214 in 27163807c2 outdated

216-                case TxVerbosity::SHOW_DETAILS_AND_PREVOUT:
217-                    UniValue o_script_pub_key(UniValue::VOBJ);
218-                    ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /* includeHex */ true);
219+            if (verbosity == TxVerbosity::SHOW_DETAILS_AND_PREVOUT) {
220+                UniValue o_script_pub_key(UniValue::VOBJ);
221+                ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /* include_hex */ true);

MarcoFalke commented at 8:37 am on October 20, 2021:

nit: Add = for clang-tidy?

0                ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /*include_hex=*/ true);

kiminuo commented at 8:51 am on October 20, 2021:

Yes, thank you.

Address review comments from #22918

* fix English in release notes
* Simplify `switch` to `if`.

1bdd5f6322

kiminuo force-pushed on Oct 20, 2021

kiminuo marked this as ready for review on Oct 20, 2021

DrahtBot commented at 6:57 pm on October 25, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments by MarcoFalke)

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.

theStack commented at 3:03 am on November 7, 2021: member

Concept ACK

kiminuo closed this on Nov 23, 2021

kiminuo deleted the branch on Nov 23, 2021

laanwj commented at 12:13 pm on November 23, 2021: member

Any specific reason for closing?

kiminuo restored the branch on Nov 23, 2021

kiminuo reopened this on Nov 23, 2021

kiminuo commented at 12:14 pm on November 23, 2021: contributor

@laanwj No, that was just a mistake as I was deleting old branches and this one was deleted by mistake too. Sorry.

laanwj commented at 12:17 pm on November 23, 2021: member

@kiminuo No problem! Will take a look it seems very close to ready to merge.

laanwj renamed this:
~~rpc: Add RPC help for verbosity level 3~~
rpc: Add RPC help for getblock verbosity level 3
on Nov 23, 2021

in src/rpc/blockchain.cpp:1001 in 08d129a406 outdated

 996+                            {RPCResult::Type::ARR, "vin", "",
 997+                            {
 998+                                {RPCResult::Type::OBJ, "", "",
 999+                                {
1000+                                    {RPCResult::Type::ELISION, "", "The same output as verbosity = 2"},
1001+                                    {RPCResult::Type::OBJ, "prevout", "",

laanwj commented at 12:37 pm on November 23, 2021:

Might want to mention briefly this is only available if there is undo information for the input (e.g., not for coinbase inputs). E.g. a user might be surprised to not see it when running it on the genesis block.

kiminuo commented at 3:05 pm on November 24, 2021:

Happy to change. It’s just about finding the proper sentence. What about

0{RPCResult::Type::OBJ, "prevout", "(Only if undo information is available)",

?

Or is it too brief?

laanwj commented at 2:12 pm on January 4, 2022:

Seems fine to me, agree that the set of conditions is hard to describe precisely in the room available.

laanwj commented at 1:42 pm on November 23, 2021: member

Code review and lightly tested ACK 08d129a406947b39f7822458ba9a1af30aeacbc3 Small nit on the documentation but feel free to ignore.

kiminuo force-pushed on Nov 26, 2021

pg156 commented at 10:28 pm on November 26, 2021: none

src/bitcoin-cli help getblock doesn’t return anything related to “verbosity = 3” on macOS Big Sur. Did I do something wrong in the steps below?

0git clone https://github.com/kiminuo/bitcoin.git 23320
1cd 23320
2git checkout feature/2021-10-20-verbosity-level-3-nits      
3./autogen.sh
4./configure --with-incompatible-bdb --without-miniupnpc --without-natpmp --disable-bench --disable-wallet --without-gui
5make -j 3 src/bitcoin-cli
6./src/bitcoin-cli help getblock

theStack commented at 4:28 pm on November 27, 2021: member

src/bitcoin-cli help getblock doesn’t return anything related to “verbosity = 3” on macOS Big Sur. Did I do something wrong in the steps below? @pg156: You have to build and run bitcoind, as the help texts to the RPC calls are residing in there. bitcoin-cli is merely the RPC client that fetches it, i.e. most likely have another instance running on your system that has nothing to do with this PR. For simple tests like those, I recommend running bitcoind with -nolisten -noconnect to prevent unnecessary network activity.

pg156 commented at 3:56 am on November 28, 2021: none

Thank you @theStack! I get “verbosity = 3” after building bitcoind and stopping another bitcoind running on my machine, as you suggested.

pg156 commented at 4:12 am on November 30, 2021: none

Does this line also need to be updated for verbosity = 3?

https://github.com/bitcoin/bitcoin/blob/1bdd5f63229ebf28c24a8656f486ed8a6c8b3787/src/rpc/blockchain.cpp#L946

Add RPC help for getblock verbosity level 3 059f88b6a9

kiminuo force-pushed on Nov 30, 2021

kiminuo commented at 9:40 am on November 30, 2021: contributor

@pg156 It’s certainly better to update it. Updated. Thank you.

Ideas about better wordings are welcome :)

pg156 commented at 3:18 am on December 3, 2021: none

Code review ACK https://github.com/bitcoin/bitcoin/commit/3a6a670456b0f45edc8c70647fd49969adef2e74. (but https://github.com/bitcoin/bitcoin/commit/3a6a670456b0f45edc8c70647fd49969adef2e74 doesn’t exist in the “Commits” tab after the force-push. I don’t know if the standard practice is to ACK only what is in the tab?)

Tested the difference in output of getblock between verbosity of 2 and 3 is as described in RPC help, by adding two lines of logging in _test_getblock function in test/functional/rpc_blockchain.py:

0self.log.info(node.getblock(blockhash, 2))
1self.log.info(node.getblock(blockhash, 3))

The difference is:

0'prevout': {'generated': True, 'height': 18, 'value': Decimal('50.00000000'), 'scriptPubKey': {'asm': '0 4ae81572f06e1b88fd5ced7a1a000945432e83e1551e6f721ee9c00b8cc33260', 'hex': '00204ae81572f06e1b88fd5ced7a1a000945432e83e1551e6f721ee9c00b8cc33260', 'address': 'bcrt1qft5p2uhsdcdc3l2ua4ap5qqfg4pjaqlp250x7us7a8qqhrxrxfsqseac85', 'type': 'witness_v0_scripthash'}}

Tested “verbosity = 3” section in RPC help is as expected by running:

0./src/bitcoind -testnet -daemon         
1./src/bitcoin-cli -testnet help getblock

which generates:

 0...
 1Result (for verbosity = 3):
 2{                                        (json object)
 3  ...,                                   Same output as verbosity = 2
 4  "tx" : [                               (json array)
 5    {                                    (json object)
 6      "vin" : [                          (json array)
 7        {                                (json object)
 8          ...,                           The same output as verbosity = 2
 9          "prevout" : {                  (json object) (Only if undo information is available)
10            "generated" : true|false,    (boolean) Coinbase or not
11            "height" : n,                (numeric) The height of the prevout
12            "value" : n,                 (numeric) The value in BTC
13            "scriptPubKey" : {           (json object)
14              "asm" : "str",             (string) The asm
15              "hex" : "str",             (string) The hex
16              "address" : "str",         (string, optional) The Bitcoin address (only if a well-defined address exists)
17              "type" : "str"             (string) The type, eg 'pubkeyhash'
18            }
19          }
20        },
21        ...
22      ]
23    },
24    ...
25  ]
26}

kiminuo commented at 6:33 am on December 3, 2021: contributor

(but 3a6a670 doesn’t exist in the “Commits” tab after the force-push. I don’t know if the standard practice is to ACK only what is in the tab?)

Typically one ACKs the last commit from https://github.com/bitcoin/bitcoin/pull/23320/commits page because that’s the current version of the PR.

You can also possibly ACK a commit from previous git (force) push to say something like “I really like the previous changeset as opposed to the current changeset” but that would be rare AFAIK.

Thank you for the review!

luke-jr referenced this in commit 7bb3835c62 on Dec 14, 2021

kiminuo commented at 10:14 am on December 27, 2021: contributor

@pg156 Could you please re-review? @MarcoFalke Would you mind having a look, please?

pg156 commented at 2:45 pm on January 3, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/23320/commits/059f88b6a97b7a3ae1f033885e40ac01f91e6d60

laanwj commented at 2:28 pm on January 4, 2022: member

re-ACK 059f88b6a97b7a3ae1f033885e40ac01f91e6d60

laanwj merged this on Jan 4, 2022

laanwj closed this on Jan 4, 2022

kiminuo deleted the branch on Jan 4, 2022

sidhujag referenced this in commit 7639e6ec1c on Jan 4, 2022

MarcoFalke referenced this in commit 1337b93f50 on Feb 21, 2022

DrahtBot locked this on Jan 4, 2023

rpc: Add RPC help for getblock verbosity level 3 #23320

Conflicts