refactor: remove dependencies of CChainState on CMasternodeSync#7212
refactor: remove dependencies of CChainState on CMasternodeSync#7212knst wants to merge 7 commits intodashpay:developfrom
Conversation
|
This pull request has conflicts, please rebase. |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
knst
force-pushed
the
kernel-zero-is-mn-sync
branch
from
d65c9b9 to
521f1fc
Compare
knst
force-pushed
the
kernel-zero-is-mn-sync
branch
2 times, most recently
from
f552476 to
24f2705
Compare
|
✅ Review complete (commit 94eeabd) |
WalkthroughThis change removes CMasternodeSync dependencies from several components (CChainstateHelper, CMNPaymentsProcessor, llmq::CInstantSendManager, LLMQContext) and from node chainstate loading APIs. The InstantSend public API Sequence Diagram(s)sequenceDiagram
participant BlockConnect as CChainState::ConnectBlock
participant ISManager as llmq::CInstantSendManager
participant ChainHelper as CChainstateHelper
participant Chain as Chain/Node (IBD state)
Note over BlockConnect,Chain: Block connection flow with InstantSend checks
BlockConnect->>Chain: Check IsInitialBlockDownload()
alt Node synced (!IBD)
BlockConnect->>ISManager: ConflictingISLockIfAny(txids)
ISManager-->>BlockConnect: return conflicts (if any)
alt Conflict
BlockConnect->>BlockConnect: state.Invalid("conflict-tx-lock")
else No conflict
BlockConnect->>BlockConnect: continue connect
end
else Node in IBD
BlockConnect->>BlockConnect: skip InstantSend conflict loop
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/miner_tests.cpp (1)
583-584: Consider extracting a helper to reduce repetition.The
UpdateTxFirstSeenMapcall with the time calculation is repeated many times throughout these tests. A small helper function would improve readability.🔧 Optional helper function
// At the top of the test file or within MinerTestingSetup void AgeTxForInstantSend(const Uint256HashSet& txids, int64_t base_time = -1) { if (base_time < 0) { base_time = std::chrono::duration_cast<std::chrono::seconds>( Now<NodeSeconds>().time_since_epoch()).count(); } m_node.clhandler->UpdateTxFirstSeenMap(txids, base_time - WAIT_FOR_ISLOCK_TIMEOUT); }Then calls become:
AgeTxForInstantSend({hashMediumFeeTx, hashPrioritsedChild, ...});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/miner_tests.cpp` around lines 583 - 584, Extract a small helper (e.g. AgeTxForInstantSend) near the top of the test file or inside MinerTestingSetup that wraps the repeated call to m_node.clhandler->UpdateTxFirstSeenMap by computing the epoch seconds via std::chrono::duration_cast<std::chrono::seconds>(Now<NodeSeconds>().time_since_epoch()).count() and passing (base_time - WAIT_FOR_ISLOCK_TIMEOUT), provide an optional base_time parameter defaulting to the current epoch seconds, and replace all repeated direct UpdateTxFirstSeenMap(..., Now<NodeSeconds>()... - WAIT_FOR_ISLOCK_TIMEOUT) calls with AgeTxForInstantSend(txid_set) to reduce duplication and improve readability while keeping existing semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/miner_tests.cpp`:
- Around line 583-584: Extract a small helper (e.g. AgeTxForInstantSend) near
the top of the test file or inside MinerTestingSetup that wraps the repeated
call to m_node.clhandler->UpdateTxFirstSeenMap by computing the epoch seconds
via
std::chrono::duration_cast<std::chrono::seconds>(Now<NodeSeconds>().time_since_epoch()).count()
and passing (base_time - WAIT_FOR_ISLOCK_TIMEOUT), provide an optional base_time
parameter defaulting to the current epoch seconds, and replace all repeated
direct UpdateTxFirstSeenMap(..., Now<NodeSeconds>()... -
WAIT_FOR_ISLOCK_TIMEOUT) calls with AgeTxForInstantSend(txid_set) to reduce
duplication and improve readability while keeping existing semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 632b3c82-edc1-4141-81be-782473d695b8
📒 Files selected for processing (17)
src/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/governance/governance.cppsrc/governance/governance.hsrc/init.cppsrc/instantsend/instantsend.cppsrc/instantsend/instantsend.hsrc/llmq/context.cppsrc/llmq/context.hsrc/masternode/payments.cppsrc/masternode/payments.hsrc/masternode/sync.cppsrc/masternode/sync.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/test/miner_tests.cppsrc/test/util/setup_common.cpp
💤 Files with no reviewable changes (2)
- src/node/chainstate.h
- src/init.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR successfully removes several direct CMasternodeSync dependencies, but it also changes validation behavior in a way that is not safe during initial sync. ConnectBlock now enforces InstantSend conflicts whenever spork 3 is active, even though the network layer still accepts and stores ISLOCKs before IsBlockchainSynced() is true.
Reviewed commit: 24f2705
🔴 1 blocking
1 additional finding
🔴 blocking: Removing the blockchain-sync gate makes block acceptance depend on pre-sync ISLOCK state
src/instantsend/instantsend.cpp (lines 469-475)
This method used to disable conflict rejection until masternode sync had reached the blockchain-synced stage, which kept transient InstantSend state out of ConnectBlock. After this change, validation.cpp calls ShouldInstantSendRejectConflicts() and returns conflict-tx-lock for any locally known conflicting lock, while NetInstantSend::ProcessMessage() still accepts and queues incoming ISLOCKs without checking IsBlockchainSynced(). A node that is still catching up can therefore learn a conflicting ISLOCK from the network and reject an otherwise valid block before it has finished syncing, which is a behavioral regression in the block-validation path.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/instantsend/instantsend.cpp`:
- [BLOCKING] lines 469-475: Removing the blockchain-sync gate makes block acceptance depend on pre-sync ISLOCK state
This method used to disable conflict rejection until masternode sync had reached the blockchain-synced stage, which kept transient InstantSend state out of `ConnectBlock`. After this change, `validation.cpp` calls `ShouldInstantSendRejectConflicts()` and returns `conflict-tx-lock` for any locally known conflicting lock, while `NetInstantSend::ProcessMessage()` still accepts and queues incoming ISLOCKs without checking `IsBlockchainSynced()`. A node that is still catching up can therefore learn a conflicting ISLOCK from the network and reject an otherwise valid block before it has finished syncing, which is a behavioral regression in the block-validation path.
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
kernel-zero-is-mn-sync
branch
from
24f2705 to
a142f65
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/miner_tests.cpp (1)
159-163:⚠️ Potential issue | 🟡 MinorBackdate
hashLowFeeTxtoo, or this assertion can pass for the wrong reason.At Line 162,
hashLowFeeTxis added but not aged before Line 163 block assembly. It may be filtered by InstantSend safety timeout instead of the intended min-fee package logic.💡 Proposed fix
tx.vin[0].prevout.hash = hashFreeTx; tx.vout[0].nValue = 5000000000LL - 1000 - 50000 - feeToUse; uint256 hashLowFeeTx = tx.GetHash(); m_node.mempool->addUnchecked(entry.Fee(feeToUse).FromTx(tx)); + // Age transaction for InstantSend + m_node.clhandler->UpdateTxFirstSeenMap( + {hashLowFeeTx}, + std::chrono::duration_cast<std::chrono::seconds>(Now<NodeSeconds>().time_since_epoch()).count() - WAIT_FOR_ISLOCK_TIMEOUT); pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/miner_tests.cpp` around lines 159 - 163, The test computes hashLowFeeTx (via tx.GetHash()) and adds it to the mempool with m_node.mempool->addUnchecked(entry.Fee(feeToUse).FromTx(tx)) but never backdates that mempool entry, so the subsequent AssemblerForTest(...).CreateNewBlock(scriptPubKey) may exclude it due to InstantSend safety timeout instead of exercising the min-fee package logic; fix by setting the mempool entry time before adding (e.g. use the CTxMemPoolEntry builder's Time(...) or equivalent on the same `entry` used for FromTx(tx)) to a value older than the InstantSend safety window so hashLowFeeTx is aged and evaluated by the min-fee package logic when CreateNewBlock is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/test/miner_tests.cpp`:
- Around line 159-163: The test computes hashLowFeeTx (via tx.GetHash()) and
adds it to the mempool with
m_node.mempool->addUnchecked(entry.Fee(feeToUse).FromTx(tx)) but never backdates
that mempool entry, so the subsequent
AssemblerForTest(...).CreateNewBlock(scriptPubKey) may exclude it due to
InstantSend safety timeout instead of exercising the min-fee package logic; fix
by setting the mempool entry time before adding (e.g. use the CTxMemPoolEntry
builder's Time(...) or equivalent on the same `entry` used for FromTx(tx)) to a
value older than the InstantSend safety window so hashLowFeeTx is aged and
evaluated by the min-fee package logic when CreateNewBlock is called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 48feb5b5-c791-4aef-be0a-c13d7b0f366d
📒 Files selected for processing (20)
src/chainlock/signing.cppsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/governance/governance.cppsrc/governance/governance.hsrc/init.cppsrc/instantsend/instantsend.cppsrc/instantsend/instantsend.hsrc/llmq/context.cppsrc/llmq/context.hsrc/masternode/payments.cppsrc/masternode/payments.hsrc/masternode/sync.cppsrc/masternode/sync.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/miner.cppsrc/test/miner_tests.cppsrc/test/util/setup_common.cppsrc/validation.cpp
💤 Files with no reviewable changes (2)
- src/node/chainstate.h
- src/init.cpp
✅ Files skipped from review due to trivial changes (1)
- src/test/util/setup_common.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- src/governance/governance.cpp
- src/governance/governance.h
- src/masternode/sync.h
- src/masternode/sync.cpp
- src/evo/chainhelper.h
- src/llmq/context.h
- src/node/chainstate.cpp
- src/masternode/payments.h
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a142f65bf0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (nOffset < m_consensus_params.nBudgetPaymentsWindowBlocks) { | ||
| // NOTE: old budget system is disabled since 12.1 | ||
| if(m_mn_sync.IsSynced()) { | ||
| if (m_govman.IsValidAndSynced()) { |
There was a problem hiding this comment.
This branch now depends on m_govman.IsValidAndSynced(), but the previous logic only required masternode sync; as a result, when governance is disabled (e.g. -disablegovernance) or governance state is unavailable, a fully synced node takes the "accepting block" path and skips the blockReward cap for old-budget-window heights. That can allow overpaying coinbase amounts to be accepted between nBudgetPaymentsStartBlock and nSuperblockStartBlock, creating validation differences versus nodes that still enforce the cap.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I validated the reviewer outputs directly against the checked-out diff because the verifier ACP lane stalled during startup. Most of the refactor looks coherent: the IsInitialBlockDownload() swap is an intentional policy change, the governance helper preserves the practical payment gating, and the miner test aging updates match the new InstantSend behavior. The only review-worthy issue I found is in the newly added NullNodeSyncNotifier stub: its documented invariant is enforced only by debug-only asserts, so the stub does not actually stay inert in release builds.
Reviewed commit: a142f65
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/masternode/sync.cpp`:
- [SUGGESTION] lines 14-15: NullNodeSyncNotifier silently stops enforcing its invariant in release builds
The new stub is documented as keeping `CMasternodeSync` permanently unsynced for chainstate-only contexts, but these callbacks only do `assert(false)`. Under `-DNDEBUG` they become no-ops, so `CMasternodeSync::SwitchToNextAsset()` can still advance `nCurrentAsset` to `MASTERNODE_SYNC_FINISHED` before `SyncFinished()` runs. That means the newly added stub does not actually guarantee `IsBlockchainSynced()==false` / `IsSynced()==false` in production builds, which defeats the contract described in `sync.h`. If this notifier is used for the planned chainstate-only path, release builds can accidentally re-enable network-dependent validation gates that were supposed to stay off.
…PackageSelection, final part)
There's new helper IsValidAndSynced in CGovernanceManager that incapsulate usages of CMasternodeSync in CMNPayment
…otifier is introduced to use it for CChainState
knst
force-pushed
the
kernel-zero-is-mn-sync
branch
from
a142f65 to
94eeabd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/miner_tests.cpp (1)
309-324:⚠️ Potential issue | 🟠 MajorAdd transaction aging to "child with higher feerate" test—un-aged transactions will be excluded from block assembly.
Both transactions (parent at line 314 and child at line 323) lack aging via
UpdateTxFirstSeenMap. Without aging,IsTxSafeForMiningreturns false (since un-aged transactions have age=0 and threshold is 601 seconds), preventing inclusion in the block. This contradicts the test's apparent intent to verify parent-child transaction ordering. All similar tests in this file explicitly age transactions before expecting them to be included.Add before each
addUncheckedcall:m_node.clhandler->UpdateTxFirstSeenMap({hash}, pblocktemplate->block.nTime - WAIT_FOR_ISLOCK_TIMEOUT);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/miner_tests.cpp` around lines 309 - 324, The two transactions added with m_node.mempool->addUnchecked (the parent and the child around tx.GetHash()/hash) are un-aged so IsTxSafeForMining rejects them; before each addUnchecked call call m_node.clhandler->UpdateTxFirstSeenMap({hash}, pblocktemplate->block.nTime - WAIT_FOR_ISLOCK_TIMEOUT) using the transaction hash computed by tx.GetHash(), then proceed with entry.Fee(...).FromTx(tx). This ensures the txs are aged for mining checks (see UpdateTxFirstSeenMap, m_node.clhandler, addUnchecked, IsTxSafeForMining, pblocktemplate, WAIT_FOR_ISLOCK_TIMEOUT).
🤖 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/masternode/sync.h`:
- Around line 35-41: The doc comment for the chainstate-only stub overstates its
guarantee by saying CMasternodeSync "permanently" returns
IsBlockchainSynced()=false and IsSynced()=false; change the wording to a softer
statement such as "expected in chainstate-only usage" and note that
SwitchToNextAsset() can advance sync state (e.g., to MASTERNODE_SYNC_GOVERNANCE)
before notifier asserts, so IsBlockchainSynced()/IsSynced() are not strictly
guaranteed to be permanently false; keep the note that any actual sync
advancement should use a real notifier (NodeSyncNotifierImpl) and that the stub
asserts on calls.
---
Outside diff comments:
In `@src/test/miner_tests.cpp`:
- Around line 309-324: The two transactions added with
m_node.mempool->addUnchecked (the parent and the child around tx.GetHash()/hash)
are un-aged so IsTxSafeForMining rejects them; before each addUnchecked call
call m_node.clhandler->UpdateTxFirstSeenMap({hash}, pblocktemplate->block.nTime
- WAIT_FOR_ISLOCK_TIMEOUT) using the transaction hash computed by tx.GetHash(),
then proceed with entry.Fee(...).FromTx(tx). This ensures the txs are aged for
mining checks (see UpdateTxFirstSeenMap, m_node.clhandler, addUnchecked,
IsTxSafeForMining, pblocktemplate, WAIT_FOR_ISLOCK_TIMEOUT).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d1c91f47-da04-4848-a177-5f6687580699
📒 Files selected for processing (20)
src/chainlock/signing.cppsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/governance/governance.cppsrc/governance/governance.hsrc/init.cppsrc/instantsend/instantsend.cppsrc/instantsend/instantsend.hsrc/llmq/context.cppsrc/llmq/context.hsrc/masternode/payments.cppsrc/masternode/payments.hsrc/masternode/sync.cppsrc/masternode/sync.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/miner.cppsrc/test/miner_tests.cppsrc/test/util/setup_common.cppsrc/validation.cpp
💤 Files with no reviewable changes (2)
- src/node/chainstate.h
- src/init.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- src/node/miner.cpp
- src/test/util/setup_common.cpp
- src/llmq/context.h
- src/masternode/payments.h
- src/node/chainstate.cpp
- src/masternode/payments.cpp
- src/masternode/sync.cpp
- src/instantsend/instantsend.cpp
| /** Stub implementation for use in chainstate-only (non-network) contexts. | ||
| * CMasternodeSync constructed with this notifier permanently returns | ||
| * IsBlockchainSynced()=false and IsSynced()=false, which correctly disables | ||
| * network-dependent validation paths. | ||
| * | ||
| * Asserts on any call — if sync state is being advanced, a real notifier | ||
| * (NodeSyncNotifierImpl) must be used instead. */ |
There was a problem hiding this comment.
Doc comment overstates the notifier guarantee
Line 36–38 says this setup permanently keeps IsBlockchainSynced() and IsSynced() false, but CMasternodeSync::SwitchToNextAsset() can move from MASTERNODE_SYNC_BLOCKCHAIN to MASTERNODE_SYNC_GOVERNANCE before any notifier assert, which makes IsBlockchainSynced() true. Please soften this to “expected in chainstate-only usage” to avoid a misleading contract.
Suggested wording tweak
-/** Stub implementation for use in chainstate-only (non-network) contexts.
- * CMasternodeSync constructed with this notifier permanently returns
- * IsBlockchainSynced()=false and IsSynced()=false, which correctly disables
- * network-dependent validation paths.
+/** Stub implementation for use in chainstate-only (non-network) contexts.
+ * In expected chainstate-only usage, sync state is not advanced, so
+ * IsBlockchainSynced() and IsSynced() remain false, disabling
+ * network-dependent validation paths.📝 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.
| /** Stub implementation for use in chainstate-only (non-network) contexts. | |
| * CMasternodeSync constructed with this notifier permanently returns | |
| * IsBlockchainSynced()=false and IsSynced()=false, which correctly disables | |
| * network-dependent validation paths. | |
| * | |
| * Asserts on any call — if sync state is being advanced, a real notifier | |
| * (NodeSyncNotifierImpl) must be used instead. */ | |
| /** Stub implementation for use in chainstate-only (non-network) contexts. | |
| * In expected chainstate-only usage, sync state is not advanced, so | |
| * IsBlockchainSynced() and IsSynced() remain false, disabling | |
| * network-dependent validation paths. | |
| * | |
| * Asserts on any call — if sync state is being advanced, a real notifier | |
| * (NodeSyncNotifierImpl) must be used instead. */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/masternode/sync.h` around lines 35 - 41, The doc comment for the
chainstate-only stub overstates its guarantee by saying CMasternodeSync
"permanently" returns IsBlockchainSynced()=false and IsSynced()=false; change
the wording to a softer statement such as "expected in chainstate-only usage"
and note that SwitchToNextAsset() can advance sync state (e.g., to
MASTERNODE_SYNC_GOVERNANCE) before notifier asserts, so
IsBlockchainSynced()/IsSynced() are not strictly guaranteed to be permanently
false; keep the note that any actual sync advancement should use a real notifier
(NodeSyncNotifierImpl) and that the stub asserts on calls.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both reviewers converge on a real behavioral change in ConnectBlock: replacing RejectConflictingBlocks() (gated on CMasternodeSync::IsBlockchainSynced()) with !IsInitialBlockDownload() widens the window during which the islock-conflict check runs. Because IsInitialBlockDownload() latches once and CMasternodeSync can be reset back to MASTERNODE_SYNC_BLOCKCHAIN after long sleeps, a fully-synced node post-resync will now reject blocks that conflict with persisted islocks — arguably more correct than before, but a semantic shift not justified in the PR description. The m_govman.IsValidAndSynced() substitution in IsOldBudgetBlockValueValid introduces a similar, smaller relaxation. The new NullNodeSyncNotifier is dead in this PR, and the miner-test adaptation duplicates a magic constant from chainlock/handler.cpp. No build- or consensus-breaking issues; downgrading Codex's blocking call to a suggestion since the new behavior is defensible.
Reviewed commit: 94eeabd
🟡 2 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
🟡 suggestion: `IsOldBudgetBlockValueValid`: switch to `IsValidAndSynced()` relaxes the strict path when governance failed to load
src/masternode/payments.cpp (lines 161-172)
Pre-PR the strict reward check at this site triggered on m_mn_sync.IsSynced() alone; the new m_govman.IsValidAndSynced() is is_valid && IsSynced(). On a fully-synced node where governance failed to load (is_valid==false), the old code still ran the strict check, while the new code falls through to the permissive 'rely on online nodes' path. The PR description only justifies the equivalence at the IsBlockValueValid/IsBlockPayeeValid sites — the change here is a real (small) relaxation. In practice the path is mostly dead on mainnet (the window is nBudgetPaymentsStartBlock to nSuperblockStartBlock), but it is reachable on devnet/regtest. Either preserve prior behavior (m_govman.IsValid() ? m_govman.IsValidAndSynced() : m_mn_sync.IsSynced()) or call out the intentional behavior change in the commit message.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] line 2615: `!IsInitialBlockDownload()` is not equivalent to the old `RejectConflictingBlocks()` gate after a sync reset
Pre-PR, this islock-conflict check only ran when `CInstantSendManager::RejectConflictingBlocks()` returned true, which in practice required `CMasternodeSync::IsBlockchainSynced()` to be true. The replacement `!IsInitialBlockDownload()` is not equivalent: `CChainState::IsInitialBlockDownload()` latches to false permanently (`src/validation.cpp:1715`), but `CMasternodeSync` can be reset back to `MASTERNODE_SYNC_BLOCKCHAIN` after a long sleep / lost-tip condition (`src/node/sync_manager.cpp:154-158`, `src/masternode/sync.cpp:27-44`). During such a resync window, `NetInstantSend::TransactionAddedToMempool` and `BlockConnected` (`src/instantsend/net_instantsend.cpp:432, 522`) still skip IS processing because they check `IsBlockchainSynced()`, yet `ConnectBlock` will now reject any block that conflicts with whatever islocks happen to be persisted in `CInstantSendDb`. This is arguably more correct (signed islocks don't go stale, and the `has_chainlock` override is preserved), but the PR description does not document the change and there is no functional test exercising a freshly started or post-reset node receiving a chain that conflicts with persisted islocks. Either add a regression test or note the semantic widening explicitly in the commit message.
In `src/masternode/payments.cpp`:
- [SUGGESTION] lines 161-172: `IsOldBudgetBlockValueValid`: switch to `IsValidAndSynced()` relaxes the strict path when governance failed to load
Pre-PR the strict reward check at this site triggered on `m_mn_sync.IsSynced()` alone; the new `m_govman.IsValidAndSynced()` is `is_valid && IsSynced()`. On a fully-synced node where governance failed to load (`is_valid==false`), the old code still ran the strict check, while the new code falls through to the permissive 'rely on online nodes' path. The PR description only justifies the equivalence at the `IsBlockValueValid`/`IsBlockPayeeValid` sites — the change here is a real (small) relaxation. In practice the path is mostly dead on mainnet (the window is `nBudgetPaymentsStartBlock` to `nSuperblockStartBlock`), but it is reachable on devnet/regtest. Either preserve prior behavior (`m_govman.IsValid() ? m_govman.IsValidAndSynced() : m_mn_sync.IsSynced()`) or call out the intentional behavior change in the commit message.
| // DASH : CHECK TRANSACTIONS FOR INSTANTSEND | ||
|
|
||
| if (m_chain_helper->ShouldInstantSendRejectConflicts()) { | ||
| if (!IsInitialBlockDownload()) { |
There was a problem hiding this comment.
🟡 Suggestion: !IsInitialBlockDownload() is not equivalent to the old RejectConflictingBlocks() gate after a sync reset
Pre-PR, this islock-conflict check only ran when CInstantSendManager::RejectConflictingBlocks() returned true, which in practice required CMasternodeSync::IsBlockchainSynced() to be true. The replacement !IsInitialBlockDownload() is not equivalent: CChainState::IsInitialBlockDownload() latches to false permanently (src/validation.cpp:1715), but CMasternodeSync can be reset back to MASTERNODE_SYNC_BLOCKCHAIN after a long sleep / lost-tip condition (src/node/sync_manager.cpp:154-158, src/masternode/sync.cpp:27-44). During such a resync window, NetInstantSend::TransactionAddedToMempool and BlockConnected (src/instantsend/net_instantsend.cpp:432, 522) still skip IS processing because they check IsBlockchainSynced(), yet ConnectBlock will now reject any block that conflicts with whatever islocks happen to be persisted in CInstantSendDb. This is arguably more correct (signed islocks don't go stale, and the has_chainlock override is preserved), but the PR description does not document the change and there is no functional test exercising a freshly started or post-reset node receiving a chain that conflicts with persisted islocks. Either add a regression test or note the semantic widening explicitly in the commit message.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] line 2615: `!IsInitialBlockDownload()` is not equivalent to the old `RejectConflictingBlocks()` gate after a sync reset
Pre-PR, this islock-conflict check only ran when `CInstantSendManager::RejectConflictingBlocks()` returned true, which in practice required `CMasternodeSync::IsBlockchainSynced()` to be true. The replacement `!IsInitialBlockDownload()` is not equivalent: `CChainState::IsInitialBlockDownload()` latches to false permanently (`src/validation.cpp:1715`), but `CMasternodeSync` can be reset back to `MASTERNODE_SYNC_BLOCKCHAIN` after a long sleep / lost-tip condition (`src/node/sync_manager.cpp:154-158`, `src/masternode/sync.cpp:27-44`). During such a resync window, `NetInstantSend::TransactionAddedToMempool` and `BlockConnected` (`src/instantsend/net_instantsend.cpp:432, 522`) still skip IS processing because they check `IsBlockchainSynced()`, yet `ConnectBlock` will now reject any block that conflicts with whatever islocks happen to be persisted in `CInstantSendDb`. This is arguably more correct (signed islocks don't go stale, and the `has_chainlock` override is preserved), but the PR description does not document the change and there is no functional test exercising a freshly started or post-reset node receiving a chain that conflicts with persisted islocks. Either add a regression test or note the semantic widening explicitly in the commit message.
| * network-dependent validation paths. | ||
| * | ||
| * Asserts on any call — if sync state is being advanced, a real notifier | ||
| * (NodeSyncNotifierImpl) must be used instead. */ | ||
| class NullNodeSyncNotifier final : public NodeSyncNotifier | ||
| { | ||
| public: | ||
| void SyncReset() override; | ||
| void SyncFinished() override; | ||
| }; | ||
|
|
||
| // | ||
| // CMasternodeSync : Sync masternode assets in stages |
There was a problem hiding this comment.
💬 Nitpick: NullNodeSyncNotifier is added but never instantiated in this PR
The commit message for 94eeabd says the stub is introduced 'to use it for CChainState', but a grep across src/ finds only the declaration in src/masternode/sync.h:42 and the asserting bodies in src/masternode/sync.cpp:14-15 — no construction site. The class is dead code as merged. Either move the commit into the follow-up that actually consumes it, or note in the PR description that this is staging for a follow-up so reviewers don't expect the integration here. (The assert(false) bodies are safe — src/util/check.h makes building with NDEBUG a hard error in this codebase, so the asserts cannot be elided in release builds.)
source: ['claude']
| BlockAssembler AssemblerForTest(const CChainParams& params); | ||
| }; | ||
|
|
||
| static constexpr int WAIT_FOR_ISLOCK_TIMEOUT{601}; |
There was a problem hiding this comment.
💬 Nitpick: Duplicated WAIT_FOR_ISLOCK_TIMEOUT magic constant drifts from production source-of-truth
miner_tests defines static constexpr int WAIT_FOR_ISLOCK_TIMEOUT{601} (seconds), duplicating the production constant static constexpr auto WAIT_FOR_ISLOCK_TIMEOUT{10min} in src/chainlock/handler.cpp:37. If the production value ever changes, these tests will silently start backdating txs by the wrong amount — passing if the new value is smaller (false confidence) or becoming flaky if larger. Either expose the production constant in a header and reuse it, or at minimum add a comment cross-referencing chainlock/handler.cpp:37 so future changes stay in sync. The +1 second is also undocumented — it appears to compensate for the strict-greater-than comparison but isn't called out.
source: ['claude']
| // Age transaction for InstantSend | ||
| m_node.clhandler->UpdateTxFirstSeenMap({hashParentTx, hashMediumFeeTx, hashHighFeeTx}, std::chrono::duration_cast<std::chrono::seconds>(Now<NodeSeconds>().time_since_epoch()).count() - WAIT_FOR_ISLOCK_TIMEOUT); | ||
| std::unique_ptr<CBlockTemplate> pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey); |
There was a problem hiding this comment.
💬 Nitpick: Pre-aging txs via UpdateTxFirstSeenMap couples tests to chainlock internals
Each UpdateTxFirstSeenMap({...}, now - WAIT_FOR_ISLOCK_TIMEOUT) call backdates txs so IsTxSafeForMining returns true unconditionally — restoring the pre-PR behavior where RejectConflictingBlocks() == false simply skipped the gate. The result is that the tests no longer exercise the IS gating path at all and instead reach into chainlock-handler internals from miner tests. Consider extracting a single helper (e.g. AgeTxForIS({hashes...}) on MinerTestingSetup) instead of inlining the same std::chrono::duration_cast<std::chrono::seconds>(...) ten times, and consider a parallel test variant that does NOT pre-age txs, to verify the gate still rejects un-aged unconfirmed txs once IS is enabled.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two valid findings against this CMasternodeSync decoupling refactor. (1) Claude flagged that NullNodeSyncNotifier uses bare assert(false) which is a no-op under NDEBUG, breaking the documented invariant in release builds. (2) Codex flagged that removing RejectConflictingBlocks() from BlockAssembler::TestPackageTransactions changes real mining behavior on regtest/devnet/isolated-node InstantSend setups — confirmed by the test fixture having to artificially age transactions via UpdateTxFirstSeenMap. Claude's secondary finding about the ConnectBlock IBD-vs-IsBlockchainSynced semantic shift is real but lower-impact and likely intentional; kept as a nitpick asking for a clarifying comment.
Reviewed commit: 94eeabd
🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🟡 suggestion: Removing RejectConflictingBlocks() gate changes mining behavior beyond the unit tests
src/node/miner.cpp (lines 381-385)
Pre-PR this gate was RejectConflictingBlocks() || !IsInstantSendEnabled() || IsLocked(txid) — the first clause returned m_mn_sync.IsBlockchainSynced(), which short-circuited the IsTxSafeForMining check until masternode/blockchain sync had progressed past the blockchain stage. That sync gate also protected callers that don't wait on mn_sync: the hidden mining RPCs (generateblocks, generatetoaddress, generateblock) call BlockAssembler::CreateNewBlock() directly (src/rpc/mining.cpp). On regtest/devnet or any isolated InstantSend-enabled node, freshly submitted mempool transactions will now be silently filtered out of mined blocks until they are 10 minutes old or manually IS-locked. The numerous UpdateTxFirstSeenMap(..., now - WAIT_FOR_ISLOCK_TIMEOUT) additions in src/test/miner_tests.cpp are direct evidence the change affects real assembly behavior, not just a test harness quirk. Either gate this on !IsInitialBlockDownload() (to mirror the new ConnectBlock gate) or document/test the regtest impact explicitly.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/masternode/sync.cpp`:
- [SUGGESTION] lines 14-15: NullNodeSyncNotifier uses lowercase assert(false) — compiled out under NDEBUG
The header at src/masternode/sync.h:35-41 documents that NullNodeSyncNotifier 'asserts on any call — if sync state is being advanced, a real notifier (NodeSyncNotifierImpl) must be used instead.' The implementation uses the C `assert()` macro from <cassert>, which expands to nothing when NDEBUG is defined. Release builds of dashd define NDEBUG (only `--enable-debug` clears it), so in release builds these methods silently return rather than abort. If a chainstate-only context wired with NullNodeSyncNotifier ever routes a call into CMasternodeSync::SwitchToNextAsset() or Reset(true, /*fNotifyReset=*/true), the stub will silently no-op and the documented invariant will be violated. Use Bitcoin Core's `Assert()` from util/check.h, which survives NDEBUG.
In `src/node/miner.cpp`:
- [SUGGESTION] lines 381-385: Removing RejectConflictingBlocks() gate changes mining behavior beyond the unit tests
Pre-PR this gate was `RejectConflictingBlocks() || !IsInstantSendEnabled() || IsLocked(txid)` — the first clause returned `m_mn_sync.IsBlockchainSynced()`, which short-circuited the IsTxSafeForMining check until masternode/blockchain sync had progressed past the blockchain stage. That sync gate also protected callers that don't wait on mn_sync: the hidden mining RPCs (`generateblocks`, `generatetoaddress`, `generateblock`) call BlockAssembler::CreateNewBlock() directly (src/rpc/mining.cpp). On regtest/devnet or any isolated InstantSend-enabled node, freshly submitted mempool transactions will now be silently filtered out of mined blocks until they are 10 minutes old or manually IS-locked. The numerous `UpdateTxFirstSeenMap(..., now - WAIT_FOR_ISLOCK_TIMEOUT)` additions in src/test/miner_tests.cpp are direct evidence the change affects real assembly behavior, not just a test harness quirk. Either gate this on `!IsInitialBlockDownload()` (to mirror the new ConnectBlock gate) or document/test the regtest impact explicitly.
| void NullNodeSyncNotifier::SyncReset() { assert(false); } | ||
| void NullNodeSyncNotifier::SyncFinished() { assert(false); } |
There was a problem hiding this comment.
🟡 Suggestion: NullNodeSyncNotifier uses lowercase assert(false) — compiled out under NDEBUG
The header at src/masternode/sync.h:35-41 documents that NullNodeSyncNotifier 'asserts on any call — if sync state is being advanced, a real notifier (NodeSyncNotifierImpl) must be used instead.' The implementation uses the C assert() macro from , which expands to nothing when NDEBUG is defined. Release builds of dashd define NDEBUG (only --enable-debug clears it), so in release builds these methods silently return rather than abort. If a chainstate-only context wired with NullNodeSyncNotifier ever routes a call into CMasternodeSync::SwitchToNextAsset() or Reset(true, /fNotifyReset=/true), the stub will silently no-op and the documented invariant will be violated. Use Bitcoin Core's Assert() from util/check.h, which survives NDEBUG.
💡 Suggested change
| void NullNodeSyncNotifier::SyncReset() { assert(false); } | |
| void NullNodeSyncNotifier::SyncFinished() { assert(false); } | |
| void NullNodeSyncNotifier::SyncReset() { Assert(false); } | |
| void NullNodeSyncNotifier::SyncFinished() { Assert(false); } |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/masternode/sync.cpp`:
- [SUGGESTION] lines 14-15: NullNodeSyncNotifier uses lowercase assert(false) — compiled out under NDEBUG
The header at src/masternode/sync.h:35-41 documents that NullNodeSyncNotifier 'asserts on any call — if sync state is being advanced, a real notifier (NodeSyncNotifierImpl) must be used instead.' The implementation uses the C `assert()` macro from <cassert>, which expands to nothing when NDEBUG is defined. Release builds of dashd define NDEBUG (only `--enable-debug` clears it), so in release builds these methods silently return rather than abort. If a chainstate-only context wired with NullNodeSyncNotifier ever routes a call into CMasternodeSync::SwitchToNextAsset() or Reset(true, /*fNotifyReset=*/true), the stub will silently no-op and the documented invariant will be violated. Use Bitcoin Core's `Assert()` from util/check.h, which survives NDEBUG.
| // DASH : CHECK TRANSACTIONS FOR INSTANTSEND | ||
|
|
||
| if (m_chain_helper->ShouldInstantSendRejectConflicts()) { | ||
| if (!IsInitialBlockDownload()) { |
There was a problem hiding this comment.
💬 Nitpick: ConnectBlock IS-conflict gate now fires earlier than the previous IsBlockchainSynced check
The previous gate m_chain_helper->ShouldInstantSendRejectConflicts() resolved to m_mn_sync.IsBlockchainSynced() (true only once nCurrentAsset > MASTERNODE_SYNC_BLOCKCHAIN). The new gate is !IsInitialBlockDownload(). There is a window where !IsInitialBlockDownload() && !IsBlockchainSynced() during which the node will now reject islock-conflicting blocks where it previously would not. Using IBD as the gate for chainstate validation is reasonable, and chain-split risk is low because islock DB state is locally consistent across peers in the same situation. Adding a brief comment explaining why IBD is the correct gate here would help future readers and reduce the risk of someone undoing the change in a future refactor.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
No blockers from me on the current head. My earlier request-changes review applied to an older commit; the remaining comments are advisory suggestions/nits.
2 participants
Issue being fixed or feature implemented
The class CMasternodeSync is mostly used in network context. It should not be a direct dependency of CChainState.
The changes are split-off from #7178 to reduce scope of 7178 tidy as CInstantSendManager related only.
What was done?
IsBlockchainSyncedis duplicated; no spork validation is required since feat: remove SPORK_3_INSTANTSEND_BLOCK_FILTERING and SPORK_9_SUPERBLOCKS_ENABLED #7278BlockAssembler::TestPackageTransactionsbut it should not affect anything beside regression tests [which are fixed], because blocks are not expected to be mined when sync is not done yet.IsInitialBlockDownloadforCChainState::ConnectBlockwhich makes more senseCMNPaymentsProcessoruses a new helperCGovernanceManager::IsValidAndSyncedinstead direct usages ofCMasternodeSyncHow Has This Been Tested?
Run unit & functional tests.
Breaking Changes
N/A
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.