Skip to content

fix: reconstruct ISLOCK locally when recsig arrived first #7291

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

Closed
knst wants to merge 1 commit into from
Closed

fix: reconstruct ISLOCK locally when recsig arrived first #7291

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 30 additions & 4 deletions src/instantsend/signing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,10 +384,6 @@ void InstantSendSigner::TrySignInstantSendLock(const CTransaction& tx)

auto id = islock.GetRequestId();

if (m_sigman.HasRecoveredSigForId(llmqType, id)) {
return;
}

const auto& llmq_params_opt = Params().GetLLMQ(llmqType);
assert(llmq_params_opt);
const auto quorum = llmq::SelectQuorumForSigning(llmq_params_opt.value(), m_chainstate.m_chain, m_qman, id);
Expand All @@ -402,6 +398,9 @@ void InstantSendSigner::TrySignInstantSendLock(const CTransaction& tx)
quorum->m_quorum_base_block_index->nHeight % llmq_params_opt->dkgInterval;
islock.cycleHash = quorum->m_quorum_base_block_index->GetAncestor(cycle_height)->GetBlockHash();

// Emplace into creatingInstantSendLocks first so that any concurrent recsig listener
// (firing on a parallel verification worker) finds the entry and handles reconstruction
// through HandleNewInstantSendLockRecoveredSig.
{
LOCK(cs_creating);
auto e = creatingInstantSendLocks.emplace(id, std::move(islock));
Expand All @@ -411,6 +410,33 @@ void InstantSendSigner::TrySignInstantSendLock(const CTransaction& tx)
txToCreatingInstantSendLocks.emplace(tx.GetHash(), &e.first->second);
}

// After emplacing, check whether the recsig is already in db. If yes, the listener
// either already ran while creating was empty (so we must reconstruct here), or it
// ran after our emplace and already removed the entry (so the find below misses).
// Either case is handled correctly by the find/erase pattern.
if (llmq::CRecoveredSig recSig; m_sigman.GetRecoveredSigForId(llmqType, id, recSig)) {
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 26, 2026

Choose a reason for hiding this comment

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

P2 Badge Recompute cycle hash from recovered signature quorum

When GetRecoveredSigForId returns true, this branch reuses the cycleHash computed earlier from SelectQuorumForSigning at the current tip and only swaps in recSig.sig. If the recovered signature was created in an older active set (for example, tx seen after a quorum rotation), the reconstructed ISLOCK can carry a cycleHash that doesn't match recSig's quorum, so peers that verify via cycleHash (see net_instantsend.cpp quorum selection path) may reject the relayed lock. This path should derive/validate cycleHash from recSig.getQuorumHash() before enqueuing.

Useful? React with 👍 / 👎.

InstantSendLockPtr islock_ptr;
{
LOCK(cs_creating);
auto it = creatingInstantSendLocks.find(id);
if (it == creatingInstantSendLocks.end()) {
// The listener already fired after our emplace and reconstructed; nothing to do.
return;
}
islock_ptr = std::make_shared<InstantSendLock>(std::move(it->second));
txToCreatingInstantSendLocks.erase(islock_ptr->txid);
creatingInstantSendLocks.erase(it);
}
if (recSig.getMsgHash() != tx.GetHash()) {
LogPrintf("%s -- txid=%s: existing recovered islock sig has conflicting msgHash %s\n",
__func__, tx.GetHash().ToString(), recSig.getMsgHash().ToString());
return;
}
islock_ptr->sig = recSig.sig;
m_isman.TryEmplacePendingLock(::SerializeHash(*islock_ptr), /*id=*/-1, islock_ptr);
return;
}

m_shareman.AsyncSignIfMember(llmqType, m_sigman, id, tx.GetHash(), quorum->m_quorum_base_block_index->GetBlockHash());
Comment on lines 401 to 440
Copy link
Copy Markdown

@thepastaclaw thepastaclaw Apr 26, 2026

Choose a reason for hiding this comment

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

🟡 Suggestion: The race fix changed ISLOCK reconstruction without adding regression coverage for the reordered path

This change only touches TrySignInstantSendLock() and no unit or functional test was added or updated alongside it. The new behavior depends on a narrow interleaving between creatingInstantSendLocks, HandleNewRecoveredSig(), and GetRecoveredSigForId(): the code is now correct only if the "recovered sig already exists or arrives concurrently" path keeps routing reconstruction through the new find/erase flow. Because that path is timing-dependent and not exercised explicitly anywhere in the checked-in tests, CI still has no deterministic signal that this race is actually closed or that a later refactor will not reopen it.

source: ['codex']

🤖 Fix this with AI agents 
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/instantsend/signing.cpp`:
- [SUGGESTION] lines 401-440: The race fix changed ISLOCK reconstruction without adding regression coverage for the reordered path
  This change only touches `TrySignInstantSendLock()` and no unit or functional test was added or updated alongside it. The new behavior depends on a narrow interleaving between `creatingInstantSendLocks`, `HandleNewRecoveredSig()`, and `GetRecoveredSigForId()`: the code is now correct only if the "recovered sig already exists or arrives concurrently" path keeps routing reconstruction through the new find/erase flow. Because that path is timing-dependent and not exercised explicitly anywhere in the checked-in tests, CI still has no deterministic signal that this race is actually closed or that a later refactor will not reopen it.

}
} // namespace instantsend
Loading