refactor: remove dependencies of CChainState on CMasternodeSync

[knst]

Issue being fixed or feature implemented

The class CMasternodeSync is about network communication. It should not be a direct dependency of CChainState.

Most changes are split-off from #7178 to keep 7178 tidy.

What was done?

• fixed Regression tests of miner to be compatible with Instant Send
• Instant Send doesn't use CMasternodeSync status during the checks inside 'RejectConflictingBlocks` (split from 7178)
• CMNPaymentsProcessor uses a new helper CGovernanceManager::IsValidAndSynced instead direct usages of CMasternodeSync

Note: usages of RejectConflictingBlocks

• ChainLockSigner::TrySignChainTip. This IsBlockchainSynced still checked directly
• Behavior of Miner BlockAssembler::TestPackageTransactions is changed now. But it should not affect anything beside regression tests, because blocks are not expected to be mined when sync is not done yet.
• CChainState::ConnectBlock uses it indirectly over helper CChainstateHelper::ShouldInstantSendRejectConflicts

How Has This Been Tested?

Run unit & functional tests.

Breaking Changes

N/A

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

• feat(llmq): add trustless quorum proof chain generation and verification #7107 - feat(llmq): add trustless quorum proof chain generation and verification

Please coordinate with the authors of these PRs to avoid merge conflicts.

[thepastaclaw]

✅ Review complete (commit 24f2705)

[coderabbitai]

Walkthrough

This change removes the CMasternodeSync dependency from several core components in the chainstate and payment processing layers, including CChainstateHelper, CMNPaymentsProcessor, CInstantSendManager, and LLMQContext. A new IsValidAndSynced() method was added to CGovernanceManager that combines validity and sync status checks. Related logic in the masternode payments processor was updated to use governance sync status rather than directly checking masternode sync. A NullNodeSyncNotifier class was introduced to provide stub implementations for sync notifications. Test utilities were updated to initialize InstantSend timestamps for newly created transactions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title directly describes the main refactoring objective: removing CMasternodeSync dependencies from CChainState, which is the central theme across all changed files.
Description check	✅ Passed	The pull request description clearly explains the motivation (CMasternodeSync is about network communication and shouldn't be a CChainState dependency), details the specific changes made, and documents the testing approach.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/test/miner_tests.cpp (1)

583-584: Consider extracting a helper to reduce repetition.

The UpdateTxFirstSeenMap call 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2ca9da3 and 24f2705.

📒 Files selected for processing (17)

• src/evo/chainhelper.cpp
• src/evo/chainhelper.h
• src/governance/governance.cpp
• src/governance/governance.h
• src/init.cpp
• src/instantsend/instantsend.cpp
• src/instantsend/instantsend.h
• src/llmq/context.cpp
• src/llmq/context.h
• src/masternode/payments.cpp
• src/masternode/payments.h
• src/masternode/sync.cpp
• src/masternode/sync.h
• src/node/chainstate.cpp
• src/node/chainstate.h
• src/test/miner_tests.cpp
• src/test/util/setup_common.cpp

💤 Files with no reviewable changes (2)

• src/node/chainstate.h
• src/init.cpp

[thepastaclaw]

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.