test: Write assumeutxo tests #28648

1. 
   maflcko commented at 10:24 am on October 13, 2023: member

   Motivation

   Tests are useful to catch bugs, so they should be written.

   Possible solution

   Write tests, similar to #28647

   Refer to the TODO comments in ./test/functional/feature_assumeutxo.py, or to the uncovered lines in the AssumeUtxo code via a coverage report. For example, https://maflcko.github.io/b-c-cov/total.coverage/src/validation.cpp.gcov.html

   Useful Skills

   • Compiling Bitcoin Core from source
   • Running the C++ unit tests and the Python functional tests
   • Basic AssumeUtxo understanding
   • Bitcoin Core serialization and test framework understanding
   • Python serialization understanding

   Guidance for new contributors

   Want to work on this issue?

   For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

2. maflcko added the label Tests on Oct 13, 2023
3. maflcko added the label good first issue on Oct 13, 2023
4. 
   ryanofsky commented at 2:02 pm on October 13, 2023: contributor

   Original review comment might provide more explanation (https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1331521797):

   Test seems to have pretty good coverage for the happy case, but it would be good to have coverage for pathological cases to make sure checks in the code written to handle them work correctly.

   It should be possible to write a test using itertools.product or something similar to test different cases and starting states.

   Interesting test cases could be loading an assumeutxo snapshot file with:

   1. An invalid hash
   2. Valid hash but invalid snapshot file (bad coin height or truncated file or bad other serialization)
   3. Valid snapshot file, but referencing an unknown block
   4. Valid snapshot file, but referencing a snapshot block that turns out to be invalid, or has an invalid parent
   5. Valid snapshot file and snapshot block, but the block is not on the most-work chain

   Interesting starting states could be loading a snapshot when the current chain tip is:

   1. An ancestor of snapshot block
   2. Not an ancestor of the snapshot block but has less work
   3. The snapshot block
   4. A descendant of the snapshot block
   5. Not an ancestor or a descendant of the snapshot block and has more work

   Most combinations of test cases / starting states above should be possible. I don’t think a test like this needs to be part of this PR, but would be nice as a followup. It might make sense to include a consolidated TODO comment in this PR documenting ways the test could be improved (also mentioning other test ideas #27596 (comment) #27596 (comment))

5. 
   BrandonOdiwuor commented at 10:27 am on October 19, 2023: contributor

   @maflcko would like to work on this
6. 
   kashifs commented at 2:57 pm on October 22, 2023: contributor

   I will help review this PR
7. 
   jrakibi commented at 1:18 pm on November 2, 2023: contributor

   Is anyone working on this? I’d be happy to take a look
8. 
   BrandonOdiwuor commented at 1:30 pm on November 2, 2023: contributor

   @jrakibi I’m currently working on bitcoin-core/gui/769, you can work on it
9. 
   maflcko commented at 2:22 pm on November 8, 2023: member

   Another idea would be to start a new wallet test. For now it could only check that it throws an error. See the example hacky diff I wrote, which will need to be moved to a new test file: #28616 (review)
10. maflcko added the label Wallet on Nov 8, 2023
11. maflcko added the label Consensus on Nov 8, 2023
12. 
    Sjors commented at 2:40 am on November 9, 2023: member

    I created a wallet test file as part of #28616. Feel free to lift that commit out and make a fresh pull request. Right now it doesn’t actually test the functionality that’s introduced in 28616, though I might expand it.
13. 
    mohamedawnallah commented at 5:29 pm on December 12, 2023: none

    Hey, @maflcko @jrakibi Is there anyone working on this issue? If not I’d like to pick it up. Thanks
14. 
    nafisalawalidris commented at 5:39 pm on December 14, 2023: none

    Hello @maflcko, I’m interested in working on this issue and adding tests for the AssumeUtxo feature. I will follow the guidance provided in the issue description and the template for writing tests. I will keep you posted on my progress and submit a pull request once the tests are ready.
15. 
    jrakibi commented at 4:52 pm on December 15, 2023: contributor

    Hey, @maflcko @jrakibi Is there anyone working on this issue? If not I’d like to pick it up. Thanks

    Hey @mohamedawnallah , I am currently preoccupied with other assignments, feel free to take it over

16. 
    maflcko commented at 6:34 pm on January 18, 2024: member

    There’d be #27596 (review) if someone wants to tackle it.
17. 
    maflcko commented at 11:08 am on February 2, 2024: member

    Another test idea would be to check what happens when downgrading to a previous release of Bitcoin Core during the background sync.
18. 
    hernanmarino commented at 5:45 am on February 7, 2024: contributor

    There’d be #27596 (comment) if someone wants to tackle it.

    Done, reviews welcome https://github.com/bitcoin/bitcoin/pull/29394

19. 
    paplorinc commented at 8:28 am on February 18, 2024: contributor

    #29394 is merged, anything else to do here?
20. 
    fanquake commented at 9:47 am on February 19, 2024: member

    There are more PRs open. i.e #29428.
21. 
    maflcko commented at 1:43 pm on February 19, 2024: member

    @paplorinc You can refer to the issue description, which says:

    Refer to the TODO comments in ./test/functional/feature_assumeutxo.py, or to the uncovered lines in the AssumeUtxo code via a coverage report. For example, https://maflcko.github.io/b-c-cov/total.coverage/src/validation.cpp.gcov.html

22. achow101 referenced this in commit 62ef33a718 on May 2, 2024
23. 
    fjahr commented at 5:14 pm on July 8, 2024: contributor

    I think this may be ready for closing since all remaining TODOs have been addressed IMO. There are three remaining PRs that still need to get merged but I think we can expect that nobody will start a new PR based on the TODOs comment anymore.

    For reference, the remaining PRs are #30403, #30320, #29996. See also #30403 (comment)

24. 
    fjahr commented at 5:19 pm on July 8, 2024: contributor

    Or rather, the problem is that the issue here is not specific and so none of the issues can close this issue because it’s unclear which will be the last one to get merged. I will recommend in the PRs that they mention the issue here as related to so it’s at least clear what is being worked on where.

Contributors
maflcko ryanofsky BrandonOdiwuor kashifs jrakibi Sjors mohamedawnallah nafisalawalidris hernanmarino paplorinc fanquake fjahr

Labels
Wallet Tests Consensus good first issue