refactor: TxDownloadManager + fuzzing by glozow · Pull Request #30110 · bitcoin/bitcoin

glozow · 2024-05-15T09:49:51Z

Part of #27463.

This PR does 3 things:

(1) It modularizes transaction download logic into a TxDownloadManager. Transaction download logic refers to the process of deciding what transactions to request, download, and validate.[1] There should be no behavior changes. Using --color_moved=dimmed_zebra -w may help.
(2) It adds unit and fuzz (:magic_wand:) testing for transaction download.
(3) It makes a few small behavioral changes:

Stop (debug-only) logging tx invs during IBD
Just like all other transactions, require orphans have RecursiveDynamicUsage < 100k before adding to vExtraTxnForCompact
Don't return a 1p1c that contains a parent or child in recent rejects. Don't process any orphan already in recent rejects. These cases should not happen in actual node operation; it's just to allow tighter sanity checks during fuzzing.

There are several benefits to this interface, such as:

Unit test coverage and fuzzing for logic that currently isn't feasible to test as thoroughly (without lots of overhead) and/or currently only lightly tested through assert_debug_log (not good) in functional tests.
When we add more functionality (e.g. package relay messages, more robust orphan handling), the vast majority of it will be within TxDownloadManager instead of PeerManager, making it easier to review and test. See Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling #28031 for what this looks like.
PeerManager will no longer know anything about / have access to TxOrphanage, TxRequestTracker or the rejection caches. Its primary interface with TxDownloadManager would be much simpler:
- Passing on ValidationInterface callbacks
- Telling txdownloadman when a peer {connects, disconnects}
- Telling txdownloadmanwhen a {transaction, package} is {accepted, rejected} from mempool
- Telling txdownloadman when invs, notfounds, and txs are received.
- Getting instructions on what to download.
- Getting instructions on what {transactions, packages, orphans} to validate.
- Get whether a peer HaveMoreWork for the ProessMessages loop
(todo) Thread-safety can be handled internally.

[1]: This module is concerned with tx download, not upload. It excludes transaction announcements/gossip which happens after we download/accept a transaction. Txreconciliation (erlay) is excluded from this module, as it only relates to deciding which invs to send or helping the other peer decide which invs to send. It is independent from this logic.

DrahtBot · 2024-05-15T09:49:54Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, instagibbs, naumenkogs, achow101
Concept ACK	l0rinc
Approach ACK	ismaelsadeeq

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:

#30572 (Halt processing of unrequested transactions v2 by ariard)
#30538 (Doc: add a comment referencing past vulnerability next to where it was fixed by darosior)
#30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
#29492 (refactor: Remove redundant definitions by Empact)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)

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.

DrahtBot · 2024-05-15T10:56:48Z

🚧 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.

_{Debug: https://github.com/bitcoin/bitcoin/runs/24992798939}

glozow · 2024-05-20T10:55:50Z

Rebased for #29817 and added a "ensure we can always download a tx as long as we have 1 good outbound peer" fuzz test

src/node/txdownload_impl.h

src/node/txdownload_impl.cpp

These invs are ignored anyway, and this allows us to more easily move the inv handling to TxDownloadManager in the next commit.

ProcessInvalidTx will return a PackageToValidate, so it needs to be defined afterward.

…Compact There does not appear to be any reason why orphan transactions should be given special treatment.

Move-only.

Move-only. Also delete external RecentRejectsFilter() access since it is no longer necessary.

Also delete external RecentRejectsReconsiderableFilter() access since it is no longer necessary.

…adImpl

…to TxDownloadMan internals

This is a slight behavior change: if a transaction is in both reconsiderable rejects and AlreadyHaveTx in another way, we don't try to return a 1p1c package. This is the correct thing to do, as we don't want to reconsider transactions that have multiple things wrong with them. For example, if a transaction is low feerate, and then later found to have a bad signature, we shouldn't try it again in a package.

This should never happen normally, but just in case.

Avoid the fuzzer situation where: 1. Orphanage has 2 transactions with the same txid, one with witness, one without witness. 2. The transaction with witness is found to have `TX_INPUTS_NOT_STANDARD` error. The txid is added to recent rejects filter, and the tx with witness is deleted from orphanage. 3. A low feerate parent is found. Find1P1CPackage finds the transaction with no witness in orphanage, and returns the package. 4. net_processing has just been handed a package in which the child is already in recent rejects.

Forward this bool to the TxRequestTracker ctor. This is needed for stablity in TxDownloadManager fuzzers

The txdownload_impl is similar but allows us to check specific invariants within its implementation. It will also change a lot more than the external interface (txdownloadman) will, so we will add more to this target later.

theStack

ACK 0f4bc63

theStack · 2024-10-25T12:53:41Z

src/test/txdownload_tests.cpp

+    bool m_ignore_inv_txid;
+    bool m_ignore_inv_wtxid;
+
+    // Constructor. We are passing and casting ints because they are more readable in a table (see all_expected_results).


Suggested change

// Constructor. We are passing and casting ints because they are more readable in a table (see all_expected_results).

// Constructor. We are passing and casting ints because they are more readable in a table (see expected_behaviors).

Will fix if I retouch / in the next PR

picked up in #31190

theStack · 2024-10-25T13:20:54Z

src/test/fuzz/txdownloadman.cpp

+        // Jump ahead in time
+        time += fuzzed_data_provider.PickValueInArray(TIME_SKIPS);
+        CheckInvariants(txdownload_impl, max_orphan_count);


is it intentional to do no backward jumps in time here? (in contrast to the txdownloadman fuzz test above)

Not intentional! I haven't tried negative yet.

added in #31190

instagibbs

reACK 0f4bc63

could use some fuzzing c @marcofleon

naumenkogs

ACK 0f4bc63

src/node/txdownloadman_impl.h

src/node/txdownloadman_impl.cpp

naumenkogs · 2024-10-28T11:26:19Z

src/node/txdownloadman_impl.cpp

Okay, so

/** New inv has been received. May be added as a candidate to txrequest. */ is inaccurate, right? There is no new INV received in p2p_inv=false case, but rather it's called from orphan consideration after processing a TX?

src/net_processing.cpp

marcofleon · 2024-10-29T12:11:20Z

Fuzzed the two targets for about 250 cpu hours each. Coverage looks good.
txdownloadman
txdownloadman_impl

Finally, the children hanging out in the recent reject filter are quiet 😌

I'll look to run the one honest peer test as well once it's ready.

achow101 · 2024-10-29T18:11:39Z

light ACK 0f4bc63

glozow · 2024-10-30T20:03:53Z

Thanks for the reviews. Working on the followups + one_honest_peer fuzzer PR now

glozow added the Refactoring label May 15, 2024

glozow force-pushed the 2024-05-txdownload branch from dc30dc1 to 1d9a82c Compare May 15, 2024 10:56

DrahtBot added the CI failed label May 15, 2024

glozow force-pushed the 2024-05-txdownload branch from 1d9a82c to 1d04155 Compare May 15, 2024 11:48

This was referenced May 15, 2024

locks: introduce mutex for tx download, flush rejection filters once per tip change #30111

Merged

Package Relay Project Tracking #27463

Closed

DrahtBot added the Needs rebase label May 15, 2024

glozow force-pushed the 2024-05-txdownload branch 2 times, most recently from 0937e08 to 2e58028 Compare May 15, 2024 16:05

DrahtBot removed the Needs rebase label May 15, 2024

glozow mentioned this pull request May 16, 2024

p2p: index TxOrphanage by wtxid, allow entries with same txid #30000

Merged

glozow force-pushed the 2024-05-txdownload branch 3 times, most recently from 0b40677 to b1cc0c3 Compare May 20, 2024 10:50

dergoegge reviewed May 20, 2024

View reviewed changes

src/node/txdownload_impl.h Outdated Show resolved Hide resolved

src/node/txdownload_impl.cpp Outdated Show resolved Hide resolved

DrahtBot removed the CI failed label May 20, 2024

DrahtBot mentioned this pull request May 20, 2024

scripted-diff: Use LogInfo over LogPrintf #29641

Merged

glozow force-pushed the 2024-05-txdownload branch from b1cc0c3 to 13622fc Compare May 21, 2024 12:44

DrahtBot mentioned this pull request May 31, 2024

refactor: Improve assumeutxo state representation #30214

Merged

DrahtBot added the Needs rebase label Jun 3, 2024

glozow force-pushed the 2024-05-txdownload branch from 13622fc to 4340fb9 Compare June 6, 2024 12:35

glozow added 22 commits October 24, 2024 21:23

[p2p] don't log tx invs when in IBD

58e09f2

These invs are ignored anyway, and this allows us to more easily move the inv handling to TxDownloadManager in the next commit.

[refactor] move tx inv/getdata handling to txdownload

042a97c

[refactor] move notfound processing to txdownload

3a41926

[refactor] move ProcessInvalidTx and ProcessValidTx definitions down

c8e67b9

ProcessInvalidTx will return a PackageToValidate, so it needs to be defined afterward.

[refactor] move new orphan handling to ProcessInvalidTx

416fbc9

[refactor] move Find1P1CPackage into ProcessInvalidTx

798cc8f

[p2p] restrict RecursiveDynamicUsage of orphans added to vExtraTxnFor…

6797bc4

…Compact There does not appear to be any reason why orphan transactions should be given special treatment.

[refactor] put peerman tasks at the end of ProcessInvalidTx

f497414

[refactor] move Find1P1CPackage to txdownload

a8cf3b6

Move-only.

[refactor] move valid tx processing to TxDownload

c6b2174

[refactor] move invalid tx processing to TxDownload

c4ce0c1

Move-only. Also delete external RecentRejectsFilter() access since it is no longer necessary.

[refactor] move invalid package processing to TxDownload

257568e

[refactor] move new tx logic to txdownload

1e08195

Also delete external RecentRejectsReconsiderableFilter() access since it is no longer necessary.

[refactor] make AlreadyHaveTx and Find1P1CPackage private to TxDownlo…

f150fb9

…adImpl

[refactor] wrap {Have,Get}TxToReconsider in txdownload

969b072

[refactor] add CheckIsEmpty and GetOrphanTransactions, remove access …

fa7027d

…to TxDownloadMan internals

[p2p] don't process orphan if in recent rejects

5269d57

This should never happen normally, but just in case.

[p2p] add TxDownloadOptions bool to make TxRequestTracker deterministic

fa584cb

Forward this bool to the TxRequestTracker ctor. This is needed for stablity in TxDownloadManager fuzzers

[unit test] MempoolRejectedTx

699643f

[fuzz] txdownloadman and txdownload_impl

0f4bc63

The txdownload_impl is similar but allows us to check specific invariants within its implementation. It will also change a lot more than the external interface (txdownloadman) will, so we will add more to this target later.

theStack approved these changes Oct 25, 2024

View reviewed changes

instagibbs approved these changes Oct 25, 2024

View reviewed changes

naumenkogs reviewed Oct 28, 2024

View reviewed changes

glozow mentioned this pull request Oct 31, 2024

TxDownloadManager followups #31190

Merged

glozow mentioned this pull request Sep 2, 2025

[NO MERGE] BIP331 Ancestor Package Relay #27742

Closed

	// Constructor. We are passing and casting ints because they are more readable in a table (see all_expected_results).
	// Constructor. We are passing and casting ints because they are more readable in a table (see expected_behaviors).

Conversation

glozow commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

DrahtBot commented May 15, 2024

Uh oh!

glozow commented May 20, 2024

Uh oh!

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

theStack Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

glozow Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

glozow Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

theStack Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

glozow Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

glozow Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

naumenkogs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

naumenkogs Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcofleon commented Oct 29, 2024

Uh oh!

achow101 commented Oct 29, 2024

Uh oh!

glozow commented Oct 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

glozow commented May 15, 2024 •

edited

Loading

DrahtBot commented May 15, 2024 •

edited

Loading