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 | alfonsoromanz |
| Concept ACK | tdb3 |
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?
