fix: initialize quorum connections on startup#7240
fix: initialize quorum connections on startup#7240UdjinM6 wants to merge 4 commits intodashpay:developfrom
Conversation
After PR dashpay#7063 (commit 1360d9d), CQuorumManager::UpdatedBlockTip was moved from CDSNotificationInterface::UpdatedBlockTip (which is called directly at startup via InitializeCurrentBlockTip) to the ActiveContext/ObserverContext CValidationInterface subscribers. These subscribers only receive UpdatedBlockTip via GetMainSignals() broadcast, which is never triggered at startup on idle chains — ActivateBestChain early-returns when pindexMostWork == m_chain.Tip(). This means that after a full restart with no new blocks: - QuorumObserver::UpdatedBlockTip never fires - CheckQuorumConnections is never called - Quorum connections are never re-established - MNs cannot exchange signing shares - InstantSend and ChainLock signing fails Fix by adding InitializeCurrentBlockTip() to ActiveContext and ObserverContext, called from the loadblk thread right after CDSNotificationInterface::InitializeCurrentBlockTip(). This method also calls the new QuorumObserver::InitializeQuorumConnections(), which bypasses the IsBlockchainSynced() guard (mnsync is not yet complete at that point in the startup sequence). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests that InstantSend works after a full cluster restart without mining any new blocks. This exercises the quorum connection re-establishment path added in the previous commit. The test: - Funds a sender, chainlocks the tip - Restarts all nodes (simple nodes and masternodes) - Reconnects all nodes to node 0 - Bumps mocktime past WAIT_FOR_ISLOCK_TIMEOUT - Sends a transaction and verifies it gets an IS lock Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
![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: c25c61a8d9
ℹ️ 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".
UdjinM6
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds explicit initialization entry points for the current block tip: Sequence Diagram(s)sequenceDiagram
participant AppInitMain as AppInitMain (init.cpp)
participant ChainMan as ChainManager
participant ActiveCtx as ActiveContext / ObserverContext
participant QMan as QuorumManager (qman_handler)
participant QObserver as QuorumObserver
AppInitMain->>ChainMan: ActiveTip() / IsInitialBlockDownload()
ChainMan-->>AppInitMain: tip, ibd
AppInitMain->>ActiveCtx: InitializeCurrentBlockTip(tip, ibd)
activate ActiveCtx
ActiveCtx->>ActiveCtx: UpdatedBlockTip(tip, nullptr, ibd)
alt tip != nullptr
ActiveCtx->>QMan: InitializeQuorumConnections(tip)
activate QMan
QMan->>QObserver: InitializeQuorumConnections(tip)
activate QObserver
QObserver->>QObserver: for each llmq: CheckQuorumConnections(params, tip)
deactivate QObserver
deactivate QMan
end
deactivate ActiveCtx
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-targeted fix for a real regression where quorum connections were not re-established after a full restart on idle chains. The new InitializeQuorumConnections method correctly bypasses the IsBlockchainSynced() guard, and the functional test directly exercises the fix scenario. No blocking issues found; two minor suggestions for code hygiene and test robustness.
Reviewed commit: c25c61a
🟡 2 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/active/context.cpp`:
- [SUGGESTION] lines 100-106: Identical InitializeCurrentBlockTip bodies in ActiveContext and ObserverContext
The method body is identical in ActiveContext (lines 100-106) and ObserverContext (src/llmq/observer/context.cpp:50-56): both call UpdatedBlockTip then qman_handler->InitializeQuorumConnections. The differentiated behavior comes from two layers: (1) UpdatedBlockTip itself differs — ActiveContext also calls nodeman, ehf_sighandler, gov_signer — and (2) InitializeQuorumConnections dispatches through the virtual CheckQuorumConnections (QuorumParticipant overrides QuorumObserver). Both classes are `final` and share only CValidationInterface as a base, so extracting a common implementation isn't straightforward. Acceptable duplication for now, but worth noting if more initialization steps are added.
In `test/functional/p2p_instantsend.py`:
- [SUGGESTION] line 199: Post-restart IS lock only verified on sender; verifying on receiver would strengthen the test
The pre-restart funding TX verifies IS locks on all nodes (lines 157-158), but the post-restart TX only checks the sender (line 199). Since the purpose of this test is to confirm quorum signing infrastructure works across the cluster after restart, verifying the lock propagates to at least the receiver would provide stronger assurance.
| void ActiveContext::InitializeCurrentBlockTip(const CBlockIndex* tip, bool ibd) | ||
| { | ||
| UpdatedBlockTip(tip, nullptr, ibd); | ||
| if (tip) { | ||
| qman_handler->InitializeQuorumConnections(tip); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Identical InitializeCurrentBlockTip bodies in ActiveContext and ObserverContext
The method body is identical in ActiveContext (lines 100-106) and ObserverContext (src/llmq/observer/context.cpp:50-56): both call UpdatedBlockTip then qman_handler->InitializeQuorumConnections. The differentiated behavior comes from two layers: (1) UpdatedBlockTip itself differs — ActiveContext also calls nodeman, ehf_sighandler, gov_signer — and (2) InitializeQuorumConnections dispatches through the virtual CheckQuorumConnections (QuorumParticipant overrides QuorumObserver). Both classes are final and share only CValidationInterface as a base, so extracting a common implementation isn't straightforward. Acceptable duplication for now, but worth noting if more initialization steps are added.
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/active/context.cpp`:
- [SUGGESTION] lines 100-106: Identical InitializeCurrentBlockTip bodies in ActiveContext and ObserverContext
The method body is identical in ActiveContext (lines 100-106) and ObserverContext (src/llmq/observer/context.cpp:50-56): both call UpdatedBlockTip then qman_handler->InitializeQuorumConnections. The differentiated behavior comes from two layers: (1) UpdatedBlockTip itself differs — ActiveContext also calls nodeman, ehf_sighandler, gov_signer — and (2) InitializeQuorumConnections dispatches through the virtual CheckQuorumConnections (QuorumParticipant overrides QuorumObserver). Both classes are `final` and share only CValidationInterface as a base, so extracting a common implementation isn't straightforward. Acceptable duplication for now, but worth noting if more initialization steps are added.
Move InitializeCurrentBlockTip for active_ctx (masternode mode) to after nodeman->Init() so that GetProTxHash() is guaranteed to be set before EnsureQuorumConnections runs. Currently this works by accident: UpdatedBlockTip calls Init() internally when the MN state is not READY. But this is a non-obvious side effect and would silently break quorum connections if someone refactored UpdatedBlockTip to add an early return. Also, if IsInitialBlockDownload() is true, UpdatedBlockTip returns early and Init() never runs, leaving a null proTxHash for quorum setup. Making the dependency explicit improves robustness without changing observable behavior in normal operation. Also add a MN-to-MN quorum connection assertion in the restart test to verify that quorum connections form between masternodes after restart, not just that IS locks work (which can succeed via concentrated sigshare sending over any authenticated connection). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
UdjinM6
force-pushed
the
fix/initialize-quorum-connections-on-startup
branch
from
62b970c to
5a43003
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a430037bd
ℹ️ 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".
2 participants
Issue being fixed or feature implemented
After PR #7063 (commit 1360d9d),
CQuorumManager::UpdatedBlockTipwas moved fromCDSNotificationInterface::UpdatedBlockTip(which is called directly at startup viaInitializeCurrentBlockTip) to theActiveContext/ObserverContextCValidationInterfacesubscribers. These subscribers only receiveUpdatedBlockTipviaGetMainSignals()broadcast, which is never triggered at startup on idle chains —ActivateBestChainearly-returns whenpindexMostWork == m_chain.Tip().This means that after a full restart with no new blocks:
QuorumObserver::UpdatedBlockTipnever firesCheckQuorumConnectionsis never calledWhat was done?
Fix by adding
InitializeCurrentBlockTip()toActiveContextandObserverContext, called from theloadblkthread right afterCDSNotificationInterface::InitializeCurrentBlockTip(). This method also calls the newQuorumObserver::InitializeQuorumConnections(), which bypasses theIsBlockchainSynced()guard (mnsync is not yet complete at that point in the startup sequence).How Has This Been Tested?
See new "IS after MNs restart" test which fails on develop and passes with the patch applied
Breaking Changes
n/a
Checklist: