locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip #30111

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

See #27463 for full project tracking.

This contains the first 4 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, UpdatedBlockTipSync, which is a synchronous version of UpdatedBlockTip.
Flush m_recent_rejects and m_recent_rejects_reconsiderable on UpdatedBlockTipSync just once instead of every time AlreadyHaveTx is called. This should speed up calls to that function and removes the need to lock cs_main every time it is called.

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

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:

#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#30186 (fuzz: increase txorphan harness stability by marcofleon)
#30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
#30110 (refactor: TxDownloadManager by glozow)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#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

hebasto · 2024-05-15T16:00:46Z

src/validation.cpp

-        }
+            } // release MempoolMutex
+            if (m_chainman.m_options.signals) {
+                m_chainman.m_options.signals->UpdatedBlockTipSync(pindexNewTip);


This call invokes logging, i.e. I/O operations, while holding the ::cs_main ~~and m_mempool->cs~~ mutexes. It is considered as a suboptimal approach from the performance point of view. Could it be avoided?

I don't think it's true that m_mempool->cs is still held here (see line above)?

I don't think it's true that m_mempool->cs is still held here (see line above)?

You're right. I was confused by indentation. My apologies.

As for cs_main, it seems necessary for the chain tip to be what we think it is when the callback happens. IIUC the logging only happens in debug?

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

ajtowns · 2024-05-15T16:32:08Z

src/validationinterface.cpp

@@ -183,6 +183,12 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo
                          fInitialDownload);
 }

+void ValidationSignals::UpdatedBlockTipSync(const CBlockIndex *pindexNew)


Doesn't this also need to be called from InvalidateBlock(), potentially each time the tip is updated?

seems to be covered at 9d413af#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R3635 now

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

instagibbs · 2024-05-20T18:59:55Z

src/validationinterface.cpp

@@ -183,6 +183,12 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo
                          fInitialDownload);
 }

+void ValidationSignals::UpdatedBlockTipSync(const CBlockIndex *pindexNew)


seems to be covered at 9d413af#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R3635 now

instagibbs · 2024-05-20T19:05:37Z

src/net_processing.cpp

@@ -493,9 +493,9 @@ class PeerManagerImpl final : public PeerManager

    /** Overridden from CValidationInterface. */
    void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected) override
-        EXCLUSIVE_LOCKS_REQUIRED(!m_recent_confirmed_transactions_mutex);
+        EXCLUSIVE_LOCKS_REQUIRED(!m_recent_confirmed_transactions_mutex, !m_tx_download_mutex);


not a locking expert but would LOCKS_EXCLUDED be easier to read?

Oh, I didn't know you could do both. Happy to change if people want, but generally prefer to follow convention of the existing annotations.

EXCLUSIVE_LOCKS_REQUIRED(!foo) is a stronger assertion; it says that if the caller can see the mutex, it also has to have the same assertion. LOCKS_EXCLUDED(foo) just says you can't LOCK() or have EXCLUSIVE_LOCKS_REQUIRED(foo) -- so there's nothing preventing the caller's caller from having taken the lock.

Ah that makes sense. I will leave this as is.

ok, makes sense why I only see it used in cs_main contexts, since it's a global mutex. thanks

src/test/orphanage_tests.cpp

src/txorphanage.h

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

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

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

instagibbs

utACK ef8de26

instagibbs · 2024-05-23T14:31:25Z

src/net_processing.cpp

@@ -779,7 +779,10 @@ class PeerManagerImpl final : public PeerManager
    BanMan* const m_banman;
    ChainstateManager& m_chainman;
    CTxMemPool& m_mempool;
-    TxRequestTracker m_txrequest GUARDED_BY(::cs_main);
+
+    /** Protects tx download including TxRequestTracker, rejection filters, and TxOrphanage. */


non-blocking nit: commit message still referencing TxRequest

glozow added the P2P label May 15, 2024

glozow mentioned this pull request May 15, 2024

Package Relay Project Tracking #27463

Open

56 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 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

ajtowns reviewed May 15, 2024

View reviewed changes

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

glozow added 7 commits May 23, 2024 13:24

guard TxRequest and rejection caches with new mutex

e71a5ae

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.

remove obsoleted hashRecentRejectsChainTip

1fe4e3f

This also means AlreadyHaveTx no longer needs cs_main held.

lock m_recent_confirmed_transactions using m_tx_download_mutex

afd112d

remove obsoleted TxOrphanage::m_mutex

1d7c2c9

The TxOrphanage is now guarded externally by m_tx_download_mutex.

[refactor] delete EraseTxNoLock, just use EraseTx

ef8de26

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 #30110

Draft

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

Open

instagibbs approved these changes May 24, 2024

View reviewed changes

This was referenced May 24, 2024

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

Open

fuzz: increase txorphan harness stability #30186

Open

DrahtBot mentioned this pull request May 31, 2024

refactor: Improve assumeutxo state representation #30214

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip #30111

locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip #30111

glozow commented May 15, 2024 •

edited

DrahtBot commented May 15, 2024 •

edited

hebasto May 15, 2024 •

edited

glozow May 15, 2024

hebasto May 15, 2024

glozow May 15, 2024

ajtowns left a comment

ajtowns May 15, 2024

instagibbs May 20, 2024

glozow commented May 16, 2024

ajtowns commented May 16, 2024

glozow commented May 16, 2024 •

edited

ajtowns commented May 16, 2024

instagibbs commented May 17, 2024

glozow commented May 20, 2024

instagibbs left a comment

instagibbs May 20, 2024

instagibbs May 20, 2024

glozow May 23, 2024

ajtowns May 23, 2024 •

edited

glozow May 23, 2024

instagibbs May 23, 2024

instagibbs commented May 23, 2024

instagibbs left a comment

instagibbs May 23, 2024

locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip #30111

Are you sure you want to change the base?

locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip #30111

Conversation

glozow commented May 15, 2024 • edited

DrahtBot commented May 15, 2024 • edited

Code Coverage

Reviews

Conflicts

hebasto May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajtowns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow commented May 16, 2024

ajtowns commented May 16, 2024

glozow commented May 16, 2024 • edited

ajtowns commented May 16, 2024

instagibbs commented May 17, 2024

glozow commented May 20, 2024

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajtowns May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented May 23, 2024

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow commented May 15, 2024 •

edited

DrahtBot commented May 15, 2024 •

edited

hebasto May 15, 2024 •

edited

glozow commented May 16, 2024 •

edited

ajtowns May 23, 2024 •

edited