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

pull kiminuo wants to merge 6 commits into bitcoin:master from kiminuo:feature/2021-09-verbose-level-3-for-getblock changing 9 files +148 −45
  1. 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

    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

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

  2. fanquake cross-referenced this on Sep 8, 2021 from issue rpc: Add level 3 verbosity to getblock RPC call. by fyquah
  3. 0xB10C commented at 11:38 AM on September 8, 2021: contributor

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

  4. DrahtBot commented at 1:14 PM on September 8, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  5. DrahtBot added the label RPC/REST/ZMQ on Sep 8, 2021
  6. DrahtBot cross-referenced this on Sep 8, 2021 from issue rpc: deprecate top-level fee fields in getmempool RPCs by josibake
  7. DrahtBot cross-referenced this on Sep 8, 2021 from issue Remove -deprecatedrpc=addresses flag and corresponding code/logic by mjdietzx
  8. DrahtBot cross-referenced this on Sep 15, 2021 from issue refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager` by dongcarl
  9. DrahtBot cross-referenced this on Sep 15, 2021 from issue doc: Fix incorrect C++ named args by MarcoFalke
  10. 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.

  11. laanwj added the label Feature on Sep 16, 2021
  12. luke-jr approved
  13. luke-jr commented at 2:11 AM on September 21, 2021: member

    utACK 305a59a7b55e2507f66729c9a42d5e8fd1cf23a7

  14. DrahtBot added the label Needs rebase on Sep 28, 2021
  15. kiminuo force-pushed on Sep 29, 2021
  16. DrahtBot removed the label Needs rebase on Sep 29, 2021
  17. DrahtBot cross-referenced this on Sep 30, 2021 from issue consensus: move amount.h into consensus by fanquake
  18. DrahtBot cross-referenced this on Sep 30, 2021 from issue refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag by mjdietzx
  19. DrahtBot cross-referenced this on Oct 1, 2021 from issue rpc: various fixups for dumptxoutset by jamesob
  20. DrahtBot cross-referenced this on Oct 2, 2021 from issue assumeutxo by jamesob
  21. kiminuo force-pushed on Oct 4, 2021
  22. DrahtBot added the label Needs rebase on Oct 5, 2021
  23. 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
  24. 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
  25. rpc: Add test for level 3 verbosity getblock rpc call. 4330af6f72
  26. rest: Add test for prevout fields in getblock 459104b2aa
  27. release-notes: Add release note about getblock verbosity level 3. 8edf6204a8
  28. core_write: Rename calculate_fee to have_undo for clarity 5c34507ecb
  29. kiminuo force-pushed on Oct 5, 2021
  30. DrahtBot removed the label Needs rebase on Oct 5, 2021
  31. kiminuo commented at 8:59 PM on October 6, 2021: contributor

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

  32. luke-jr approved
  33. luke-jr commented at 10:32 PM on October 6, 2021: member

    re-utACK b0f7af35481c1ed6db4f20cec41443f9c0159f2b

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

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

  36. 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?

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

  38. luke-jr referenced this in commit ab615b119c on Oct 10, 2021
  39. luke-jr referenced this in commit 600468b246 on Oct 10, 2021
  40. luke-jr referenced this in commit c272b5c7f6 on Oct 10, 2021
  41. luke-jr referenced this in commit 369b3e4c6b on Oct 10, 2021
  42. kiminuo commented at 6:38 AM on October 11, 2021: contributor

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

  43. 0xB10C commented at 3:03 PM on October 11, 2021: contributor

    ACK 5c34507ecbbdc29c086276d1c62835b461823507

  44. 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:

        } // 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)

    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.

  45. theStack approved
  46. theStack commented at 8:34 AM on October 12, 2021: contributor

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

  47. 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;?

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

    if (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)

  49. promag commented at 8:57 AM on October 12, 2021: member

    Concept ACK 5c34507ecbbdc29c086276d1c62835b461823507

  50. 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?

  51. laanwj merged this on Oct 19, 2021
  52. laanwj closed this on Oct 19, 2021
  53. 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.

  54. sidhujag referenced this in commit 20541b82b7 on Oct 19, 2021
  55. 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).

  56. kiminuo deleted the branch on Oct 20, 2021
  57. kiminuo referenced this in commit 1dbafcb484 on Oct 20, 2021
  58. kiminuo cross-referenced this on Oct 20, 2021 from issue rpc: Add RPC help for getblock verbosity level 3 by kiminuo
  59. kiminuo referenced this in commit 1bdd5f6322 on Oct 20, 2021
  60. 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

  61. laanwj referenced this in commit 66be456d93 on Jan 4, 2022
  62. sidhujag referenced this in commit 7639e6ec1c on Jan 4, 2022
  63. niVelion cross-referenced this on Jan 14, 2022 from issue Pick up "transaction fees in getblock" pull request by MarcoFalke
  64. mzumsande referenced this in commit f8d984f5b4 on Jan 17, 2022
  65. rebroad referenced this in commit cb0ef94c51 on Feb 3, 2022
  66. achow101 cross-referenced this on Oct 26, 2022 from issue [RPC] Getting the spent transaction outputs of transactions in latest blocks by NicolasDorier
  67. bitcoin locked this on Oct 30, 2022
Contributors
kiminuo0xB10CDrahtBotlaanwjluke-jrmeshcolliderjonatacktheStackpromagfanquakemjdietzx
Labels
FeatureRPC/REST/ZMQ
Linked (view graph)
#18771 Pick up "transaction fees in getblock" pull request#19003 [RPC] Getting the spent transaction outputs of transactions in latest blocks#21245 rpc: Add level 3 verbosity to getblock RPC call.#23320 rpc: Add RPC help for getblock verbosity level 3
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:53 UTC