rpc, test: Improve getblockstats for unspendables #19888

pull fjahr wants to merge 4 commits into bitcoin:master from fjahr:genesisblockstats changing 6 files +115 −53
  1. fjahr commented at 10:25 pm on September 5, 2020: contributor

    Fixes #19885

    The genesis block does not have undo data saved to disk so the RPC errored because of that.

  2. fanquake added the label RPC/REST/ZMQ on Sep 5, 2020
  3. in test/functional/rpc_getblockstats.py:165 in 3b40c6f365 outdated 
    159@@ -160,6 +160,9 @@ def run_test(self):
160         assert_raises_rpc_error(-1, 'getblockstats hash_or_height ( stats )', self.nodes[0].getblockstats, '00', 1, 2)
161         assert_raises_rpc_error(-1, 'getblockstats hash_or_height ( stats )', self.nodes[0].getblockstats)
162 
163+        self.log.info('Test block height 0')
164+        genesis_stats = self.nodes[0].getblockstats(0)
165+        assert_equal(genesis_stats["subsidy"], 5000000000)
    n-thumann commented at 10:47 pm on September 5, 2020: 
    0        assert_equal(genesis_stats["blockhash"], "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206")

    I would suggest making the check more strict by checking for the exact expected blockhash, not just the subsidy which is the same in the next blocks ✌️

    fjahr commented at 10:59 pm on September 5, 2020:
    done
  4. fjahr force-pushed on Sep 5, 2020
  5. n-thumann approved
  6. n-thumann commented at 11:30 pm on September 5, 2020: contributor
    tACK bb608b7 on master at 3ba25e3, ran functional & unit tests, confirmed that bug is fixed now :)
  7. in src/rpc/blockchain.cpp:819 in bb608b76bf outdated 
    814@@ -815,8 +815,11 @@ static CBlockUndo GetUndoChecked(const CBlockIndex* pblockindex)
815         throw JSONRPCError(RPC_MISC_ERROR, "Undo data not available (pruned data)");
816     }
817 
818-    if (!UndoReadFromDisk(blockUndo, pblockindex)) {
819-        throw JSONRPCError(RPC_MISC_ERROR, "Can't read undo data from disk");
820+    // The Genesis block does not have undo data
821+    if (pblockindex->nHeight > 0) {
    promag commented at 11:35 pm on September 5, 2020:
    Could early return before prune check?
    fjahr commented at 11:20 am on September 6, 2020:
    yepp, done!
  8. fjahr force-pushed on Sep 6, 2020
  9. maflcko commented at 12:26 pm on September 6, 2020: member
    The genesis block has no effect on the utxo set, so the returned result is wrong
  10. fjahr force-pushed on Sep 6, 2020
  11. fjahr commented at 11:44 pm on September 6, 2020: contributor

    The genesis block has no effect on the utxo set, so the returned result is wrong

    Heh, yeah, the whole RPC actually didn’t deal with unspendables at all so far. I am fixing this with my latest changes, fixing the Genesis block but also BIP30 and unspendable outputs. I think the numbers make much more sense this way. I also needed to regenerate the test data because the witness commitment is now excluded from the stats.

  12. fjahr renamed this:
    rpc: Fix getblockstats for block height 0
    rpc: Fix getblockstats issues
    on Sep 7, 2020
  13. theStack commented at 2:43 pm on September 12, 2020: contributor
    Concept ACK Both enabling getblockstats to work with the genesis block and improving the accuracy of the utxo set statistics make big sense to me.
  14. DrahtBot commented at 1:13 am on October 2, 2020: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #26308 (rpc/rest/zmq: reduce LOCK(cs_main) scope: ~6 times as many requests per second by andrewtoth)
    • #25740 (assumeutxo: background validation completion by jamesob)
    • #24008 (assumeutxo: net_processing changes by jamesob)
    • #15606 (assumeutxo by jamesob)

    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.

  15. in src/rpc/blockchain.cpp:1750 in c266571561 outdated 
    1746@@ -1743,7 +1747,7 @@ static UniValue getblockstats(const JSONRPCRequest& request)
1747                 {RPCResult::Type::NUM, "totalfee", "The fee total"},
1748                 {RPCResult::Type::NUM, "txs", "The number of transactions (including coinbase)"},
1749                 {RPCResult::Type::NUM, "utxo_increase", "The increase/decrease in the number of unspent outputs"},
1750-                {RPCResult::Type::NUM, "utxo_size_inc", "The increase/decrease in size for the utxo index (not discounting op_return and similar)"},
1751+                {RPCResult::Type::NUM, "utxo_size_inc", "The increase/decrease in size for the utxo index"},
    luke-jr commented at 3:01 pm on October 24, 2020:
    With this change to the documented API, I think you’d better add a new field instead…
    fjahr commented at 1:31 pm on December 24, 2020:
    Slightly would have preferred to just fix the existing values but I have now introduced new values with a *_actual naming scheme.
  16. in src/rpc/blockchain.cpp:1836 in c266571561 outdated 
    1828@@ -1824,14 +1829,27 @@ static UniValue getblockstats(const JSONRPCRequest& request)
1829     std::vector<std::pair<CAmount, int64_t>> feerate_array;
1830     std::vector<int64_t> txsize_array;
1831 
1832+    // The Genesis block and the repeated BIP30 blocks don't change the UTXO
1833+    // set, so they can be excluded from the statistics
1834+    const bool is_genesis = pindex->nHeight == 0;
1835+    const bool is_bip30_repeat = (pindex->nHeight == 91880 && pindex->GetBlockHash() == uint256S("0x00000000000743f190a18c5577a3c2d2a1f610ae9601ac046a38084ccb7cd721")) ||
1836+                                 (pindex->nHeight == 91842 && pindex->GetBlockHash() == uint256S("0x00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec"));
    luke-jr commented at 3:03 pm on October 24, 2020:
    It would be nice to abstract this to a block-aware IsUnspendable instead.
    laanwj commented at 5:57 pm on December 17, 2020:
    +1, it would be better not to repeat this in multiple places
    fjahr commented at 1:27 pm on December 24, 2020:
    Right, but it’s a bit more complicated because these blocks’ coinbases are not actually unspendable, they are the ones that overwrote the other previous coinbases aifaik. But I have refactored both of these out into helper functions that would also be helpful to me for Coinstatsindex. Let me know what you think about the naming, ‘repeat’ was the best I came up with.
  17. in test/functional/rpc_getblockstats.py:172 in c266571561 outdated 
    159@@ -160,6 +160,9 @@ def run_test(self):
160         assert_raises_rpc_error(-1, 'getblockstats hash_or_height ( stats )', self.nodes[0].getblockstats, '00', 1, 2)
161         assert_raises_rpc_error(-1, 'getblockstats hash_or_height ( stats )', self.nodes[0].getblockstats)
162 
163+        self.log.info('Test block height 0')
164+        genesis_stats = self.nodes[0].getblockstats(0)
165+        assert_equal(genesis_stats["blockhash"], "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206")
    luke-jr commented at 3:05 pm on October 24, 2020:
    Would be nice to test the other corner-case result values
    fjahr commented at 1:34 pm on December 24, 2020:
    I am not sure if you had more values in mind (let me know) but I have added the old and new values that this PR touches.
    fjahr commented at 7:16 pm on April 11, 2021:
    Added a test with an OP_RETURN output.
    ryanofsky commented at 4:27 pm on February 6, 2022:

    re: #19888 (review)

    Added a test with an OP_RETURN output.

    Note: This is referring to commit f87d4b9594aa1005a9a2004630b097176035ca32 “Added a test with an OP_RETURN output” I think

  18. luke-jr changes_requested
  19. fjahr force-pushed on Dec 24, 2020
  20. fjahr commented at 1:35 pm on December 24, 2020: contributor
    Addressed feedback comments. Thanks for reviewing and sorry for the long wait!
  21. DrahtBot added the label Needs rebase on Feb 16, 2021
  22. in test/functional/data/rpc_getblockstats.json:105 in 37bbfe9667 outdated 
    101@@ -102,8 +102,8 @@
102     "00000020f44e7a48b9f221af95f3295c8dcefc5358934a68dc79e2933dc0794b350cad0a90fad2cd50b41d4ef45e76c2a456b98c180632bb4b44e0cd18ce90679fe54e552b4ae75affff7f200000000001020000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff0401630101ffffffff0200f2052a010000001976a9142b4569203694fc997e13f2c0a1383b9e16c77a0d88ac0000000000000000266a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf90120000000000000000000000000000000000000000000000000000000000000000000000000",
103     "0000002087454276cce83f4d19e0120f6e9728ac5905f7adaf6b27e3f5bbe43ab823f85db7d1f44666531483df3d67c15f2c231718ad93b63b851dce5ff4c4a67f524ffa2b4ae75affff7f200100000001020000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff0401640101ffffffff0200f2052a010000001976a9142b4569203694fc997e13f2c0a1383b9e16c77a0d88ac0000000000000000266a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf90120000000000000000000000000000000000000000000000000000000000000000000000000",
104     "000000202cdc3e99f07a80252dd6097faa0eddf3f2dde5ae390610e0bca94ecc25931551d31fceb8fe0a682f6017ca3dbb582f3a2f06e5d99ec99c42c8a744dd4c9216b82b4ae75affff7f200300000001020000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff0401650101ffffffff0200f2052a010000001976a9142b4569203694fc997e13f2c0a1383b9e16c77a0d88ac0000000000000000266a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf90120000000000000000000000000000000000000000000000000000000000000000000000000",
105-    "000000209b3ace9bd510918d20e87518c0cf5976cab3e28cc7af41259a89c6dd7668a32922808b8a082be71bcd6152cb8fd223650b5579a41344ba749e4d17b9bf211a9e2b4ae75affff7f200000000002020000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff0401660101ffffffff026c03062a010000001976a9142b4569203694fc997e13f2c0a1383b9e16c77a0d88ac0000000000000000266a24aa21a9edb85d8f3c122c43a72f1e0dd122c8f7af040aa0b0a46001621110fb37818021510120000000000000000000000000000000000000000000000000000000000000000000000000020000000128394022bf44bff30d7399cb5a16e3b94fed67dc174c2e1d77df91bad5a51cb3000000006a47304402201c16d06a5c4353168b3881071aea7d1eb4d88eedfea53a9d6af9abb56da9060002205abf3ae535f1f1b5cfe8ba955535c2b20ac003e7d7720c5b7d2640ac2a04d19001210227d85ba011276cf25b51df6a188b75e604b38770a462b2d0e9fb2fc839ef5d3ffeffffff0294b89a3b000000001976a9142b4569203694fc997e13f2c0a1383b9e16c77a0d88ac00286bee0000000017a91452bab4f229415d0dc5c6d30b162f93a1a0cac5958765000000",
    luke-jr commented at 4:20 am on February 28, 2021:
    Can you retain the old test?
    fjahr commented at 7:15 pm on April 11, 2021:
    Done now in this particular commit but now need to regenerate it in another commit :-/
  23. fjahr force-pushed on Apr 11, 2021
  24. fjahr commented at 7:12 pm on April 11, 2021: contributor

    Finally got back to give this the attention it deserves. First of all, it is rebased. Then I followed some recommendation from @luke-jr which also let me to find some other issue.

    Here is a recap per commit to make it easier to understand what changed:

    c7ccc0731bae0f263cc9776072dad05462ec8af5: (unchanged) Refactors BIP30 logic into helper functions. This will also be helpful for #19521 where the logic is currently duplicated as well. 5d0649c1f093e2419cae81f738e9534af6ce56b7: Fixes getblockstats behavior for unspendable statistics (genesis, BIP30, OP_RETURN). Here I now followed Luke’s suggestion and retain the old test. 50c956f0a2e659cd97499fbc6780e9eb4bbaf0c2: While making changes and testing I noticed that the data generator is currently broken since the default wallet was removed, this is fixed here. I initially used MiniWallet but it would have been a much larger change (mostly due to the next commit) and since this is generator code it’s not critical I think. 1c18449b200018c26c911b4c90a9f4687f804c3e: Following Luke’s request to test more corner cases I added a test with an OP_RETURN output. But this made it impractical to keep the current test, so the test data is now regenerated in this commit.

  25. fjahr renamed this:
    rpc: Fix getblockstats issues
    rpc, test: Improve getblockstats for unspendables
    on Apr 11, 2021
  26. DrahtBot removed the label Needs rebase on Apr 11, 2021
  27. DrahtBot added the label Needs rebase on May 31, 2021
  28. fjahr force-pushed on May 31, 2021
  29. DrahtBot removed the label Needs rebase on May 31, 2021
  30. fjahr commented at 9:30 pm on May 31, 2021: contributor
    rebased
  31. in src/validation.cpp:5202 in 0af88a85e5 outdated 
    5197+    return (pindex->nHeight==91722 && pindex->GetBlockHash() == uint256S("0x00000000000271a2dc26e7667f8419f2e15416dc6955e5a6c6cdf3f2574dd08e")) ||
5198+           (pindex->nHeight==91812 && pindex->GetBlockHash() == uint256S("0x00000000000af0aed4792b1acee3d966af36cf5def14935db8de83d6f9306f2f"));
5199+}
5200+
5201+bool IsUnspendableCoinbase(const CBlockIndex* pindex) {
5202+    return (IsBIP30Unspendable(pindex) || (pindex->nHeight == 0 && pindex == ::ChainActive().Genesis()));
    luke-jr commented at 1:26 am on August 13, 2021:
    Invisible conflict here. But nothing seems to use this function anyway…?
    fjahr commented at 10:13 pm on August 24, 2021:
    Ah, thanks! I had initially planned to use this in coinstatsindex to simplify the code there but since I wrote this helper the code in the index has changed a lot and so now this is not helpful anymore and will be using IsBIP30Unspendable directly instead. So I removed this method now and added a commit solving the code dedublication Todo in coinstatsindex.
  32. fjahr force-pushed on Aug 24, 2021
  33. fjahr force-pushed on Aug 24, 2021
  34. fjahr commented at 10:15 pm on August 24, 2021: contributor

    Rebased, fixed silent conflict, and added a commit using IsBIP30Unspendable to deduplicate the BIP30 code in coinstatsindex.

    EDIT: plus another push to fix typo in commit message

  35. fjahr force-pushed on Aug 24, 2021
  36. in src/validation.cpp:5055 in 25fef33c93 outdated 
    5050@@ -5052,3 +5051,13 @@ void ChainstateManager::MaybeRebalanceCaches()
5051         }
5052     }
5053 }
5054+
5055+bool IsBIP30Repeat(const CBlockIndex* pindex) {
    maflcko commented at 8:35 am on August 26, 2021:

    style-nit:

    0bool IsBIP30Repeat(const CBlockIndex& block_index)
1{
    fjahr commented at 9:30 am on August 26, 2021:
    done
  37. fjahr force-pushed on Aug 26, 2021
  38. fjahr force-pushed on Aug 26, 2021
  39. DrahtBot added the label Needs rebase on Sep 9, 2021
  40. fjahr force-pushed on Sep 9, 2021
  41. fjahr commented at 11:23 pm on September 9, 2021: contributor
    rebased
  42. DrahtBot removed the label Needs rebase on Sep 9, 2021
  43. DrahtBot added the label Needs rebase on Sep 23, 2021
  44. fjahr force-pushed on Sep 23, 2021
  45. fjahr commented at 11:49 pm on September 23, 2021: contributor

    rebased

    Edit: Hm, CI failure looks unrelated

  46. DrahtBot removed the label Needs rebase on Sep 24, 2021
  47. luke-jr referenced this in commit 54b9c6788a on Oct 10, 2021
  48. luke-jr referenced this in commit f94e11c19b on Oct 10, 2021
  49. luke-jr referenced this in commit e3c2c442c8 on Oct 10, 2021
  50. luke-jr referenced this in commit 66d59e4c44 on Oct 10, 2021
  51. DrahtBot added the label Needs rebase on Dec 9, 2021
  52. fjahr force-pushed on Dec 10, 2021
  53. fjahr commented at 0:21 am on December 10, 2021: contributor
    rebased
  54. DrahtBot removed the label Needs rebase on Dec 10, 2021
  55. ghost commented at 9:01 am on January 15, 2022: none
    Concept ACK. Will test today.
  56. unknown approved
  57. unknown commented at 10:33 am on January 15, 2022: none

    tACK only for https://github.com/bitcoin/bitcoin/pull/19888/commits/97f4eb50b590422678f0ad5bb4f61cc45c97dd87

    Neither I have reviewed other commits nor have any opinion. Read one SE Q&A, created a PR to add another error for genesis block and found this PR fixes it in a better way.

    Master Branch:

    0bitcoin-cli getblockstats 0
1
2error code: -1
3error message:
4Can't read undo data from disk

    PR Branch:

     0bitcoin-cli getblockstats 0
 1{
 2  "avgfee": 0,
 3  "avgfeerate": 0,
 4  "avgtxsize": 0,
 5  "blockhash": "00000008819873e925422c1ff0f99f7cc9bbb232af63a077a480a3633bee1ef6",
 6  "feerate_percentiles": [
 7    0,
 8    0,
 9    0,
10    0,
11    0
12  ],
13  "height": 0,
14  "ins": 0,
15  "maxfee": 0,
16  "maxfeerate": 0,
17  "maxtxsize": 0,
18  "medianfee": 0,
19  "mediantime": 1598918400,
20  "mediantxsize": 0,
21  "minfee": 0,
22  "minfeerate": 0,
23  "mintxsize": 0,
24  "outs": 1,
25  "subsidy": 5000000000,
26  "swtotal_size": 0,
27  "swtotal_weight": 0,
28  "swtxs": 0,
29  "time": 1598918400,
30  "total_out": 0,
31  "total_size": 0,
32  "total_weight": 0,
33  "totalfee": 0,
34  "txs": 1,
35!  "utxo_increase": 1,
36  "utxo_size_inc": 117,
37!  "utxo_increase_actual": 0,
38!  "utxo_size_inc_actual": 0
39}

    Comparing with block 1:

     0//Block 0
 1
 2"utxo_increase": 1,
 3"utxo_size_inc": 117,
 4+"utxo_increase_actual": 0,
 5+"utxo_size_inc_actual": 0
 6  
 7//Block 1
 8
 9"utxo_increase": 2,
10"utxo_size_inc": 241,
11+"utxo_increase_actual": 1,
12+"utxo_size_inc_actual": 72
  58. in src/validation.cpp:5299 in 90b1986f89 outdated 
    5101+{
5102+    return (block_index.nHeight==91842 && block_index.GetBlockHash() == uint256S("0x00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec")) ||
5103+           (block_index.nHeight==91880 && block_index.GetBlockHash() == uint256S("0x00000000000743f190a18c5577a3c2d2a1f610ae9601ac046a38084ccb7cd721"));
5104+}
5105+
5106+bool IsBIP30Unspendable(const CBlockIndex& block_index)
    ryanofsky commented at 8:16 pm on February 4, 2022:

    In commit “validation, refactor: Add unspendable coinbase helper functions” (90b1986f8984167b2bf38153fc6bf6888202de45)

    ~Unlike IsBIP30Repeat, IsBIP30Unspendable is a new function that isn’t called anywhere yet. Any suggestions for reviewers how to verify it? Or otherwise compare the block information it’s using? May be worth saying in commit message where you got or how you derived the values.~

    Update: I see this is actually moved code, not added code. The other copy of this code is deleted in the fifth commit ca5de4bf4ab9f78bcc04a9f7fd0732db6a2b9156 “index: Deduplicate BIP30 block identification in coinstatsindex”. I’d suggest squashing that commit and this commit together. Or alternately moving the new IsBIP30Unspendable function to that commit so it is obvious that this code is being moved not added and easy to verify that it was moved correctly without having to compare different commits.

    fjahr commented at 1:32 am on February 13, 2022:
    done, I wrote this PR before coinstatsindex was merged and then just tagged the commit on at the end but it makes sent to square
  59. in test/functional/rpc_getblockstats.py:48 in 441e5bd7b9 outdated 
    42@@ -43,6 +43,10 @@ def get_stats(self):
43     def generate_test_data(self, filename):
44         mocktime = 1525107225
45         self.nodes[0].setmocktime(mocktime)
46+        self.nodes[0].createwallet(wallet_name='test')
47+        privkey = self.nodes[0].get_deterministic_priv_key().key
48+        self.nodes[0].importprivkey(privkey)
    ryanofsky commented at 1:00 am on February 5, 2022:

    In commit “test: Fix getblockstats test data generator” (441e5bd7b9a29c63e2f9c885e0c60b4b0665c2a3)

    Unclear what is happening in this commit. Can commit message say what error is fixed by this, and what broke this, or when it got broken? Could something be done to prevent breakage in the future? etc

    fjahr commented at 1:32 am on February 13, 2022:
    True, I added a more descriptive commit message
  60. ryanofsky approved
  61. ryanofsky commented at 3:30 pm on February 7, 2022: contributor
    Code review ACK ca5de4bf4ab9f78bcc04a9f7fd0732db6a2b9156
  62. fjahr force-pushed on Feb 13, 2022
  63. fjahr commented at 1:33 am on February 13, 2022: contributor
    Addressed comments by @ryanofsky , thanks a lot for reviewing!
  64. DrahtBot added the label Needs rebase on Apr 20, 2022
  65. fjahr force-pushed on Apr 24, 2022
  66. fjahr commented at 2:57 pm on April 24, 2022: contributor
    Rebased
  67. DrahtBot removed the label Needs rebase on Apr 24, 2022
  68. ryanofsky approved
  69. ryanofsky commented at 4:26 pm on April 25, 2022: contributor
    Code review ACK a04cfab8111e61ce3a6ecffa0b577dd0f7ecdbbf. Just rebased and consolidated commits since last review
  70. DrahtBot added the label Needs rebase on Apr 28, 2022
  71. fjahr force-pushed on Apr 30, 2022
  72. DrahtBot removed the label Needs rebase on Apr 30, 2022
  73. fjahr commented at 7:22 pm on April 30, 2022: contributor
    Rebased and turned the free helper functions into member functions of CBlockIndex. This seems like a natural location for them and in general there seems to be a preference to avoid free functions when possible.
  74. maflcko commented at 6:10 am on May 1, 2022: member

    Rebased and turned the free helper functions into member functions of CBlockIndex. This seems like a natural location for them and in general there seems to be a preference to avoid free functions when possible.

    I am seeing CBlockIndex a bit like a storage primitive, like ./primitives/transaction.h. So I am not sure to what extent consensus code like BIP30 should be moved into it. The reason I suggested to move a helper function in another pull into BlockManager was that it was blockstorage related and the change was also needed to get rid of cs_main.

  75. fjahr force-pushed on May 1, 2022
  76. fjahr commented at 2:28 pm on May 1, 2022: contributor

    I am seeing CBlockIndex a bit like a storage primitive, like ./primitives/transaction.h. So I am not sure to what extent consensus code like BIP30 should be moved into it. The reason I suggested to move a helper function in another pull into BlockManager was that it was blockstorage related and the change was also needed to get rid of cs_main.

    Hm, ok, I switched back to the old approach then since it’s also a smaller change and received ACKs already. So this is now just https://github.com/bitcoin/bitcoin/commit/a04cfab8111e61ce3a6ecffa0b577dd0f7ecdbbf with rebase.

    The approach with member functions is still here if anyone wants to take a look: https://github.com/fjahr/bitcoin/commit/6bb33163b2414ff4b47c843f447c2d05c5e33cd7

  77. ryanofsky approved
  78. ryanofsky commented at 0:12 am on May 19, 2022: contributor

    Code review almost-ACK d41254ab4693bc301f41b66ac704acf9b8010f58. Almost-ACK because there’s a silent conflict with #24804 which changes pindex from a pointer to a reference https://cirrus-ci.com/task/4900951441014784?logs=ci#L3420. So this needs to be rebased again.

    There were no changes since my previous review other than a rebase. I also agree with Marco’s comment #19888 (comment) about preferring standalone functions to member functions here. A class like CBlockIndex can be used for many different things and not every usage should be become part of the class definition. If code is just using the class’s external interface, not accessing private state, I think it’s generally better to add it as a normal function instead of as class method.

  79. fanquake added the label Needs rebase on May 19, 2022
  80. DrahtBot removed the label Needs rebase on May 19, 2022
  81. fjahr force-pushed on May 21, 2022
  82. fjahr commented at 9:43 pm on May 21, 2022: contributor
    Rebased and fixed the silent merge conflict noted by @ryanofsky . Thanks!
  83. luke-jr referenced this in commit 84a8699365 on May 21, 2022
  84. luke-jr referenced this in commit 629a738de3 on May 21, 2022
  85. luke-jr referenced this in commit 42a9b1aefd on May 21, 2022
  86. luke-jr referenced this in commit 5c573714da on May 21, 2022
  87. DrahtBot added the label Needs rebase on Jul 19, 2022
  88. fjahr force-pushed on Jul 20, 2022
  89. fjahr commented at 10:04 pm on July 20, 2022: contributor
    rebased
  90. DrahtBot removed the label Needs rebase on Jul 20, 2022
  91. DrahtBot added the label Needs rebase on Oct 13, 2022
  92. validation, index: Add unspendable coinbase helper functions 
    Making the checks to identify BIP30 available outside of validation.cpp is needed for reporting and tracking statistics on specific blocks and the UTXO set correctly.
    cb94db119f
  93. rpc: Improve getblockstats 
    - Fix getblockstats for block height 0 which previously returned an error.
- Introduce alternative utxo_*_actual statistics which exclude unspendables: Genesis block, BIP30, unspendable outputs
- Update test data
- Explicitly test Genesis block results
    2ca5a496c2
  94. test: Fix getblockstats test data generator 
    The getblockstats RPC functional test is using previously generated test data that is part of the repository. That test data can be regenerated by running the test file with `--gen-test-data` which invokes the `generate_test_data()` function. That function still relied on the old wallet behavior of having a default wallet to work. Because of this the function was broken and this change fixes this. The fact that this was broken did was not noticed previously because the function is not used by the automated test suite by default.
    ba9d288b24
  95. test: Test exclusion of OP_RETURN from getblockstats d885bb2f6e
  96. fjahr force-pushed on Oct 23, 2022
  97. DrahtBot removed the label Needs rebase on Oct 23, 2022
  98. fjahr commented at 12:55 pm on October 23, 2022: contributor
    rebased
  99. aureleoules approved
  100. aureleoules commented at 9:13 am on October 27, 2022: member

    ACK d885bb2f6ea34cd21dacfebe763a07dbb389c1bd

    Some end results I tried:

    0$ diff ./stats_master ./stats_pr # testnet block 000000007be904ff76a13eb6f0a10d945dbdc7a8ce18d66dd62af0d7e31d69fb 
136c36,38
2<   "utxo_size_inc": 614
3---
4>   "utxo_size_inc": 614,
5>   "utxo_increase_actual": 6,
6>   "utxo_size_inc_actual": 393

    Master

     0./src/bitcoin-cli getblockstats '"000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f"' # genesis
 1error code: -1
 2error message:
 3Can't read undo data from disk
 4
 5./src/bitcoin-cli getblockstats '"00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec"'
 6{
 7  "avgfee": 0,
 8  "avgfeerate": 0,
 9  "avgtxsize": 0,
10  "blockhash": "00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec",
11  "feerate_percentiles": [
12    0,
13    0,
14    0,
15    0,
16    0
17  ],
18  "height": 91842,
19  "ins": 0,
20  "maxfee": 0,
21  "maxfeerate": 0,
22  "maxtxsize": 0,
23  "medianfee": 0,
24  "mediantime": 1289767893,
25  "mediantxsize": 0,
26  "minfee": 0,
27  "minfeerate": 0,
28  "mintxsize": 0,
29  "outs": 1,
30  "subsidy": 5000000000,
31  "swtotal_size": 0,
32  "swtotal_weight": 0,
33  "swtxs": 0,
34  "time": 1289768691,
35  "total_out": 0,
36  "total_size": 0,
37  "total_weight": 0,
38  "totalfee": 0,
39  "txs": 1,
40  "utxo_increase": 1,
41  "utxo_size_inc": 117
42}

    PR

     0./src/bitcoin-cli getblockstats '"000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f"' # genesis
 1{
 2  "avgfee": 0,
 3  "avgfeerate": 0,
 4  "avgtxsize": 0,
 5  "blockhash": "000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f",
 6  "feerate_percentiles": [
 7    0,
 8    0,
 9    0,
10    0,
11    0
12  ],
13  "height": 0,
14  "ins": 0,
15  "maxfee": 0,
16  "maxfeerate": 0,
17  "maxtxsize": 0,
18  "medianfee": 0,
19  "mediantime": 1231006505,
20  "mediantxsize": 0,
21  "minfee": 0,
22  "minfeerate": 0,
23  "mintxsize": 0,
24  "outs": 1,
25  "subsidy": 5000000000,
26  "swtotal_size": 0,
27  "swtotal_weight": 0,
28  "swtxs": 0,
29  "time": 1231006505,
30  "total_out": 0,
31  "total_size": 0,
32  "total_weight": 0,
33  "totalfee": 0,
34  "txs": 1,
35  "utxo_increase": 1,
36  "utxo_size_inc": 117,
37  "utxo_increase_actual": 0,
38  "utxo_size_inc_actual": 0
39}
40
41./src/bitcoin-cli getblockstats '"00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec"'
42{
43  "avgfee": 0,
44  "avgfeerate": 0,
45  "avgtxsize": 0,
46  "blockhash": "00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec",
47  "feerate_percentiles": [
48    0,
49    0,
50    0,
51    0,
52    0
53  ],
54  "height": 91842,
55  "ins": 0,
56  "maxfee": 0,
57  "maxfeerate": 0,
58  "maxtxsize": 0,
59  "medianfee": 0,
60  "mediantime": 1289767893,
61  "mediantxsize": 0,
62  "minfee": 0,
63  "minfeerate": 0,
64  "mintxsize": 0,
65  "outs": 1,
66  "subsidy": 5000000000,
67  "swtotal_size": 0,
68  "swtotal_weight": 0,
69  "swtxs": 0,
70  "time": 1289768691,
71  "total_out": 0,
72  "total_size": 0,
73  "total_weight": 0,
74  "totalfee": 0,
75  "txs": 1,
76  "utxo_increase": 1,
77  "utxo_size_inc": 117,
78  "utxo_increase_actual": 0,
79  "utxo_size_inc_actual": 0
80}
  101. in src/validation.h:1088 in d885bb2f6e 
    1081@@ -1082,4 +1082,10 @@ bool DeploymentEnabled(const ChainstateManager& chainman, DEP dep)
1082  */
1083 const AssumeutxoData* ExpectedAssumeutxo(const int height, const CChainParams& params);
1084 
1085+/** Identifies blocks that overwrote an existing coinbase output in the UTXO set (see BIP30) */
1086+bool IsBIP30Repeat(const CBlockIndex& block_index);
1087+
1088+/** Identifies blocks which coinbase output was subsequently overwritten in the UTXO set (see BIP30) */
    stickies-v commented at 11:30 am on October 27, 2022:

    nit (see e.g. this)

    0/** Identifies blocks whose coinbase output was subsequently overwritten in the UTXO set (see BIP30) */
    stickies-v commented at 11:35 am on October 27, 2022:

    nit

    0/** Was this block's coinbase output overwritten in the UTXO set by a later block? (see BIP30) */
  102. in src/validation.h:1085 in d885bb2f6e 
    1081@@ -1082,4 +1082,10 @@ bool DeploymentEnabled(const ChainstateManager& chainman, DEP dep)
1082  */
1083 const AssumeutxoData* ExpectedAssumeutxo(const int height, const CChainParams& params);
1084 
1085+/** Identifies blocks that overwrote an existing coinbase output in the UTXO set (see BIP30) */
    stickies-v commented at 11:34 am on October 27, 2022:

    nit:

    0/** Did this block overwrite an existing coinbase output in the UTXO set? (see BIP30) */
  103. in src/rpc/blockchain.cpp:1781 in d885bb2f6e 
    1774@@ -1771,8 +1775,10 @@ static RPCHelpMan getblockstats()
1775                 {RPCResult::Type::NUM, "total_weight", /*optional=*/true, "Total weight of all non-coinbase transactions"},
1776                 {RPCResult::Type::NUM, "totalfee", /*optional=*/true, "The fee total"},
1777                 {RPCResult::Type::NUM, "txs", /*optional=*/true, "The number of transactions (including coinbase)"},
1778-                {RPCResult::Type::NUM, "utxo_increase", /*optional=*/true, "The increase/decrease in the number of unspent outputs"},
1779+                {RPCResult::Type::NUM, "utxo_increase", /*optional=*/true, "The increase/decrease in the number of unspent outputs (not discounting op_return and similar)"},
1780                 {RPCResult::Type::NUM, "utxo_size_inc", /*optional=*/true, "The increase/decrease in size for the utxo index (not discounting op_return and similar)"},
1781+                {RPCResult::Type::NUM, "utxo_increase_actual", /*optional=*/true, "The increase/decrease in the number of unspent outputs, not counting unspendables"},
1782+                {RPCResult::Type::NUM, "utxo_size_inc_actual", /*optional=*/true, "The increase/decrease in size for the utxo index, not counting unspendables"},
    stickies-v commented at 11:44 am on October 27, 2022:

    I think “actual” is quite vague, “spendable” would be more immediately obvious imo?

    0                {RPCResult::Type::NUM, "utxo_increase_spendable", /*optional=*/true, "The increase/decrease in the number of unspent outputs, not counting unspendables"},
1                {RPCResult::Type::NUM, "utxo_size_inc_spendable", /*optional=*/true, "The increase/decrease in size for the utxo index, not counting unspendables"},
  104. in src/rpc/blockchain.cpp:1778 in d885bb2f6e 
    1774@@ -1771,8 +1775,10 @@ static RPCHelpMan getblockstats()
1775                 {RPCResult::Type::NUM, "total_weight", /*optional=*/true, "Total weight of all non-coinbase transactions"},
1776                 {RPCResult::Type::NUM, "totalfee", /*optional=*/true, "The fee total"},
1777                 {RPCResult::Type::NUM, "txs", /*optional=*/true, "The number of transactions (including coinbase)"},
1778-                {RPCResult::Type::NUM, "utxo_increase", /*optional=*/true, "The increase/decrease in the number of unspent outputs"},
1779+                {RPCResult::Type::NUM, "utxo_increase", /*optional=*/true, "The increase/decrease in the number of unspent outputs (not discounting op_return and similar)"},
    stickies-v commented at 11:47 am on October 27, 2022:

    I think using different terminology here compared to in the 2 new lines makes it less clear what the difference in functionality is. Realigning the phrasing makes sense for readability imo.

    “including unspendables such as op_return” and “excluding unspendables such as op_return” makes the difference more immediately obvious I think.

  105. in src/rpc/blockchain.cpp:1780 in d885bb2f6e 
    1774@@ -1771,8 +1775,10 @@ static RPCHelpMan getblockstats()
1775                 {RPCResult::Type::NUM, "total_weight", /*optional=*/true, "Total weight of all non-coinbase transactions"},
1776                 {RPCResult::Type::NUM, "totalfee", /*optional=*/true, "The fee total"},
1777                 {RPCResult::Type::NUM, "txs", /*optional=*/true, "The number of transactions (including coinbase)"},
1778-                {RPCResult::Type::NUM, "utxo_increase", /*optional=*/true, "The increase/decrease in the number of unspent outputs"},
1779+                {RPCResult::Type::NUM, "utxo_increase", /*optional=*/true, "The increase/decrease in the number of unspent outputs (not discounting op_return and similar)"},
1780                 {RPCResult::Type::NUM, "utxo_size_inc", /*optional=*/true, "The increase/decrease in size for the utxo index (not discounting op_return and similar)"},
1781+                {RPCResult::Type::NUM, "utxo_increase_actual", /*optional=*/true, "The increase/decrease in the number of unspent outputs, not counting unspendables"},
    stickies-v commented at 1:17 pm on October 27, 2022:
    nit: every other line is alphabetically sorted, this one isn’t
  106. in src/rpc/blockchain.cpp:1834 in d885bb2f6e 
    1830@@ -1825,7 +1831,9 @@ static RPCHelpMan getblockstats()
1831     int64_t swtxs = 0;
1832     int64_t total_size = 0;
1833     int64_t total_weight = 0;
1834+    int64_t utxos = 0;
    stickies-v commented at 1:43 pm on October 27, 2022:

    nit: spendable_utxo_count would be my preferred, but that doesn’t seem to be in line with naming convention here, so I’m happy with:

    0    int64_t utxos_spendable = 0;
  107. in test/functional/rpc_getblockstats.py:182 in d885bb2f6e 
    177+
178+        self.log.info('Test tip including OP_RETURN')
179+        tip_stats = self.nodes[0].getblockstats(tip)
180+        assert_equal(tip_stats["utxo_increase"], 6)
181+        assert_equal(tip_stats["utxo_size_inc"], 441)
182+        assert_equal(tip_stats["utxo_increase_actual"], 4)
    stickies-v commented at 2:14 pm on October 27, 2022:
    Would you mind adding a docstring that explains why 4 (i.e. 2 unspendable UTXOs)? From generate_test_data it would seem there is only 1 OP_RETURN, and genesis/bip30 wouldn’t apply here either?
  108. in test/functional/rpc_getblockstats.py:183 in d885bb2f6e 
    178+        self.log.info('Test tip including OP_RETURN')
179+        tip_stats = self.nodes[0].getblockstats(tip)
180+        assert_equal(tip_stats["utxo_increase"], 6)
181+        assert_equal(tip_stats["utxo_size_inc"], 441)
182+        assert_equal(tip_stats["utxo_increase_actual"], 4)
183+        assert_equal(tip_stats["utxo_size_inc_actual"], 300)
    stickies-v commented at 3:06 pm on October 27, 2022:
    When I run --gen-test-data, I get slightly different sizes: 441->438 and 300->297. Not sure how local ./configure options would change that, so… wondering if it’s just me or leftover from using an older wallet version in earlier versions of the PR?
  109. stickies-v approved
  110. stickies-v commented at 3:12 pm on October 27, 2022: contributor

    ACK d885bb2f6

    No blocking comments/okay in follow-up. However, since it’s difficult to change RPC variable names after they’re part of the public API, might be best to consider that first.

  111. achow101 commented at 10:27 pm on December 5, 2022: member
    ACK d885bb2f6ea34cd21dacfebe763a07dbb389c1bd
  112. achow101 merged this on Dec 5, 2022
  113. achow101 closed this on Dec 5, 2022
  114. sidhujag referenced this in commit 477381a935 on Dec 6, 2022
  115. bitcoin locked this on Dec 5, 2023


fjahr n-thumann promag maflcko theStack DrahtBot luke-jr laanwj ryanofsky ghost aureleoules stickies-v achow101

Labels
RPC/REST/ZMQ

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: 2024-11-21 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me