fuzz: explicitly cap the vsize of RBFs for diagram checks #29879
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
src/test/fuzz/rbf.cpp
Outdated
| @@ -23,7 +23,15 @@ namespace { | |||
| const BasicTestingSetup* g_setup; | |||
| } // namespace | |||
|
|
|||
| // Maximum number of things we will effect during an RBF | |||
There was a problem hiding this comment.
this is hopefully a conservative over-estimate since clusters should only be 101kvB total each
|
I'm not convinced about this approach. A nice thing about fuzz test is that they can be written to exercise scenarios which go beyond what matters in practice, as more extreme values may trigger bugs more easily (and those bugs may affect realistic cases too, but be harder to trigger there). More specifically, in this case it seems the issue is the code under test not working if the sum of sizes exceeds 2^31. The standard transaction size shouldn't matter for this code, so I'd prefer to restrict to supported cases more directly. E.g. by reading from the fuzzer only sizes in the range (1...2^31-1-sum_of_sizes_so_far) and/or stop adding transactions when the sum of sizes overflows. If that's too hard to do, the current approach is fine of course, but even then, there is no strict need to tie it to standard sizes. |
Pretty sure we can overflow if we allow ~1MvB transactions(which is effectively being done via sigops), as Made a more direct limiting attempt. I will squash if it is preferred |
9ec18cf to
914b6db
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
914b6db to
9d76d77
Compare
|
@glozow was waiting for positive feedback. Squashed. |
|
utACK 9d76d77 |
9d76d77 to
016ed24
Compare
|
pushed a fixup since I think it could have resulted in UB when selecting direct conflicts from the |
There was a problem hiding this comment.
I ran into this bug when I first started fuzzing package_rbf a couple weeks ago... was unsure of what to do about it at the time. But glad to see it addressed here.
ACK 016ed24. I ran libFuzzer on package_rbf on the current master branch until the overflow was encountered. Then I built the PR branch and ran the fuzzer using the crash input.
FUZZ=package_rbf ./src/test/fuzz/fuzz ../crash-d96abc99c0e424a47aa8f4080040e1bc3e3851f7No crash.
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1016851959
INFO: Loaded 1 modules (1208798 inline 8-bit counters): 1208798 [0x107caa4c0, 0x107dd169e),
INFO: Loaded 1 PC tables (1208798 PCs): 1208798 [0x107dd16a0,0x109043480),
./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
Running: ../crash-d96abc99c0e424a47aa8f4080040e1bc3e3851f7
Executed ../crash-d96abc99c0e424a47aa8f4080040e1bc3e3851f7 in 1 ms
***
*** NOTE: fuzzing was not performed, you have only
*** executed the target code on a fixed set of inputs.
***I've also been running libFuzzer on package_rbf for a while now and the overflow bug hasn't come up.
In master we are hitting a case where vsize transactions much larger than max standard size are causing an overflow in not-yet-exposed RBF diagram checking code: #29757 (comment)
ConsumeTxMemPoolEntryis creating entries with tens of thousands of sigops cost, causing the resulting RBFs to be "overly large".To fix this I cause the fuzz test to stop adding transactions to the mempool when we reach a potential overflow of
int32_t.