hopefully resolves #30241
The fuzz target is generating a large amount of transactions, but the core of the logic is ConsumeTxMemPoolEntry making the mempool entries for adding to the mempool. Since ConsumeTxMemPoolEntry generates its own transaction “vsize”, we can improve efficiency of the target by explicitly creating very small transactions, reducing the hashing and memory burden.
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 | maflcko, hodlinator, glozow |
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.
The fuzz target is generating a large amount of
transactions, but the core of the logic is
ConsumeTxMemPoolEntry making the mempool
entries for adding to the mempool. Since
ConsumeTxMemPoolEntry generates its own transaction
"vsize", we can improve efficiency of the target
by explicitly creating very small transactions,
reducing the hashing and memory burden.
ConsumeTxMemPoolEntry generates its own transaction “vsize”,
Can you add a reference to this claim? Looking at src/test/fuzz/util/mempool.cpp, I couldn’t find it.
sig_op_cost?
lgtm ACK 4ccb3d6d0d576d32da8a1b9c6e70962cbd0f19fe
(Assuming my assumption about the vsize mocking via sigop cost is correct)
122+ CMutableTransaction parent;
123 assert(iter <= g_outpoints.size());
124- parent->vin.resize(1);
125- parent->vin[0].prevout = g_outpoints[iter++];
126+ parent.vin.resize(1);
127+ parent.vin[0].prevout = g_outpoints[iter++];
Might as well
0 parent.vin.emplace_back(g_outpoints[iter++]);
114@@ -113,15 +115,13 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
115 LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), NUM_ITERS)
116 {
117 // Make sure txns only have one input, and that a unique input is given to avoid circular references
vins of parent generated from fuzz data are overwritten with one input. Time to change/remove?
129@@ -130,10 +130,10 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
130 break;
131 }
132 pool.addUnchecked(parent_entry);
133- if (fuzzed_data_provider.ConsumeBool() && !child->vin.empty()) {
134- child->vin[0].prevout = COutPoint{mempool_txs.back().GetHash(), 0};
135+ if (fuzzed_data_provider.ConsumeBool()) {
136+ child.vin[0].prevout = COutPoint{mempool_txs.back().GetHash(), 0};
Good that you removed the unnecessary vin.empty() check.
Until the first time ConsumeBool() returns true here, we’ll be using a child with a default-initialized vin[0]. This was true before the change as well. Does some later code hook up default-constructed CTxIn to coinbase transactions or something?
It seems unnecessary to have created a parent at all if the ConsumeBool() returns false. But I guess it’s good to test adding unrelated transactions between each child.
ACK 4ccb3d6d0d576d32da8a1b9c6e70962cbd0f19fe
Agree value of GetTxSize() is chosen by ConsumeTxMemPoolEntry and conflicts aren’t calculated using outpoints, so there’s no need to make mempool transactions through ConsumeDeserializable<CMutableTransaction>.
seed 95a3b1b42d43ac7190f24ff116b2cd6515e7a566 didn’t give me an oom but took 1981ms / now takes 71ms.
package_rbf fuzzer with the current qa-assets fuzz inputs by about 600 LOC. I’ll make a PR to update the fuzz inputs.
ConsumeDeserializable<CMutableTransaction> can no longer be hit. Also, the fuzz input format changed, but that should fix itself over time, as new fuzz inputs are found.