From 6a834fe0d5471c4cc85f56bd073e0ddbca2bad80 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Oct 2026 22:15:35 -0500 Subject: [PATCH 1/2] fix(validation): remove conflicting islocks in one bounded pass ConnectBlock looped while ConflictingISLockIfAny found a conflict and removed one lock per round, so the loop only ended if every removal made the lock invisible to the next lookup. Remove every lock conflicting with the transaction's inputs in a single pass instead, so the work done under cs_main is bounded: at most one root lock removal per input. Co-Authored-By: Claude Opus 5.5 --- src/instantsend/instantsend.cpp | 16 +++++++++++----- src/instantsend/instantsend.h | 2 +- src/validation.cpp | 4 ++-- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index 138d409c4e30..5610ce670fc3 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -450,12 +450,18 @@ std::optional> CInstantSend return std::make_pair(::SerializeHash(*islock), islock->txid); } -bool CInstantSendManager::RemoveConflictingISLockByTx(const CTransaction& tx) +void CInstantSendManager::RemoveConflictingISLocksByTx(const CTransaction& tx) { - const auto islock = GetConflictingLock(tx); - if (!islock) return false; - RemoveConflictingLock(::SerializeHash(*islock), *islock); - return true; + if (!IsInstantSendEnabled()) { + return; + } + + for (const auto& outpoint : instantsend::GetLockInputs(tx)) { + const auto islock = db.GetInstantSendLockByInput(outpoint); + if (islock && islock->txid != tx.GetHash()) { + RemoveConflictingLock(::SerializeHash(*islock), *islock); + } + } } size_t CInstantSendManager::GetInstantSendLockCount() const diff --git a/src/instantsend/instantsend.h b/src/instantsend/instantsend.h index 22a5a8a46a11..44b2ea3b6ca5 100644 --- a/src/instantsend/instantsend.h +++ b/src/instantsend/instantsend.h @@ -120,7 +120,7 @@ class CInstantSendManager bool IsWaitingForTx(const uint256& txHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); instantsend::InstantSendLockPtr GetConflictingLock(const CTransaction& tx) const; std::optional> ConflictingISLockIfAny(const CTransaction& tx) const; - bool RemoveConflictingISLockByTx(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache); + void RemoveConflictingISLocksByTx(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache); /* Helpers for communications between CInstantSendManager & NetInstantSend */ // This helper returns up to 32 pending locks and remove them from queue of pending diff --git a/src/validation.cpp b/src/validation.cpp index f596fe5fe0b1..2ca3ca862e43 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3014,11 +3014,11 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, const bool has_chainlock = m_chainlocks.HasChainLock(pindex->nHeight, pindex->GetBlockHash()); for (const auto& tx : block.vtx) { if (!instantsend::HasLockInputs(*tx)) continue; - while (auto conflictLockOpt = m_isman->ConflictingISLockIfAny(*tx)) { + if (auto conflictLockOpt = m_isman->ConflictingISLockIfAny(*tx)) { auto [conflict_islock_hash, conflict_txid] = conflictLockOpt.value(); if (has_chainlock) { LogPrint(BCLog::ALL, "ConnectBlock(DASH): chain-locked transaction %s overrides islock %s\n", tx->GetHash().ToString(), conflict_islock_hash.ToString()); - m_isman->RemoveConflictingISLockByTx(*tx); + m_isman->RemoveConflictingISLocksByTx(*tx); } else { // The node which relayed this should switch to correct chain. // TODO: relay instantsend data/proof. From cfbfeef17ccbbe75b4ff158582a6ed6df727b428 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Oct 2026 22:15:35 -0500 Subject: [PATCH 2/2] fix(instantsend): purge cached entries of a removed confirmed islock RemoveConfirmedInstantSendLocks erases a confirmed lock from the db but keeps its islock, txid and outpoint cache entries. When a ChainLocked block later conflicts with that lock, RemoveChainedInstantSendLocks finds no db copy and so never erased those entries. The stale txid entry kept IsLocked() returning true for the overridden lock's transaction, because the hash it points to is archived and therefore still known. Lookups by input or txid only stop returning the removed lock because the use_cache=false db read overwrites its islock cache entry with nullptr as a side effect. Pass the lock itself and always erase its cache entries so removal no longer depends on that write. Co-Authored-By: Claude Opus 5.5 --- src/instantsend/db.cpp | 29 +++++++++++++++++++---------- src/instantsend/db.h | 6 ++++-- src/instantsend/instantsend.cpp | 2 +- src/test/evo_islock_tests.cpp | 33 +++++++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 13 deletions(-) diff --git a/src/instantsend/db.cpp b/src/instantsend/db.cpp index bd07f39c5457..b80972b2a7db 100644 --- a/src/instantsend/db.cpp +++ b/src/instantsend/db.cpp @@ -80,11 +80,18 @@ void CInstantSendDb::RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, } if (!keep_cache) { - islockCache.erase(hash); - txidCache.erase(islock.txid); - for (const auto& in : islock.inputs) { - outpointCache.erase(in); - } + EraseInstantSendLockCache(hash, islock); + } +} + +void CInstantSendDb::EraseInstantSendLockCache(const uint256& hash, const InstantSendLock& islock) +{ + AssertLockHeld(cs_db); + + islockCache.erase(hash); + txidCache.erase(islock.txid); + for (const auto& in : islock.inputs) { + outpointCache.erase(in); } } @@ -345,15 +352,15 @@ std::vector CInstantSendDb::GetInstantSendLocksByParent(const uint256& return result; } -std::vector CInstantSendDb::RemoveChainedInstantSendLocks(const uint256& islockHash, const uint256& txid, - int nHeight) +std::vector CInstantSendDb::RemoveChainedInstantSendLocks(const uint256& islockHash, + const InstantSendLock& islock, int nHeight) { LOCK(cs_db); std::vector result; std::vector stack; Uint256HashSet added; - stack.emplace_back(txid); + stack.emplace_back(islock.txid); CDBBatch batch(*db); while (!stack.empty()) { @@ -376,9 +383,11 @@ std::vector CInstantSendDb::RemoveChainedInstantSendLocks(const uint256 } } - if (auto islock = GetInstantSendLockByHashInternal(islockHash, /*use_cache=*/false)) { - RemoveInstantSendLock(batch, islockHash, *islock, false); + if (GetInstantSendLockByHashInternal(islockHash, /*use_cache=*/false)) { + RemoveInstantSendLock(batch, islockHash, islock); } + // Also needed when the lock was already confirmed: that removed it from the db but kept it cached + EraseInstantSendLockCache(islockHash, islock); WriteInstantSendLockArchived(batch, islockHash, nHeight); result.emplace_back(islockHash); diff --git a/src/instantsend/db.h b/src/instantsend/db.h index e8b5deb405c6..3c96ba5016b2 100644 --- a/src/instantsend/db.h +++ b/src/instantsend/db.h @@ -56,6 +56,7 @@ class CInstantSendDb */ void RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, const InstantSendLock& islock, bool keep_cache = true) EXCLUSIVE_LOCKS_REQUIRED(cs_db); + void EraseInstantSendLockCache(const uint256& hash, const InstantSendLock& islock) EXCLUSIVE_LOCKS_REQUIRED(cs_db); /** * Marks an InstantSend Lock as archived. * @param batch Object used to batch many calls together @@ -154,11 +155,12 @@ class CInstantSendDb /** * Called when a ChainLock invalidated a IS Lock, removes any chained/children IS Locks and the invalidated IS Lock * @param islockHash IS Lock hash which has been invalidated - * @param txid Transaction id associated with the islockHash + * @param islock The invalidated IS Lock itself * @param nHeight height of the block which received a chainlock and invalidated the IS Lock * @return A vector of IS Lock hashes of all IS Locks removed */ - std::vector RemoveChainedInstantSendLocks(const uint256& islockHash, const uint256& txid, int nHeight) EXCLUSIVE_LOCKS_REQUIRED(!cs_db); + std::vector RemoveChainedInstantSendLocks(const uint256& islockHash, const InstantSendLock& islock, + int nHeight) EXCLUSIVE_LOCKS_REQUIRED(!cs_db); }; } // namespace instantsend diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index 5610ce670fc3..716a1563f4da 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -343,7 +343,7 @@ void CInstantSendManager::RemoveConflictingLock(const uint256& islockHash, const islock.txid.ToString(), islockHash.ToString()); const int tipHeight = GetTipHeight(); - auto removedIslocks = db.RemoveChainedInstantSendLocks(islockHash, islock.txid, tipHeight); + auto removedIslocks = db.RemoveChainedInstantSendLocks(islockHash, islock, tipHeight); for (const auto& h : removedIslocks) { LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: removed (child) ISLOCK %s\n", __func__, islock.txid.ToString(), islockHash.ToString(), h.ToString()); diff --git a/src/test/evo_islock_tests.cpp b/src/test/evo_islock_tests.cpp index 43e5d9c99c29..d8ea2919b6ad 100644 --- a/src/test/evo_islock_tests.cpp +++ b/src/test/evo_islock_tests.cpp @@ -223,6 +223,39 @@ uint256 CalculateRequestId(const std::vector& inputs) return hw.GetHash(); } +BOOST_FIXTURE_TEST_CASE(chainlocked_conflict_overrides_confirmed_locks, NetInstantSendTest) +{ + // Confirming a lock keeps it only in the db caches; a ChainLocked transaction that later + // overrides such locks must leave neither a conflict nor their transactions reported as locked + auto& isman = *m_node.isman; + CMutableTransaction conflict; + conflict.vin.emplace_back(COutPoint{GetRandHash(), 0}); + conflict.vin.emplace_back(COutPoint{GetRandHash(), 1}); + conflict.vout.emplace_back(COIN, CScript{}); + const CTransaction conflict_tx{conflict}; + + const auto* tip = WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Tip()); + std::vector locked_txids; + for (const auto& in : conflict_tx.vin) { + auto islock = std::make_shared(); + islock->txid = GetRandHash(); + islock->inputs.push_back(in.prevout); + isman.WriteNewISLock(::SerializeHash(*islock), islock, tip->nHeight); + locked_txids.push_back(islock->txid); + } + isman.RemoveConfirmedInstantSendLocks(tip); + for (const auto& txid : locked_txids) { + BOOST_REQUIRE(isman.IsLocked(txid)); + } + BOOST_REQUIRE(isman.ConflictingISLockIfAny(conflict_tx)); + + isman.RemoveConflictingISLocksByTx(conflict_tx); + BOOST_CHECK(!isman.ConflictingISLockIfAny(conflict_tx)); + for (const auto& txid : locked_txids) { + BOOST_CHECK(!isman.IsLocked(txid)); + } +} + BOOST_AUTO_TEST_CASE(getrequestid) { // Create an empty InstantSendLock