refactor: remove dependencies of CInstantSendManager on mempool, signer, chainstate#7178
Open
knst wants to merge 15 commits intodashpay:developfrom
Open
refactor: remove dependencies of CInstantSendManager on mempool, signer, chainstate#7178knst wants to merge 15 commits intodashpay:developfrom
knst wants to merge 15 commits intodashpay:developfrom
Conversation
|
WalkthroughThis PR refactors InstantSend and related wiring: ActiveContext no longer holds m_isman; CDSNotificationInterface drops its LLMQContext dependency; CInstantSendManager is reworked with a new storage/pending-lock model and many lifecycle hooks removed; NetInstantSend is extended to implement validation callbacks (block tip, mempool, chainlock handling) and coordinate IS lock processing; LLMQContext and initialization sites are simplified to pass explicit dependencies (signer, sigman, qman, chainlocks, mempool, mn_sync) to NetInstantSend. Sequence Diagram(s)sequenceDiagram
participant Chain as Blockchain
participant NetIS as NetInstantSend
participant ISMan as CInstantSendManager
participant Mempool as Mempool
participant Signer as InstantSendSigner
Chain->>NetIS: UpdatedBlockTip / BlockConnected
NetIS->>NetIS: Cache tip height
NetIS->>ISMan: WriteBlockISLocks(...)
NetIS->>Signer: TruncateRecoveredSigsForInputs(...) / NotifyChainLock(...)
Mempool->>NetIS: TransactionAddedToMempool(tx)
NetIS->>ISMan: AttachISLockToTx(tx)
alt IS lock attached
ISMan-->>NetIS: InstantSendLockPtr
NetIS->>Mempool: RemoveMempoolConflictsForLock(hash, islock)
NetIS->>Signer: ClearSignerInputsForConflicts(...)
else pending lock or no tx
NetIS->>NetIS: Add to pending/non-locked tracking
NetIS->>Signer: SignalMissingTx(...)
end
NetIS->>ISMan: AddPendingISLock / WriteNewISLock(...)
NetIS->>Chain: ResolveBlockConflicts(...)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
src/instantsend/instantsend.h (1)
30-30: Consider includingprimitives/transaction.hinstead of re-declaringCTransactionRef.This file-level typedef duplicates the canonical definition from
primitives/transaction.h. If the canonical typedef ever changes (e.g., to use a different smart pointer type), this local copy would silently diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/instantsend.h` at line 30, The file-level typedef duplicates the canonical CTransactionRef; remove the local typedef in instantsend.h and include the canonical header instead: add an `#include` of primitives/transaction.h and delete the line "typedef std::shared_ptr<const CTransaction> CTransactionRef;" so the file uses the single source-of-truth CTransactionRef definition (referenced here as CTransactionRef in instantsend.h).src/instantsend/net_instantsend.cpp (5)
74-88:ResolveCycleHeightduplicates the fallback already performed insideGetBlockHeight.
GetBlockHeight(lines 40–55) already checks the cache, falls back toLookupBlockIndex, caches the result, and returns the height. If it returnsstd::nullopt, the block simply isn't known. The secondLookupBlockIndexcall on line 81 will hit the same state under the samecs_mainlock and produce the samenullptr.Suggested simplification
std::optional<int> NetInstantSend::ResolveCycleHeight(const uint256& cycle_hash) { - auto cycle_height = GetBlockHeight(m_is_manager, m_chainstate, cycle_hash); - if (cycle_height) { - return cycle_height; - } - - const auto block_index = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(cycle_hash)); - if (block_index == nullptr) { - return std::nullopt; - } - - m_is_manager.CacheBlockHeight(block_index); - return block_index->nHeight; + return GetBlockHeight(m_is_manager, m_chainstate, cycle_hash); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/net_instantsend.cpp` around lines 74 - 88, ResolveCycleHeight duplicates the fallback logic already implemented in GetBlockHeight: call GetBlockHeight(m_is_manager, m_chainstate, cycle_hash) and return its result directly instead of performing a second WITH_LOCK + m_chainstate.m_blockman.LookupBlockIndex and manual caching; remove the extra LookupBlockIndex and m_is_manager.CacheBlockHeight usage from ResolveCycleHeight so it simply returns the std::optional<int> produced by GetBlockHeight.
25-31: Forward-declaringnode::GetTransactionto break a header dependency is fragile.If the signature in
node/transaction.hever changes, this forward declaration will silently diverge, leading to linker errors or undefined behavior. Consider adding a static_assert or a comment pointing to the canonical declaration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/net_instantsend.cpp` around lines 25 - 31, The forward declaration of node::GetTransaction (CTransactionRef GetTransaction(const CBlockIndex* const, const CTxMemPool*, const uint256&, const Consensus::Params&, uint256&)) is fragile; update the file to either include the canonical declaration from node/transaction.h or add a compile-time check to ensure the signature matches: add a static_assert that compares decltype(&node::GetTransaction) to the expected function pointer type (or otherwise references the exact signature), and/or add a clear comment pointing to node/transaction.h as the authoritative declaration to prevent silent divergence.
353-415:ProcessInstantSendLocklooks correct; one observation on the dual-lookup pattern.The method first calls
GetTransaction(nullptr, &m_mempool, ...)which may return a mined tx withhashBlockset, then callsGetBlockHeightwhich may miss the cache and do aLookupBlockIndex. If the block is known, lines 370–375 do a third lookup viaLookupBlockIndex. Consider consolidating: ifGetTransactionreturns a non-nullhashBlock, you could do theLookupBlockIndexonce and pass the result to both the height-caching path and the ChainLock check.This is a minor efficiency nit and not a correctness issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/net_instantsend.cpp` around lines 353 - 415, ProcessInstantSendLock performs multiple block-index lookups: after GetTransaction and GetBlockHeight, code calls LookupBlockIndex again (in the block handling found_transaction) causing a redundant third lookup; consolidate by performing a single LookupBlockIndex once when the transaction is found and minedHeight is empty (capture result in a local pindexMined), call m_is_manager.CacheBlockHeight(pindexMined) and set minedHeight from pindexMined->nHeight, then use that minedHeight for the subsequent m_chainlocks.HasChainLock(hash, hashBlock) check and for WriteNewISLock; update the block that currently does LookupBlockIndex (the code around lines using WITH_LOCK(::cs_main) and pindexMined) to reuse this single lookup and remove the duplicate lookup path so LookupBlockIndex is only called once per code path.
591-591: Note:IsKnownTxsemantics are inverted from what the name suggests (seeinstantsend.cppcomment).At line 591,
isLockedTxKnownwill betruewhen the islock is not inpendingNoTxInstantSendLocks(meaning we already have the transaction). This works correctly in context but the naming is confusing. See related comment oninstantsend.cpp:273-277.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/net_instantsend.cpp` at line 591, The variable isLockedTxKnown is misleading because m_is_manager.IsKnownTx(islockHash) returns true when the IS lock is NOT in pendingNoTxInstantSendLocks (i.e., we already have the transaction); rename or invert the boolean to match semantics: either rename isLockedTxKnown to isLockedTxMissing or isLockedTxPresent (choose a name that matches the actual meaning), or assign with a negation (e.g., bool isLockedTxMissing = !m_is_manager.IsKnownTx(islockHash)) and update all subsequent uses accordingly (references around net_instantsend.cpp where isLockedTxKnown is checked). Ensure consistency with the comment in instantsend.cpp about IsKnownTx semantics.
222-241: Theelsebranch withassert(false)is unreachable thanks to therequiresclause.The C++20
requiresconstraint on line 224 already limitsTtoCTxInorCOutPoint, so theelsebranch on line 235 can never be reached. This is harmless but adds dead code.Simplified version
template <typename T> requires std::same_as<T, CTxIn> || std::same_as<T, COutPoint> Uint256HashSet GetIdsFromLockable(const std::vector<T>& vec) { Uint256HashSet ret{}; if (vec.empty()) return ret; ret.reserve(vec.size()); for (const auto& in : vec) { if constexpr (std::is_same_v<T, COutPoint>) { ret.emplace(instantsend::GenInputLockRequestId(in)); - } else if constexpr (std::is_same_v<T, CTxIn>) { + } else { + static_assert(std::is_same_v<T, CTxIn>); ret.emplace(instantsend::GenInputLockRequestId(in.prevout)); - } else { - assert(false); } } return ret; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/net_instantsend.cpp` around lines 222 - 241, The unreachable else branch in the template function GetIdsFromLockable (anonymous namespace) should be removed: since the requires clause already constrains T to CTxIn or COutPoint, delete the final "else { assert(false); }" block and keep only the two constexpr branches that call instantsend::GenInputLockRequestId (one for COutPoint and one for CTxIn.prevout); this removes dead code without changing behavior.src/instantsend/instantsend.cpp (2)
273-277:IsKnownTxname is misleading — it returnstruewhen the islock is absent from the no-tx pending map.The method returns
truewhen the hash is not found inpendingNoTxInstantSendLocks, meaning "we already have the transaction for this islock." While the logic is correct in context (used inResolveBlockConflictsto decide whether to activate best chain), the name reads as the opposite of what the lookup does. A name likeHasTransactionForLockorIsNotAwaitingTxwould be clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/instantsend.cpp` around lines 273 - 277, The method CInstantSendManager::IsKnownTx is misleading because it returns true when islockHash is NOT present in pendingNoTxInstantSendLocks; rename the method to something like HasTransactionForLock or IsNotAwaitingTx and update all call sites (e.g., usages in ResolveBlockConflicts) to the new name, preserving the current boolean logic (return pendingNoTxInstantSendLocks.find(islockHash) == pendingNoTxInstantSendLocks.end()); alternatively, if you prefer to keep the name, invert the return and adjust callers to match the new semantics—ensure consistency between the method name and its boolean meaning across the codebase.
126-144: Linear scan inAttachISLockToTxcould be replaced with a reverse index.
AttachISLockToTxiterates the entirependingNoTxInstantSendLocksmap to find a match bytxid. If this map grows large, this becomes O(n). Consider maintaining a secondarytxid → islockHashindex for O(1) lookups. Not urgent for this PR, but worth noting for future optimization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/instantsend.cpp` around lines 126 - 144, AttachISLockToTx currently does an O(n) linear scan over pendingNoTxInstantSendLocks to find an islock by tx->GetHash(); add a secondary unordered_map/index (e.g., pendingNoTxIndex mapping txid -> islockHash/key) and maintain it wherever entries are inserted or erased so AttachISLockToTx can do an O(1) lookup: find the key via pendingNoTxIndex[txid], move the entry from pendingNoTxInstantSendLocks to pendingInstantSendLocks (as currently done), erase from pendingNoTxInstantSendLocks and pendingNoTxIndex, and return the islock; ensure consistency on all code paths that add/remove from pendingNoTxInstantSendLocks so the new index stays in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/instantsend/instantsend.cpp`:
- Around line 456-465: Fix the typos in the comment block around the cached tip
height check in instantsend.cpp: change "throught" to "through" and remove the
duplicate "is" so "tip is is not set" becomes "tip is not set"; this comment
appears above the LOCK(cs_height_cache) check that reads and returns
m_cached_tip_height.
- Around line 384-399: The log in CInstantSendManager::TransactionIsRemoved uses
the wrong class name string ("NetInstantSend::%s"); update the LogPrint call
(BCLog::INSTANTSEND) so the formatted prefix reflects this class (e.g.,
"CInstantSendManager::%s -- ...") to avoid misleading log output when
TransactionIsRemoved is invoked; locate the LogPrint invocation and change only
the class-name literal portion of the format string.
In `@src/instantsend/net_instantsend.cpp`:
- Around line 480-484: The LogPrintf call in net_instantsend.cpp uses the wrong
format specifier: replace the `%d` used for hash.ToString() with `%s` and pass a
C-string (e.g., hash.ToString().c_str()) so the LogPrintf invocation in the
block that builds toDelete and logs the mempool conflict (the LogPrintf inside
the same scope where toDelete.emplace and it->second->GetHash() are referenced)
prints the string correctly.
In `@src/instantsend/signing.cpp`:
- Around line 207-222: The code currently continues with blockHeight==0 when
hashBlock is unset which lets age calculations overestimate and allow locks;
update the logic so you guard hashBlock.IsNull() before attempting to determine
parent age: if hashBlock.IsNull() skip the
LookupBlockIndex/GetCachedHeight/CacheBlockHeight path and treat the parent as
unmined (e.g. return false or otherwise prevent locking), or ensure subsequent
age checks only run when blockHeight > 0; change the block that uses
m_isman.GetCachedHeight, WITH_LOCK(... LookupBlockIndex(hashBlock) ...),
m_isman.CacheBlockHeight and pindex->nHeight to be conditional on
!hashBlock.IsNull() and verify callers check blockHeight validity before
computing age.
---
Nitpick comments:
In `@src/instantsend/instantsend.cpp`:
- Around line 273-277: The method CInstantSendManager::IsKnownTx is misleading
because it returns true when islockHash is NOT present in
pendingNoTxInstantSendLocks; rename the method to something like
HasTransactionForLock or IsNotAwaitingTx and update all call sites (e.g., usages
in ResolveBlockConflicts) to the new name, preserving the current boolean logic
(return pendingNoTxInstantSendLocks.find(islockHash) ==
pendingNoTxInstantSendLocks.end()); alternatively, if you prefer to keep the
name, invert the return and adjust callers to match the new semantics—ensure
consistency between the method name and its boolean meaning across the codebase.
- Around line 126-144: AttachISLockToTx currently does an O(n) linear scan over
pendingNoTxInstantSendLocks to find an islock by tx->GetHash(); add a secondary
unordered_map/index (e.g., pendingNoTxIndex mapping txid -> islockHash/key) and
maintain it wherever entries are inserted or erased so AttachISLockToTx can do
an O(1) lookup: find the key via pendingNoTxIndex[txid], move the entry from
pendingNoTxInstantSendLocks to pendingInstantSendLocks (as currently done),
erase from pendingNoTxInstantSendLocks and pendingNoTxIndex, and return the
islock; ensure consistency on all code paths that add/remove from
pendingNoTxInstantSendLocks so the new index stays in sync.
In `@src/instantsend/instantsend.h`:
- Line 30: The file-level typedef duplicates the canonical CTransactionRef;
remove the local typedef in instantsend.h and include the canonical header
instead: add an `#include` of primitives/transaction.h and delete the line
"typedef std::shared_ptr<const CTransaction> CTransactionRef;" so the file uses
the single source-of-truth CTransactionRef definition (referenced here as
CTransactionRef in instantsend.h).
In `@src/instantsend/net_instantsend.cpp`:
- Around line 74-88: ResolveCycleHeight duplicates the fallback logic already
implemented in GetBlockHeight: call GetBlockHeight(m_is_manager, m_chainstate,
cycle_hash) and return its result directly instead of performing a second
WITH_LOCK + m_chainstate.m_blockman.LookupBlockIndex and manual caching; remove
the extra LookupBlockIndex and m_is_manager.CacheBlockHeight usage from
ResolveCycleHeight so it simply returns the std::optional<int> produced by
GetBlockHeight.
- Around line 25-31: The forward declaration of node::GetTransaction
(CTransactionRef GetTransaction(const CBlockIndex* const, const CTxMemPool*,
const uint256&, const Consensus::Params&, uint256&)) is fragile; update the file
to either include the canonical declaration from node/transaction.h or add a
compile-time check to ensure the signature matches: add a static_assert that
compares decltype(&node::GetTransaction) to the expected function pointer type
(or otherwise references the exact signature), and/or add a clear comment
pointing to node/transaction.h as the authoritative declaration to prevent
silent divergence.
- Around line 353-415: ProcessInstantSendLock performs multiple block-index
lookups: after GetTransaction and GetBlockHeight, code calls LookupBlockIndex
again (in the block handling found_transaction) causing a redundant third
lookup; consolidate by performing a single LookupBlockIndex once when the
transaction is found and minedHeight is empty (capture result in a local
pindexMined), call m_is_manager.CacheBlockHeight(pindexMined) and set
minedHeight from pindexMined->nHeight, then use that minedHeight for the
subsequent m_chainlocks.HasChainLock(hash, hashBlock) check and for
WriteNewISLock; update the block that currently does LookupBlockIndex (the code
around lines using WITH_LOCK(::cs_main) and pindexMined) to reuse this single
lookup and remove the duplicate lookup path so LookupBlockIndex is only called
once per code path.
- Line 591: The variable isLockedTxKnown is misleading because
m_is_manager.IsKnownTx(islockHash) returns true when the IS lock is NOT in
pendingNoTxInstantSendLocks (i.e., we already have the transaction); rename or
invert the boolean to match semantics: either rename isLockedTxKnown to
isLockedTxMissing or isLockedTxPresent (choose a name that matches the actual
meaning), or assign with a negation (e.g., bool isLockedTxMissing =
!m_is_manager.IsKnownTx(islockHash)) and update all subsequent uses accordingly
(references around net_instantsend.cpp where isLockedTxKnown is checked). Ensure
consistency with the comment in instantsend.cpp about IsKnownTx semantics.
- Around line 222-241: The unreachable else branch in the template function
GetIdsFromLockable (anonymous namespace) should be removed: since the requires
clause already constrains T to CTxIn or COutPoint, delete the final "else {
assert(false); }" block and keep only the two constexpr branches that call
instantsend::GenInputLockRequestId (one for COutPoint and one for
CTxIn.prevout); this removes dead code without changing behavior.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/active/context.cppsrc/active/context.hsrc/dsnotificationinterface.cppsrc/dsnotificationinterface.hsrc/init.cppsrc/instantsend/instantsend.cppsrc/instantsend/instantsend.hsrc/instantsend/net_instantsend.cppsrc/instantsend/net_instantsend.hsrc/instantsend/signing.cppsrc/instantsend/signing.hsrc/llmq/context.cppsrc/llmq/context.hsrc/net_processing.cppsrc/node/chainstate.cpptest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (1)
- src/active/context.cpp
Comment on lines
456
to
465
| { | ||
| { | ||
| LOCK(cs_height_cache); | ||
| if (m_cached_tip_height >= 0) { | ||
| return m_cached_tip_height; | ||
| } | ||
| // It returns the cached tip height which is updated throught notification mechanism | ||
| // If cached tip is is not set by any reason, it's okay to return 0 because | ||
| // chainstate is not fully loaded yet and tip is not set | ||
| LOCK(cs_height_cache); | ||
| if (m_cached_tip_height >= 0) { | ||
| return m_cached_tip_height; | ||
| } | ||
|
|
||
| const CBlockIndex* tip = WITH_LOCK(::cs_main, return m_chainstate.m_chain.Tip()); | ||
|
|
||
| CacheTipHeight(tip); | ||
| return tip ? tip->nHeight : -1; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Typos in comments.
Line 457: "throught" → "through"; Line 458: "tip is is" → "tip is".
Proposed fix
- // It returns the cached tip height which is updated throught notification mechanism
- // If cached tip is is not set by any reason, it's okay to return 0 because
+ // It returns the cached tip height which is updated through notification mechanism
+ // If cached tip is not set for any reason, it's okay to return 0 because🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/instantsend/instantsend.cpp` around lines 456 - 465, Fix the typos in the
comment block around the cached tip height check in instantsend.cpp: change
"throught" to "through" and remove the duplicate "is" so "tip is is not set"
becomes "tip is not set"; this comment appears above the LOCK(cs_height_cache)
check that reads and returns m_cached_tip_height.
Comment on lines
480
to
484
| toDelete.emplace(it->second->GetHash(), m_mempool.get(it->second->GetHash())); | ||
|
|
||
| LogPrintf("%s -- txid=%s, mempool TX %s with input %s conflicts with islock=%d\n", __func__, | ||
| islock.txid.ToString(), it->second->GetHash().ToString(), in.ToStringShort(), hash.ToString()); | ||
| } |
There was a problem hiding this comment.
Format specifier mismatch: %d used for a string argument.
Line 483 uses %d for hash.ToString() which produces a std::string. This should be %s.
Proposed fix
- LogPrintf("%s -- txid=%s, mempool TX %s with input %s conflicts with islock=%d\n", __func__,
+ LogPrintf("%s -- txid=%s, mempool TX %s with input %s conflicts with islock=%s\n", __func__,
islock.txid.ToString(), it->second->GetHash().ToString(), in.ToStringShort(), hash.ToString());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| toDelete.emplace(it->second->GetHash(), m_mempool.get(it->second->GetHash())); | |
| LogPrintf("%s -- txid=%s, mempool TX %s with input %s conflicts with islock=%d\n", __func__, | |
| islock.txid.ToString(), it->second->GetHash().ToString(), in.ToStringShort(), hash.ToString()); | |
| } | |
| toDelete.emplace(it->second->GetHash(), m_mempool.get(it->second->GetHash())); | |
| LogPrintf("%s -- txid=%s, mempool TX %s with input %s conflicts with islock=%s\n", __func__, | |
| islock.txid.ToString(), it->second->GetHash().ToString(), in.ToStringShort(), hash.ToString()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/instantsend/net_instantsend.cpp` around lines 480 - 484, The LogPrintf
call in net_instantsend.cpp uses the wrong format specifier: replace the `%d`
used for hash.ToString() with `%s` and pass a C-string (e.g.,
hash.ToString().c_str()) so the LogPrintf invocation in the block that builds
toDelete and logs the mempool conflict (the LogPrintf inside the same scope
where toDelete.emplace and it->second->GetHash() are referenced) prints the
string correctly.
Comment on lines
+207
to
222
| int blockHeight{0}; | ||
| if (!hashBlock.IsNull()) { | ||
| auto ret = m_isman.GetCachedHeight(hashBlock); | ||
| if (ret) blockHeight = *ret; | ||
|
|
||
| const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock)); | ||
| if (pindex == nullptr) { | ||
| if (printDebug) { | ||
| LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: failed to determine mined height for parent TX %s\n", | ||
| __func__, txHash.ToString(), outpoint.hash.ToString()); | ||
| } | ||
| return false; | ||
| } | ||
| return false; | ||
| m_isman.CacheBlockHeight(pindex); | ||
| blockHeight = pindex->nHeight; | ||
| } |
There was a problem hiding this comment.
Guard null hashBlock before computing parent age.
If GetTransaction returns a tx with an unset hashBlock, blockHeight stays 0, and Line 234 can overestimate age and incorrectly allow locking.
💡 Proposed fix
- int blockHeight{0};
- if (!hashBlock.IsNull()) {
- auto ret = m_isman.GetCachedHeight(hashBlock);
- if (ret) blockHeight = *ret;
-
- const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock));
- if (pindex == nullptr) {
- if (printDebug) {
- LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: failed to determine mined height for parent TX %s\n",
- __func__, txHash.ToString(), outpoint.hash.ToString());
- }
- return false;
- }
- m_isman.CacheBlockHeight(pindex);
- blockHeight = pindex->nHeight;
- }
+ if (hashBlock.IsNull()) {
+ if (printDebug) {
+ LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: parent TX %s is not mined yet\n",
+ __func__, txHash.ToString(), outpoint.hash.ToString());
+ }
+ return false;
+ }
+
+ int blockHeight{0};
+ if (const auto cached_height = m_isman.GetCachedHeight(hashBlock)) {
+ blockHeight = *cached_height;
+ } else {
+ const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock));
+ if (pindex == nullptr) {
+ if (printDebug) {
+ LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: failed to determine mined height for parent TX %s\n",
+ __func__, txHash.ToString(), outpoint.hash.ToString());
+ }
+ return false;
+ }
+ m_isman.CacheBlockHeight(pindex);
+ blockHeight = pindex->nHeight;
+ }Also applies to: 234-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/instantsend/signing.cpp` around lines 207 - 222, The code currently
continues with blockHeight==0 when hashBlock is unset which lets age
calculations overestimate and allow locks; update the logic so you guard
hashBlock.IsNull() before attempting to determine parent age: if
hashBlock.IsNull() skip the LookupBlockIndex/GetCachedHeight/CacheBlockHeight
path and treat the parent as unmined (e.g. return false or otherwise prevent
locking), or ensure subsequent age checks only run when blockHeight > 0; change
the block that uses m_isman.GetCachedHeight, WITH_LOCK(...
LookupBlockIndex(hashBlock) ...), m_isman.CacheBlockHeight and pindex->nHeight
to be conditional on !hashBlock.IsNull() and verify callers check blockHeight
validity before computing age.
knst
force-pushed
the
kernel-zero-is
branch
2 times, most recently
from
044a98a to
a928d11
Compare
knst
changed the title
…ce instead of returning std::variant
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/instantsend/signing.cpp (1)
207-237:⚠️ Potential issue | 🟠 MajorGuard null
hashBlockbefore computing parent age and use cache result correctly.At Line 234,
nTxAgeis computed even ifhashBlockis null (withblockHeightleft at0), which can incorrectly allow locking. Also, Line 209-Line 222 still doesLookupBlockIndexeven after a cache hit.💡 Suggested fix
- int blockHeight{0}; - if (!hashBlock.IsNull()) { - auto ret = m_isman.GetCachedHeight(hashBlock); - if (ret) blockHeight = *ret; - - const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock)); - if (pindex == nullptr) { - if (printDebug) { - LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: failed to determine mined height for parent TX %s\n", - __func__, txHash.ToString(), outpoint.hash.ToString()); - } - return false; - } - m_isman.CacheBlockHeight(pindex); - blockHeight = pindex->nHeight; - } + if (hashBlock.IsNull()) { + if (printDebug) { + LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: parent TX %s is not mined yet\n", + __func__, txHash.ToString(), outpoint.hash.ToString()); + } + return false; + } + + int blockHeight{0}; + if (const auto cached_height = m_isman.GetCachedHeight(hashBlock)) { + blockHeight = *cached_height; + } else { + const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock)); + if (pindex == nullptr) { + if (printDebug) { + LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: failed to determine mined height for parent TX %s\n", + __func__, txHash.ToString(), outpoint.hash.ToString()); + } + return false; + } + m_isman.CacheBlockHeight(pindex); + blockHeight = pindex->nHeight; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/signing.cpp` around lines 207 - 237, Guard against hashBlock being null and honor cached height to avoid unnecessary LookupBlockIndex: call auto ret = m_isman.GetCachedHeight(hashBlock) only if !hashBlock.IsNull(), and if ret has a value set blockHeight = *ret and skip the LookupBlockIndex/CacheBlockHeight work; if hashBlock.IsNull() return false (or skip age checks) before computing nTxAge so blockHeight isn't treated as 0, and keep the subsequent check using m_chainlocks.HasChainLock(blockHeight, hashBlock) unchanged.
🧹 Nitpick comments (5)
src/instantsend/net_instantsend.h (1)
110-110: Consider in-class initialization for the nullable pointer.Per project conventions for header declarations, POD types should use in-class initialization. Since
m_signeris documented as nullable (non-null only for masternodes), consider initializing it explicitly.Proposed change
- instantsend::InstantSendSigner* m_signer; // non-null only for masternode + instantsend::InstantSendSigner* m_signer{nullptr}; // non-null only for masternodeBased on learnings: "In header files, prefer in-class initialization for POD types (e.g., bool m_flag = false; int m_value = 0)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/net_instantsend.h` at line 110, The member pointer m_signer is nullable but currently uninitialized in the header; initialize it in-class to a clear null value (e.g., = nullptr) to follow POD in-class initialization conventions and avoid indeterminate pointer state—update the declaration of instantsend::InstantSendSigner* m_signer to include the in-class initializer (nullptr) so code that checks m_signer can rely on a defined initial value.src/instantsend/instantsend.cpp (2)
293-297: Method name could be clearer given its semantics.
IsKnownTxtakes anislockHashparameter but returns whether the corresponding transaction is known. Consider renaming toIsTransactionKnownForLockor adding a brief doc comment to clarify that it returnstruewhen the ISLOCK's transaction has been seen (i.e., the ISLOCK is not in the "pending without transaction" map).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/instantsend.cpp` around lines 293 - 297, Rename or clarify CInstantSendManager::IsKnownTx to make its semantics explicit: either rename the method to IsTransactionKnownForLock (or IsTxKnownForIslock) and update all callers, or add a one-line doc comment above IsKnownTx stating that given an islockHash it returns true when the ISLOCK's transaction has been seen (i.e., the islockHash is NOT present in pendingNoTxInstantSendLocks). Ensure the implementation still locks cs_pendingLocks and uses pendingNoTxInstantSendLocks.find(islockHash) == pendingNoTxInstantSendLocks.end(), and update any unit tests or usages to call the new name if you rename the method.
146-164: Loop iteration can be simplified.The manual iterator loop can be replaced with
std::find_ifor a range-based approach for clarity. Also, once a match is found and erased, the method returns immediately, so the++itis unreachable after the erase.Proposed simplification
instantsend::InstantSendLockPtr CInstantSendManager::AttachISLockToTx(const CTransactionRef& tx) { - instantsend::InstantSendLockPtr ret_islock{nullptr}; LOCK(cs_pendingLocks); - auto it = pendingNoTxInstantSendLocks.begin(); - while (it != pendingNoTxInstantSendLocks.end()) { + for (auto it = pendingNoTxInstantSendLocks.begin(); it != pendingNoTxInstantSendLocks.end(); ++it) { if (it->second.islock->txid == tx->GetHash()) { - // we received an islock earlier, let's put it back into pending and verify/lock LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__, tx->GetHash().ToString(), it->first.ToString()); - ret_islock = it->second.islock; + auto ret_islock = it->second.islock; pendingInstantSendLocks.try_emplace(it->first, it->second); pendingNoTxInstantSendLocks.erase(it); return ret_islock; } - ++it; } - return ret_islock; // not found, nullptr + return nullptr; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/instantsend.cpp` around lines 146 - 164, The manual iterator loop in CInstantSendManager::AttachISLockToTx over pendingNoTxInstantSendLocks can be simplified and clarified by using std::find_if (or a range-based for to locate the matching entry) instead of manually advancing the iterator and performing an erase-then-return (which makes the ++it after erase unreachable); find the element where it->second.islock->txid == tx->GetHash(), then inside the cs_pendingLocks lock assign ret_islock = it->second.islock, move the entry into pendingInstantSendLocks (pendingInstantSendLocks.try_emplace(it->first, it->second)), erase the element from pendingNoTxInstantSendLocks, and return ret_islock.src/instantsend/net_instantsend.cpp (2)
600-624: Fatal assertions on block invalidation/activation failures are intentional but aggressive.The
assert(false)calls afterInvalidateBlockandActivateBestChainfailures will terminate the node. While the comments explain this is intentional for safety, consider whether logging a critical error and returning might be more appropriate for production resilience. However, given this is consensus-critical code handling ISLOCK conflicts, the fail-fast approach may be justified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/net_instantsend.cpp` around lines 600 - 624, The code currently uses assert(false) after failures from m_chainstate.InvalidateBlock(...) and m_chainstate.ActivateBestChain(...), which will hard-crash the node; replace each assert(false) with a non-fatal error path: LogPrintf a critical/error-level message including state.ToString() (preserve the existing message), then cleanly abort processing by returning from the enclosing function (or otherwise propagate the failure to the caller) so you do not continue in an unsafe state; ensure any locks (e.g., ::cs_main) are properly released and that activateBestChain/isLockedTxKnown logic is not executed after the failure. Include references to BlockValidationState state, pindex2, m_chainstate.InvalidateBlock, m_chainstate.ActivateBestChain, and the existing LogPrintf calls when making the change.
222-241: Unreachable branch in constexpr if.The
elsebranch withassert(false)at line 236 is unreachable because therequiresclause already constrainsTto be eitherCTxInorCOutPoint. The compiler will reject any other type at compile time.Proposed simplification
template <typename T> requires std::same_as<T, CTxIn> || std::same_as<T, COutPoint> Uint256HashSet GetIdsFromLockable(const std::vector<T>& vec) { Uint256HashSet ret{}; if (vec.empty()) return ret; ret.reserve(vec.size()); for (const auto& in : vec) { if constexpr (std::is_same_v<T, COutPoint>) { ret.emplace(instantsend::GenInputLockRequestId(in)); - } else if constexpr (std::is_same_v<T, CTxIn>) { - ret.emplace(instantsend::GenInputLockRequestId(in.prevout)); } else { - assert(false); + ret.emplace(instantsend::GenInputLockRequestId(in.prevout)); } } return ret; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/net_instantsend.cpp` around lines 222 - 241, The function template GetIdsFromLockable has an unreachable else branch (assert(false)) because the requires clause limits T to CTxIn or COutPoint; remove that unreachable branch and simplify the constexpr branching: keep the if constexpr (std::is_same_v<T, COutPoint>) { ret.emplace(instantsend::GenInputLockRequestId(in)); } and make the other branch a plain else that handles CTxIn (ret.emplace(instantsend::GenInputLockRequestId(in.prevout));), deleting the final assert; this keeps behavior the same while removing dead code in GetIdsFromLockable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/instantsend/signing.cpp`:
- Around line 207-237: Guard against hashBlock being null and honor cached
height to avoid unnecessary LookupBlockIndex: call auto ret =
m_isman.GetCachedHeight(hashBlock) only if !hashBlock.IsNull(), and if ret has a
value set blockHeight = *ret and skip the LookupBlockIndex/CacheBlockHeight
work; if hashBlock.IsNull() return false (or skip age checks) before computing
nTxAge so blockHeight isn't treated as 0, and keep the subsequent check using
m_chainlocks.HasChainLock(blockHeight, hashBlock) unchanged.
---
Nitpick comments:
In `@src/instantsend/instantsend.cpp`:
- Around line 293-297: Rename or clarify CInstantSendManager::IsKnownTx to make
its semantics explicit: either rename the method to IsTransactionKnownForLock
(or IsTxKnownForIslock) and update all callers, or add a one-line doc comment
above IsKnownTx stating that given an islockHash it returns true when the
ISLOCK's transaction has been seen (i.e., the islockHash is NOT present in
pendingNoTxInstantSendLocks). Ensure the implementation still locks
cs_pendingLocks and uses pendingNoTxInstantSendLocks.find(islockHash) ==
pendingNoTxInstantSendLocks.end(), and update any unit tests or usages to call
the new name if you rename the method.
- Around line 146-164: The manual iterator loop in
CInstantSendManager::AttachISLockToTx over pendingNoTxInstantSendLocks can be
simplified and clarified by using std::find_if (or a range-based for to locate
the matching entry) instead of manually advancing the iterator and performing an
erase-then-return (which makes the ++it after erase unreachable); find the
element where it->second.islock->txid == tx->GetHash(), then inside the
cs_pendingLocks lock assign ret_islock = it->second.islock, move the entry into
pendingInstantSendLocks (pendingInstantSendLocks.try_emplace(it->first,
it->second)), erase the element from pendingNoTxInstantSendLocks, and return
ret_islock.
In `@src/instantsend/net_instantsend.cpp`:
- Around line 600-624: The code currently uses assert(false) after failures from
m_chainstate.InvalidateBlock(...) and m_chainstate.ActivateBestChain(...), which
will hard-crash the node; replace each assert(false) with a non-fatal error
path: LogPrintf a critical/error-level message including state.ToString()
(preserve the existing message), then cleanly abort processing by returning from
the enclosing function (or otherwise propagate the failure to the caller) so you
do not continue in an unsafe state; ensure any locks (e.g., ::cs_main) are
properly released and that activateBestChain/isLockedTxKnown logic is not
executed after the failure. Include references to BlockValidationState state,
pindex2, m_chainstate.InvalidateBlock, m_chainstate.ActivateBestChain, and the
existing LogPrintf calls when making the change.
- Around line 222-241: The function template GetIdsFromLockable has an
unreachable else branch (assert(false)) because the requires clause limits T to
CTxIn or COutPoint; remove that unreachable branch and simplify the constexpr
branching: keep the if constexpr (std::is_same_v<T, COutPoint>) {
ret.emplace(instantsend::GenInputLockRequestId(in)); } and make the other branch
a plain else that handles CTxIn
(ret.emplace(instantsend::GenInputLockRequestId(in.prevout));), deleting the
final assert; this keeps behavior the same while removing dead code in
GetIdsFromLockable.
In `@src/instantsend/net_instantsend.h`:
- Line 110: The member pointer m_signer is nullable but currently uninitialized
in the header; initialize it in-class to a clear null value (e.g., = nullptr) to
follow POD in-class initialization conventions and avoid indeterminate pointer
state—update the declaration of instantsend::InstantSendSigner* m_signer to
include the in-class initializer (nullptr) so code that checks m_signer can rely
on a defined initial value.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/active/context.cppsrc/active/context.hsrc/dsnotificationinterface.cppsrc/dsnotificationinterface.hsrc/init.cppsrc/instantsend/instantsend.cppsrc/instantsend/instantsend.hsrc/instantsend/net_instantsend.cppsrc/instantsend/net_instantsend.hsrc/instantsend/signing.cppsrc/instantsend/signing.hsrc/llmq/context.cppsrc/llmq/context.hsrc/net_processing.cppsrc/node/chainstate.cpptest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (1)
- src/active/context.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/net_processing.cpp
- test/lint/lint-circular-dependencies.py
knst
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Basically, CInstantSendManager's responsibility is remembering all known instant-send locks (by using
CInstantSendDb) and it is used all over codebase to answer a question a basic but important question "Is transaction X locked?".In details, there are 4 public interfaces that are used to answer that question:
IsLockedIsKnown,AlreadyHaveGetInstantSendLockByTxid,GetInstantSendLockByHash).In the reality, current implementation of CInstantSendManager has knowledges about multiple systems and components that are irrelevant, but not really needed to provide this base functionality.
What was done?
Removed dependency of CInstantSendManager on
instantsend::InstantSendSigner,llmq::CSigningManager,CChainState,chainlock::Chainlocks,CTxMemPool.Removing dependency on CMasternodeSync is excluded from this PR because it requires bitcoin#25073 to be done first.
How Has This Been Tested?
Run unit & functional tests, linters.
Breaking Changes
N/A
Checklist: