rpc: Add level 3 verbosity to getblock RPC call. #21245
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
8f41540 to
6eede79
Compare
|
Concept ACK. Thanks for picking this up! Will have a closer look and test this soon. |
luke-jr
suggested changes
Feb 24, 2021
6eede79 to
dc68479
Compare
luke-jr
suggested changes
Feb 24, 2021
13974de to
53ea5bd
Compare
7569d7c to
d39eb54
Compare
|
Ideally, this should probably be 3 commits: one changing the bool to the class enum (with 2 values), one adding the new feature, and another adding tests. |
d39eb54 to
5710e3f
Compare
Done |
luke-jr
suggested changes
Feb 27, 2021
37f2c4e to
d512661
Compare
c8ecb1d to
4aaebc1
Compare
3f70a60 to
72dbe98
Compare
|
@kiminuo i think your comments should be addressed now. Can you mark this as reviewed once this looks good? |
|
Currently Console outputIt would be great to:
Edit: I'll try to carve out some more time tomorrow to test it more. |
|
The following examples may be worth adding to your original post: Examples of the
|
kiminuo
reviewed
Aug 6, 2021
| @@ -1017,7 +1025,16 @@ static RPCHelpMan getblock() | |||
| return strHex; | |||
| } | |||
|
|
|||
| return blockToJSON(block, tip, pblockindex, verbosity >= 2); | |||
| TxVerbosity block_to_json_verbosity; | |||
There was a problem hiding this comment.
nit: The name block_to_json_verbosity seems long and it does not add any more meaning than just tx_verbosity, imho.
| @@ -20,6 +20,12 @@ class uint256; | |||
| class UniValue; | |||
| class CTxUndo; | |||
|
|
|||
| enum class TxVerbosity { | |||
There was a problem hiding this comment.
Some enum classes have these nice doxygen comments:
bitcoin/src/consensus/validation.h
Line 63 in d67330d
| @@ -312,6 +312,16 @@ def run_test(self): | |||
| if 'coinbase' not in tx['vin'][0]} | |||
| assert_equal(non_coinbase_txs, set(txs)) | |||
|
|
|||
| # Verify that the non-coinbase tx has "prevout" field set | |||
There was a problem hiding this comment.
nit:
Suggested change
| # Verify that the non-coinbase tx has "prevout" field set | |
| # Verify that the non-coinbase tx has "prevout" key set |
A Python dictionary is a set of key-value pairs.
Comment on lines
+320
to
+321
| prevout = vin["prevout"] | ||
| assert_equal(prevout["generated"], False) |
There was a problem hiding this comment.
Given that prevout variable is used just once, it may be shorter and actually easier to read:
Suggested change
| prevout = vin["prevout"] | |
| assert_equal(prevout["generated"], False) | |
| assert_equal(vin["prevout"]["generated"], False) |
| def assert_vin_contains_prevout(verbosity): | ||
| block = node.getblock(blockhash, verbosity) | ||
| tx = block["tx"][1] | ||
| total_vin = 0 |
There was a problem hiding this comment.
I believe this should be more like:
Suggested change
| total_vin = 0 | |
| total_vin = Decimal("0.00000000") |
|
|
||
| for (size_t i = 0; i < block.vtx.size(); ++i) { | ||
| const CTransactionRef& tx = block.vtx.at(i); | ||
| // coinbase transaction (i == 0) doesn't have undo data |
There was a problem hiding this comment.
nit:
Suggested change
| // coinbase transaction (i == 0) doesn't have undo data | |
| // coinbase transaction (i.e. i == 0) doesn't have undo data |
|
|
||
| case TxVerbosity::SHOW_DETAILS_AND_PREVOUT: | ||
| UniValue o_script_pub_key(UniValue::VOBJ); | ||
| ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /* includeHex */ true, /* include_addresses */ true); |
There was a problem hiding this comment.
Question: There is include_addresses parameter which is not used here. true is passed instead. Is that correct or should it look like:
Suggested change
| ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /* includeHex */ true, /* include_addresses */ true); | |
| ScriptPubKeyToUniv(prev_txout.scriptPubKey, o_script_pub_key, /* includeHex */ true, include_addresses); |
It looks to me that include_addresses = true is deprecated actually.
Edit: This PR will be most likely affected by #22650.
There was a problem hiding this comment.
Since this is adding new output, I think it should probably just be a constant false here, actually.
|
@fyquah I have attempted to address my comments above and add RPC help here: https://github.com/kiminuo/bitcoin/tree/feature/2021-08-08-verbosity-level-3-getblock-iteration-3: ./bitcoin-cli -testnet getblockreports for me: Console outputIf you find any of the modifications useful, please include it in your PR. |
theStack
reviewed
Aug 9, 2021
There was a problem hiding this comment.
re-ACK 72dbe98
Verified via git range-diff cecbfed9...72dbe981 that since my last ACK all requested changes since then (e.g. rename calculate_fee to have_undo (#21245 (comment)), more detailled "prevout" checking w.r.t. to coinbase/non-coinbase txs (#21245 (comment)), RPC help improvement (#21245 (comment))) were tackled. Also compiled locally and successfully ran the involved functional tests.
Did some more review, tested it and the PR is shaping up very nicely.
Agree! :)
@kiminuo made some very good points for further improvements, especially considering the missing result desription for the newly introduced level in the RPC help. Since none of them are really blockers IMHO and the PR author does seem to be quite busy, maybe they can be done in a follow-up PR, to move things forward?
This has only been ACK'd by a single reviewer. |
|
I would re-ACK except for #21245 (comment) |
This was referenced Aug 25, 2021
|
@fyquah I think #21245 (comment) is important so I would ACK once this is fixed. Also, the work is actually done here #21245 (comment) I believe (the last commit is extra and is for a follow-up PR). |
|
@fyquah Could you find a moment to address #21245 (comment)? (PS: I'm willing to take over your PR to work on it if you are too busy. Anyway, it seems to me one little change is needed here, nothing time consuming.) |
|
Closing this for now that #22918 is open. |
This was referenced Sep 30, 2021
Closed
laanwj
added a commit
to bitcoin-core/gui
that referenced
this pull request
Oct 19, 2021
…PC call (#21245 modified) 5c34507 core_write: Rename calculate_fee to have_undo for clarity (fyquah) 8edf620 release-notes: Add release note about getblock verbosity level 3. (fyquah) 459104b rest: Add test for prevout fields in getblock (fyquah) 4330af6 rpc: Add test for level 3 verbosity getblock rpc call. (fyquah) 51dbc16 rpc: Add level 3 verbosity to getblock RPC call. (fyquah) 3cc9534 rpc: Replace boolean argument for tx details with enum class. (fyquah) Pull request description: Author of #21245 expressed [time issues](bitcoin/bitcoin#21245 (comment)) in the original PR. Given that #21245 has received a lot of review*, I have decided to open this new pull request with [modifications required to get ACK from luke-jr ](bitcoin/bitcoin#21245 (comment)) and a few nits of mine. ### Original PR description > Display the prevout in transaction inputs when calling getblock level 3 verbosity. This PR affects the existing `/rest/block` API by adding a `prevout` fields to tx inputs. This is mentioned in the change to the release notes. > > I added some functional tests that > > * checks that the RPC call still works when TxUndo can't be found > > * Doesn't display the "value" or "scriptPubKey" of the previous output when at a lower verbosity level > > > This "completes" the issue #18771 ### Possible improvements * kiminuo/bitcoin@b0bf4f2 - I can include even this commit to this PR if deemed useful or I can leave it for a follow-up PR. See bitcoin/bitcoin#21245 (comment) for more context. ### Examples Examples of the `getblock` output with various verbose levels. Note that `000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5` contains only 2 transactions. #### Verbose level 0 ```bash ./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 0 ``` ##### Verbose level 1 ```bash ./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 1 ``` ##### Verbose level 2 ```bash ./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 2 ``` ##### Verbose level 3 ```bash ./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 3 ``` #### REST ```bash curl -H "content-type:text/plain;" http://127.0.0.1:18332/rest/block/000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5.json ``` <sub>* ... and my everyday obsessive checking of my email inbox whether the PR moves forward.</sub> Edit laanwj: Removed at symbol from message, and large example output to prevent it from all ending up in the commit message. ACKs for top commit: 0xB10C: ACK 5c34507 meshcollider: utACK 5c34507 theStack: ACK 5c34507 👘 promag: Concept ACK 5c34507 Tree-SHA512: bbff120d8fd76e617b723b102b0c606e0d8eb27f21c631d5f4cdab0892137c4bc7c65b1df144993405f942c91be47a26e80480102af55bff22621c19f518aea3
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 19, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Display the prevout in transaction inputs when calling getblock level 3 verbosity. This PR affects the existing
/rest/blockAPI by adding aprevoutfields to tx inputs. This is mentioned in the change to the release notes.I added some functional tests that
This "completes" the issue #18771