rpc, test: Improve getblockstats for unspendables #19888

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.

fanquake added the label RPC/REST/ZMQ on Sep 5, 2020

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:

        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

fjahr force-pushed on Sep 5, 2020

n-thumann approved

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

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!

fjahr force-pushed on Sep 6, 2020

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

fjahr force-pushed on Sep 6, 2020

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.

fjahr renamed this:
~~rpc: Fix getblockstats for block height 0~~
rpc: Fix getblockstats issues
on Sep 7, 2020

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.

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.

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.

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.

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

luke-jr changes_requested

fjahr force-pushed on Dec 24, 2020

fjahr commented at 1:35 PM on December 24, 2020: contributor

Addressed feedback comments. Thanks for reviewing and sorry for the long wait!

DrahtBot added the label Needs rebase on Feb 16, 2021

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 :-/

fjahr force-pushed on Apr 11, 2021

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.

fjahr renamed this:
~~rpc: Fix getblockstats issues~~
rpc, test: Improve getblockstats for unspendables
on Apr 11, 2021

DrahtBot removed the label Needs rebase on Apr 11, 2021

DrahtBot added the label Needs rebase on May 31, 2021

fjahr force-pushed on May 31, 2021

DrahtBot removed the label Needs rebase on May 31, 2021

fjahr commented at 9:30 PM on May 31, 2021: contributor

rebased

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.

fjahr force-pushed on Aug 24, 2021

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

fjahr force-pushed on Aug 24, 2021

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:

bool IsBIP30Repeat(const CBlockIndex& block_index)
{

fjahr commented at 9:30 AM on August 26, 2021:

done

fjahr force-pushed on Aug 26, 2021

DrahtBot added the label Needs rebase on Sep 9, 2021

fjahr force-pushed on Sep 9, 2021

fjahr commented at 11:23 PM on September 9, 2021: contributor

rebased

DrahtBot removed the label Needs rebase on Sep 9, 2021

DrahtBot added the label Needs rebase on Sep 23, 2021

fjahr force-pushed on Sep 23, 2021

fjahr commented at 11:49 PM on September 23, 2021: contributor

rebased

Edit: Hm, CI failure looks unrelated

DrahtBot removed the label Needs rebase on Sep 24, 2021

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

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

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

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

DrahtBot added the label Needs rebase on Dec 9, 2021

fjahr force-pushed on Dec 10, 2021

fjahr commented at 12:21 AM on December 10, 2021: contributor

rebased

DrahtBot removed the label Needs rebase on Dec 10, 2021

ghost commented at 9:01 AM on January 15, 2022: none

Concept ACK. Will test today.

unknown approved

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:

bitcoin-cli getblockstats 0

error code: -1
error message:
Can't read undo data from disk

PR Branch:

bitcoin-cli getblockstats 0
{
  "avgfee": 0,
  "avgfeerate": 0,
  "avgtxsize": 0,
  "blockhash": "00000008819873e925422c1ff0f99f7cc9bbb232af63a077a480a3633bee1ef6",
  "feerate_percentiles": [
    0,
    0,
    0,
    0,
    0
  ],
  "height": 0,
  "ins": 0,
  "maxfee": 0,
  "maxfeerate": 0,
  "maxtxsize": 0,
  "medianfee": 0,
  "mediantime": 1598918400,
  "mediantxsize": 0,
  "minfee": 0,
  "minfeerate": 0,
  "mintxsize": 0,
  "outs": 1,
  "subsidy": 5000000000,
  "swtotal_size": 0,
  "swtotal_weight": 0,
  "swtxs": 0,
  "time": 1598918400,
  "total_out": 0,
  "total_size": 0,
  "total_weight": 0,
  "totalfee": 0,
  "txs": 1,
!  "utxo_increase": 1,
  "utxo_size_inc": 117,
!  "utxo_increase_actual": 0,
!  "utxo_size_inc_actual": 0
}

Comparing with block 1:

//Block 0

"utxo_increase": 1,
"utxo_size_inc": 117,
+"utxo_increase_actual": 0,
+"utxo_size_inc_actual": 0
  
//Block 1

"utxo_increase": 2,
"utxo_size_inc": 241,
+"utxo_increase_actual": 1,
+"utxo_size_inc_actual": 72

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

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

ryanofsky approved

ryanofsky commented at 3:30 PM on February 7, 2022: contributor

Code review ACK ca5de4bf4ab9f78bcc04a9f7fd0732db6a2b9156

fjahr force-pushed on Feb 13, 2022

fjahr commented at 1:33 AM on February 13, 2022: contributor

Addressed comments by @ryanofsky , thanks a lot for reviewing!

DrahtBot added the label Needs rebase on Apr 20, 2022

fjahr force-pushed on Apr 24, 2022

fjahr commented at 2:57 PM on April 24, 2022: contributor

Rebased

DrahtBot removed the label Needs rebase on Apr 24, 2022

ryanofsky approved

ryanofsky commented at 4:26 PM on April 25, 2022: contributor

Code review ACK a04cfab8111e61ce3a6ecffa0b577dd0f7ecdbbf. Just rebased and consolidated commits since last review

DrahtBot added the label Needs rebase on Apr 28, 2022

fjahr force-pushed on Apr 30, 2022

DrahtBot removed the label Needs rebase on Apr 30, 2022

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.

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.

fjahr force-pushed on May 1, 2022

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

ryanofsky approved

ryanofsky commented at 12: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.

fanquake added the label Needs rebase on May 19, 2022

DrahtBot removed the label Needs rebase on May 19, 2022

fjahr force-pushed on May 21, 2022

fjahr commented at 9:43 PM on May 21, 2022: contributor

Rebased and fixed the silent merge conflict noted by @ryanofsky . Thanks!

luke-jr referenced this in commit 84a8699365 on May 21, 2022

luke-jr referenced this in commit 629a738de3 on May 21, 2022

luke-jr referenced this in commit 42a9b1aefd on May 21, 2022

luke-jr referenced this in commit 5c573714da on May 21, 2022

DrahtBot added the label Needs rebase on Jul 19, 2022

fjahr force-pushed on Jul 20, 2022

fjahr commented at 10:04 PM on July 20, 2022: contributor

rebased

DrahtBot removed the label Needs rebase on Jul 20, 2022

DrahtBot added the label Needs rebase on Oct 13, 2022

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

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

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

test: Test exclusion of OP_RETURN from getblockstats d885bb2f6e

fjahr force-pushed on Oct 23, 2022

DrahtBot removed the label Needs rebase on Oct 23, 2022

fjahr commented at 12:55 PM on October 23, 2022: contributor

rebased

aureleoules approved

aureleoules commented at 9:13 AM on October 27, 2022: member

ACK d885bb2f6ea34cd21dacfebe763a07dbb389c1bd

Some end results I tried:

$ diff ./stats_master ./stats_pr # testnet block 000000007be904ff76a13eb6f0a10d945dbdc7a8ce18d66dd62af0d7e31d69fb 
36c36,38
<   "utxo_size_inc": 614
---
>   "utxo_size_inc": 614,
>   "utxo_increase_actual": 6,
>   "utxo_size_inc_actual": 393

Master

<details> <summary>Details</summary>

./src/bitcoin-cli getblockstats '"000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f"' # genesis
error code: -1
error message:
Can't read undo data from disk

./src/bitcoin-cli getblockstats '"00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec"'
{
  "avgfee": 0,
  "avgfeerate": 0,
  "avgtxsize": 0,
  "blockhash": "00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec",
  "feerate_percentiles": [
    0,
    0,
    0,
    0,
    0
  ],
  "height": 91842,
  "ins": 0,
  "maxfee": 0,
  "maxfeerate": 0,
  "maxtxsize": 0,
  "medianfee": 0,
  "mediantime": 1289767893,
  "mediantxsize": 0,
  "minfee": 0,
  "minfeerate": 0,
  "mintxsize": 0,
  "outs": 1,
  "subsidy": 5000000000,
  "swtotal_size": 0,
  "swtotal_weight": 0,
  "swtxs": 0,
  "time": 1289768691,
  "total_out": 0,
  "total_size": 0,
  "total_weight": 0,
  "totalfee": 0,
  "txs": 1,
  "utxo_increase": 1,
  "utxo_size_inc": 117
}

</details>

PR

<details> <summary>Details</summary>

./src/bitcoin-cli getblockstats '"000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f"' # genesis
{
  "avgfee": 0,
  "avgfeerate": 0,
  "avgtxsize": 0,
  "blockhash": "000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f",
  "feerate_percentiles": [
    0,
    0,
    0,
    0,
    0
  ],
  "height": 0,
  "ins": 0,
  "maxfee": 0,
  "maxfeerate": 0,
  "maxtxsize": 0,
  "medianfee": 0,
  "mediantime": 1231006505,
  "mediantxsize": 0,
  "minfee": 0,
  "minfeerate": 0,
  "mintxsize": 0,
  "outs": 1,
  "subsidy": 5000000000,
  "swtotal_size": 0,
  "swtotal_weight": 0,
  "swtxs": 0,
  "time": 1231006505,
  "total_out": 0,
  "total_size": 0,
  "total_weight": 0,
  "totalfee": 0,
  "txs": 1,
  "utxo_increase": 1,
  "utxo_size_inc": 117,
  "utxo_increase_actual": 0,
  "utxo_size_inc_actual": 0
}

./src/bitcoin-cli getblockstats '"00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec"'
{
  "avgfee": 0,
  "avgfeerate": 0,
  "avgtxsize": 0,
  "blockhash": "00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec",
  "feerate_percentiles": [
    0,
    0,
    0,
    0,
    0
  ],
  "height": 91842,
  "ins": 0,
  "maxfee": 0,
  "maxfeerate": 0,
  "maxtxsize": 0,
  "medianfee": 0,
  "mediantime": 1289767893,
  "mediantxsize": 0,
  "minfee": 0,
  "minfeerate": 0,
  "mintxsize": 0,
  "outs": 1,
  "subsidy": 5000000000,
  "swtotal_size": 0,
  "swtotal_weight": 0,
  "swtxs": 0,
  "time": 1289768691,
  "total_out": 0,
  "total_size": 0,
  "total_weight": 0,
  "totalfee": 0,
  "txs": 1,
  "utxo_increase": 1,
  "utxo_size_inc": 117,
  "utxo_increase_actual": 0,
  "utxo_size_inc_actual": 0
}

</details>

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)

/** 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

/** Was this block's coinbase output overwritten in the UTXO set by a later block? (see BIP30) */

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:

/** Did this block overwrite an existing coinbase output in the UTXO set? (see BIP30) */

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?

                {RPCResult::Type::NUM, "utxo_increase_spendable", /*optional=*/true, "The increase/decrease in the number of unspent outputs, not counting unspendables"},
                {RPCResult::Type::NUM, "utxo_size_inc_spendable", /*optional=*/true, "The increase/decrease in size for the utxo index, not counting unspendables"},

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.

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

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:

    int64_t utxos_spendable = 0;

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?

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?

stickies-v approved

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.

achow101 commented at 10:27 PM on December 5, 2022: member

ACK d885bb2f6ea34cd21dacfebe763a07dbb389c1bd

achow101 merged this on Dec 5, 2022

achow101 closed this on Dec 5, 2022

sidhujag referenced this in commit 477381a935 on Dec 6, 2022

bitcoin locked this on Dec 5, 2023