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

glozow · 2024-05-15T14:16:19Z

See #27463 for full project tracking.

This contains the first few commits of #30110, which require some thinking about thread safety in review.

Introduce a new m_tx_download_mutex which guards the transaction download data structures including m_txrequest, the rolling bloom filters, and m_orphanage. Later this should become the mutex guarding TxDownloadManager.
- m_txrequest doesn't need to be guarded using cs_main anymore
- m_recent_confirmed_transactions doesn't need its own lock anymore
- m_orphanage doesn't need its own lock anymore
Adds a new ValidationInterface event, ActiveTipChanged, which is a synchronous callback whenever the tip of the active chainstate changes.
Flush m_recent_rejects and m_recent_rejects_reconsiderable on ActiveTipChanged just once instead of checking the tip every time AlreadyHaveTx is called. This should speed up calls to that function (no longer comparing a block hash each time) and removes the need to lock cs_main every time it is called.

Motivation:

These data structures need synchronization. While we are holding m_tx_download_mutex, these should hold:
- a tx hash in m_txrequest is not also in m_orphanage
- a tx hash in m_txrequest is not also in m_recent_rejects or m_recent_confirmed_transactions
- In the future, orphan resolution tracking should also be synchronized. If a tx has an entry in the orphan resolution tracker, it is also in m_orphanage, and not in m_txrequest, etc.
Currently, cs_main is used to e.g. sync accesses to m_txrequest. We should not broaden the scope of things it locks.
Currently, we need to know the current chainstate every time we call AlreadyHaveTx so we can decide whether we should update it. Every call compares the current tip hash with hashRecentRejectsChainTip. It is more efficient to have a validation interface callback that updates the rejection filters whenever the chain tip changes.

DrahtBot · 2024-05-15T14:16:23Z

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	instagibbs, dergoegge, theStack, hebasto
Concept ACK	mzumsande
Stale ACK	vasild, achow101

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:

#30413 (fuzz: Version handshake by dergoegge)
#30233 (refactor: move m_is_inbound out of CNodeState by sr-gi)
#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#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.

src/net_processing.cpp

src/validation.cpp

src/net_processing.cpp

ajtowns

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.

src/net_processing.cpp

src/validationinterface.cpp

glozow · 2024-05-16T11:41:14Z

I'm not sure how to be confident that the UpdateBlockTipSync / hashRecentRejectsChainTip would be correct and didn't miss an edge case.

I (locally) split the commit into (1) update on validation interface callback and asserting that hashRecentRejectsChainTip is equal to the chain tip whenever we call AlreadyHaveTx + (2) removing hashRecentRejectsChainTip. I think if we see that everything runs fine with (1) it's probably correct.

I've (locally) hit a bug though, so will figure out what's wrong and then push.

ajtowns · 2024-05-16T14:01:13Z

I'm not sure how to be confident that the UpdateBlockTipSync / hashRecentRejectsChainTip would be correct and didn't miss an edge case.

I (locally) split the commit into (1) update on validation interface callback and asserting that hashRecentRejectsChainTip is equal to the chain tip whenever we call AlreadyHaveTx + (2) removing hashRecentRejectsChainTip. I think if we see that everything runs fine with (1) it's probably correct.

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.

glozow · 2024-05-16T14:21:46Z

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?)

Was it mempool_packages.py maybe? Mine tripped there on invalidateblock when I was adding UpdatedBlockTip to InvalidateBlock, and it was a lock ordering problem. My hierarchy is cs_main -> tx_download_mutex -> mempool.cs.

Hm. The alternative is to hold cs_main and tell txdownloadman the current chain tip pretty much all the time...

ajtowns · 2024-05-16T15:02:25Z

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?)

Was it mempool_packages.py maybe? Mine tripped there on invalidateblock when I was adding UpdatedBlockTip to InvalidateBlock, and it was a lock ordering problem. My hierarchy is cs_main -> tx_download_mutex -> mempool.cs.

Ah, yes, I think it was.

instagibbs · 2024-05-17T13:44:06Z

thanks for splitting this up, next on my review docket

glozow · 2024-05-20T10:07:23Z

Rebased for #29817

instagibbs

looking pretty straightforward but I'm not an expert in the current locking setup

will give another pass in a bit

src/net_processing.cpp

src/validationinterface.cpp

src/net_processing.cpp

src/test/orphanage_tests.cpp

src/txorphanage.h

instagibbs · 2024-05-23T14:09:43Z

suggested changes were done, reviewed via git range-diff master 7c3fb97 ef8de26

instagibbs

utACK ef8de26be5478729328ac9d8a9ad6898351552b6

src/net_processing.cpp

glozow

rebased

src/net_processing.cpp

src/validation.cpp

theStack · 2024-07-15T16:38:39Z

Concept ACK

mzumsande

Concept ACK - don't know the txrequest logic very well though.

The PR description should be updated (UpdatedBlockTipSync was renamed).

src/net_processing.cpp

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 callback notifying clients of all tip changes. 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. For example, the transaction might have 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), so it does not need to be reset immediately.

glozow · 2024-07-16T09:02:25Z

Updated the PR title and description, thanks @mzumsande
Rebased for net: fix race condition in self-connect detection #30394
Renamed variable locks: introduce mutex for tx download, flush rejection filters once per tip change #30111 (comment)
Added some more documentation to m_tx_download_mutex to address locks: introduce mutex for tx download, flush rejection filters once per tip change #30111 (comment).

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 TxOrphanage is now guarded externally by m_tx_download_mutex.

instagibbs · 2024-07-16T13:59:06Z

reACK c85acce

reviewed via git range-diff master 17d41e7 c85acce

dergoegge

Code review ACK c85acce

hebasto · 2024-07-22T10:04:10Z

src/net_processing.cpp

    bool HasAllDesirableServiceFlags(ServiceFlags services) const override;
    bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override
-        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex);
+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex, !m_tx_download_mutex);


The PeerManagerImpl::ProcessMessages implementation has multiple early return statements. In such a case, the Developer Notes suggest to:

Combine annotations in function declarations with run-time asserts in function definitions

i.e., AssertLockNotHeld(m_tx_download_mutex);

UPD. Same for other places, for instance, PeerManagerImpl::SendMessages.

thanks, added in #30507

hebasto · 2024-07-22T10:13:35Z

src/net_processing.cpp

        //
        // Message: getdata (transactions)
        //
+        LOCK(m_tx_download_mutex);


Do we need to hold m_tx_download_mutex for this call:

bitcoin/src/net_processing.cpp

Lines 6327 to 6328 in c85acce

if (!vGetData.empty())

MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData);

a few lines down?

no we don't, changed in #30507

theStack

Light code-review ACK c85acce

Fwiw, I've also spun up a fresh mainnet node instance on a publicly reachable machine running this PR for the last few days (out of IBD since Friday morning) and didn't notice any problems so far.

theStack · 2024-07-18T16:44:11Z

src/net_processing.cpp

+    for (const auto& ptx : pblock->vtx) {
+        m_recent_confirmed_transactions.insert(ptx->GetHash().ToUint256());
+        if (ptx->HasWitness()) {
+            m_recent_confirmed_transactions.insert(ptx->GetWitnessHash().ToUint256());
        }
    }
-    {
-        for (const auto& ptx : pblock->vtx) {
-            m_txrequest.ForgetTxHash(ptx->GetHash());
-            m_txrequest.ForgetTxHash(ptx->GetWitnessHash());
-        }
+    for (const auto& ptx : pblock->vtx) {
+        m_txrequest.ForgetTxHash(ptx->GetHash());
+        m_txrequest.ForgetTxHash(ptx->GetWitnessHash());


nit: as a follow-up, could maybe consolidate those two loops into a single one, as they operate now under the same lock

good idea, done in #30507

hebasto · 2024-07-22T12:18:52Z

Introduce a new m_tx_download_mutex which guards the transaction download data structures including m_txrequest, the rolling bloom filters, and m_orphanage.

m_orphanage doesn't need its own lock anymore

In the master branch, the design of the TxOrphanage class is thread-safe for all purposes. I don't think that the fact that m_orphanage does not require additional synchronization justifies the change of TxOrphanage class design. The new m_tx_download_mutex guarantees no contention when locking TxOrphanage::m_mutex, which makes the latter cheap.

If still insisting on the TxOrphanage class change, then perhaps document that it is no longer thread-safe and requires an external synchronization?

glozow · 2024-07-23T12:39:06Z

In the master branch, the design of the TxOrphanage class is thread-safe for all purposes. I don't think that the fact that m_orphanage does not require additional synchronization justifies the change of TxOrphanage class design.

See #30111 (comment) for background. TxOrphanage is not a multi-purpose container. It has 1 specific purpose, and its user will need to externally synchronize it with other data structures to fulfill that purpose. It seems it would just slow things down to acquire and release a lock whenever we use it.

perhaps document that it is no longer thread-safe and requires an external synchronization?

I've opened #30507 to add documentation to txorphanage.h, as well as the other suggestions from @hebasto and @theStack (thanks!).

hebasto

ACK c85acce, I have reviewed the code and it looks OK.

7c29e55 m_tx_download_mutex followups (glozow) e543c65 release m_tx_download_mutex before MakeAndPushMessage GETDATA (glozow) bce5f37 [refactor] change ActiveTipChange to use CBlockIndex ref instead of ptr (glozow) 7cc5ac5 [doc] TxOrphanage is no longer thread-safe (glozow) 6f49548 [refactor] combine block vtx loops in BlockConnected (glozow) Pull request description: Followup to #30111. Includes suggestions: - #30111 (comment) - #30111 (comment) - #30111 (comment) - #30111 (comment) - #30111 (comment) ACKs for top commit: instagibbs: reACK 7c29e55 theStack: re-ACK 7c29e55 dergoegge: reACK 7c29e55 Tree-SHA512: 79a9002d74739367789bbc64bb1d431f4d43a25a7934231e55814c2cb6981c15ef2d8465544ae2a4fbd734d9bed6cc41b37a923938a88cb8fea139523c1e98da

glozow added the P2P label May 15, 2024

glozow mentioned this pull request May 15, 2024

Package Relay Project Tracking #27463

Open

57 tasks

hebasto reviewed May 15, 2024

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

hebasto reviewed May 15, 2024

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

glozow force-pushed the 2024-05-txdownload-mutex branch from c280fe7 to 5ba125d Compare May 15, 2024 15:42

hebasto reviewed May 15, 2024

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

ajtowns reviewed May 15, 2024

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/validationinterface.cpp Outdated Show resolved Hide resolved

glozow force-pushed the 2024-05-txdownload-mutex branch from 5ba125d to 063fc63 Compare May 16, 2024 11:21

glozow force-pushed the 2024-05-txdownload-mutex branch from 063fc63 to a1f36eb Compare May 16, 2024 14:14

glozow force-pushed the 2024-05-txdownload-mutex branch from a1f36eb to 7c3fb97 Compare May 20, 2024 10:06

instagibbs reviewed May 22, 2024

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/validationinterface.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/test/orphanage_tests.cpp Show resolved Hide resolved

src/txorphanage.h Outdated Show resolved Hide resolved

glozow force-pushed the 2024-05-txdownload-mutex branch from 7c3fb97 to ef8de26 Compare May 23, 2024 12:26

DrahtBot mentioned this pull request May 23, 2024

p2p: Fill reconciliation sets (Erlay) attempt 2 #30116

Open

This was referenced May 23, 2024

refactor: TxDownloadManager + fuzzing #30110

Merged

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Merged

instagibbs approved these changes May 24, 2024

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

This was referenced May 24, 2024

test: rpc: add last block announcement time to getpeerinfo result #27052

Open

fuzz: increase txorphan harness stability #30186

Merged

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-mutex branch from ef8de26 to a9a6de5 Compare June 6, 2024 12:14

glozow commented Jun 6, 2024

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

Eunovo reviewed Jul 11, 2024

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

mzumsande reviewed Jul 15, 2024

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

glozow changed the title ~~locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip~~ locks: introduce mutex for tx download, flush rejection filters once per tip change Jul 16, 2024

glozow added 2 commits July 16, 2024 10:01

guard TxRequest and rejection caches with new mutex

3eb1307

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.

glozow force-pushed the 2024-05-txdownload-mutex branch from 17d41e7 to 3f274fb Compare July 16, 2024 09:01

glozow added 5 commits July 16, 2024 10:21

remove obsoleted hashRecentRejectsChainTip

723ea0f

This also means AlreadyHaveTx no longer needs cs_main held.

lock m_recent_confirmed_transactions using m_tx_download_mutex

61745c7

remove obsoleted TxOrphanage::m_mutex

6ff8406

The TxOrphanage is now guarded externally by m_tx_download_mutex.

[refactor] delete EraseTxNoLock, just use EraseTx

c85acce

glozow force-pushed the 2024-05-txdownload-mutex branch from 3f274fb to c85acce Compare July 16, 2024 09:22

DrahtBot requested review from dergoegge, mzumsande and theStack July 16, 2024 13:59

dergoegge approved these changes Jul 19, 2024

View reviewed changes

hebasto reviewed Jul 22, 2024

View reviewed changes

theStack approved these changes Jul 22, 2024

View reviewed changes

glozow mentioned this pull request Jul 23, 2024

m_tx_download_mutex followups #30507

Merged

hebasto approved these changes Jul 24, 2024

View reviewed changes

fanquake merged commit 9607277 into bitcoin:master Jul 24, 2024

glozow deleted the 2024-05-txdownload-mutex branch July 25, 2024 09:06

bitcoin locked and limited conversation to collaborators Jan 12, 2026

	if (!vGetData.empty())
	MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData);

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

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

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow commented May 16, 2024

Uh oh!

ajtowns commented May 16, 2024

Uh oh!

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

Uh oh!

Uh oh!

ajtowns commented May 16, 2024

Uh oh!

instagibbs commented May 17, 2024

Uh oh!

glozow commented May 20, 2024

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs commented May 23, 2024

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theStack commented Jul 15, 2024

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow commented Jul 16, 2024

Uh oh!

instagibbs commented Jul 16, 2024

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

glozow Jul 23, 2024

Choose a reason for hiding this comment

glozow commented May 15, 2024 •

edited

Loading

DrahtBot commented May 15, 2024 •

edited

Loading

glozow commented May 16, 2024 •

edited

Loading

hebasto Jul 22, 2024 •

edited

Loading