[FreshEyes] locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip #17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
The lock doesn't exist anymore. -BEGIN VERIFY SCRIPT- sed -i 's/EraseTxNoLock/EraseTxInternal/g' src/txorphanage.* -END VERIFY SCRIPT-))'
|
There were 14 comments left by 3 reviewers, 1 bot and the author for this pull request |
![fresheyes-staging-bot[bot]](https://avatars.githubusercontent.com/in/855196?s=60&v=4){kind=small}
| LOCK(m_recent_confirmed_transactions_mutex); | ||
| if (m_recent_confirmed_transactions.contains(hash)) return true; | ||
| } | ||
| if (m_recent_confirmed_transactions.contains(hash)) return true; |
There was a problem hiding this comment.
2 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601841903at 2024/05/15, 15:19:53 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601878592at 2024/05/15, 15:43:26 UTC.
| @@ -1075,7 +1074,7 @@ class PeerManagerImpl final : public PeerManager | |||
| int m_peers_downloading_from GUARDED_BY(cs_main) = 0; | |||
|
|
|||
| /** Storage for orphan information */ | |||
| TxOrphanage m_orphanage; | |||
| TxOrphanage m_orphanage GUARDED_BY(m_tx_download_mutex); | |||
There was a problem hiding this comment.
4 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601902513at 2024/05/15, 15:59:50 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1602816185at 2024/05/16, 08:07:47 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1602847832at 2024/05/16, 08:23:19 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1603602970at 2024/05/16, 15:29:17 UTC.
| } | ||
| } // release MempoolMutex | ||
| if (notify_updated_tip) { | ||
| m_chainman.m_options.signals->UpdatedBlockTipSync(pindexNewTip); |
There was a problem hiding this comment.
4 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601903709at 2024/05/15, 16:00:46 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601920676at 2024/05/15, 16:13:15 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601923988at 2024/05/15, 16:15:58 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601946313at 2024/05/15, 16:33:58 UTC.
| @@ -183,6 +183,12 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo | |||
| fInitialDownload); | |||
| } | |||
|
|
|||
| void ValidationSignals::UpdatedBlockTipSync(const CBlockIndex *pindexNew) | |||
There was a problem hiding this comment.
An author commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601944049at 2024/05/15, 16:32:08 UTC.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The author glozow wrote the following PR called locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip, issue number 30111 in bitcoin/bitcoin cloned by FreshEyes below:
See
#27463for full project tracking.This contains the first 4 commits of
#30110, which require some thinking about thread safety in review.m_tx_download_mutexwhich guards the transaction download data structures includingm_txrequest, the rolling bloom filters, andm_orphanage. Later this should become the mutex guardingTxDownloadManager.m_txrequestdoesn't need to be guarded usingcs_mainanymorem_recent_confirmed_transactionsdoesn't need its own lock anymorem_orphanagedoesn't need its own lock anymoreValidationInterfaceevent,UpdatedBlockTipSync, which is a synchronous version ofm_txrequest0.m_txrequest1 andm_txrequest2 onm_txrequest3 just once instead of every timem_txrequest4 is called. This should speed up calls to that function and removes the need to lockm_txrequest5 every time it is called.