-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: TxDownloadManager #30110
base: master
Are you sure you want to change the base?
refactor: TxDownloadManager #30110
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, 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. |
dc30dc1
to
1d9a82c
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
1d9a82c
to
1d04155
Compare
0937e08
to
2e58028
Compare
0b40677
to
b1cc0c3
Compare
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
Outdated
static constexpr auto OVERLOADED_PEER_TX_DELAY{2s}; | ||
/** How long to wait before downloading a transaction from an additional peer */ | ||
static constexpr auto GETDATA_TX_INTERVAL{60s}; | ||
struct TxDownloadOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Structs that are part of the public interface should probably go into txdownloadman.h
. Otherwise one needs to include the impl header, e.g. like txdownloadman.h
does right now, which defeats the purpose of pimpling to some extend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, in the previous setup, we can change any of txdownload_impl.cpp without net_processing.cpp noticing, but yeah maybe less clean to depend on txdownload_impl.h.
I've updated with a new setup where I've added a txdownloadman.cpp depending on txdownload_impl.h, and txdownloadman.h is included by txdownload_impl.h instead of the other way around. And so net_processing.cpp no longer depends on txdownload_impl.h, which makes sense to me. Had to update lint-circular-dependencies.py
but I do like that we can change txdownload_impl.h without recompiling everything. lmk if this seems better?
|
||
bool TxDownloadImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable) | ||
{ | ||
const uint256& hash = gtxid.GetHash(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iiuc correctly, clearing the filters here has been removed as we don't want TxDownloadManager
to depend on ChainstateManager
.
An alternative to the new synchronous validation interface callback could be to preserve the previous behavior (clearing the filters in AlreadyHaveTx
on a tip change) by introducing an interface for TxDownloadMan to fetch the latest tip hash. For example, TxDownloadOptions
could be extended to hold a lambda that returns the latest tip hash, which in production simply fetches the hash from the ChainstateManager
(and for tests it can be mocked). This might be easier to review since it preserves the previous mechanism? Although perhaps also more difficult w.r.t to lock inversion of cs_main
and m_tx_download_mutex
.
Another alternative could be to directly pass the current block hash as a param into the relevant TxDownloadManager
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'd ever need to make TxDownloadManager
depend on ChainstateManager
as we could just pass in the block hash, i.e.
Another alternative could be to directly pass the current block hash as a param into the relevant TxDownloadManager methods.
This would avoid the addition of UpdateBlockTipSync
etc, as it's basically what we do now. I find this way of keeping synchronized with the chain tip pretty ugly - we'd need to hold cs_main
and pass the blockhash to the TxDownloadManager
all the time, bloating the interface. Subscribing to the validation interface and updating on every chain tip update seems much more sensible.
b1cc0c3
to
13622fc
Compare
13622fc
to
4340fb9
Compare
The TxOrphanage is now guarded externally by m_tx_download_mutex.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
…er, and bloom filters This module is going to be responsible for managing everything related to transaction download, including txrequest, orphan transactions and package relay. It will be responsible for managing usage of the TxOrphanage and instructing PeerManager: - what tx or package-related messages to send to which peer - whether a tx or package-related message is allowed or useful - what transactions are available to try accepting to mempool Future commits will consolidate the interface and re-delegate interactions from PeerManager to TxDownloadManager.
This is move-only.
The information stored in TxDownloadConnectionInfo isn't used until the next commit.
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.
e4b45c0
to
bc30c16
Compare
Part of #27463.
Draft while #30000 is not merged, and because I am still smoothing out the interface, making sure the commits are clean, and writing more tests.
This modularizes transaction download logic into a
TxDownloadManager
. Transaction download logic refers to the process of deciding what transactions to request, download, and validate.[1]All of the changes here should result in no behavior change.
There are several benefits to this interface, such as:
PeerManager
without a ton of overhead) and/or currently only lightly tested throughassert_debug_log
( 👎 ) in functional tests.MempoolRejectedTx
test, where we can define a matrix of expected outcomes for eachTxValidationResult
and test them very easily.txdownloadman
andtxdownload_impl
targets.TxDownloadManager
instead ofPeerManager
, making it easier to review and test.PeerManager
, which will no longer know anything about / have access toTxOrphanage
or rejection caches. Its primary interface withTxDownloadManager
would be:txdownloadman
of tip changes (i.e. passing on someValidationInterface
callbacks)txdownloadman
when a {transaction, package} is {accepted, rejected} from mempool, and receive a few instructionsinv
s,notfound
s totxdownloadman
and receive instructions on what to validategetdata
requests to sendProcessOrphanTx
HaveMoreWork
for theProessMessages
loopFind1P1CPackage
private toTxDownloadImpl
PackageToValidate
struct to return the thingspeerman
needs for validation.PeerManager
handle it (e.g. currently it lockscs_main
to synchronize accesses tom_txrequest
!)[1]: This does not include transaction announcements, i.e. what we send to our peers. All of that happens after we download/accept a tx. Txreconciliation (erlay) is excluded from this module, as it is part of announcements and/or part of helping the other peer decide which
inv
s to send.