There were no tests that checked for the Block not found error called in ParseHashOrHeight when using gettxoutsetinfo, this change adds coverage to it.
You can see there are no tests that do the following by doing the below
grep -nri "Block not found.*gettxoutsetinfo" ./test/functional/
which leads to no results
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | tdb3, kristapsk, brunoerg, alfonsoromanz, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
394+ "-coinstatsindex=1",
395+ "-reindex"
396+ ],
397+ )
398+ # Block not found error
399+ assert_raises_rpc_error(-5, "Block not found", self.nodes[0].gettxoutsetinfo, "none", "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09", True)
In 341d5e1da1a40ed6dec449bc64132de5ae8788c2:
0 assert_raises_rpc_error(-5, "Block not found", node.gettxoutsetinfo, "none", "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09", True)
Looks like this is checking for a block hash that doesn’t exist after block hash for block 1 is reconsidered, which makes sense. Recommend adding self.log.info() to make this clearer to the reader.
0b1hash = node.getblockhash(1)
1node.invalidateblock(b1hash)
2...
3self.log.info("Test gettxoutsetinfo returns the same result after invalidate/reconsider block")
4node.reconsiderblock(b1hash)
gettxoutsetinfo with its hash I just used a random block hash. I can try doing this aswell
Looks like that’s the block hash for block 1000 (mainnet). What’s the regtest minimum difficulty? The hash that we don’t want to be found should be something that definitely isn’t possible.
Would also be good to inform the log, for example:
0diff --git a/test/functional/feature_coinstatsindex.py b/test/functional/feature_coinstatsindex.py
1index c6edbe7c31..8e37a3c537 100755
2--- a/test/functional/feature_coinstatsindex.py
3+++ b/test/functional/feature_coinstatsindex.py
4@@ -242,6 +242,7 @@ class CoinStatsIndexTest(BitcoinTestFramework):
5 res12 = index_node.gettxoutsetinfo('muhash')
6 assert_equal(res12, res10)
7
8+ self.log.info("Test obtaining info for a non-existent block hash")
9 assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height="00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09", use_index=True)
10
11 def _test_use_index_option(self):
thanks updated in 6809ffd
also the earlier comment I addressed below #30340 (comment)
393+ extra_args=[
394+ "-coinstatsindex=1",
395+ "-reindex"
396+ ],
397+ )
398+ # Block not found error
In 341d5e1da1a40ed6dec449bc64132de5ae8788c2, I don’t think this comment is necessary since the next line is clear and “self explanatory”:
gettxoutsetinfo with block hash depends on coinstatsindex. It would be simpler to address it on feature_coinstatsindex. It would be just one line, and would avoid the node restart and the function reordering.
Since calling
gettxoutsetinfowith block hash depends oncoinstatsindex. It would be simpler to address it onfeature_coinstatsindex. It would be just one line, and would avoid the node restart and the function reordering.
That makes more sense I went ahead and did so in f169562d85b332576b3a1cf32755e43023227ed9
241@@ -242,6 +242,8 @@ def _test_coin_stats_index(self):
242 res12 = index_node.gettxoutsetinfo('muhash')
243 assert_equal(res12, res10)
244
245+ assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, "none", "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09", True)
"none" and True are.
tACK 6809ffd8a6c589c515af48da2dd8367e4c6c2c26. I ran test/functional/feature_coinstatsindex.py, and the test passes.
+1 to informing the log as @tdb3 suggests.
You can now update your PR description to remove the note about the function reordering and the node restart.
thanks @tdb3 and @alfonsoromanz I added a log in 82c454c
Also @tdb3 I tried doing
0b1hash = index_node.getblockhash(1)
1index_node.invalidateblock(b1hash)
2
3self.log.info("Test obtaining info for a non-existent block hash")
4assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True)
5
6index_node.reconsiderblock(b1hash)
But it failed, I think its because even though we invalidate the block the node is still able to find it. Where as a random hash will lead to no outcomes
thanks @tdb3 and @alfonsoromanz I added a log in 82c454c
Also @tdb3 I tried doing
0b1hash = index_node.getblockhash(1) 1index_node.invalidateblock(b1hash) 2 3self.log.info("Test obtaining info for a non-existent block hash") 4assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True) 5 6index_node.reconsiderblock(b1hash)But it failed, I think its because even though we invalidate the block the node is still able to find it. Where as a random hash will lead to no outcomes
Hmm, not sure if that is expected behavior or not for gettxoutsetinfo. If the block is invalidated, seems counterintuitive to consider it as a hash_or_height for calculating txout set info. Not sure what piece is missing here, but will dig more.
Regarding the hash used for hash_or_height, from what I can tell regtest blocks are allowed a higher target. While using 00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09 (block hash of mainnet block 1000) would be extremely improbably to collide, wouldn’t it be better to use something guaranteed not to collide? Something above the max target for regtest?
thanks @tdb3 and @alfonsoromanz I added a log in 82c454c Also @tdb3 I tried doing
0b1hash = index_node.getblockhash(1) 1index_node.invalidateblock(b1hash) 2 3self.log.info("Test obtaining info for a non-existent block hash") 4assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True) 5 6index_node.reconsiderblock(b1hash)But it failed, I think its because even though we invalidate the block the node is still able to find it. Where as a random hash will lead to no outcomes
Hmm, not sure if that is expected behavior or not for gettxoutsetinfo. If the block is invalidated, seems counterintuitive to consider it as a hash_or_height for calculating txout set info. Not sure what piece is missing here, but will dig more.
Regarding the hash used for hash_or_height, from what I can tell regtest blocks are allowed a higher target. While using 00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09 (block hash of mainnet block 1000) would be extremely improbably to collide, wouldn’t it be better to use something guaranteed not to collide? Something above the max target for regtest?
sounds good that makes sense I just pushed 837af6e and am now using 0000000000000000000195940324e8fbd1f1d9f872cd16980c826930453bf65e from block 850020 on mainnet
Regarding the hash used for hash_or_height, from what I can tell regtest blocks are allowed a higher target. While using 00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09 (block hash of mainnet block 1000) would be extremely improbably to collide, wouldn’t it be better to use something guaranteed not to collide? Something above the max target for regtest?
sounds good that makes sense I just pushed 837af6e and am now using 0000000000000000000195940324e8fbd1f1d9f872cd16980c826930453bf65e from block 850020 on mainnet
I could be off, but the regtest powlimit may be: https://github.com/bitcoin/bitcoin/blob/2f6dca4d1c01ee47275a4292f128d714736837a1/src/kernel/chainparams.cpp#L423
which means we would want to use a value higher than that for the test (i.e. a hash that would have no chance of colliding with a generated block). Someone should correct if I’m mistaken.
As a sanity check, I generated 10,000 blocks on regtest and none had a block hash greater than 0x7ff…
I could be off, but the regtest powlimit may be:
which means we would want to use a value higher than that for the test (i.e. a hash that would have no chance of colliding with a generated block). Someone should correct if I’m mistaken.
As a sanity check, I generated 10,000 blocks on regtest and none had a block hash greater than 0x7ff…
Thats a good idea! I set it to ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff since that is larger than 7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff and then pushed to 8ec24bd
Hmm, not sure if that is expected behavior or not for gettxoutsetinfo. If the block is invalidated, seems counterintuitive to consider it as a hash_or_height for calculating txout set info. Not sure what piece is missing here, but will dig more.
Following up:
gettxoutsetinfo with no arguments returns info for the chain (of height 1) as expected.gettxoutsetinfo with a block height >= 2 returns the expected error message (Target block height X after current typ 1).gettxoutsetinfo with an explicitly specified hash of a block 2+ returns the info for the chain up to that hash (ignoring invalidation). At first glance this seemed to be a bug, but it seems like there could be an argument to be made that if the user is providing an explicit block hash then they want info including it (even if it is part of a chain that was invalidated). Conversely, if info is not provided because the hash is part of a chain that was invalidated but the node is aware of it, then this also seems a bit off.
