rpc: Add level 3 verbosity to getblock RPC call (#21245 modified) by kiminuo · Pull Request #22918 · bitcoin/bitcoin

kiminuo · 2021-09-08T11:10:06Z

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

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@b0bf4f2 - I can include even this commit to this PR if deemed useful or I can leave it for a follow-up PR. See rpc: Add level 3 verbosity to getblock RPC call. #21245 (comment) for more context.

Examples

Examples of the getblock output with various verbose levels. Note that 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 contains only 2 transactions.

(See: #21245 (comment))

Verbose level 0

./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 0

Verbose level 1

./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 1

Verbose level 2

./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 2

Verbose level 3

./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 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.

0xB10C · 2021-09-08T11:38:41Z

Concept ACK. Thanks for picking this up. Will review now that the requested changes are in.

DrahtBot · 2021-09-08T13:14:05Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22981 (doc: Fix incorrect C++ named args by MarcoFalke)
#22924 (refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag by mjdietzx)
#22564 ([WIP] refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl)

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.

laanwj · 2021-09-16T14:52:41Z

Concept ACK, this is a useful feature to have, an interesting approach to use the undo data.

luke-jr

utACK 305a59a7b55e2507f66729c9a42d5e8fd1cf23a7

Co-authored-by: Luke Dashjr <[email protected]> Co-authored-by: 0xB10C <[email protected]>

Display the prevout in transaction inputs when calling getblock level 3 verbosity. Co-authored-by: Luke Dashjr <[email protected]> Co-authored-by: 0xB10C <[email protected]>

kiminuo · 2021-10-06T20:59:23Z

@luke-jr Could you please re-ACK if you feel like it?

luke-jr

re-utACK b0f7af35481c1ed6db4f20cec41443f9c0159f2b

meshcollider

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.

meshcollider · 2021-10-08T11:57:05Z

doc/release-notes.md

  `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


nit: verbose -> verbosity and inputs -> inputs'

Thank you. I will fix in a next rebase or in a follow-up PR.

kiminuo · 2021-10-08T12:24:41Z

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.

Do you possibly mean kiminuo@b0bf4f2 mentioned in the OP (under Possible improvements)? Or what do you mean by helptext, please?

kiminuo · 2021-10-08T12:28:16Z

re-utACK b0f7af3

@luke-jr Ah, you acked previous version, not the current one.

Co-authored-by: Luke Dashjr <[email protected]> Co-authored-by: 0xB10C <[email protected]> Github-Pull: bitcoin#22918 Rebased-From: 6cd4414096438958a768151d7d4661395fa5ca98

Display the prevout in transaction inputs when calling getblock level 3 verbosity. Co-authored-by: Luke Dashjr <[email protected]> Co-authored-by: 0xB10C <[email protected]> Github-Pull: bitcoin#22918 Rebased-From: dda6ef11edbccc14d25dd7dd850755931855d2c9

Github-Pull: bitcoin#22918 Rebased-From: cc529a12fc54d749243c78c55492b1b1ed47f4a8

Github-Pull: bitcoin#22918 Rebased-From: 83dfadb25a0b0ed65c54058a18569c5052679c2a

kiminuo · 2021-10-11T06:38:41Z

@theStack Would you please re-ACK if you feel like it?

0xB10C · 2021-10-11T15:03:19Z

ACK 5c34507

jonatack · 2021-10-11T15:43:25Z

src/core_write.cpp

+                    p.pushKV("scriptPubKey", o_script_pub_key);
+                    in.pushKV("prevout", p);
+                    break;
+            }


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)

Thank you. I would like to address that a next rebase or in a follow-up PR.

theStack

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.).

promag

Concept ACK 5c34507

promag · 2021-10-12T08:47:54Z

src/rpc/blockchain.cpp

+                UniValue objTx(UniValue::VOBJ);
+                TxToUniv(*tx, uint256(), objTx, true, RPCSerializationFlags(), txundo);
+                txs.push_back(objTx);
+            }


3cc9534

nit, add break;?

promag · 2021-10-12T08:52:42Z

src/core_write.cpp

+            const CTxOut& prev_txout = prev_coin.out;
+
            amt_total_in += prev_txout.nValue;
+            switch (verbosity) {


51dbc16

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.

Thank you. I would like to address that a next rebase or in a follow-up PR.

I have actually proposed the same in the original PR: #21245 (comment)

kiminuo · 2021-10-13T05:53:35Z

@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?

laanwj · 2021-10-19T14:20:38Z

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.

…itcoin#21245 …

fanquake · 2021-10-20T05:33:36Z

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`.

mjdietzx · 2021-10-25T15:56:55Z

Post merge ACK 5c34507

Did some review and light testing while rebasing #22924

I'll be joining in on review of #23320 bc I also had some nits

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`.

fanquake mentioned this pull request Sep 8, 2021

rpc: Add level 3 verbosity to getblock RPC call. #21245

Closed

DrahtBot added the RPC/REST/ZMQ label Sep 8, 2021

This was referenced Sep 8, 2021

rpc: deprecate top-level fee fields in getmempool RPCs #22689

Merged

Remove -deprecatedrpc=addresses flag and corresponding code/logic #22650

Merged

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

DrahtBot mentioned this pull request Sep 15, 2021

doc: Fix incorrect C++ named args #22981

Merged

laanwj added the Feature label Sep 16, 2021

luke-jr approved these changes Sep 21, 2021

View reviewed changes

DrahtBot added the Needs rebase label Sep 28, 2021

kiminuo force-pushed the feature/2021-09-verbose-level-3-for-getblock branch from 305a59a to d4d043b Compare September 29, 2021 19:40

DrahtBot removed the Needs rebase label Sep 29, 2021

This was referenced Sep 30, 2021

consensus: move amount.h into consensus #22951

Merged

refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag #22924

Closed

rpc: various fixups for dumptxoutset #23155

Merged

assumeutxo #15606

Closed

kiminuo force-pushed the feature/2021-09-verbose-level-3-for-getblock branch from d4d043b to b0f7af3 Compare October 4, 2021 08:06

DrahtBot added the Needs rebase label Oct 5, 2021

fyquah and others added 6 commits October 5, 2021 10:42

rpc: Replace boolean argument for tx details with enum class.

3cc9534

Co-authored-by: Luke Dashjr <[email protected]> Co-authored-by: 0xB10C <[email protected]>

rpc: Add level 3 verbosity to getblock RPC call.

51dbc16

Display the prevout in transaction inputs when calling getblock level 3 verbosity. Co-authored-by: Luke Dashjr <[email protected]> Co-authored-by: 0xB10C <[email protected]>

rpc: Add test for level 3 verbosity getblock rpc call.

4330af6

rest: Add test for prevout fields in getblock

459104b

release-notes: Add release note about getblock verbosity level 3.

8edf620

core_write: Rename calculate_fee to have_undo for clarity

5c34507

kiminuo force-pushed the feature/2021-09-verbose-level-3-for-getblock branch from b0f7af3 to 5c34507 Compare October 5, 2021 08:42

DrahtBot removed the Needs rebase label Oct 5, 2021

luke-jr approved these changes Oct 6, 2021

View reviewed changes

meshcollider reviewed Oct 8, 2021

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021

rpc: Add test for level 3 verbosity getblock rpc call.

c272b5c

Github-Pull: bitcoin#22918 Rebased-From: cc529a12fc54d749243c78c55492b1b1ed47f4a8

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021

rest: Add test for prevout fields in getblock

369b3e4

Github-Pull: bitcoin#22918 Rebased-From: 83dfadb25a0b0ed65c54058a18569c5052679c2a

jonatack reviewed Oct 11, 2021

View reviewed changes

theStack approved these changes Oct 12, 2021

View reviewed changes

promag reviewed Oct 12, 2021

View reviewed changes

laanwj merged commit 986003a into bitcoin:master Oct 19, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 19, 2021

Merge bitcoin#22918: rpc: Add level 3 verbosity to getblock RPC call (b…

20541b8

…itcoin#21245 …

kiminuo deleted the feature/2021-09-verbose-level-3-for-getblock branch October 20, 2021 05:46

kiminuo mentioned this pull request Oct 20, 2021

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

Merged

kiminuo added a commit to kiminuo/bitcoin that referenced this pull request Oct 20, 2021

Address review comments from bitcoin#22918

1bdd5f6

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

niVelion mentioned this pull request Jan 14, 2022

Pick up "transaction fees in getblock" pull request #18771

Closed

rebroad pushed a commit to rebroad/bitcoin that referenced this pull request Feb 3, 2022

Address review comments from bitcoin#22918

cb0ef94

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

achow101 mentioned this pull request Oct 26, 2022

[RPC] Getting the spent transaction outputs of transactions in latest blocks #19003

Closed

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Conversation

kiminuo commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Original PR description

Possible improvements

Examples

Verbose level 0

Verbose level 1

Verbose level 2

Verbose level 3

REST

Uh oh!

0xB10C commented Sep 8, 2021

Uh oh!

DrahtBot commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Sep 16, 2021

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

kiminuo commented Oct 6, 2021

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

kiminuo Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

kiminuo commented Oct 8, 2021

Uh oh!

kiminuo commented Oct 8, 2021

Uh oh!

kiminuo commented Oct 11, 2021

Uh oh!

0xB10C commented Oct 11, 2021

Uh oh!

jonatack Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiminuo Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

promag Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

kiminuo Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

kiminuo Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

kiminuo commented Oct 13, 2021

Uh oh!

laanwj commented Oct 19, 2021

Uh oh!

fanquake commented Oct 20, 2021

Uh oh!

mjdietzx commented Oct 25, 2021

Uh oh!

Reviewers

Assignees

kiminuo commented Sep 8, 2021 •

edited

Loading

DrahtBot commented Sep 8, 2021 •

edited

Loading

jonatack Oct 11, 2021 •

edited

Loading