Add package evaluation fuzzer

instagibbs commented at 2:10 pm on September 11, 2023: member

This fuzzer target caught the issue in #28251 within 5 minutes on master branch, and an additional issue which I’ve applied a preliminary patch to cover.

Fuzzer target does the following:

Picks mempool confgs, including max package size, count, mempool size, etc
Generates 1 to 26 transactions with arbitrary coins/fees, the first N-1 spending only confirmed outpoints
Nth transaction, if >1, sweeps all unconfirmed outpoints in mempool
If N==1, it may submit it through single-tx submission path, to allow for more interesting topologies
Otherwise submits through package submission interface
Repeat 1-5 a few hundred times per mempool instance

In other words, it ends up building chains of txns in the mempool using parents-and-children packages, which is currently the topology supported on master.

The test itself is a direct rip of tx_pool.cpp, with a number of assertions removed because they were failing for unknown reasons, likely due to the notification changes of single tx submission to package, which is used to track addition/removal of transactions in the test. I’ll continue working on re-adding these assertions for further invariant testing.

DrahtBot commented at 2:10 pm on September 11, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	murchandamus, glozow, dergoegge
Concept ACK	darosior, brunoerg, MarcoFalke

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:

#26711 (validate package transactions with their in-package ancestor sets by glozow)

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.

instagibbs commented at 2:11 pm on September 11, 2023: member

cc @glozow @MarcoFalke

glozow added the label Tests on Sep 11, 2023

in src/test/fuzz/package_eval.cpp:88 in f06cf5b054 outdated

83+    args.ForceSetArg("-limitdescendantsize",
84+                     ToString(fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 202)));
85+    args.ForceSetArg("-maxmempool",
86+                     ToString(fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 200)));
87+    args.ForceSetArg("-mempoolexpiry",
88+                     ToString(fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 999)));

glozow commented at 2:15 pm on September 11, 2023:

Maybe worth adding -bytespersigop too?

instagibbs commented at 3:02 pm on September 11, 2023:

to take advantage of this, probably would need to do some bare mulitisigs? wouldn’t hurt to add I suppose

darosior commented at 2:18 pm on September 11, 2023: member

Awesome, Concept ACK.

glozow commented at 2:22 pm on September 11, 2023: member

concept ACK, really nice :rocket:

instagibbs force-pushed on Sep 11, 2023

DrahtBot added the label CI failed on Sep 11, 2023

fanquake requested review from dergoegge on Sep 11, 2023

in src/test/fuzz/package_eval.cpp:126 in 3654ad4152 outdated

127+    CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)};
128+
129+    // ...override specific options for this specific fuzz suite
130+    mempool_opts.estimator = nullptr;
131+    mempool_opts.check_ratio = 1;
132+    mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();

dergoegge commented at 3:08 pm on September 11, 2023:

could set all the options here instead of round tripping through the args man?

instagibbs commented at 6:06 pm on September 13, 2023:

done

dergoegge commented at 3:16 pm on September 11, 2023: member

Concept ACK

brunoerg commented at 3:46 pm on September 12, 2023: contributor

Concept ACK

DrahtBot added the label Needs rebase on Sep 13, 2023

instagibbs force-pushed on Sep 13, 2023

instagibbs renamed this:
~~WIP: Add package evaluation fuzzer~~
Add package evaluation fuzzer
on Sep 13, 2023

instagibbs commented at 6:09 pm on September 13, 2023: member

rebased on master, removed arg round-trip, un-marked WIP

DrahtBot removed the label CI failed on Sep 13, 2023

DrahtBot removed the label Needs rebase on Sep 13, 2023

maflcko commented at 9:52 am on September 14, 2023: member

Concept ACK, I guess this also picks up #25778 ?

glozow commented at 10:47 am on September 14, 2023: member

Any interest in using CheckPackageMempoolAcceptResult from https://github.com/bitcoin/bitcoin/pull/26711/commits/baf475de599c82e7718ffefd7f0481fc88217853? It checks that a result contains the fields we expect based on whether it’s supposed to be valid.

instagibbs commented at 2:00 pm on September 14, 2023: member

@MarcoFalke oh, had no idea it existed. Yes I think this should subsume it. I kept it a separate fuzz target to allow more specialization for what we’re covering.

Any interest in using CheckPackageMempoolAcceptResult from https://github.com/bitcoin/bitcoin/commit/baf475de599c82e7718ffefd7f0481fc88217853? It checks that a result contains the fields we expect based on whether it’s supposed to be valid.

I might take a subset of that, but a few parts wouldn’t be useful since I’m firing off random packages that may or may not be valid, and the mempool may trim things even if they’re valid?

edit: In other words, please consider this PR review-ready :+1:

in src/test/fuzz/package_eval.cpp:212 in 28f45d6883 outdated

206+                const auto amount_fee = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(0, amount_in);
207+                const auto amount_out = (amount_in - amount_fee) / num_out;
208+                for (int i = 0; i < num_out; ++i) {
209+                    tx_mut.vout.emplace_back(amount_out, P2WSH_OP_TRUE);
210+                }
211+                // TODO vary transaction sizes to catch size-related issues

instagibbs commented at 6:42 pm on September 14, 2023:

future work: making the tx >40kWu sometimes, along with other invariant checks, would have likely allowed the fuzzer to catch https://github.com/bitcoin/bitcoin/pull/28472

instagibbs force-pushed on Sep 15, 2023

instagibbs commented at 1:46 pm on September 15, 2023: member

removed the second commit to not impede review of the various bugfix PRs individually

rebasing on this should result in no known crashes: #28471 https://github.com/bitcoin/bitcoin/pull/28472

in src/test/fuzz/package_eval.cpp:140 in 93d7c5c09c outdated

135+    MockTime(fuzzed_data_provider, chainstate);
136+
137+    // All RBF-spendable outpoints outside of the immediate package
138+    std::set<COutPoint> outpoints_rbf;
139+    // All outpoints counting toward the total supply (subset of outpoints_rbf)
140+    std::set<COutPoint> outpoints_supply;

achow101 commented at 11:26 am on September 20, 2023:

I don’t think this set is ever actually used.

instagibbs commented at 2:01 pm on September 20, 2023:

it is added to in insert_tx when used in created_by_tx arg. Lets each new package consider newly-entered outpoints in the mempool

glozow commented at 12:46 pm on September 25, 2023:

I think achow is wondering if this is redundant with outpoints_rbf? Since new transactions are always created using outpoints_rbf inputs.

instagibbs commented at 1:38 pm on September 26, 2023:

was used prior but no longer, removed until it’s needed again

in src/test/fuzz/package_eval.cpp:295 in 93d7c5c09c outdated

290+                    removed.erase(tx);
291+                }
292+            } else {
293+                for (const auto& [k, v] : result_package.m_tx_results) {
294+                    if (v.m_result_type != MempoolAcceptResult::ResultType::INVALID) {
295+                        Assert(true);

achow101 commented at 11:26 am on September 20, 2023:

Assert(true) seems to not be useful.

instagibbs commented at 2:02 pm on September 20, 2023:

removed debugging code

in src/test/fuzz/package_eval.cpp:118 in 93d7c5c09c outdated

110+
111+
112+    // ...override specific options for this specific fuzz suite
113+    mempool_opts.limits.ancestor_count = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 50);
114+    mempool_opts.limits.ancestor_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 202) * 1'000;
115+    mempool_opts.limits.descendant_count = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 50);

achow101 commented at 11:27 am on September 20, 2023:

Seems a bit odd to me that the chain limits could go up to 50 but we only create packages of up to 26.

instagibbs commented at 1:40 pm on September 20, 2023:

That’s chain limits vs package limit checks, I think it’s useful

e.g., we could have 26 txns in the mempool, and submit another 25

instagibbs force-pushed on Sep 20, 2023

instagibbs commented at 3:45 pm on September 20, 2023: member

updated with cleanups

instagibbs commented at 10:27 am on September 21, 2023: member

from discussions with others:

I have a PR built on #26711 which generalized from this fairly basic topology to general ancestor packages, including the last tx spending any subset of outpoints, and any package transactions potentially spending other package outpoints. I’d like to make sure it’s getting relatively targeted coverage before switching to that, so deferring to after this PR.
We can add coverage where we mutate the wtxid of txns
Also should allow duplication of inputs

I plan on doing these in the follow-up on top of #26711

in src/test/fuzz/package_eval.cpp:263 in bf7436cde8 outdated

252+        // to allow it into the mempool by itself to make more interesting mempool packages
253+        auto single_submit = txs.size() == 1 && fuzzed_data_provider.ConsumeBool();
254+        auto package_submit = !single_submit;
255+
256+        const auto result_package = WITH_LOCK(::cs_main,
257+                                    return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/!package_submit));

brunoerg commented at 10:37 am on September 21, 2023:

In bf7436cde8ba7eac0912817f6f8a41828cb60bc2: In ProcessNewPackage, couldn’t we just use single_submit for /*test_accept=*/ directly, instead of creating package_submit?

instagibbs commented at 11:24 am on September 21, 2023:

I find this ever so slightly more readable myself, opinions may vary

murchandamus commented at 7:16 pm on September 27, 2023:

I also found the new variable package_submit confusing as well. I don’t see the point of creating an additional variable, especially since it’s only passed once and negated.

instagibbs commented at 8:29 pm on September 27, 2023:

ok fine, removed :)

in src/test/fuzz/package_eval.cpp:155 in bf7436cde8 outdated

150+    CTxMemPool tx_pool_{MakeMempool(fuzzed_data_provider, node)};
151+    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(&tx_pool_);
152+
153+    chainstate.SetMempool(&tx_pool);
154+
155+    const CCoinsViewMemPool amount_view{WITH_LOCK(::cs_main, return &chainstate.CoinsTip()), tx_pool};

brunoerg commented at 10:45 am on September 21, 2023:

In bf7436cde8ba7eac0912817f6f8a41828cb60bc2: I think amount_view hasn’t been used.

instagibbs commented at 11:24 am on September 21, 2023:

deleted

instagibbs force-pushed on Sep 21, 2023

instagibbs commented at 4:17 pm on September 21, 2023: member

rebased on latest master on top of #28471

Should not be crashing anymore :crossed_fingers: cc @dergoegge

in src/test/fuzz/package_eval.cpp:84 in 2ecdc2183f outdated

79+        BlockAssembler::Options options;
80+        options.nBlockMaxWeight = fuzzed_data_provider.ConsumeIntegralInRange(0U, MAX_BLOCK_WEIGHT);
81+        options.blockMinFeeRate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)};
82+        auto assembler = BlockAssembler{chainstate, &tx_pool, options};
83+        auto block_template = assembler.CreateNewBlock(CScript{} << OP_TRUE);
84+        Assert(block_template->block.vtx.size() >= 1);

murchandamus commented at 2:41 pm on September 22, 2023:

Since every block needs a coinbase transaction, is >= 1 really what you wanted to check here?

glozow commented at 11:51 am on September 25, 2023:

Question: this effectively tests that CreateNewBlock creates a coinbase, is that intended?

instagibbs commented at 1:39 pm on September 26, 2023:

removed as it’s not very interesting a check

instagibbs commented at 8:24 pm on September 27, 2023:

removed this already circa https://github.com/bitcoin/bitcoin/pull/28450/commits/70879e4f02e522ee82b563917ef6700efe4cf5dd

in src/test/fuzz/package_eval.cpp:234 in 2ecdc2183f outdated

225+            txs.push_back(tx);
226+            wtxid_to_tx[tx->GetWitnessHash()] = tx;
227+        }
228+
229+        if (fuzzed_data_provider.ConsumeBool()) {
230+            MockTime(fuzzed_data_provider, chainstate);

brunoerg commented at 4:39 pm on September 24, 2023:

Why is it calling MockTime again? I guess to trigger the nLockTime?

instagibbs commented at 7:12 pm on September 24, 2023:

it’s called for each package separately, sometimes, yes

in src/test/fuzz/package_eval.cpp:244 in 2ecdc2183f outdated

235+        if (fuzzed_data_provider.ConsumeBool()) {
236+            const auto& txid = fuzzed_data_provider.ConsumeBool() ?
237+                                   txs.back()->GetHash() :
238+                                   PickValue(fuzzed_data_provider, outpoints_rbf).hash;
239+            const auto delta = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(-50 * COIN, +50 * COIN);
240+            tx_pool.PrioritiseTransaction(txid, delta);

brunoerg commented at 5:27 pm on September 24, 2023:

In 2ecdc2183f0a1291730ae8728fa3d847b9cf80b5: Couldn’t we prioritise more than 1 transaction?

instagibbs commented at 7:16 pm on September 24, 2023:

yes, I’m not sure if it will end up covering more interesting cases though

in src/test/fuzz/package_eval.cpp:276 in 2ecdc2183f outdated

271+        UnregisterSharedValidationInterface(txr);
272+
273+        if (single_submit) {
274+            Assert(accepted != added.empty());
275+            Assert(accepted == res.m_state.IsValid());
276+            Assert(accepted != res.m_state.IsInvalid());

brunoerg commented at 5:35 pm on September 24, 2023:

In 2ecdc2183f0a1291730ae8728fa3d847b9cf80b5: Does it make sense to check whether the state is invalid if we previously assert that is valid?

instagibbs commented at 7:17 pm on September 24, 2023:

sorry can you be a bit more explicit in what you’re asking to do?

glozow commented at 12:11 pm on September 25, 2023:

I think he’s suggesting that the checks are redundant and to remove one

in src/test/fuzz/package_eval.cpp:92 in 2ecdc2183f outdated

87+    if (!info_all.empty()) {
88+        const auto& tx_to_remove = *PickValue(fuzzed_data_provider, info_all).tx;
89+        WITH_LOCK(tx_pool.cs, tx_pool.removeRecursive(tx_to_remove, MemPoolRemovalReason::BLOCK /* dummy */));
90+        std::vector<uint256> all_txids;
91+        tx_pool.queryHashes(all_txids);
92+        assert(all_txids.size() < info_all.size());

glozow commented at 11:58 am on September 25, 2023:

Could be slightly stronger by getting the descendant count of tx_to_remove and checking here that all_txids.size() == info_all.size() - descendant_count

instagibbs commented at 1:39 pm on September 26, 2023:

done

in src/test/fuzz/package_eval.cpp:260 in 2ecdc2183f outdated

248+        const bool bypass_limits = fuzzed_data_provider.ConsumeBool();
249+
250+        // Single-tx packages should be rejected, so do that sometimes, and sometimes send it via single submission
251+        // to allow it into the mempool by itself to make more interesting mempool packages
252+        auto single_submit = txs.size() == 1 && fuzzed_data_provider.ConsumeBool();
253+        auto package_submit = !single_submit;

glozow commented at 12:20 pm on September 25, 2023:

This was a bit confusing. Perhaps adding this comment beforehand would help clarify things

“When there are multiple transactions in the package, we call ProcessNewPackage(txs, test_accept=false) and AcceptToMemoryPool(txs.back(), test_accept=true). When there is only 1 transaction, we might flip it (the package is a test accept and ATMP is a submission).”

glozow commented at 4:41 pm on September 25, 2023:

Also… wondering why allow num_txs to be less than 2 anyway? Isn’t this test kind of the same as what’s in the tx_pool fuzzer?

instagibbs commented at 1:40 pm on September 26, 2023:

Also… wondering why allow num_txs to be less than 2 anyway?

it can get rejected by ProcessNewPackage
it can cause the existing mempool to be somewhat more interesting maybe

instagibbs commented at 12:43 pm on September 28, 2023:

Oh I was mistaken, AcceptPackage takes package sizes of 1(I was thinking of BIP331 probably!). This PR could have been a bit simpler I think by removing single accept path.

in src/test/fuzz/package_eval.cpp:227 in 2ecdc2183f outdated

217+                }
218+                // Cache the in-package outpoints being made
219+                for (size_t i = 0; i < tx->vout.size(); ++i) {
220+                    package_outpoints.emplace(tx->GetHash(), i);
221+                    outpoints_value[COutPoint(tx->GetHash(), i)] = tx->vout[i].nValue;
222+                }

glozow commented at 12:53 pm on September 25, 2023:

This can also go under if (!last_tx), right? There’s no need to add the last tx’s outputs since we won’t need them

instagibbs commented at 1:40 pm on September 26, 2023:

done, had to keep the outpoints_value check outside though since it needs all generated outpoints

in src/test/fuzz/package_eval.cpp:277 in 2ecdc2183f outdated

268+        const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID;
269+
270+        SyncWithValidationInterfaceQueue();
271+        UnregisterSharedValidationInterface(txr);
272+
273+        if (single_submit) {

glozow commented at 12:55 pm on September 25, 2023:

Would be nice to remind ourselves what this means, e.g. “There is only 1 transaction in the package. We did a test-package-accept and a ATMP”

instagibbs commented at 1:39 pm on September 26, 2023:

done

in src/test/fuzz/package_eval.cpp:214 in 2ecdc2183f outdated

207+                for (int i = 0; i < num_out; ++i) {
208+                    tx_mut.vout.emplace_back(amount_out, P2WSH_OP_TRUE);
209+                }
210+                // TODO vary transaction sizes to catch size-related issues
211+                auto tx = MakeTransactionRef(tx_mut);
212+                // Restore previously removed outpoints, except in-package outpoints

glozow commented at 1:05 pm on September 25, 2023:

Question: this means transactions in the package can conflict with each other, right? It’s possible to select the same outpoint from outpoints_rbf for 2 parents in the same package?

instagibbs commented at 1:39 pm on September 26, 2023:

yes

in src/test/fuzz/package_eval.cpp:173 in 2ecdc2183f outdated

169+            // so the last transaction to be generated(in a >1 package) must spend all package-made outputs
170+            // Note that this test currently only spends unconfirmed outputs in last transaction.
171+            bool last_tx = num_txs > 1 && txs.size() == num_txs - 1;
172+
173+            // Create transaction to add to the mempool
174+            const CTransactionRef tx = [&] {

glozow commented at 1:09 pm on September 25, 2023:

maybe add Assert(!outpoints_rbf.empty()) at the top

instagibbs commented at 1:39 pm on September 26, 2023:

done

in src/test/fuzz/package_eval.cpp:287 in 2ecdc2183f outdated

329+                outpoints_rbf.erase(p);
330+            }
331+            for (const auto& p : consumed_supply) {
332+                outpoints_supply.erase(p);
333+            }
334+        }

glozow commented at 1:14 pm on September 25, 2023:

This is kinda convoluted… could we just update outpoints_supply and outpoints_rbf directly on the TransactionAddedToMempool and TransactionRemovedFromMempool callbacks instead of the added/removed sets? took a stab in https://github.com/glozow/bitcoin/commit/df433f82364502b443d433918af0a0cf4196d210

instagibbs commented at 1:39 pm on September 26, 2023:

taken, thanks

glozow commented at 4:43 pm on September 25, 2023: member

A few quick questions, looks correct to me though some bits are a little confusing. I know future updates are planned so feel free to save for later if I’m scope creeping too much.

glozow commented at 4:45 pm on September 25, 2023: member

CI failure is a functional test so I’d assume unrelated

dergoegge commented at 10:24 am on September 26, 2023: member

Made a coverage report: https://dergoegge.github.io/bitcoin-coverage/pr28450/fuzz.coverage/index.html

glozow commented at 11:12 am on September 26, 2023: member

Made a coverage report: https://dergoegge.github.io/bitcoin-coverage/pr28450/fuzz.coverage/index.html

Looks pretty good to me, noting the main missing bits:

same-txid-different-witness
policy script checks failing
bad-txns-too-many-sigops
being granted CPFP carve out and then failing

^which have already been marked as followups.

There’s no package-not-child-with-unconfirmed-parents which is by design. Also looks like RBF rules are red, perhaps it’ll take a while to hit them?

dergoegge commented at 12:04 pm on September 26, 2023: member

Also looks like RBF rules are red, perhaps it’ll take a while to hit them?

The seed corpus from the report was the result of 5000+ cpu hours, so I’d say it’s likely just not able to hit the rbf paths.

instagibbs force-pushed on Sep 26, 2023

instagibbs commented at 1:42 pm on September 26, 2023: member

Also looks like RBF rules are red, perhaps it’ll take a while to hit them?

A couple of those cases would look difficult to hit without intention/design to do so. One I’m unsure how it hasn’t been hit, something to look at later especially as we approach package rbf.

I think achow is wondering if this is redundant with outpoints_rbf? Since new transactions are always created using outpoints_rbf inputs.

Can’t find the actual place to reply(thanks github), removed outpoints_supply entirely as it’s vestigial

Took all suggestions, pushed

in src/test/fuzz/package_eval.cpp:28 in 3707ed7c0d outdated

23+
24+namespace {
25+
26+const TestingSetup* g_setup;
27+std::vector<COutPoint> g_outpoints_coinbase_init_mature;
28+std::vector<COutPoint> g_outpoints_coinbase_init_immature;

glozow commented at 3:03 pm on September 26, 2023:

note: g_outpoints_coinbase_init_immature seems unused rn, I assume the plan is to maybe spend immature coinbases later?

instagibbs commented at 12:51 pm on September 27, 2023:

removed, won’t be used in near future

in src/test/fuzz/package_eval.cpp:199 in 3707ed7c0d outdated

194+        std::set<COutPoint> package_outpoints;
195+        while (txs.size() < num_txs) {
196+
197+            // Last transaction in a package needs to be a child of parents to get further in validation
198+            // so the last transaction to be generated(in a >1 package) must spend all package-made outputs
199+            // Note that this test currently only spends unconfirmed outputs in last transaction.

glozow commented at 3:05 pm on September 26, 2023:

nit: outpoints_rbf contains unconfirmed, no?

0            // Note that this test currently only spends package outputs in last transaction.

instagibbs commented at 12:51 pm on September 27, 2023:

taken

in src/test/fuzz/package_eval.cpp:318 in 3707ed7c0d outdated

313+            if (accepted) {
314+                Assert(added.size() == 1);
315+                Assert(txs.back() == *added.begin());
316+            } else {
317+                // Do not consider rejected transaction removed
318+                removed.erase(txs.back());

glozow commented at 3:11 pm on September 26, 2023:

nit: I think these removed.erase() calls might be unnecessary now that you don’t use it to update outpoints_rbf.

instagibbs commented at 12:51 pm on September 27, 2023:

removed removed entirely

glozow commented at 3:20 pm on September 26, 2023: member

ACK 3707ed7c0d02d86ff86c159bbd746acfa52db145 Code and coverage look good. Will run overnight to sanity check that outpointsupdater hasn’t broken anything.

Couple more ideas for post-26711 expansions:

add nonexistent outpoint to hit missing inputs cases
shuffle before submission

in src/test/fuzz/package_eval.cpp:279 in 3707ed7c0d outdated

274+            const auto delta = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(-50 * COIN, +50 * COIN);
275+            tx_pool.PrioritiseTransaction(txid, delta);
276+        }
277+
278+        // Remember all removed and added transactions
279+        std::set<CTransactionRef> removed;

achow101 commented at 5:20 pm on September 26, 2023:

removed doesn’t seem to actually be used anywhere.

instagibbs commented at 12:51 pm on September 27, 2023:

removed removed entirely

instagibbs force-pushed on Sep 27, 2023

instagibbs commented at 12:52 pm on September 27, 2023: member

Removed the Final subroutine to get a slight speedup since it seems unnecessary, leaving just the tx_pool.check() which should catch all issues.

instagibbs force-pushed on Sep 27, 2023

DrahtBot added the label CI failed on Sep 27, 2023

instagibbs commented at 1:47 pm on September 27, 2023: member

Going to hold off on all subsequent nits to get this in to un-draft #26711 and start building on top

instagibbs commented at 2:36 pm on September 27, 2023: member

https://github.com/bitcoin/bitcoin/pull/28450/checks?check_run_id=17181111860 multiprocess build having an issue of some kind

in src/test/fuzz/package_eval.cpp:96 in 6e46822537 outdated

91+        m_added.insert(tx);
92+    }
93+
94+    void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override
95+    {
96+        // Transactions may be entered and booted any number of times

glozow commented at 2:46 pm on September 27, 2023:

I think this can cause a false positive crash on line 281. In the “mempool full” case where a tx gets added and then removed in TrimToSize(), it’ll fire TransactionAddedToMempool and TransactionRemovedFromMempool. Then accepted will be false but added won’t be empty.

0        // Transactions may be entered and booted any number of times
1        m_added.erase(tx);

instagibbs commented at 5:24 pm on September 27, 2023:

Sounds right, surprised I haven’t hit this since it quite often chooses tiny mempool sizes too small for even a single transaction to enter.

Taken.

DrahtBot removed the label CI failed on Sep 27, 2023

instagibbs force-pushed on Sep 27, 2023

in src/test/fuzz/package_eval.cpp:161 in 70879e4f02 outdated

156+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)
157+    {
158+        Assert(!outpoints_rbf.empty());
159+
160+        std::vector<CTransactionRef> txs;
161+        std::map<uint256, CTransactionRef> wtxid_to_tx;

murchandamus commented at 7:50 pm on September 27, 2023:

This map appears to get filled but never read from.

instagibbs commented at 8:27 pm on September 27, 2023:

removed

in src/test/fuzz/package_eval.cpp:141 in 70879e4f02 outdated

136+    auto& chainstate{static_cast<DummyChainState&>(node.chainman->ActiveChainstate())};
137+
138+    MockTime(fuzzed_data_provider, chainstate);
139+
140+    // All RBF-spendable outpoints outside of the unsubmitted package
141+    std::set<COutPoint> outpoints_rbf;

murchandamus commented at 7:53 pm on September 27, 2023:

I found this name confusing, because it doesn’t really seem related to RBF, but rather just a list of all outpoints that were ever available for spending, even if they already got spent by another transaction.

instagibbs commented at 8:26 pm on September 27, 2023:

was going to change this anyways, renaming to mempool_outpoints

Add package evaluation fuzzer 262ab8ef78

instagibbs force-pushed on Sep 27, 2023

murchandamus commented at 8:35 pm on September 27, 2023: contributor

ACK 262ab8ef7860d43cebc9d04721e3a075b4edf06e

Note that this is the first time I review something from the package relay project, so take that for whatever it’s worth

DrahtBot requested review from glozow on Sep 27, 2023

glozow commented at 10:31 am on September 28, 2023: member

reACK 262ab8ef7860d43cebc9d04721e3a075b4edf06e

glozow requested review from dergoegge on Sep 28, 2023

dergoegge approved

dergoegge commented at 10:59 am on September 28, 2023: member

tACK 262ab8ef7860d43cebc9d04721e3a075b4edf06e

Will generate and submit inputs to qa-assets once this is merged

glozow merged this on Sep 28, 2023

glozow closed this on Sep 28, 2023

Frank-GER referenced this in commit 461bf19b50 on Oct 5, 2023

bitcoin locked this on Sep 27, 2024

Add package evaluation fuzzer #28450

Reviews

Conflicts