fuzz: Add check in p2p_headers_presync that chain work never exceeds minimum work #30918

1. 
   marcofleon commented at 5:21 pm on September 17, 2024: contributor

   A followup to #30661

   The added assertion just makes sure that the fuzz test is working as intended. If we’re sure that the total work of the test chain is never more than minimum chain work, then we can be sure that the later assertion failure would actually mean that a bug in the headers presync logic was found.

   This PR also addresses: #30661 (review) #30661 (review) #30661 (review)

2. 
   DrahtBot commented at 5:21 pm on September 17, 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	instagibbs, maflcko, achow101

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

3. DrahtBot added the label Tests on Sep 17, 2024
4. in src/test/fuzz/p2p_headers_presync.cpp:207 in 7b85fd1443 outdated

   201             });
   202+
   203+        total_work += CalculateClaimedHeadersWork(all_headers);
   204+
   205+        // This test should never create a chain with more work than MinimumChainWork.
   206+        assert(total_work < chainman.MinimumChainWork());
   

   ---

   ---

   

   instagibbs commented at 1:10 pm on September 18, 2024:

   IIUC this would be a conservative overestimate, since the compact block and block messages don’t move base forward?

   If so, probably should note that here.

   ---

   

   marcofleon commented at 2:08 pm on September 20, 2024:

   That’s true, good note. Added a claifying comment above https://github.com/bitcoin/bitcoin/blob/284bd17309ab3b124d9dcddfec62f5506383343b/src/test/fuzz/p2p_headers_presync.cpp#L204
5. instagibbs approved
6. 
   instagibbs commented at 1:14 pm on September 18, 2024: member

   ACK 7b85fd1443488afe60235e4c33bd956cb9c8d562

   Thanks for the quick follow-up on suggestion! Makes the test significantly easier to read.

7. add clarification in comment 9aa5d1c3fc
8. add check that chainwork doesn't exceed minimum work 284bd17309
9. marcofleon force-pushed on Sep 20, 2024
10. 
    marcofleon commented at 2:12 pm on September 20, 2024: contributor

    Updated the PR description to include some additional changes.
11. 
    instagibbs commented at 2:26 pm on September 20, 2024: member

    reACK 284bd17309ab3b124d9dcddfec62f5506383343b
12. 
    maflcko commented at 2:46 pm on September 20, 2024: member

    review ACK 284bd17309ab3b124d9dcddfec62f5506383343b
13. 
    achow101 commented at 6:27 pm on September 20, 2024: member

    ACK 284bd17309ab3b124d9dcddfec62f5506383343b
14. achow101 merged this on Sep 20, 2024
15. achow101 closed this on Sep 20, 2024

    ---

Contributors
marcofleon DrahtBot instagibbs maflcko achow101

Labels
Tests