From ce694802d99e42a087595ae132ebf2fa1d9a8f91 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 25 Jan 2023 14:26:08 -0700 Subject: [PATCH] Fetch recently conflicted transactions incrementally in ThreadNotifyWallet. We no longer fetch updates from the mempool unless we have fetched all updates from the chain, as we would otherwise notify the wallet of mempool changes for which they have not observed parent transactions in the chain. Co-authored-by: Jack Grigg --- src/main.cpp | 37 +++++++++++++++++------------ src/main.h | 3 ++- src/validationinterface.cpp | 46 ++++++++++++++++++++++++++++--------- 3 files changed, 59 insertions(+), 27 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 5a5bd9e95..4e1d816dd 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4062,8 +4062,8 @@ static int64_t nTimeChainState = 0; static int64_t nTimePostConnect = 0; // Protected by cs_main -std::map> recentlyConflictedTxs; -uint64_t nRecentlyConflictedSequence = 0; +std::map> recentlyConflictedTxs; +uint64_t nConnectedSequence = 0; uint64_t nNotifiedSequence = 0; /** @@ -4115,7 +4115,9 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, // Cache the conflicted transactions for subsequent notification. // Updates to connected wallets are triggered by ThreadNotifyWallets recentlyConflictedTxs.insert(std::make_pair(pindexNew, txConflicted)); - nRecentlyConflictedSequence += 1; + + // Increment the count of `ConnectTip` calls. + nConnectedSequence += 1; EnforceNodeDeprecation(pindexNew->nHeight); @@ -4126,29 +4128,34 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, return true; } -std::pair>, uint64_t> DrainRecentlyConflicted() +std::pair, std::optional> TakeRecentlyConflicted(const CBlockIndex* pindex) { - uint64_t recentlyConflictedSequence; - std::map> txs; - { - LOCK(cs_main); - recentlyConflictedSequence = nRecentlyConflictedSequence; - txs.swap(recentlyConflictedTxs); - } + AssertLockHeld(cs_main); - return std::make_pair(txs, recentlyConflictedSequence); + std::list conflictedTxs = recentlyConflictedTxs.at(pindex); + recentlyConflictedTxs.erase(pindex); + if (recentlyConflictedTxs.empty()) { + return std::make_pair(conflictedTxs, nConnectedSequence); + } else { + return std::make_pair(conflictedTxs, std::nullopt); + } } -void SetChainNotifiedSequence(const CChainParams& chainparams, uint64_t recentlyConflictedSequence) { +uint64_t GetChainConnectedSequence() { + LOCK(cs_main); + return nConnectedSequence; +} + +void SetChainNotifiedSequence(const CChainParams& chainparams, uint64_t connectedSequence) { assert(chainparams.NetworkIDString() == "regtest"); LOCK(cs_main); - nNotifiedSequence = recentlyConflictedSequence; + nNotifiedSequence = connectedSequence; } bool ChainIsFullyNotified(const CChainParams& chainparams) { assert(chainparams.NetworkIDString() == "regtest"); LOCK(cs_main); - return nRecentlyConflictedSequence == nNotifiedSequence; + return nConnectedSequence == nNotifiedSequence; } /** diff --git a/src/main.h b/src/main.h index 6cc4df786..dabba715d 100644 --- a/src/main.h +++ b/src/main.h @@ -668,7 +668,8 @@ CMutableTransaction CreateNewContextualCMutableTransaction( int nHeight, bool requireV4); -std::pair>, uint64_t> DrainRecentlyConflicted(); +std::pair, std::optional> TakeRecentlyConflicted(const CBlockIndex* pindex); +uint64_t GetChainConnectedSequence(); void SetChainNotifiedSequence(const CChainParams& chainparams, uint64_t recentlyConflictedSequence); bool ChainIsFullyNotified(const CChainParams& chainparams); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 476dab819..446f9ee53 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -163,8 +163,9 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) // The stack of blocks we will notify as having been connected. // Pushed in reverse, popped in order. std::vector blockStack; - // Transactions that have been recently conflicted out of the mempool. - std::pair>, uint64_t> recentlyConflicted; + // Sequence number indicating that we have notified wallets of transactions up to + // the ConnectBlock() call that generated this sequence number. + std::optional chainNotifiedSequence; // Transactions that have been recently added to the mempool. std::pair, uint64_t> recentlyAdded; @@ -176,12 +177,8 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) CBlockIndex *pindex = chainActive.Tip(); pindexFork = chainActive.FindFork(pindexLastTip); - // Fetch recently-conflicted transactions. These will include any - // block that has been connected since the last cycle, but we only - // notify for the conflicts created by the current active chain. - recentlyConflicted = DrainRecentlyConflicted(); - // Iterate backwards over the connected blocks we need to notify. + bool originalTipAtFork = pindex && pindex == pindexFork; while (pindex && pindex != pindexFork) { MerkleFrontiers oldFrontiers; // Get the Sprout commitment tree as of the start of this block. @@ -214,15 +211,38 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) OrchardMerkleFrontier::empty_root(), oldFrontiers.orchard)); } + // Fetch recently-conflicted transactions. These will include any + // block that has been connected since the last cycle, but we only + // notify for the conflicts created by the current active chain. + auto recentlyConflicted = TakeRecentlyConflicted(pindex); + blockStack.emplace_back( pindex, oldFrontiers, - recentlyConflicted.first[pindex]); + recentlyConflicted.first); + + chainNotifiedSequence = recentlyConflicted.second; pindex = pindex->pprev; } - recentlyAdded = mempool.DrainRecentlyAdded(); + // This conditional can be true in the case that in the interval + // since the last second-boundary, two reorgs occurred: one that + // shifted over to a different chain history, and then a second + // that returned the chain to the original pre-reorg tip. This + // should never occur unless a caller has manually used + // `invalidateblock` to force the second reorg or we have a long + // persistent set of dueling chains. In such a case, wallets may + // not be fully notified of conflicted transactions, but they will + // still have a correct view of the current main chain, and they + // will still be notified properly of the current state of + // transactions in the mempool. + if (originalTipAtFork) { + chainNotifiedSequence = GetChainConnectedSequence(); + } + if (chainNotifiedSequence.has_value()) { + recentlyAdded = mempool.DrainRecentlyAdded(); + } } // @@ -540,8 +560,12 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) // Update the notified sequence numbers. We only need this in regtest mode, // and should not lock on cs or cs_main here otherwise. if (chainParams.NetworkIDString() == "regtest") { - SetChainNotifiedSequence(chainParams, recentlyConflicted.second); - mempool.SetNotifiedSequence(recentlyAdded.second); + if (chainNotifiedSequence.has_value()) { + SetChainNotifiedSequence(chainParams, chainNotifiedSequence.value()); + } + if (recentlyAdded.second > 0) { + mempool.SetNotifiedSequence(recentlyAdded.second); + } } } }