test: check that loading snapshot not matching AssumeUTXO parameters fails #28625

1. 
   theStack commented at 7:55 AM on October 10, 2023: contributor

   This PR adds test coverage for the failed loading of an AssumeUTXO snapshot in case the referenced block hash doesn't match the parameters in the chainparams. Right now, I expect this would be the most common error-case for loadtxoutset out in the wild, as for mainnet the m_assumeutxo_data map is empty and this error condition would obviously always be triggered for any (otherwise valid, correctly encoded) snapshot. Note that this test-case is the simplest scenario and doesn't cover any of the TODO ideas mentioned at the top of the functional test yet.

2. test: check that loading snapshot not matching AssumeUTXO parameters fails 2e31250027
3. 
   DrahtBot commented at 7:55 AM on October 10, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	jamesob, Sjors, achow101

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. DrahtBot added the label Tests on Oct 10, 2023
5. 
   jamesob commented at 5:02 PM on October 10, 2023: member

   ACK https://github.com/bitcoin/bitcoin/pull/28625/commits/2e31250027ac580a7a72221fe2ff505b30836175

6. 
   Sjors commented at 7:45 AM on October 11, 2023: member

   utACK 2e31250027ac580a7a72221fe2ff505b30836175

7. maflcko added this to the milestone 26.0 on Oct 11, 2023
8. 
   achow101 commented at 6:19 PM on October 11, 2023: member

   ACK 2e31250027ac580a7a72221fe2ff505b30836175

9. achow101 merged this on Oct 11, 2023
10. achow101 closed this on Oct 11, 2023

    ---

11. in test/functional/feature_assumeutxo.py:85 in 2e31250027

      80 | +            bad_snapshot_block_hash = self.nodes[0].getblockhash(bad_snapshot_height)
      81 | +            # block hash of the snapshot base is stored right at the start (first 32 bytes)
      82 | +            f.write(bytes.fromhex(bad_snapshot_block_hash)[::-1] + valid_snapshot_contents[32:])
      83 | +
      84 | +        expected_log = f"assumeutxo height in snapshot metadata not recognized ({bad_snapshot_height}) - refusing to load snapshot"
      85 | +        with self.nodes[1].assert_debug_log(expected_log):
    

    ---

    ---

    

    maflcko commented at 8:42 AM on October 12, 2023:

    This is wrong. assert_debug_log accepts an array of strings, not a string. Otherwise it will search for any single character in the string, which will always pass.

    ---

    

    maflcko commented at 9:16 AM on October 12, 2023:

    https://github.com/bitcoin/bitcoin/pull/28639

    ---

    

    theStack commented at 1:44 PM on October 12, 2023:

    Good catch. Unfortunately, I just noticed that the exact same mistake also happened in #28634 :scream: . Given that this is really hard to catch (in total five out of six reviewers didn't see it), it may make sense to assert the type in assert_debug_log to avoid bugs like that in the future.

    ---

    

    maflcko commented at 1:52 PM on October 12, 2023:

    Also, this isn't the first issue. It keeps coming back in variants. Longer term a type safe language can be considered. However, in the short term a hack on top of python could make sense (mypy or manual asserts)?

12. theStack deleted the branch on Oct 12, 2023
13. theStack referenced this in commit 616c090e21 on Oct 12, 2023
14. theStack referenced this in commit ac4caf3366 on Oct 13, 2023
15. achow101 referenced this in commit 78b7e95518 on Oct 13, 2023
16. Frank-GER referenced this in commit e3a3c74fe0 on Oct 13, 2023
17. Frank-GER referenced this in commit 5629efd2d6 on Oct 21, 2023
18. bitcoin locked this on Oct 11, 2024

Contributors

theStackDrahtBotjamesobSjorsachow101maflcko

Milestone
26.0