feat: instantsend cache heights and reduce cs_main contention #6953
Conversation
|
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6953.ce5601c6. A new comment will be made when the image is pushed. |
WalkthroughAdds a thread-safe block-height cache to CInstantSendManager (m_cached_block_heights, m_cached_tip_height) and exposes APIs: CacheBlockHeightInternal, CacheBlockHeight, GetBlockHeight, CacheTipHeight, and GetTipHeight. InstantSend flows (P2P message handling, ProcessInstantSendLock, ResolveBlockConflicts, RemoveConflictingLock, signing, and net handling) now consult and update the height cache, use std::optional for missing heights, and persist minedHeight when available. BlockConnected, BlockDisconnected, and UpdatedBlockTip keep the cache and cached tip height synchronized. Several methods gained cs_height_cache locking annotations and adjusted signatures to enforce cache locking discipline. ChainLock checks and islock drop logic now use cached heights when available. Sequence Diagram(s)sequenceDiagram
autonumber
participant P2P as P2P
participant ISM as InstantSendManager
participant Cache as HeightCache
participant Chain as ChainState
participant Mempool as Mempool
P2P->>ISM: Receive ISLOCK (ProcessMessage)
ISM->>Cache: GetBlockHeight(cycleHash)
alt cache hit
Cache-->>ISM: cycleHeight (optional<int>)
else cache miss
ISM->>Chain: LookupBlockIndex(cycleHash)
Chain-->>ISM: CBlockIndex or null
ISM->>Cache: CacheBlockHeight(CBlockIndex) if found
end
ISM->>ISM: validate cycleHeight vs dkgInterval (may bail on missing)
ISM->>ISM: ProcessInstantSendLock(...)
ISM->>Mempool: Find TX / check inputs
alt TX mined
ISM->>Cache: GetBlockHeight(hashBlock)
alt cache hit
Cache-->>ISM: minedHeight
else cache miss
ISM->>Chain: LookupBlockIndex(hashBlock)
Chain-->>ISM: CBlockIndex or null
ISM->>Cache: CacheBlockHeight(CBlockIndex) if found
end
ISM->>ISM: WriteInstantSendLockMined(minedHeight) when available
ISM->>ISM: Drop ISLOCK uses minedHeight and HasChainLock(hashBlock, minedHeight)
else TX unmined
ISM->>ISM: mark pending / mempool flow
end
sequenceDiagram
autonumber
participant Node as Node
participant ISM as InstantSendManager
participant Cache as HeightCache
participant Chain as ChainState
Node->>ISM: BlockConnected(pindex)
ISM->>Cache: CacheBlockHeightInternal(pindex)
ISM->>Cache: CacheTipHeight(pindex) -> update m_cached_tip_height
Node->>ISM: BlockDisconnected(pindex)
ISM->>Cache: erase cached height for pindex->GetBlockHash()
ISM->>Chain: lookup pindex->pprev (new tip)
alt new tip found
ISM->>Cache: CacheTipHeight(pprev) -> update m_cached_tip_height
else no tip
ISM-->>Cache: set m_cached_tip_height = -1
end
sequenceDiagram
autonumber
participant Signer as InstantSendSigner
participant ISM as InstantSendManager
participant Cache as HeightCache
Signer->>ISM: Request validation (signing path)
ISM->>Cache: GetBlockHeight(hashBlock)
ISM->>Cache: GetTipHeight()
alt heights available
Cache-->>ISM: blockHeight, tipHeight
ISM->>Signer: compute nTxAge = tipHeight - blockHeight + 1
else missing
ISM-->>Signer: bail/log debug (missing height)
end
sequenceDiagram
autonumber
participant ISM as InstantSendManager
participant Cache as HeightCache
participant Chain as ChainState
ISM->>Cache: GetTipHeight()
ISM->>ISM: RemoveConflictingLock uses GetTipHeight() to decide conflicts
alt tip known
ISM->>ISM: resolve conflicts using tipHeight
else unknown
ISM->>Chain: fallback behavior (previous chain query)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6953.ce5601c6. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6953.8fc12bc1. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6953.8fc12bc1. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6953.8a5b845a. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6953.8a5b845a. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6953.8bdb71d4. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6953.8bdb71d4. The image should be on dockerhub soon. |
| Uint256HashMap<int64_t> timingsTxSeen GUARDED_BY(cs_timingsTxSeen); | ||
|
|
||
| mutable Mutex cs_height_cache; | ||
| static constexpr size_t MAX_BLOCK_HEIGHT_CACHE{16384}; |
There was a problem hiding this comment.
isn't 16k a bit too much? rotation quorums are not alive that much; also it's very unlikely situation that tx will exist that long before it will be mined.
16k is roughly 28 days
|
Deployed analysis during load on testnet. Note; that these 2 nodes can't be compared 1:1, they may be slightly differently connected, but we see significant improvements. 🔍 Overall Contention Summary
Interpretation: ⚙️ Top Locks by Total Contention Time1. Major Improvements
2. New or Increased Contention
📈 Top Locks by Contention Count
Observations:
🔄 Structural & Behavioral Shifts✅ Improved
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/instantsend/instantsend.h (1)
97-97: Reconsider the cache size constant.The MAX_BLOCK_HEIGHT_CACHE constant is set to 16384, which represents approximately 28 days of blocks (at ~2.5 minute block time). As noted in previous review feedback, this may be excessive for the typical lifetime of transactions in the InstantSend flow. Consider reducing this to a more reasonable value based on typical InstantSend lock processing times and memory constraints.
🧹 Nitpick comments (2)
src/instantsend/instantsend.cpp (2)
131-141: Consider minor optimization in cache update.When the block index is found via fallback lookup (line 133-138), you're calling
blockIndex->GetBlockHash()again on line 139. Sinceislock->cycleHashshould already match the block hash, you could useislock->cycleHashdirectly for caching.Apply this diff:
- CacheBlockHeight(blockIndex->GetBlockHash(), blockIndex->nHeight); + CacheBlockHeight(islock->cycleHash, blockIndex->nHeight);
391-399: Simplify fallback logic by relying on GetBlockHeight.The fallback pattern here (lines 393-399) duplicates logic already present in
GetBlockHeight(lines 977-995). SinceGetBlockHeightalready performs the lookup and caching internally, the explicit fallback here may be redundant. The current code callsGetBlockHeightonce (line 391), then does manual lookup and caching if it returns nullopt (lines 393-399).However,
GetBlockHeightalready does this internally. The only scenario where this explicit fallback helps is if you need to ensure caching happens even when the block isn't initially found. ButGetBlockHeightalready caches on successful lookup.Consider whether this duplication is necessary or if a single
GetBlockHeightcall suffices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/instantsend/instantsend.cpp(9 hunks)src/instantsend/instantsend.h(6 hunks)src/instantsend/signing.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/instantsend/signing.cpp
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash PR: 6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/instantsend/instantsend.h
🧬 Code graph analysis (2)
src/instantsend/instantsend.cpp (1)
src/node/interfaces.cpp (23)
hash(893-910)hash(893-893)hash(932-936)hash(932-932)hash(958-963)hash(958-958)LOCK(581-585)LOCK(591-598)LOCK(599-606)LOCK(868-877)LOCK(911-915)LOCK(1095-1099)height(563-573)height(563-563)height(878-885)height(878-878)height(886-892)height(886-886)height(937-941)height(937-937)tip(97-105)tip(586-590)tip(607-615)
src/instantsend/instantsend.h (2)
src/saltedhasher.h (1)
uint256(38-44)src/instantsend/instantsend.cpp (14)
CacheBlockHeightInternal(965-969)CacheBlockHeightInternal(965-965)ProcessPendingInstantSendLocks(183-246)ProcessPendingInstantSendLocks(183-183)ProcessPendingInstantSendLocks(248-360)ProcessPendingInstantSendLocks(248-251)ProcessInstantSendLock(362-449)ProcessInstantSendLock(362-363)CacheBlockHeight(971-975)CacheBlockHeight(971-971)GetBlockHeight(977-995)GetBlockHeight(977-977)GetTipHeight(997-1015)GetTipHeight(997-997)
🪛 GitHub Actions: Clang Diff Format Check
src/instantsend/instantsend.cpp
[error] 393-395: Clang format differences found. Apply clang-format changes to src/instantsend/instantsend.cpp. Exit code 1 due to unformatted code.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (10)
src/instantsend/instantsend.cpp (9)
25-25: LGTM!Adding the cassert include is appropriate for the assert statements used in the implementation.
280-292: LGTM!The height cache integration correctly uses
GetBlockHeightto fetch the cycle height andGetTipHeightto determine the appropriate signing height for quorum selection. The thread-safe cache access reduces cs_main contention as intended.
509-513: LGTM!The block height caching during connection correctly maintains the cache and updates the tip height atomically under the lock. This ensures the cache stays synchronized with chain state.
542-550: LGTM with minor observation.The cache invalidation logic correctly removes the disconnected block and updates the tip. The code handles the case where there's no previous block (genesis disconnect) by setting
m_cached_tip_heightto -1.One minor note: when caching
new_tip(line 548), we assumepprevis a valid block. This is safe becausepprevis either a valid ancestor or nullptr, and we guard the caching withif (new_tip).
673-677: LGTM!The tip height caching in
UpdatedBlockTipcorrectly maintains synchronization between the chain state and the height cache.
859-859: LGTM!Using
GetTipHeighthere reduces cs_main contention as intended by the PR objectives.
965-975: LGTM!The internal/external caching method pattern correctly separates lock acquisition from the locked operation, allowing efficient batch caching when the lock is already held.
977-995: LGTM!The
GetBlockHeightimplementation correctly:
- Returns
std::nulloptfor null/unknown hashes- Checks the cache first to avoid cs_main contention
- Falls back to block index lookup when necessary
- Caches the result before returning
This achieves the PR's goal of reducing cs_main contention in the InstantSend flow.
997-1015: Document assumption in GetTipHeight assertion.The assertion at line 1007 (
assert(tip != nullptr)) assumes the chain tip is always valid. While this is typically true after genesis, consider whether this is always guaranteed in all call contexts (e.g., during initialization or chain reorgs).If this assumption is architectural (i.e., GetTipHeight should never be called before the chain is initialized), the assert is appropriate but could benefit from a comment explaining the invariant. Otherwise, consider returning a sentinel value or throwing a more descriptive error.
Based on learnings about safe handling of potential nullptr returns from chain operations.
src/instantsend/instantsend.h (1)
98-99: StaticSaltedHasher is properly compatible with uint256.The
SaltedHasherImpl<uint256>specialization exists and usesSipHashUint256(k0, k1, v)for hashing uint256 values. StaticSaltedHasher's templatedoperator()delegates toSaltedHasherImpl<T>::CalcHash, which for uint256 keys provides proper cryptographic distribution. This pattern is used consistently throughout the codebase for LRU caches with uint256 keys (e.g., masternode/meta.h, llmq/signing.h), and there's even a dedicatedUint256LruHashMaptype alias for this use case.
| std::optional<int> minedHeight = GetBlockHeight(hashBlock); | ||
| if (found_transaction) { | ||
| if (!minedHeight.has_value()) { | ||
| const CBlockIndex* pindexMined = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock)); | ||
| if (pindexMined != nullptr) { | ||
| CacheBlockHeight(pindexMined->GetBlockHash(), pindexMined->nHeight); | ||
| minedHeight = pindexMined->nHeight; | ||
| } | ||
| } | ||
| // Let's see if the TX that was locked by this islock is already mined in a ChainLocked block. If yes, | ||
| // we can simply ignore the islock, as the ChainLock implies locking of all TXs in that chain | ||
| if (pindexMined != nullptr && clhandler.HasChainLock(pindexMined->nHeight, pindexMined->GetBlockHash())) { | ||
| LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txlock=%s, islock=%s: dropping islock as it already got a ChainLock in block %s, peer=%d\n", __func__, | ||
| islock->txid.ToString(), hash.ToString(), hashBlock.ToString(), from); | ||
| if (minedHeight.has_value() && clhandler.HasChainLock(*minedHeight, hashBlock)) { | ||
| LogPrint(BCLog::INSTANTSEND, /* Continued */ | ||
| "CInstantSendManager::%s -- txlock=%s, islock=%s: dropping islock as it already got a " /* Continued */ | ||
| "ChainLock in block %s, peer=%d\n", | ||
| __func__, islock->txid.ToString(), hash.ToString(), hashBlock.ToString(), from); | ||
| return {}; | ||
| } | ||
| } | ||
|
|
||
| if (found_transaction) { | ||
| db.WriteNewInstantSendLock(hash, islock); | ||
| if (pindexMined) { | ||
| db.WriteInstantSendLockMined(hash, pindexMined->nHeight); | ||
| if (minedHeight.has_value()) { | ||
| db.WriteInstantSendLockMined(hash, *minedHeight); | ||
| } |
There was a problem hiding this comment.
Fix clang-format issue.
The pipeline indicates clang-format differences at lines 393-395. Please run clang-format to resolve the formatting issue.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 393-395: Clang format differences found. Apply clang-format changes to src/instantsend/instantsend.cpp. Exit code 1 due to unformatted code.
🤖 Prompt for AI Agents
In src/instantsend/instantsend.cpp around lines 391 to 415, the submitted
changes trigger a clang-format mismatch specifically around lines 393-395;
reformat the file (or at least that region) with the project's clang-format
configuration to fix spacing/line breaks so the code matches style rules, then
stage and commit the formatted file. Ensure no semantic changes are
introduced—only whitespace/formatting adjustments.
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6953.13727fb3. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6953.13727fb3. The image should be on dockerhub soon. |
|
pls consider ffd84b6 |
|
@UdjinM6 applied |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/instantsend/instantsend.cpp (1)
393-406: Run clang-format to fix formatting violationsCI’s “Clang Diff Format Check” is still failing for this file around these lines. Please run the repository’s clang-format tooling (or clang-format-diff script mentioned in the pipeline output) on
src/instantsend/instantsend.cppand commit the resulting whitespace‑only changes so the pipeline goes green.
🧹 Nitpick comments (2)
src/instantsend/instantsend.cpp (2)
131-141: Avoid double LookupBlockIndex for cycleHash in ProcessMessageFunctionally this is fine: a missing
cycleHashstill leads to a small misbehaving score and a present block height gets cached and checked againstdkgInterval. However, whenGetBlockHeight(islock->cycleHash)returnsstd::nulloptfor a non‑null hash, it already impliesLookupBlockIndexreturnednullptr, so the explicit secondLookupBlockIndexcall in the fallback path is redundant work.You could simplify by treating a
nulloptresult directly as the “unknown cycleHash” case (and ban with score 1) or by adjustingGetBlockHeightto optionally return both height andCBlockIndex*so you don’t need to re‑lookup here.Also applies to: 147-149
954-965: Height-cache helper implementations and locking look correctThe LRU-backed helpers (
CacheBlockHeightInternal,CacheBlockHeight,GetBlockHeight,CacheTipHeight,GetTipHeight) only holdcs_height_cachearound cache operations and perform cs_main lookups (LookupBlockIndex,m_chainstate.m_chain.Tip()) entirely outside that scope, so there is no cs_main+cs_height_cache lock nesting.Using
StaticSaltedHasherfor theunordered_lru_cache<uint256, int, ...>key matches other salted-hash usage in the codebase, and them_cached_tip_height == -1sentinel plusCacheTipHeight(nullptr)branch are coherent. You might add a short comment documenting the-1sentinel form_cached_tip_heightto make the intent obvious to future readers.Based on learnings
Also applies to: 966-984, 986-995, 997-1010
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
-
src/instantsend/instantsend.cpp(9 hunks) -
src/instantsend/instantsend.h(6 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-02-14T15:15:58.165Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-07-09T15:05:36.250Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/rpc/evo.cpp:1273-1273
Timestamp: 2025-07-09T15:05:36.250Z
Learning: When clang-format suggestions significantly harm readability (like splitting logical parameter groups across multiple lines), it's acceptable to use `// clang-format off` and `// clang-format on` directives to exclude the problematic section from automatic formatting, prioritizing code readability over strict line length compliance.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash PR: 6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/instantsend/instantsend.cpp
🧬 Code graph analysis (2)
src/instantsend/instantsend.h (2)
src/saltedhasher.h (1)
uint256(38-44)src/instantsend/instantsend.cpp (16)
CacheBlockHeightInternal(954-958)CacheBlockHeightInternal(954-954)TransactionRemovedFromMempool(486-501)TransactionRemovedFromMempool(486-486)BlockDisconnected(535-546)BlockDisconnected(535-536)RemoveConflictingLock(844-855)RemoveConflictingLock(844-844)CacheBlockHeight(960-964)CacheBlockHeight(960-960)GetBlockHeight(966-984)GetBlockHeight(966-966)CacheTipHeight(986-995)CacheTipHeight(986-986)GetTipHeight(997-1010)GetTipHeight(997-997)
src/instantsend/instantsend.cpp (2)
src/rpc/rawtransaction.cpp (1)
WITH_LOCK(651-651)src/node/interfaces.cpp (15)
hash(893-910)hash(893-893)hash(932-936)hash(932-932)hash(958-963)hash(958-958)LOCK(581-585)LOCK(591-598)LOCK(599-606)LOCK(868-877)LOCK(911-915)LOCK(1095-1099)tip(97-105)tip(586-590)tip(607-615)
🪛 GitHub Actions: Clang Diff Format Check
src/instantsend/instantsend.cpp
[error] 393-394: Clang format differences found. Run 'clang-format' (or the repository's clang-format-diff script) to reformat this file. Command failed: git diff -U0 origin/develop --
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
🔇 Additional comments (8)
src/instantsend/instantsend.cpp (4)
25-26: Assert header inclusion matches usageAdding
<cassert>aligns with theassert(false)inGetIdsFromLockableand avoids relying on indirect includes. No issues here.
280-283: Cycle/tip height usage in batch verify looks correctUsing
GetBlockHeight(islock->cycleHash)plusGetTipHeight()to derivenSignHeightkeeps cs_main access encapsulated and preserves existing behaviour (tip height falls back to -1 before the first tip, matchingCChain::Height()semantics). The handling of missingcycleHashas a bad source without crashing is also appropriate.Also applies to: 288-292
391-399: Unified minedHeight handling and chainlock check in ProcessInstantSendLockThe new
minedHeightflow computes the height once viaGetBlockHeight(hashBlock)with a cs_main fallback and then reuses it both forHasChainLock(*minedHeight, hashBlock)and forWriteInstantSendLockMined. This addresses the earlier duplication issue and keeps the single source of truth for the mined height.The early
GetBlockHeight(hashBlock)is safe even whenhashBlockis null (it returnsstd::nullopt) and you correctly guard all subsequent uses withhas_value(). Overall the change looks sound.Also applies to: 402-407, 413-414
509-510: Tip-height cache maintenance in block/tip handlers looks consistentCalling
CacheTipHeight(pindex)inBlockConnected, erasing the disconnected block and thenCacheTipHeight(pindexDisconnected->pprev)inBlockDisconnected, andCacheTipHeight(pindexNew)inUpdatedBlockTip, together keepm_cached_tip_heightand the block-height cache aligned with the active chain.RemoveConflictingLockthen correctly usesGetTipHeight()instead of touchingm_chainstatedirectly, avoiding additional cs_main contention.All these call sites only touch
cs_height_cachelocally and don’t hold it while calling out to other subsystems, so I don’t see new lock‑order risks here.Also applies to: 538-543, 666-667, 848-849
src/instantsend/instantsend.h (4)
21-25: New includes align with added functionalityIncluding
<optional>and<saltedhasher.h>matches the newstd::optional<int>return type andStaticSaltedHashertemplate parameter usage. No concerns here.
96-103: Height-cache state and locking are well-scopedDefining a dedicated
cs_height_cache, the boundedMAX_BLOCK_HEIGHT_CACHE, andm_cached_block_heightsplusm_cached_tip_heightunder that mutex cleanly isolates the height cache from the other InstantSend manager state. The privateCacheBlockHeightInternalhelper restricted toEXCLUSIVE_LOCKS_REQUIRED(cs_height_cache)matches the cpp usage.
127-136: Thread-safety annotations for height-cache usage are consistentMarking the main processing methods (
ProcessPendingInstantSendLocks,ProcessInstantSendLock,ResolveBlockConflicts,ProcessMessage,TransactionRemovedFromMempool,BlockConnected,BlockDisconnected,UpdatedBlockTip,RemoveConflictingLock) asEXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache)and the cache helpers (CacheBlockHeight,GetBlockHeight,CacheTipHeight,GetTipHeight) asEXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache)matches the implementation pattern: none of the external call sites enter withcs_height_cacheheld, and the helpers themselves acquire it internally.This keeps the new cache lock integrated into the existing LLMQ/InstantSend locking scheme managed via
LLMQContextwithout introducing obvious lock-order conflicts.Based on learnings
Also applies to: 148-149, 161-163, 164-167, 168-170, 180-183, 189-192
189-192: New height-cache API on InstantSendSignerParent looks goodExposing
CacheBlockHeight,GetBlockHeight(const uint256&) -> std::optional<int>,CacheTipHeight, andGetTipHeight() -> inton the manager (and overriding the parent interface) is a clean way to centralize height lookups and caching. The optional return for unknown heights is already handled defensively at call sites and should help keep cs_main usage down.
| if (found_transaction && !hashBlock.IsNull()) { | ||
| pindexMined = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock)); | ||
|
|
||
| std::optional<int> minedHeight = GetBlockHeight(hashBlock); |
There was a problem hiding this comment.
nit: use snake_case for local variables for new code; it should be mined_height
doc/developer-nodes.md:
- **Symbol naming conventions**. These are preferred in new code, but are not
required when doing so would need changes to significant pieces of existing
code.
- Variable (including function arguments) and namespace names are all lowercase and may use `_` to
separate words (snake_case).
- Class member variables have a `m_` prefix.
- Global variables have a `g_` prefix.
- Constant names are all uppercase, and use `_` to separate words.
- Enumerator constants may be `snake_case`, `PascalCase` or `ALL_CAPS`.
This is a more tolerant policy than the [C++ Core
Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-caps),
which recommend using `snake_case`. Please use what seems appropriate.
- Class names, function names, and method names are UpperCamelCase
(PascalCase). Do not prefix class names with `C`. See [Internal interface
naming style](#internal-interface-naming-style) for an exception to this
convention.
src/instantsend/instantsend.cpp
Outdated
| islock->txid.ToString(), hash.ToString(), hashBlock.ToString(), from); | ||
| if (minedHeight.has_value() && clhandler.HasChainLock(*minedHeight, hashBlock)) { | ||
| LogPrint(BCLog::INSTANTSEND, /* Continued */ | ||
| "CInstantSendManager::%s -- txlock=%s, islock=%s: dropping islock as it already got a " /* Continued */ |
There was a problem hiding this comment.
nit: no need 2nd time continued; look like accidentally added after multiple rebase or clang-format
There was a problem hiding this comment.
if you give an exact diff; I can try it, I remember needing this to appease linter
There was a problem hiding this comment.
$ git diff
diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp
index 0145d73e01..68766bf0c7 100644
--- a/src/instantsend/instantsend.cpp
+++ b/src/instantsend/instantsend.cpp
@@ -160,7 +160,7 @@ std::variant<uint256, CTransactionRef, std::monostate> CInstantSendManager::Proc
// we can simply ignore the islock, as the ChainLock implies locking of all TXs in that chain
if (minedHeight.has_value() && clhandler.HasChainLock(*minedHeight, hashBlock)) {
LogPrint(BCLog::INSTANTSEND, /* Continued */
- "CInstantSendManager::%s -- txlock=%s, islock=%s: dropping islock as it already got a " /* Continued */
+ "CInstantSendManager::%s -- txlock=%s, islock=%s: dropping islock as it already got a "
"ChainLock in block %s, peer=%d\n",
__func__, islock->txid.ToString(), hash.ToString(), hashBlock.ToString(), from);
return std::monostate{};
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6953.99fac535. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6953.99fac535. The image should be on dockerhub soon. |
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
force-pushed
the
feat/no-cs-main-islock
branch
from
99fac53 to
617ed07
Compare
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6953.617ed07e. A new comment will be made when the image is pushed. |
PastaPastaPasta
force-pushed
the
feat/no-cs-main-islock
branch
from
617ed07 to
186c81d
Compare
|
Squashed down commits |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/instantsend/instantsend.cpp (1)
1-200: Fix clang-format diff reported by CIThe Clang Diff Format Check is failing for this file. Please run the project’s formatter (for example:
./contrib/devtools/clang-format-diff.py -p1) and commit the resulting changes so CI passes. Ensure no semantic changes are introduced—only formatting.
🧹 Nitpick comments (1)
src/instantsend/instantsend.cpp (1)
146-174: Avoid unnecessarycs_mainlocking for mempool txs inProcessInstantSendLock
minedHeightis initialized viaGetBlockHeight(hashBlock), which is cheap whenhashBlock.IsNull()(returns immediately, nocs_main). However, the fallback:if (found_transaction) { if (!minedHeight.has_value()) { const CBlockIndex* pindexMined = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock)); if (pindexMined != nullptr) { CacheBlockHeight(pindexMined); minedHeight = pindexMined->nHeight; } } if (minedHeight.has_value() && clhandler.HasChainLock(*minedHeight, hashBlock)) { ... } }runs for all
found_transactioncases, including the common case where the tx is only in the mempool andhashBlock.IsNull(). That means you still take::cs_mainon a hot path where no block height can exist, which undermines the contention reduction this PR is targeting.A small refactor gating the height lookup on
!hashBlock.IsNull()avoids taking::cs_mainfor mempool-only txs while keeping the fallback for the (rare) non-null hash cases:- // we ignore failure here as we must be able to propagate the lock even if we don't have the TX locally - std::optional<int> minedHeight = GetBlockHeight(hashBlock); - if (found_transaction) { - if (!minedHeight.has_value()) { - const CBlockIndex* pindexMined = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock)); - if (pindexMined != nullptr) { - CacheBlockHeight(pindexMined); - minedHeight = pindexMined->nHeight; - } - } + // we ignore failure here as we must be able to propagate the lock even if we don't have the TX locally + std::optional<int> minedHeight; + if (!hashBlock.IsNull()) { + minedHeight = GetBlockHeight(hashBlock); + if (found_transaction && !minedHeight.has_value()) { + const CBlockIndex* pindexMined = + WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock)); + if (pindexMined != nullptr) { + CacheBlockHeight(pindexMined); + minedHeight = pindexMined->nHeight; + } + } + } if (found_transaction) { // Let's see if the TX that was locked by this islock is already mined in a ChainLocked block. If yes, // we can simply ignore the islock, as the ChainLock implies locking of all TXs in that chain if (minedHeight.has_value() && clhandler.HasChainLock(*minedHeight, hashBlock)) { ... } }This keeps behavior the same for mined transactions while eliminating unnecessary
cs_maintraffic for mempool ones.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/instantsend/instantsend.cpp(6 hunks)src/instantsend/instantsend.h(5 hunks)src/instantsend/net_instantsend.cpp(2 hunks)src/instantsend/signing.cpp(1 hunks)src/instantsend/signing.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/instantsend/signing.cpp
- src/instantsend/net_instantsend.cpp
- src/instantsend/signing.h
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
📚 Learning: 2025-02-14T15:15:58.165Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-07-09T15:05:36.250Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/rpc/evo.cpp:1273-1273
Timestamp: 2025-07-09T15:05:36.250Z
Learning: When clang-format suggestions significantly harm readability (like splitting logical parameter groups across multiple lines), it's acceptable to use `// clang-format off` and `// clang-format on` directives to exclude the problematic section from automatic formatting, prioritizing code readability over strict line length compliance.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-01-14T09:06:19.717Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/instantsend/instantsend.h
🧬 Code graph analysis (2)
src/instantsend/instantsend.cpp (1)
src/node/interfaces.cpp (15)
hash(893-910)hash(893-893)hash(932-936)hash(932-932)hash(958-963)hash(958-958)LOCK(581-585)LOCK(591-598)LOCK(599-606)LOCK(868-877)LOCK(911-915)LOCK(1095-1099)tip(97-105)tip(586-590)tip(607-615)
src/instantsend/instantsend.h (2)
src/saltedhasher.h (1)
uint256(38-44)src/instantsend/instantsend.cpp (24)
CacheBlockHeightInternal(724-728)CacheBlockHeightInternal(724-724)TransactionAddedToMempool(203-236)TransactionAddedToMempool(203-203)TransactionRemovedFromMempool(238-253)TransactionRemovedFromMempool(238-238)BlockConnected(255-285)BlockConnected(255-255)BlockDisconnected(287-298)BlockDisconnected(287-288)RemoveConflictingLock(614-625)RemoveConflictingLock(614-614)TryEmplacePendingLock(419-427)TryEmplacePendingLock(419-420)GetInstantSendLockCount(719-722)GetInstantSendLockCount(719-719)CacheBlockHeight(730-734)CacheBlockHeight(730-730)GetBlockHeight(736-754)GetBlockHeight(736-736)CacheTipHeight(756-765)CacheTipHeight(756-756)GetTipHeight(767-780)GetTipHeight(767-767)
🪛 GitHub Actions: Clang Diff Format Check
src/instantsend/instantsend.cpp
[error] 150-150: Clang format differences found. Run './contrib/devtools/clang-format-diff.py -p1' to fix code style issues in this file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (3)
src/instantsend/instantsend.h (1)
20-26: Height cache state + locking annotations look consistentThe new
cs_height_cachemutex, LRU cache members, tip-height field, and theCache*/Get*helpers form a self-contained height cache. TheEXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache)annotations on callers that invokeGetBlockHeight/GetTipHeightare appropriate to avoid re-entering the non-recursiveMutexand should help keep lock ordering sane while reducing directcs_mainusage. I don’t see correctness issues here.Also applies to: 96-103, 135-136, 155-156, 159-163, 173-176, 182-185
src/instantsend/instantsend.cpp (2)
255-297: Tip-height cache wiring in block callbacks looks correctUsing
CacheTipHeight(pindex)inBlockConnected, erasing the disconnected block and thenCacheTipHeight(pindexDisconnected->pprev)inBlockDisconnected, and callingCacheTipHeight(pindexNew)at the start ofUpdatedBlockTip, plus consumingGetTipHeight()inRemoveConflictingLock, gives a coherent picture: the height cache is kept up to date on connects, disconnects, and tip updates, and consumers no longer need to touchcs_maindirectly. The calls are idempotent and don’t holdcs_height_cacheacross any other heavy work. This looks good.Also applies to: 434-451, 614-621
724-780: Height cache helper implementations and lock ordering look safe
CacheBlockHeightInternal,CacheBlockHeight,GetBlockHeight,CacheTipHeight, andGetTipHeight:
- Always guard
m_cached_block_heights/m_cached_tip_heightwithcs_height_cache.- Use short inner scopes so
cs_height_cacheis released before anyWITH_LOCK(::cs_main, ...)call.- Reacquire
cs_height_cacheonly after::cs_mainhas been fully released (viaCacheBlockHeight/CacheTipHeight), avoiding overlapping locks and deadlock risk.- Treat unknown block hashes and missing tips gracefully (
std::nullopt/-1).This matches the intent of the new
EXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache)annotations and aligns with the PR’s goal of reducingcs_maincontention.
src/instantsend/instantsend.cpp
Outdated
| islock->txid.ToString(), hash.ToString(), hashBlock.ToString(), from); | ||
| if (minedHeight.has_value() && clhandler.HasChainLock(*minedHeight, hashBlock)) { | ||
| LogPrint(BCLog::INSTANTSEND, /* Continued */ | ||
| "CInstantSendManager::%s -- txlock=%s, islock=%s: dropping islock as it already got a " /* Continued */ |
- Add unordered_lru_cache for block heights in CInstantSendManager - Provide GetTipHeight() (non-optional) and GetBlockHeight() helpers - Use cache in InstantSendSigner::CheckCanLock - Cache-first for cycleHash height in ISLOCK message path and batch verify - Cache-first mined-height for HasChainLock and WriteInstantSendLockMined Only InstantSend files are included in this commit.
Add CacheTipHeight() helper to eliminate code duplication when updating tip height cache. Refactor cache methods to accept CBlockIndex* instead of hash/height pairs for type safety. Handle nullptr cases properly.
PastaPastaPasta
force-pushed
the
feat/no-cs-main-islock
branch
from
186c81d to
e918729
Compare
|
Resolved |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/instantsend/instantsend.cpp (1)
150-173: TidyminedHeightcomputation and avoid unnecessary cache lookup for mempool-only txesThe reworked
minedHeighthandling is nice: you centralize height lookup viaGetBlockHeight(hashBlock), reuse it for both the ChainLock short-circuit andWriteInstantSendLockMined, and only fall back toLookupBlockIndexwhen the cache/helper couldn’t provide a height.One small simplification:
GetBlockHeight(hashBlock)is invoked even whenhashBlockis still null (typical for mempool-only txes) and even whenfound_transactionis false. That just grabscs_height_cache, seeshash.IsNull(), and returns. You could avoid that entirely by gating the lookup onfound_transactionand!hashBlock.IsNull():- const bool found_transaction{tx != nullptr}; - // we ignore failure here as we must be able to propagate the lock even if we don't have the TX locally - std::optional<int> minedHeight = GetBlockHeight(hashBlock); - if (found_transaction) { + const bool found_transaction{tx != nullptr}; + // we ignore failure here as we must be able to propagate the lock even if we don't have the TX locally + std::optional<int> minedHeight; + if (found_transaction && !hashBlock.IsNull()) { + minedHeight = GetBlockHeight(hashBlock); + } + if (found_transaction) { if (!minedHeight.has_value()) { const CBlockIndex* pindexMined = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock)); if (pindexMined != nullptr) { CacheBlockHeight(pindexMined); minedHeight = pindexMined->nHeight; } } // Let's see if the TX that was locked by this islock is already mined in a ChainLocked block. If yes, // we can simply ignore the islock, as the ChainLock implies locking of all TXs in that chain if (minedHeight.has_value() && clhandler.HasChainLock(*minedHeight, hashBlock)) { ... } }Also, the CI reports a clang-format diff for this file around this region; after adjusting the logic, please run the project’s clang-format to clear that.
🧹 Nitpick comments (3)
src/instantsend/net_instantsend.cpp (2)
37-47: Avoid redundant cs_main lookup afterGetBlockHeightin ISDLOCK path
GetBlockHeight(islock->cycleHash)already falls back toLookupBlockIndexunder::cs_mainand caches the height. The subsequentWITH_LOCK(::cs_main, LookupBlockIndex)+CacheBlockHeight+ manual assignment repeats that work and reintroduces cs_main contention on a path you just cache-ified.You can simplify to rely solely on the cache helper, e.g.:
- auto cycleHeightOpt = m_is_manager.GetBlockHeight(islock->cycleHash); - if (!cycleHeightOpt) { - const auto blockIndex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(islock->cycleHash)); - if (blockIndex == nullptr) { - // Maybe we don't have the block yet or maybe some peer spams invalid values for cycleHash - m_peer_manager->PeerMisbehaving(pfrom.GetId(), 1); - return; - } - m_is_manager.CacheBlockHeight(blockIndex); - cycleHeightOpt = blockIndex->nHeight; - } - const int block_height = *cycleHeightOpt; + auto cycleHeightOpt = m_is_manager.GetBlockHeight(islock->cycleHash); + if (!cycleHeightOpt) { + // Maybe we don't have the block yet or maybe some peer spams invalid values for cycleHash + m_peer_manager->PeerMisbehaving(pfrom.GetId(), 1); + return; + } + const int block_height = *cycleHeightOpt;
128-139: Batch path height logic is correct; optionally cachetipHeightonce per batchUsing
GetBlockHeight(islock->cycleHash)andGetTipHeight()here matches the new cached height flow and avoids direct cs_main usage. If you want to shave a bit more overhead in this hot loop, you could hoistconst int tipHeight = m_is_manager.GetTipHeight();out of theforand reuse it for all pending locks in this batch, since a small drift in tip during verification is acceptable.src/instantsend/instantsend.h (1)
19-26: Ensure header self-contains<unordered_map>and briefly document height-cache intentThe new height-cache members (
cs_height_cache,MAX_BLOCK_HEIGHT_CACHE,m_cached_block_heights,m_cached_tip_height) and use ofunordered_lru_cache<uint256, int, StaticSaltedHasher, MAX_BLOCK_HEIGHT_CACHE>look fine.This header still declares
std::unordered_map<COutPoint, uint256, SaltedOutpointHasher> nonLockedTxsByOutpoints, but no longer explicitly includes<unordered_map>. If that include was removed here and isn’t guaranteed via another header in this file’s includes, it’s safer to restore it so the header is self-contained.It could also help future maintainers if you add a short comment near
cs_height_cache/ the cache members explaining that these caches are best-effort helpers to reduce cs_main contention and should not be accessed while holding::cs_main(to keep lock ordering clear, matching the .cpp helpers).Also applies to: 96-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/instantsend/instantsend.cpp(6 hunks)src/instantsend/instantsend.h(5 hunks)src/instantsend/net_instantsend.cpp(2 hunks)src/instantsend/signing.cpp(1 hunks)src/instantsend/signing.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/instantsend/signing.cpp
- src/instantsend/signing.h
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-11-04T18:23:28.175Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/governance/classes.cpp:147-154
Timestamp: 2025-11-04T18:23:28.175Z
Learning: In src/governance/classes.cpp, CSuperblock::GetPaymentsLimit intentionally uses synthetic difficulty constants (nBits = 1 for mainnet, powLimit for networks with min difficulty) and simple height-based V20 activation checks instead of actual chain block data. This is by design because superblocks themselves are "synthetic" governance payment blocks, not regular mined blocks.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash PR: 6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-02-14T15:15:58.165Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-07-09T15:05:36.250Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/rpc/evo.cpp:1273-1273
Timestamp: 2025-07-09T15:05:36.250Z
Learning: When clang-format suggestions significantly harm readability (like splitting logical parameter groups across multiple lines), it's acceptable to use `// clang-format off` and `// clang-format on` directives to exclude the problematic section from automatic formatting, prioritizing code readability over strict line length compliance.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-01-14T09:06:19.717Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/instantsend/instantsend.h
🧬 Code graph analysis (3)
src/instantsend/net_instantsend.cpp (1)
src/net_processing.cpp (3)
WITH_LOCK(331-334)WITH_LOCK(3230-3230)WITH_LOCK(3252-3252)
src/instantsend/instantsend.cpp (1)
src/node/interfaces.cpp (15)
hash(893-910)hash(893-893)hash(932-936)hash(932-932)hash(958-963)hash(958-958)LOCK(581-585)LOCK(591-598)LOCK(599-606)LOCK(868-877)LOCK(911-915)LOCK(1095-1099)tip(97-105)tip(586-590)tip(607-615)
src/instantsend/instantsend.h (2)
src/saltedhasher.h (1)
uint256(38-44)src/instantsend/instantsend.cpp (22)
CacheBlockHeightInternal(724-728)CacheBlockHeightInternal(724-724)TransactionAddedToMempool(203-236)TransactionAddedToMempool(203-203)TransactionRemovedFromMempool(238-253)TransactionRemovedFromMempool(238-238)BlockConnected(255-285)BlockConnected(255-255)BlockDisconnected(287-298)BlockDisconnected(287-288)RemoveConflictingLock(614-625)RemoveConflictingLock(614-614)GetInstantSendLockCount(719-722)GetInstantSendLockCount(719-719)CacheBlockHeight(730-734)CacheBlockHeight(730-730)GetBlockHeight(736-754)GetBlockHeight(736-736)CacheTipHeight(756-765)CacheTipHeight(756-756)GetTipHeight(767-780)GetTipHeight(767-767)
🪛 GitHub Actions: Clang Diff Format Check
src/instantsend/instantsend.cpp
[error] 150-150: clang-format-diff differences detected between pre- and post-formatting for this file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (3)
src/instantsend/instantsend.cpp (2)
261-285: Height-cache wiring in block/tip callbacks and conflict removal looks consistentHooking
CacheTipHeight(pindex)intoBlockConnectedandUpdatedBlockTip, and cleaning upm_cached_block_heightsplus recaching the previous tip inBlockDisconnected, keeps the tip/height cache in sync with chainstate without exposing new cs_main callsites. Likewise, usingGetTipHeight()inRemoveConflictingLockpreserves the previous behavior (passing current tip height intoRemoveChainedInstantSendLocks) while benefiting from the cached value. I don’t see functional issues here.Also applies to: 287-297, 436-437, 618-620
724-780: Based on my inspection of the code snippets:Current call sites appear safe from lock ordering violations. All visible invocations of
GetBlockHeight()andGetTipHeight()(in signing.cpp, net_instantsend.cpp, and instantsend.cpp) do not hold::cs_mainat the call site. Both functions internally acquire::cs_mainviaWITH_LOCK, which is safe when called without pre-existing::cs_mainlock.However, the review concern is fundamentally valid: Future call sites in validation or other contexts that already hold
::cs_maincould introduce a deadlock. The code lacks explicit documentation of this constraint.Recommendation: Add a clarifying comment near the function declarations to document that
GetBlockHeight()andGetTipHeight()must not be called while holding::cs_main. The existingEXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache)annotation should be complemented with this guidance to prevent future misuse.The current visible code appears correct, but runtime verification of all potential call paths (including indirect calls through other modules) would require deeper call-graph analysis beyond what can be confirmed from code inspection alone.
src/instantsend/instantsend.h (1)
135-137: Lock annotations and public cache API match implementationExtending the thread-safety annotations to include
!cs_height_cacheonResolveBlockConflicts,ProcessInstantSendLock,TransactionRemovedFromMempool,BlockConnected/BlockDisconnected,UpdatedBlockTip, andRemoveConflictingLockmatches how these methods now call into the height-cache helpers.The public helpers (
CacheBlockHeight,GetBlockHeight,CacheTipHeight,GetTipHeight) are correctly marked as requiring!cs_height_cache, aligning with the internal locking in the .cpp. From an interface/TSAN perspective this all looks consistent.Also applies to: 155-156, 159-163, 173-176, 182-185
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6953.617ed07e. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6953.e918729c. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6953.e918729c. The image should be on dockerhub soon. |
4 participants
Issue being fixed or feature implemented
Today's testing on testnet showed under load, cs_main was a major contender:
(however, note the major contenders from before #6912 / #6468 are now significantly reduced)
What was done?
How Has This Been Tested?
Breaking Changes
Please describe any breaking changes your code introduces
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.