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)
ConsumeTxMemPoolEntry is 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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | glozow, marcofleon |
| Stale ACK | sipa |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
22@@ -23,7 +23,15 @@ namespace {
23 const BasicTestingSetup* g_setup;
24 } // namespace
25
26+// Maximum number of things we will effect during an RBF
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.
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 10,000 * 1,000,000 > 2^31
Made a more direct limiting attempt. I will squash if it is preferred
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
127@@ -130,21 +128,21 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
128
129 mempool_txs.emplace_back(*parent);
130 const auto parent_entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, mempool_txs.back());
131- // We don't want to test things we won't add to mempool (which may cause overflow in size)
132- if (parent_entry.GetTxSize() >= MAX_STANDARD_TX_SIZE) {
133+ running_vsize_total += parent_entry.GetTxSize();
134+ if (running_vsize_total > std::numeric_limits<int32_t>::max()) {
135 return;
break to allow the test to proceed still, without the transaction that would make the total overflow?
mempool_txs vector.
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 016ed248ba0ae64e3f0c93bb47a2cd9b5e49cd85. 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.
0FUZZ=package_rbf ./src/test/fuzz/fuzz ../crash-d96abc99c0e424a47aa8f4080040e1bc3e3851f7
No crash.
0INFO: Running with entropic power schedule (0xFF, 100).
1INFO: Seed: 1016851959
2INFO: Loaded 1 modules (1208798 inline 8-bit counters): 1208798 [0x107caa4c0, 0x107dd169e),
3INFO: Loaded 1 PC tables (1208798 PCs): 1208798 [0x107dd16a0,0x109043480),
4./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
5Running: ../crash-d96abc99c0e424a47aa8f4080040e1bc3e3851f7
6Executed ../crash-d96abc99c0e424a47aa8f4080040e1bc3e3851f7 in 1 ms
7***
8*** NOTE: fuzzing was not performed, you have only
9*** executed the target code on a fixed set of inputs.
10***
I’ve also been running libFuzzer on package_rbf for a while now and the overflow bug hasn’t come up.