rpc: Add level 3 verbosity to getblock RPC call (#21245 modified)

kiminuo commented at 11:10 am on September 8, 2021: contributor

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

https://github.com/kiminuo/bitcoin/commit/b0bf4f255f86aeaddce68889087c22f9068f4d97 - I can include even this commit to this PR if deemed useful or I can leave it for a follow-up PR. See #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

0./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 0

Verbose level 1

0./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 1

Verbose level 2

0./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 2

Verbose level 3

0./bitcoin-cli -testnet getblock 000000000000001f682b188971cc1a121546be4e9d5baf22934fdc7f538288d5 3

REST

0curl -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 commented at 11:38 am on September 8, 2021: member

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

DrahtBot commented at 1:14 pm on September 8, 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:

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

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

laanwj commented at 2:52 pm on September 16, 2021: member

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

laanwj added the label Feature on Sep 16, 2021

luke-jr approved

luke-jr commented at 2:11 am on September 21, 2021: member

utACK 305a59a7b55e2507f66729c9a42d5e8fd1cf23a7

DrahtBot added the label Needs rebase on Sep 28, 2021

kiminuo force-pushed on Sep 29, 2021

DrahtBot removed the label Needs rebase on Sep 29, 2021

kiminuo force-pushed on Oct 4, 2021

DrahtBot added the label Needs rebase on Oct 5, 2021

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

Co-authored-by: Luke Dashjr <luke_github1@dashjr.org>
Co-authored-by: 0xB10C <19157360+0xB10C@users.noreply.github.com>

3cc95345ca

rpc: Add level 3 verbosity to getblock RPC call.

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>

51dbc167e9

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

rest: Add test for prevout fields in getblock 459104b2aa

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

core_write: Rename calculate_fee to have_undo for clarity 5c34507ecb

kiminuo force-pushed on Oct 5, 2021

DrahtBot removed the label Needs rebase on Oct 5, 2021

kiminuo commented at 8:59 pm on October 6, 2021: contributor

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

luke-jr approved

luke-jr commented at 10:32 pm on October 6, 2021: member

re-utACK b0f7af35481c1ed6db4f20cec41443f9c0159f2b

in doc/release-notes.md:79 in 8edf6204a8 outdated

75@@ -76,6 +76,14 @@ Updated RPCs
76   `gettransaction verbose=true` and REST endpoints `/rest/tx`, `/rest/getutxos`,
77   `/rest/block` no longer return the `addresses` and `reqSigs` fields, which
78   were previously deprecated in 22.0. (#22650)
79+- The `getblock` RPC command now supports verbose level 3 containing transaction inputs

meshcollider commented at 11:57 am on October 8, 2021:

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

kiminuo commented at 6:39 am on October 11, 2021:

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

meshcollider commented at 12:00 pm on October 8, 2021: contributor

utACK 5c34507ecbbdc29c086276d1c62835b461823507

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.

kiminuo commented at 12:24 pm on October 8, 2021: contributor

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 https://github.com/kiminuo/bitcoin/commit/b0bf4f255f86aeaddce68889087c22f9068f4d97 mentioned in the OP (under Possible improvements)? Or what do you mean by helptext, please?

kiminuo commented at 12:28 pm on October 8, 2021: contributor

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

luke-jr referenced this in commit ab615b119c on Oct 10, 2021

luke-jr referenced this in commit 600468b246 on Oct 10, 2021

luke-jr referenced this in commit c272b5c7f6 on Oct 10, 2021

luke-jr referenced this in commit 369b3e4c6b on Oct 10, 2021

kiminuo commented at 6:38 am on October 11, 2021: contributor

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

0xB10C commented at 3:03 pm on October 11, 2021: member

ACK 5c34507ecbbdc29c086276d1c62835b461823507

in src/core_write.cpp:227 in 5c34507ecb

224+                    p.pushKV("height", uint64_t(prev_coin.nHeight));
225+                    p.pushKV("value", ValueFromAmount(prev_txout.nValue));
226+                    p.pushKV("scriptPubKey", o_script_pub_key);
227+                    in.pushKV("prevout", p);
228+                    break;
229+            }

jonatack commented at 3:43 pm on October 11, 2021:

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:

0    } // no default case, so the compiler can warn about missing cases
1    assert(false);
2}

(usually, the case statements have the same indentation as switch, without blank lines between them)

kiminuo commented at 5:21 pm on October 11, 2021:

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

theStack approved

theStack commented at 8:34 am on October 12, 2021: member

ACK 5c34507ecbbdc29c086276d1c62835b461823507 👘

Verified via git range-diff 72dbe981...5c34507e that since my last ACK (review done in PR #21245, see #21245#pullrequestreview-724950830) all changes are rebase-related or minor improvements (related to comments, variable renames etc.).

in src/rpc/blockchain.cpp:230 in 3cc95345ca outdated

241+                // coinbase transaction (i.e. i == 0) doesn't have undo data
242+                const CTxUndo* txundo = (have_undo && i) ? &blockUndo.vtxundo.at(i - 1) : nullptr;
243+                UniValue objTx(UniValue::VOBJ);
244+                TxToUniv(*tx, uint256(), objTx, true, RPCSerializationFlags(), txundo);
245+                txs.push_back(objTx);
246+            }

promag commented at 8:47 am on October 12, 2021:

3cc95345ca49b87e8caca9a0e6418c63ae1e463a

nit, add break;?

in src/core_write.cpp:211 in 51dbc167e9 outdated

203@@ -204,8 +204,27 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry,
204             in.pushKV("txinwitness", txinwitness);
205         }
206         if (calculate_fee) {
207-            const CTxOut& prev_txout = txundo->vprevout[i].out;
208+            const Coin& prev_coin = txundo->vprevout[i];
209+            const CTxOut& prev_txout = prev_coin.out;
210+
211             amt_total_in += prev_txout.nValue;
212+            switch (verbosity) {

promag commented at 8:52 am on October 12, 2021:

51dbc167e98daab317baa80cf80bfda337672dab

nit, we only care about one value, what’s wrong with

0if (verbosity == TxVerbosity::SHOW_DETAILS_AND_PREVOUT) {

If you do this, then declare prev_coin in the inner scope.

kiminuo commented at 10:36 am on October 12, 2021:

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

kiminuo commented at 11:35 am on October 12, 2021:

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

promag commented at 8:57 am on October 12, 2021: member

Concept ACK 5c34507ecbbdc29c086276d1c62835b461823507

kiminuo commented at 5:53 am on October 13, 2021: contributor

@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 merged this on Oct 19, 2021

laanwj closed this on Oct 19, 2021

laanwj commented at 2:20 pm on October 19, 2021: member

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.

sidhujag referenced this in commit 20541b82b7 on Oct 19, 2021

fanquake commented at 5:33 am on October 20, 2021: member

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

kiminuo deleted the branch on Oct 20, 2021

kiminuo referenced this in commit 1dbafcb484 on Oct 20, 2021

kiminuo referenced this in commit 1bdd5f6322 on Oct 20, 2021

mjdietzx commented at 3:56 pm on October 25, 2021: contributor

Post merge ACK 5c34507ecbbdc29c086276d1c62835b461823507

Did some review and light testing while rebasing #22924

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

laanwj referenced this in commit 66be456d93 on Jan 4, 2022

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

mzumsande referenced this in commit f8d984f5b4 on Jan 17, 2022

rebroad referenced this in commit cb0ef94c51 on Feb 3, 2022

DrahtBot locked this on Oct 30, 2022

rpc: Add level 3 verbosity to getblock RPC call (#21245 modified) #22918