-
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
locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip #30111
base: master
Are you sure you want to change the base?
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.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
c280fe7
to
5ba125d
Compare
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'm not sure how to be confident that the UpdateBlockTipSync
/ hashRecentRejectsChainTip
would be correct and didn't miss an edge case. Rest looks good to me.
5ba125d
to
063fc63
Compare
I (locally) split the commit into (1) update on validation interface callback and asserting that I've (locally) hit a bug though, so will figure out what's wrong and then push. |
FWIW I tried something similar, and got an assertion failure in one of the mempool functional tests (maybe mempool_reorg, and thus due to an invalidateblock call?), but it wasn't reliable, it only occurred when I ran many tests in parallel, not when I reran that test on its own. The bug will only trigger if the tip is updated via some other path and AlreadyHave is called before the tip is further updated via the expected path, so (1) doesn't seem like a terribly reliable check to me. |
063fc63
to
a1f36eb
Compare
Was it mempool_packages.py maybe? Mine tripped there on Hm. The alternative is to hold |
Ah, yes, I think it was. |
thanks for splitting this up, next on my review docket |
a1f36eb
to
7c3fb97
Compare
Rebased for #29817 |
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.
looking pretty straightforward but I'm not an expert in the current locking setup
will give another pass in a bit
7c3fb97
to
ef8de26
Compare
suggested changes were done, reviewed via |
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.
utACK ef8de26
ef8de26
to
a9a6de5
Compare
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.
rebased
reACK a9a6de5 Only relevant change is suggested commit message fix reviewed via |
We need to synchronize between various tx download structures. TxRequest does not inherently need cs_main for synchronization, and it's not appropriate to lock all of the tx download logic under cs_main.
This is a synchronous version of UpdatedBlockTip. It allows clients to respond to a new block immediately after it is connected. The synchronicity is important for things like m_recent_rejects, in which a transaction's validity can change (rejected vs accepted) when this event is processed (e.g. it has a timelock condition that has just been met). This is distinct from something like m_recent_confirmed_transactions in which the validation outcome is the same (valid vs already-have).
Resetting m_recent_rejects once per block is more efficient than comparing hashRecentRejectsChainTip with the chain tip every time we call AlreadyHaveTx. We keep hashRecentRejectsChainTip for now to assert that updates happen correctly; it is removed in the next commit.
This also means AlreadyHaveTx no longer needs cs_main held.
a9a6de5
to
2725a3b
Compare
The TxOrphanage is now guarded externally by m_tx_download_mutex.
This comment was marked as outdated.
This comment was marked as outdated.
2725a3b
to
0e0c422
Compare
Rebased |
See #27463 for full project tracking.
This contains the first few commits of #30110, which require some thinking about thread safety in review.
m_tx_download_mutex
which guards the transaction download data structures includingm_txrequest
, the rolling bloom filters, andm_orphanage
. Later this should become the mutex guardingTxDownloadManager
.m_txrequest
doesn't need to be guarded usingcs_main
anymorem_recent_confirmed_transactions
doesn't need its own lock anymorem_orphanage
doesn't need its own lock anymoreValidationInterface
event,UpdatedBlockTipSync
, which is a synchronous version ofUpdatedBlockTip
.m_recent_rejects
andm_recent_rejects_reconsiderable
onUpdatedBlockTipSync
just once instead of every timeAlreadyHaveTx
is called. This should speed up calls to that function and removes the need to lockcs_main
every time it is called.Motivation:
m_orphanage
should not be added tom_txrequest
. In the future, orphan resolution tracking should also be synchronized. Currently,cs_main
is used to e.g. sync accesses tom_txrequest
. We should not broaden the scope of things it locks.AlreadyHaveTx
so we can decide whether we should update it. Every call compares the current tip hash withhashRecentRejectsChainTip
. It is more efficient to have a validation interface callback that updates the rejection filters whenever the chain tip changes.