fuzz: Test headers pre-sync through p2p interface #28043

pull dergoegge wants to merge 3 commits into bitcoin:master from dergoegge:2023-07-fuzz-pow changing 6 files +215 −9
  1. dergoegge commented at 12:05 pm on July 7, 2023: member

    This PR adds a regression fuzz test for #26355 and some of the bugs found during review of #25717.

    Should give us more confidence in doing #25725.

  2. DrahtBot commented at 12:05 pm on July 7, 2023: 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
    Concept ACK darosior

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Tests on Jul 7, 2023
  4. dergoegge marked this as a draft on Jul 7, 2023
  5. dergoegge commented at 12:06 pm on July 7, 2023: member
    Draft until #27499 is in.
  6. in src/net_processing.h:67 in 4407469bd6 outdated 
    51+        bool ignore_incoming_txs{false};
52+        bool reconcile_txs{false};
53+        uint32_t max_orphan_txs{DEFAULT_MAX_ORPHAN_TRANSACTIONS};
54+        size_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN};
55+        bool capture_messages{false};
56+        uint32_t max_headers_result{MAX_HEADERS_RESULTS};
    maflcko commented at 9:38 am on July 8, 2023:
    Looks like this is the only option that is required by the fuzz target, so maybe remove the others for now to hopefully reduce the number of conflicting pulls and remove the dependency on the other pull?
    dergoegge commented at 10:37 am on July 10, 2023:
    3 of the PRs are mine (I don’t care about rebasing) and the package relay stuff is gonna conflict either way, so I’d prefer keeping it as is.
  7. DrahtBot added the label CI failed on Jul 18, 2023
  8. in src/test/fuzz/p2p_headers_sync.cpp:127 in 4407469bd6 outdated 
    122+void initialize()
123+{
124+    // Reduce proof-of-work check to one bit.
125+    g_check_pow_mock = [](uint256 hash, unsigned int, const Consensus::Params&) {
126+        return (hash.data()[31] & 0x80) == 0;
127+    };
    darosior commented at 7:21 pm on July 24, 2023:
    Why not disabling it completely instead of mocking it?
    dergoegge commented at 12:23 pm on July 25, 2023:
    My thinking was that this should result in more coverage because we’ll also cover paths for invalid PoW.
  9. DrahtBot added the label Needs rebase on Jul 25, 2023
  10. dergoegge force-pushed on Jul 25, 2023
  11. dergoegge marked this as ready for review on Jul 25, 2023
  12. dergoegge commented at 12:20 pm on July 25, 2023: member
    Rebased and un-drafted, ready for review!
  13. dergoegge force-pushed on Jul 25, 2023
  14. in src/test/fuzz/p2p_headers_sync.cpp:159 in 8a7feb6573 outdated 
    154+            CBlock block = ConsumeBlock(fuzzed_data_provider, prev.GetHash(), prev.nBits);
155+            FinalizeHeader(block);
156+            return block;
157+        };
158+
159+        std::vector<std::function<void()>> actions = {
    maflcko commented at 12:35 pm on July 25, 2023:
    How is this different from CallOneOf?
    dergoegge commented at 11:12 am on July 30, 2023:
    it’s not, iirc I had some trouble with thread safety annotations but I’ll give CallOneOf another try
    maflcko commented at 7:29 am on July 31, 2023:

    I don’t think you can use annotations one way or the other. As soon as you assign to std::function<void()>, the annotations are dropped, so you might as well not have them in the first place.

    If you want them, and use CallOneOf, you can use the same trick to wrap each annotated lambda into a std::function<void()>{lambda_bla} temporary to achieve the same result of dropping any annotations.

    dergoegge commented at 8:27 am on July 31, 2023:
    switched to CallOneOf and used NO_THREAD_SAFETY_ANALYSIS.
  15. DrahtBot removed the label Needs rebase on Jul 25, 2023
  16. DrahtBot removed the label CI failed on Jul 25, 2023
  17. dergoegge force-pushed on Jul 31, 2023
  18. DrahtBot added the label CI failed on Aug 26, 2023
  19. dergoegge force-pushed on Sep 11, 2023
  20. dergoegge force-pushed on Sep 11, 2023
  21. dergoegge force-pushed on Sep 12, 2023
  22. DrahtBot removed the label CI failed on Sep 12, 2023
  23. DrahtBot added the label Needs rebase on Oct 5, 2023
  24. dergoegge force-pushed on Oct 6, 2023
  25. DrahtBot removed the label Needs rebase on Oct 8, 2023
  26. laanwj added this to the milestone 27.0 on Oct 26, 2023
  27. DrahtBot added the label CI failed on Nov 17, 2023
  28. maflcko commented at 10:34 am on November 17, 2023: member 
     0test/fuzz/p2p_headers_sync.cpp:187:96:   required from here
 1./primitives/transaction.h:327:42: error: ‘class CVectorWriter’ has no member named ‘GetParams’
 2  327 |         SerializeTransaction(*this, s, s.GetParams());
 3      |                                        ~~^~~~~~~~~
 4make[2]: *** [Makefile:17531: test/fuzz/fuzz-p2p_headers_sync.o] Error 1
 5make[2]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
 6make[1]: *** [Makefile:20246: install-recursive] Error 1
 7make[1]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
 8make: *** [Makefile:811: install-recursive] Error 1
 9
10Exit status: 2������������������������������������������������������������������������������������������������������������������������
  29. dergoegge force-pushed on Nov 20, 2023
  30. dergoegge commented at 4:09 pm on November 20, 2023: member
    Thanks! rebased.
  31. darosior commented at 4:11 pm on November 20, 2023: member
    Concept ACK. Planning on reviewing soon.
  32. DrahtBot removed the label CI failed on Nov 20, 2023
  33. DrahtBot added the label CI failed on Nov 30, 2023
  34. mzumsande commented at 9:20 pm on December 1, 2023: contributor
    Needs rebase (silent merge conflict).
  35. dergoegge force-pushed on Dec 6, 2023
  36. dergoegge commented at 3:01 pm on December 6, 2023: member

    Needs rebase (silent merge conflict).

    Thanks, rebased

  37. DrahtBot removed the label CI failed on Dec 6, 2023
  38. in src/pow.cpp:129 in a95524240b outdated 
    121@@ -122,8 +122,14 @@ bool PermittedDifficultyTransition(const Consensus::Params& params, int64_t heig
122     return true;
123 }
124 
125+std::function<bool(uint256, unsigned int, const Consensus::Params&)> g_check_pow_mock = nullptr;
126+
127 bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params& params)
128 {
129+    if (g_check_pow_mock) {
    jamesob commented at 7:29 pm on January 9, 2024:
    Should probably have some belt-and-suspenders check that we’re 100% not running on mainnet, maybe based on the contents of params and whatever applicable global.
    dergoegge commented at 11:06 am on January 24, 2024:
    An alternative to the mocking would be to use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in all places that call CheckProofOfWork (i.e. validation, mining) since there is literally zero benefit in having this be a blocker for fuzzing.
    dergoegge commented at 2:19 pm on January 24, 2024:
    Opened #29305 to discuss, I think it actually makes more sense than the mocking
  39. DrahtBot added the label CI failed on Jan 13, 2024
  40. Allow mocking CheckProofOfWork 467400cf31
  41. [net processing] Make MAX_HEADERS_RESULT a PeerManager option 355a43cb7e
  42. [fuzz] Test headers sync through the p2p interface 4516b454ba
  43. dergoegge force-pushed on Jan 23, 2024
  44. DrahtBot removed the label CI failed on Jan 23, 2024
  45. dergoegge commented at 11:41 am on January 26, 2024: member
    It’s been more than 6 months since I opened this without substantial review, so closing. Let me know if anyone is actually interested in reviewing and I’ll reopen.
  46. dergoegge closed this on Jan 26, 2024
  47. mzumsande commented at 4:21 pm on January 26, 2024: contributor

    just fyi, I started reviewing at some point, my problem was that I don’t like test-specific / mocking code in production code (especially the consensus-critical parts), so I wasn’t really comfortable (concept)-acking it. On the other hand, I didn’t see a better way to do it and fuzzing is great obviously…

    As a result, I was undecided and didn’t write anything. Maybe it’d be worth to have a general discussion about the extent of mocking / test-specific production code we want as a project, since there are obviously both advantages and disadvantages to it. If there was a general consensus that these kind of mocks are fine I’d be happy to review/ack the actual fuzz target.

  48. maflcko commented at 11:46 am on January 29, 2024: member
    I’d say they are fine, if there is no better way. With global functions and variables, I think the only way to mock them is also via a global approach. Here, adding the assert(params!=main) (https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1446512814) should be fine.


dergoegge DrahtBot maflcko darosior mzumsande jamesob

Labels
Tests

Milestone
27.0

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: 2024-07-05 19:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me