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 138d409c4e30..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()); @@ -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/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 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.