test: Added coverage to Block not found error using gettxoutsetinfo #30340

pull kevkevinpal wants to merge 1 commits into bitcoin:master from kevkevinpal:testBlockNotFoundUsingGetTxOutSetInfo changing 1 files +3 −0
  1. kevkevinpal commented at 1:02 am on June 26, 2024: contributor

    Description

    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

  2. DrahtBot commented at 1:02 am on June 26, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    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.

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Tests on Jun 26, 2024
  4. 1alexbc1 approved
  5. 1alexbc1 commented at 7:02 am on June 26, 2024: none
    Yea
  6. EarlTZ approved
  7. in test/functional/rpc_blockchain.py:399 in 341d5e1da1 outdated
    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)
    


    brunoerg commented at 11:55 am on June 26, 2024:

    In 341d5e1da1a40ed6dec449bc64132de5ae8788c2:

    0        assert_raises_rpc_error(-5, "Block not found", node.gettxoutsetinfo, "none", "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09", True)
    

    tdb3 commented at 12:25 pm on June 26, 2024:

    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)
    

    kevkevinpal commented at 2:13 pm on June 26, 2024:
    thanks fixed in f169562d85b332576b3a1cf32755e43023227ed9

    kevkevinpal commented at 2:15 pm on June 26, 2024:
    do you think I should still add this after f169562d85b332576b3a1cf32755e43023227ed9, rather than invalidating a block to then use gettxoutsetinfo with its hash I just used a random block hash. I can try doing this aswell

    tdb3 commented at 4:36 pm on June 26, 2024:

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

    kevkevinpal commented at 3:32 pm on June 27, 2024:

    thanks updated in 6809ffd

    also the earlier comment I addressed below #30340 (comment)

  8. in test/functional/rpc_blockchain.py:398 in 341d5e1da1 outdated
    393+            extra_args=[
    394+                "-coinstatsindex=1",
    395+                "-reindex"
    396+            ],
    397+        )
    398+        # Block not found error
    


    brunoerg commented at 11:57 am on June 26, 2024:

    In 341d5e1da1a40ed6dec449bc64132de5ae8788c2, I don’t think this comment is necessary since the next line is clear and “self explanatory”:


    kevkevinpal commented at 2:13 pm on June 26, 2024:
    thank you fixed in f169562d85b332576b3a1cf32755e43023227ed9
  9. brunoerg commented at 12:03 pm on June 26, 2024: contributor
    Since calling 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.
  10. tdb3 commented at 12:26 pm on June 26, 2024: contributor
    Concept ACK Left one minor comment.
  11. kevkevinpal force-pushed on Jun 26, 2024
  12. kevkevinpal force-pushed on Jun 26, 2024
  13. kevkevinpal commented at 2:13 pm on June 26, 2024: contributor

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

    That makes more sense I went ahead and did so in f169562d85b332576b3a1cf32755e43023227ed9

  14. in test/functional/feature_coinstatsindex.py:245 in f169562d85 outdated
    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)
    


    brunoerg commented at 2:35 pm on June 26, 2024:
    nit: You could use named args here. It’s better to understand what "none" and True are.

    kevkevinpal commented at 3:11 pm on June 26, 2024:
    sounds good resolved in 6809ffd
  15. kevkevinpal force-pushed on Jun 26, 2024
  16. alfonsoromanz commented at 9:57 am on June 27, 2024: contributor

    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.

  17. DrahtBot requested review from tdb3 on Jun 27, 2024
  18. kevkevinpal force-pushed on Jun 27, 2024
  19. kevkevinpal commented at 3:31 pm on June 27, 2024: contributor

    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

  20. 1alexbc1 approved
  21. 1alexbc1 commented at 4:01 pm on June 27, 2024: none
    Amadeevantv yimelaja
  22. alfonsoromanz commented at 6:48 am on June 28, 2024: contributor
    Re ACK 82c454c65658439481b50fe71ed097ecb8d70737
  23. tdb3 commented at 5:09 pm on June 28, 2024: contributor

    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?

  24. kevkevinpal force-pushed on Jun 29, 2024
  25. kevkevinpal commented at 10:02 pm on June 29, 2024: contributor

    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

  26. tdb3 commented at 0:37 am on June 30, 2024: contributor

    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…

  27. test: Added coverage to Block not found error using gettxoutsetinfo 8ec24bdad8
  28. kevkevinpal force-pushed on Jun 30, 2024
  29. kevkevinpal commented at 2:48 pm on June 30, 2024: contributor

    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…

    Thats a good idea! I set it to ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff since that is larger than 7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff and then pushed to 8ec24bd

  30. tdb3 approved
  31. tdb3 commented at 3:34 pm on June 30, 2024: contributor
    ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
  32. DrahtBot requested review from alfonsoromanz on Jun 30, 2024
  33. kevkevinpal requested review from brunoerg on Jun 30, 2024
  34. kristapsk approved
  35. kristapsk commented at 9:34 pm on June 30, 2024: contributor
    ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
  36. brunoerg approved
  37. brunoerg commented at 9:04 am on July 1, 2024: contributor
    crACK 8ec24bdad89e2a72c394060ba5661a91f374b874
  38. tdb3 commented at 11:35 pm on July 1, 2024: contributor

    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:

    • Generated a chain of length 101.
    • Invalidated block 2.
    • 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.
  39. alfonsoromanz approved
  40. alfonsoromanz commented at 9:59 am on July 2, 2024: contributor
    Re ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
  41. achow101 commented at 8:18 pm on July 2, 2024: member
    ACK 8ec24bdad89e2a72c394060ba5661a91f374b874
  42. achow101 merged this on Jul 2, 2024
  43. achow101 closed this on Jul 2, 2024

  44. PastaPastaPasta referenced this in commit 9c2df8dbef on Oct 25, 2024
  45. PastaPastaPasta referenced this in commit 9793fb1a87 on Oct 27, 2024
  46. PastaPastaPasta referenced this in commit 565f2db930 on Oct 27, 2024
  47. bitcoin locked this on Jul 2, 2025

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: 2025-07-11 06:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me