Add package evaluation fuzzer #28450

instagibbs · 2023-09-11T14:10:48Z

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 · 2023-09-11T14:10:51Z

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 · 2023-09-11T14:11:27Z

cc @glozow @MarcoFalke

darosior · 2023-09-11T14:18:02Z

Awesome, Concept ACK.

glozow

concept ACK, really nice 🚀

src/test/fuzz/package_eval.cpp

dergoegge

Concept ACK

src/test/fuzz/package_eval.cpp

brunoerg

Concept ACK

instagibbs · 2023-09-13T18:09:05Z

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

maflcko · 2023-09-14T09:52:45Z

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

glozow · 2023-09-14T10:47:56Z

Any interest in using CheckPackageMempoolAcceptResult from baf475d? It checks that a result contains the fields we expect based on whether it's supposed to be valid.

instagibbs · 2023-09-14T14:00:21Z

@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 baf475d? 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 👍

instagibbs · 2023-09-14T18:42:01Z

src/test/fuzz/package_eval.cpp

+                for (int i = 0; i < num_out; ++i) {
+                    tx_mut.vout.emplace_back(amount_out, P2WSH_OP_TRUE);
+                }
+                // TODO vary transaction sizes to catch size-related issues


future work: making the tx >40kWu sometimes, along with other invariant checks, would have likely allowed the fuzzer to catch #28472

instagibbs · 2023-09-15T13:46:43Z

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

rebasing on this should result in no known crashes: #28471
#28472

src/test/fuzz/package_eval.cpp

glozow

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.

src/test/fuzz/package_eval.cpp

glozow · 2023-09-25T12:20:38Z

src/test/fuzz/package_eval.cpp

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


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)."

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?

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

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.

src/test/fuzz/package_eval.cpp

glozow · 2023-09-25T12:53:46Z

src/test/fuzz/package_eval.cpp

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


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

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

src/test/fuzz/package_eval.cpp

glozow · 2023-09-25T13:05:51Z

src/test/fuzz/package_eval.cpp

+                }
+                // TODO vary transaction sizes to catch size-related issues
+                auto tx = MakeTransactionRef(tx_mut);
+                // Restore previously removed outpoints, except in-package outpoints


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?

src/test/fuzz/package_eval.cpp

glozow · 2023-09-25T13:14:07Z

src/test/fuzz/package_eval.cpp

+        // Add created outpoints, remove spent outpoints
+        {
+            // Outpoints that no longer exist at all
+            std::set<COutPoint> consumed_erased;
+            // Outpoints that no longer count toward the total supply
+            std::set<COutPoint> consumed_supply;
+            for (const auto& removed_tx : removed) {
+                insert_tx(/*created_by_tx=*/{consumed_erased}, /*consumed_by_tx=*/{outpoints_supply}, /*tx=*/*removed_tx);
+            }
+            for (const auto& added_tx : added) {
+                insert_tx(/*created_by_tx=*/{outpoints_supply, outpoints_rbf}, /*consumed_by_tx=*/{consumed_supply}, /*tx=*/*added_tx);
+            }
+            for (const auto& p : consumed_erased) {
+                outpoints_supply.erase(p);
+                outpoints_rbf.erase(p);
+            }
+            for (const auto& p : consumed_supply) {
+                outpoints_supply.erase(p);
+            }
+        }


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 glozow@df433f8

taken, thanks

glozow · 2023-09-25T16:45:14Z

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

dergoegge · 2023-09-26T10:24:53Z

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

glozow · 2023-09-26T11:12:26Z

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 · 2023-09-26T12:04:03Z

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 · 2023-09-26T13:42:04Z

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

glozow

ACK 3707ed7
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

src/test/fuzz/package_eval.cpp

instagibbs · 2023-09-27T12:52:27Z

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 · 2023-09-27T13:47:38Z

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

instagibbs · 2023-09-27T14:36:17Z

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

glozow · 2023-09-27T14:46:50Z

src/test/fuzz/package_eval.cpp

+
+    void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override
+    {
+        // Transactions may be entered and booted any number of times


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.

Suggested change

// Transactions may be entered and booted any number of times

// Transactions may be entered and booted any number of times

m_added.erase(tx);

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.

src/test/fuzz/package_eval.cpp

murchandamus · 2023-09-27T19:16:44Z

src/test/fuzz/package_eval.cpp

+        auto package_submit = !single_submit;
+
+        const auto result_package = WITH_LOCK(::cs_main,
+                                    return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/!package_submit));


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.

src/test/fuzz/package_eval.cpp

murchandamus · 2023-09-27T20:35:33Z

ACK 262ab8e

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

glozow

reACK 262ab8e

dergoegge

tACK 262ab8e

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

262ab8e Add package evaluation fuzzer (Greg Sanders) Pull request description: This fuzzer target caught the issue in bitcoin#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: 1) Picks mempool confgs, including max package size, count, mempool size, etc 2) Generates 1 to 26 transactions with arbitrary coins/fees, the first N-1 spending only confirmed outpoints 3) Nth transaction, if >1, sweeps all unconfirmed outpoints in mempool 4) If N==1, it may submit it through single-tx submission path, to allow for more interesting topologies 5) Otherwise submits through package submission interface 6) 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. ACKs for top commit: murchandamus: ACK 262ab8e glozow: reACK 262ab8e dergoegge: tACK 262ab8e Tree-SHA512: 190784777d0f2361b051b3271db8f79b7927e3cab88596d2c30e556da721510bd17f6cc96f6bb03403bbf0589ad3f799fa54e63c1b2bd92a2084485b5e3e96a5

instagibbs mentioned this pull request Sep 11, 2023

validation: fix coins disappearing mid-package evaluation #28251

Merged

glozow added the Tests label Sep 11, 2023

glozow reviewed Sep 11, 2023

View reviewed changes

src/test/fuzz/package_eval.cpp Outdated Show resolved Hide resolved

glozow mentioned this pull request Sep 11, 2023

Package Relay Project Tracking #27463

Open

53 tasks

instagibbs force-pushed the 2023-08-mid-package-trim-sep-8-fuzz branch from f06cf5b to ad63201 Compare September 11, 2023 14:53

DrahtBot added the CI failed label Sep 11, 2023

fanquake requested a review from dergoegge September 11, 2023 15:03

dergoegge reviewed Sep 11, 2023

View reviewed changes

src/test/fuzz/package_eval.cpp Show resolved Hide resolved

This was referenced Sep 11, 2023

Fee Estimator updates from Validation Interface/CScheduler thread #28368

Merged

validate package transactions with their in-package ancestor sets #26711

Closed

wallet: Receive silent payment transactions #28453

Closed

brunoerg reviewed Sep 12, 2023

View reviewed changes

DrahtBot added the Needs rebase label Sep 13, 2023

instagibbs force-pushed the 2023-08-mid-package-trim-sep-8-fuzz branch from 3654ad4 to 28f45d6 Compare September 13, 2023 18:06

instagibbs changed the title ~~WIP: Add package evaluation fuzzer~~ Add package evaluation fuzzer Sep 13, 2023

DrahtBot removed CI failed Needs rebase labels Sep 13, 2023

instagibbs commented Sep 14, 2023

View reviewed changes

instagibbs force-pushed the 2023-08-mid-package-trim-sep-8-fuzz branch from 28f45d6 to 93d7c5c Compare September 15, 2023 13:45

achow101 reviewed Sep 20, 2023

View reviewed changes

src/test/fuzz/package_eval.cpp Outdated Show resolved Hide resolved

src/test/fuzz/package_eval.cpp Outdated Show resolved Hide resolved

src/test/fuzz/package_eval.cpp Show resolved Hide resolved

instagibbs force-pushed the 2023-08-mid-package-trim-sep-8-fuzz branch 2 times, most recently from 2302596 to bf7436c Compare September 20, 2023 14:03

brunoerg reviewed Sep 24, 2023

View reviewed changes

src/test/fuzz/package_eval.cpp Outdated Show resolved Hide resolved

glozow reviewed Sep 25, 2023

View reviewed changes

instagibbs force-pushed the 2023-08-mid-package-trim-sep-8-fuzz branch from 2ecdc21 to 3707ed7 Compare September 26, 2023 13:37

glozow reviewed Sep 26, 2023

View reviewed changes

src/test/fuzz/package_eval.cpp Outdated Show resolved Hide resolved

src/test/fuzz/package_eval.cpp Outdated Show resolved Hide resolved

src/test/fuzz/package_eval.cpp Outdated Show resolved Hide resolved

achow101 reviewed Sep 26, 2023

View reviewed changes

src/test/fuzz/package_eval.cpp Outdated Show resolved Hide resolved

instagibbs force-pushed the 2023-08-mid-package-trim-sep-8-fuzz branch from 3707ed7 to fb4d71e Compare September 27, 2023 12:51

instagibbs force-pushed the 2023-08-mid-package-trim-sep-8-fuzz branch from fb4d71e to 6e46822 Compare September 27, 2023 13:39

DrahtBot added the CI failed label Sep 27, 2023

glozow reviewed Sep 27, 2023

View reviewed changes

DrahtBot removed the CI failed label Sep 27, 2023

instagibbs force-pushed the 2023-08-mid-package-trim-sep-8-fuzz branch from 6e46822 to 70879e4 Compare September 27, 2023 17:24

murchandamus reviewed Sep 27, 2023

View reviewed changes

Add package evaluation fuzzer

262ab8e

instagibbs force-pushed the 2023-08-mid-package-trim-sep-8-fuzz branch from 70879e4 to 262ab8e Compare September 27, 2023 20:27

DrahtBot requested a review from glozow September 27, 2023 20:35

glozow reviewed Sep 28, 2023

View reviewed changes

glozow requested a review from dergoegge September 28, 2023 10:42

dergoegge approved these changes Sep 28, 2023

View reviewed changes

glozow merged commit 6619d6a into bitcoin:master Sep 28, 2023
16 checks passed

glozow mentioned this pull request Sep 28, 2023

rpc: allow submitpackage to be called outside of regtest #27609

Merged

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

Add package evaluation fuzzer #28450

Add package evaluation fuzzer #28450

Conversation

instagibbs commented Sep 11, 2023 • edited by fanquake

DrahtBot commented Sep 11, 2023 • edited

Reviews

Conflicts

instagibbs commented Sep 11, 2023

darosior commented Sep 11, 2023

glozow left a comment

Choose a reason for hiding this comment

dergoegge left a comment

Choose a reason for hiding this comment

brunoerg left a comment

Choose a reason for hiding this comment

instagibbs commented Sep 13, 2023

maflcko commented Sep 14, 2023

glozow commented Sep 14, 2023

instagibbs commented Sep 14, 2023 • edited

instagibbs Sep 14, 2023 • edited

Choose a reason for hiding this comment

instagibbs commented Sep 15, 2023 • edited

glozow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs Sep 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow commented Sep 25, 2023

dergoegge commented Sep 26, 2023

glozow commented Sep 26, 2023

dergoegge commented Sep 26, 2023

instagibbs commented Sep 26, 2023 • edited

glozow left a comment

Choose a reason for hiding this comment

instagibbs commented Sep 27, 2023

instagibbs commented Sep 27, 2023

instagibbs commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murchandamus commented Sep 27, 2023

glozow left a comment

Choose a reason for hiding this comment

dergoegge left a comment

Choose a reason for hiding this comment

instagibbs commented Sep 11, 2023 •

edited by fanquake

DrahtBot commented Sep 11, 2023 •

edited

instagibbs commented Sep 14, 2023 •

edited

instagibbs Sep 14, 2023 •

edited

instagibbs commented Sep 15, 2023 •

edited

instagibbs Sep 28, 2023 •

edited

instagibbs commented Sep 26, 2023 •

edited