Skip to content

fix: purge cache of an overridden confirmed islock and bound the ConnectBlock conflict loop #7885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PastaPastaPasta wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix: purge cache of an overridden confirmed islock and bound the ConnectBlock conflict loop #7885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a834fe
fix(validation): remove conflicting islocks in one bounded pass
PastaPastaPasta Oct 15, 2026
cfbfeef
fix(instantsend): purge cached entries of a removed confirmed islock
PastaPastaPasta Oct 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions src/instantsend/db.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -345,15 +352,15 @@ std::vector<uint256> CInstantSendDb::GetInstantSendLocksByParent(const uint256&
return result;
}

std::vector<uint256> CInstantSendDb::RemoveChainedInstantSendLocks(const uint256& islockHash, const uint256& txid,
int nHeight)
std::vector<uint256> CInstantSendDb::RemoveChainedInstantSendLocks(const uint256& islockHash,
const InstantSendLock& islock, int nHeight)
{
LOCK(cs_db);
std::vector<uint256> result;

std::vector<uint256> stack;
Uint256HashSet added;
stack.emplace_back(txid);
stack.emplace_back(islock.txid);

CDBBatch batch(*db);
while (!stack.empty()) {
Expand All @@ -376,9 +383,11 @@ std::vector<uint256> 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);

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Purge already-confirmed descendant caches too

When the overridden lock has an IS-locked child that was mined at or below the same confirmation height (for example, parent and child are in one block), RemoveConfirmedInstantSendLocks has already deleted the child's DB_HASH_BY_OUTPOINT row while retaining its cache entries. The descendant scan in this function therefore cannot find that child, and this new unconditional erase clears only the root, so after the ChainLocked replacement IsLocked(child_txid) remains true and the child outpoint still reports a conflict. Track or traverse confirmed descendants and extend the new scenario with a confirmed child so both caches are cleared.

AGENTS.md reference: AGENTS.md:L139-L139

Useful? React with 👍 / 👎.

@PastaPastaPasta PastaPastaPasta Oct 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accurate, but out of scope for this PR. Develop has the same behavior, and this PR doesn't make it worse.

A child lock that was confirmed together with its parent has already lost its DB_HASH_BY_OUTPOINT rows. Its entries survive only in the LRU caches, which have no index by parent, so GetInstantSendLocksByParent can't find it. Fixing that means changing what RemoveConfirmedInstantSendLocks keeps cached (keep_cache), not this removal path.

The impact is also limited. The child spends an output of the overridden transaction, which conflicts with the ChainLocked chain. So the child can never be mined there or re-enter the mempool, and its cache entries age out of the LRU. The ConnectBlock bound in the second commit doesn't depend on it either: that pass looks only at the block transaction's own inputs.

🤖 Posted autonomously by Claude on behalf of pasta.

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Erase only cache entries still owned by the removed lock

When an already-confirmed multi-input lock A has one cached input replaced by a newer conflicting lock B while another input still maps to A, overriding A through that other input reaches this unconditional purge and erases B's outpointCache entry as well. This state is reachable because PreVerifyIsLock only logs an overlapping input and WriteNewInstantSendLock overwrites its cache value; if B is then confirmed before another lookup repopulates the entry, confirmation removes B's DB index with keep_cache=true, so future competing blocks spending B's input no longer reach ConflictingISLockIfAny. Only erase each outpoint cache key when its current value still equals islockHash.

AGENTS.md reference: AGENTS.md:L242-L242

Useful? React with 👍 / 👎.

@PastaPastaPasta PastaPastaPasta Oct 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'm leaving this as is: the erased entry can't be one the code depends on.

  • While B is unconfirmed, it is still in the db. Erasing its outpointCache entry only means the next GetInstantSendLockByInput falls back to the db DB_HASH_BY_OUTPOINT row, which still maps to B.
  • Once B is confirmed, its outpoint entry exists only in a 10000-entry LRU cache that evicts confirmed locks all the time. "Confirmed lock no longer visible by input" is already the normal end state for every confirmed lock, and nothing relies on those entries for correctness. A competing block for a confirmed (ChainLocked) B is rejected by ChainLocks.
  • Reaching the state also requires a quorum to sign B on an input that a confirmed transaction already spends. That is the same compromised-quorum precondition the ChainLock-overrides-IS path already assumes.

An ownership check before each erase would add code to protect an entry the cache can drop at any moment. The existing RemoveInstantSendLock(..., keep_cache=false) path erases the same keys without that check.

🤖 Posted autonomously by Claude on behalf of pasta.

WriteInstantSendLockArchived(batch, islockHash, nHeight);
result.emplace_back(islockHash);

Expand Down
6 changes: 4 additions & 2 deletions src/instantsend/db.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<uint256> RemoveChainedInstantSendLocks(const uint256& islockHash, const uint256& txid, int nHeight) EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
std::vector<uint256> RemoveChainedInstantSendLocks(const uint256& islockHash, const InstantSendLock& islock,
int nHeight) EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
};
} // namespace instantsend

Expand Down
18 changes: 12 additions & 6 deletions src/instantsend/instantsend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -450,12 +450,18 @@ std::optional<std::pair</*islock_hash=*/uint256, /*txid=*/uint256>> 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
Expand Down
2 changes: 1 addition & 1 deletion src/instantsend/instantsend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::pair</*islock_hash=*/uint256, /*txid=*/uint256>> 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
Expand Down
33 changes: 33 additions & 0 deletions src/test/evo_islock_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,39 @@ uint256 CalculateRequestId(const std::vector<COutPoint>& 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<uint256> locked_txids;
for (const auto& in : conflict_tx.vin) {
auto islock = std::make_shared<instantsend::InstantSendLock>();
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
Expand Down
4 changes: 2 additions & 2 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading