New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rpc: Add level 3 verbosity to getblock RPC call (#21245 modified) #22918
rpc: Add level 3 verbosity to getblock RPC call (#21245 modified) #22918
Conversation
Concept ACK. Thanks for picking this up. Will review now that the requested changes are in. |
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. |
Concept ACK, this is a useful feature to have, an interesting approach to use the undo data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 305a59a
305a59a
to
d4d043b
Compare
d4d043b
to
b0f7af3
Compare
Co-authored-by: Luke Dashjr <luke_github1@dashjr.org> Co-authored-by: 0xB10C <19157360+0xB10C@users.noreply.github.com>
Display the prevout in transaction inputs when calling getblock level 3 verbosity. Co-authored-by: Luke Dashjr <luke_github1@dashjr.org> Co-authored-by: 0xB10C <19157360+0xB10C@users.noreply.github.com>
b0f7af3
to
5c34507
Compare
@luke-jr Could you please re-ACK if you feel like it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-utACK b0f7af3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 5c34507
I am leaning towards including the helptext for the new verbosity level. Obviously that will make the help output pretty long, but I think it should be documented.
@@ -76,6 +76,14 @@ Updated RPCs | |||
`gettransaction verbose=true` and REST endpoints `/rest/tx`, `/rest/getutxos`, | |||
`/rest/block` no longer return the `addresses` and `reqSigs` fields, which | |||
were previously deprecated in 22.0. (#22650) | |||
- The `getblock` RPC command now supports verbose level 3 containing transaction inputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: verbose
-> verbosity
and inputs
-> inputs'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I will fix in a next rebase or in a follow-up PR.
Do you possibly mean kiminuo@b0bf4f2 mentioned in the OP (under Possible improvements)? Or what do you mean by helptext, please? |
Co-authored-by: Luke Dashjr <luke_github1@dashjr.org> Co-authored-by: 0xB10C <19157360+0xB10C@users.noreply.github.com> Github-Pull: bitcoin#22918 Rebased-From: 6cd4414
Display the prevout in transaction inputs when calling getblock level 3 verbosity. Co-authored-by: Luke Dashjr <luke_github1@dashjr.org> Co-authored-by: 0xB10C <19157360+0xB10C@users.noreply.github.com> Github-Pull: bitcoin#22918 Rebased-From: dda6ef1
Github-Pull: bitcoin#22918 Rebased-From: cc529a1
Github-Pull: bitcoin#22918 Rebased-From: 83dfadb
@theStack Would you please re-ACK if you feel like it? |
ACK 5c34507 |
p.pushKV("scriptPubKey", o_script_pub_key); | ||
in.pushKV("prevout", p); | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per doc/developer-notes.md::L672-694
,the following comment and assert are usually employed with self-contained switch statements so the compiler can warn about missing cases. It looks like that could work here as the switch is scoped within the have_undo
conditional:
} // no default case, so the compiler can warn about missing cases
assert(false);
}
(usually, the case
statements have the same indentation as switch
, without blank lines between them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I would like to address that a next rebase or in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5c34507 👘
Verified via git range-diff 72dbe981...5c34507e
that since my last ACK (review done in PR #21245, see #21245 (review)) all changes are rebase-related or minor improvements (related to comments, variable renames etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK 5c34507
UniValue objTx(UniValue::VOBJ); | ||
TxToUniv(*tx, uint256(), objTx, true, RPCSerializationFlags(), txundo); | ||
txs.push_back(objTx); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, add break;
?
amt_total_in += prev_txout.nValue; | ||
switch (verbosity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, we only care about one value, what's wrong with
if (verbosity == TxVerbosity::SHOW_DETAILS_AND_PREVOUT) {
If you do this, then declare prev_coin
in the inner scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I would like to address that a next rebase or in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have actually proposed the same in the original PR: #21245 (comment)
@MarcoFalke Should I address the review comments now (and thus invalidate ACKs) or can I do it in a follow-up PR? I mean can the PR be merged? |
Merged this as there were no critical comments, many ACKs, and to not drag this on forever. I do think in general it's better to just incorporate comments and not move small nits 'to follow-up PRs' because of fear of invalidating ACKs. |
I agree. The point of PR review (among other things) is to get code to a mergable state. There are trade-offs depending on the size/complexity of a change, and whether or not everything must be addressed, but I think we have started to drift too far to the wrong side of pushing changes to followups / not actually taking review suggestions onboard for fear of invalidating ACKs (not saying that is the case here). |
* fix English in release notes * Simplify `switch` to `if`.
* fix English in release notes * Simplify `switch` to `if`.
059f88b Add RPC help for getblock verbosity level 3 (Kiminuo) 1bdd5f6 Address review comments from #22918 (Kiminuo) Pull request description: 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. ACKs for top commit: pg156: ACK 059f88b laanwj: re-ACK 059f88b Tree-SHA512: f27d53ac34b93a304ef5668701ed2b5c986a926bc8ad0df4de89695fc9e1df26acb008611451319ea897658acd9c56c6a0555d60359960c9cd28238ebefa2d50
059f88b Add RPC help for getblock verbosity level 3 (Kiminuo) 1bdd5f6 Address review comments from bitcoin#22918 (Kiminuo) Pull request description: This is a follow-up PR to bitcoin#22918 which addresses review comments (first commit). The second commit adds missing RPC help for verbosity level 3. ACKs for top commit: pg156: ACK bitcoin@059f88b laanwj: re-ACK 059f88b Tree-SHA512: f27d53ac34b93a304ef5668701ed2b5c986a926bc8ad0df4de89695fc9e1df26acb008611451319ea897658acd9c56c6a0555d60359960c9cd28238ebefa2d50
* fix English in release notes * Simplify `switch` to `if`.
* fix English in release notes * Simplify `switch` to `if`.
Author of #21245 expressed time issues 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 and a few nits of mine.
Original PR description
Possible improvements
Examples
Examples of the
getblock
output with various verbose levels. Note that000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5
contains only 2 transactions.(See: #21245 (comment))
Verbose level 0
Verbose level 1
Verbose level 2
Verbose level 3
REST
curl -H "content-type:text/plain;" http://127.0.0.1:18332/rest/block/000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5.json
* ... and my everyday obsessive checking of my email inbox whether the PR moves forward.
Edit laanwj: Removed at symbol from message, and large example output to prevent it from all ending up in the commit message.