Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/active/context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,14 @@ void ActiveContext::SetCJServer(gsl::not_null<CCoinJoinServer*> cj_server)
m_cj_server = cj_server;
}

void ActiveContext::InitializeCurrentBlockTip(const CBlockIndex* tip, bool ibd)
{
UpdatedBlockTip(tip, nullptr, ibd);
if (tip) {
qman_handler->InitializeQuorumConnections(tip);
}
}
Comment on lines +100 to +106
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.


void ActiveContext::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload)
{
if (fInitialDownload || pindexNew == pindexFork) // In IBD or blocks were disconnected without any new ones
Expand Down
1 change: 1 addition & 0 deletions src/active/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ struct ActiveContext final : public CValidationInterface {

void Start(CConnman& connman, PeerManager& peerman, int16_t worker_count);
void Stop();
void InitializeCurrentBlockTip(const CBlockIndex* tip, bool ibd);

CCoinJoinServer& GetCJServer() const;
void SetCJServer(gsl::not_null<CCoinJoinServer*> cj_server);
Expand Down
16 changes: 16 additions & 0 deletions src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2417,6 +2417,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
// but don't call it directly to prevent triggering of other listeners like zmq etc.
// GetMainSignals().UpdatedBlockTip(::ChainActive().Tip());
g_ds_notification_interface->InitializeCurrentBlockTip();
{
const CBlockIndex* tip = WITH_LOCK(::cs_main, return chainman.ActiveTip());
const bool ibd = chainman.ActiveChainstate().IsInitialBlockDownload();
if (node.observer_ctx && !node.active_ctx) {
node.observer_ctx->InitializeCurrentBlockTip(tip, ibd);
}
// Note: active_ctx initialization is deferred until after nodeman->Init()
// so that GetProTxHash() is available for quorum connection setup.
}

{
// Get all UTXOs for each MN collateral in one go so that we can fill coin cache early
Expand Down Expand Up @@ -2481,6 +2490,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)

if (node.active_ctx) {
node.active_ctx->nodeman->Init(chainman.ActiveTip());
// Initialize current block tip after nodeman->Init() so that
// GetProTxHash() is available for quorum connection setup.
// Without this ordering, EnsureQuorumConnections returns early
// because the null proTxHash makes the MN appear as a non-member.
const CBlockIndex* tip = WITH_LOCK(::cs_main, return chainman.ActiveTip());
const bool ibd = chainman.ActiveChainstate().IsInitialBlockDownload();
node.active_ctx->InitializeCurrentBlockTip(tip, ibd);
}
});
#ifdef ENABLE_WALLET
Expand Down
8 changes: 8 additions & 0 deletions src/llmq/observer/context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ void ObserverContext::Stop()
qman_handler->Stop();
}

void ObserverContext::InitializeCurrentBlockTip(const CBlockIndex* tip, bool ibd)
{
UpdatedBlockTip(tip, nullptr, ibd);
if (tip) {
qman_handler->InitializeQuorumConnections(tip);
}
}

void ObserverContext::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload)
{
if (fInitialDownload || pindexNew == pindexFork) // In IBD or blocks were disconnected without any new ones
Expand Down
1 change: 1 addition & 0 deletions src/llmq/observer/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ struct ObserverContext final : public CValidationInterface {

void Start(int16_t worker_count);
void Stop();
void InitializeCurrentBlockTip(const CBlockIndex* tip, bool ibd);

protected:
// CValidationInterface
Expand Down
7 changes: 7 additions & 0 deletions src/llmq/observer/quorums.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ void QuorumObserver::Stop()
workerPool.stop(true);
}

void QuorumObserver::InitializeQuorumConnections(gsl::not_null<const CBlockIndex*> pindexNew) const
{
for (const auto& params : Params().GetConsensus().llmqs) {
CheckQuorumConnections(params, pindexNew);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR descriptions says that:

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).

But m_mn_sync.IsBlockchainSynced and m_mn_sync.IsSynced is not the same things.
If blocks are not synced, we should not start masternode connections yet, because internal state is not full enough at this stage yet; isn't it true?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThreadOpenMasternodeConnections already has its own IsBlockchainSynced() guard, so even though InitializeQuorumConnections populates the quorum node maps at startup, no actual connections are opened until blockchain sync completes.

For the common case this patch targets — an up-to-date masternode that just restarted on an idle chain — the tip is current, so the quorum node maps are correct by the time ThreadOpenMasternodeConnections acts on them.

For the rare case of a masternode that's far behind, the maps could potentially contain stale entries. There's a brief window where ThreadOpenMasternodeConnections could attempt connections to stale quorum nodes, but a masternode that far behind has bigger problems (missed DKGs, likely PoSe-banned).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is still a bit confusing for me, but I guess it's much better than broken develop. I ack'ed it

}
}

void QuorumObserver::UpdatedBlockTip(const CBlockIndex* pindexNew, bool fInitialDownload) const
{
if (!pindexNew) return;
Expand Down
1 change: 1 addition & 0 deletions src/llmq/observer/quorums.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class QuorumObserver
void Stop();

void UpdatedBlockTip(const CBlockIndex* pindexNew, bool fInitialDownload) const;
void InitializeQuorumConnections(gsl::not_null<const CBlockIndex*> pindexNew) const;

public:
virtual bool SetQuorumSecretKeyShare(CQuorum& quorum, Span<CBLSSecretKey> skContributions) const;
Expand Down
83 changes: 82 additions & 1 deletion test/functional/p2p_instantsend.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

from test_framework.test_framework import DashTestFramework
from test_framework.util import assert_equal, assert_raises_rpc_error
from test_framework.util import assert_equal, assert_raises_rpc_error, force_finish_mnsync

'''
p2p_instantsend.py
Expand Down Expand Up @@ -36,6 +36,7 @@ def run_test(self):

self.test_mempool_doublespend()
self.test_block_doublespend()
self.test_instantsend_after_restart()

def test_block_doublespend(self):
sender = self.nodes[self.sender_idx]
Expand Down Expand Up @@ -143,5 +144,85 @@ def test_mempool_doublespend(self):
# mine more blocks
self.generate(self.nodes[0], 2)

def test_instantsend_after_restart(self):
self.log.info("Testing InstantSend works after full restart without new blocks")

# fund sender with confirmed coins
sender = self.nodes[self.sender_idx]
receiver = self.nodes[self.receiver_idx]
sender_addr = sender.getnewaddress()
fund_id = self.nodes[0].sendtoaddress(sender_addr, 1)
self.bump_mocktime(30)
self.sync_mempools()
for node in self.nodes:
self.wait_for_instantlock(fund_id, node)
tip = self.generate(self.nodes[0], 2)[-1]
self.bump_mocktime(30)
self.wait_for_chainlocked_block_all_nodes(tip)
self.sync_blocks()
assert sender.getbalance() >= 0.5

receiver_addr = receiver.getnewaddress()

# restart all nodes without mining new blocks
self.log.info("Restarting all nodes")
num_simple_nodes = self.num_nodes - self.mn_count
self.stop_nodes()

for i in range(num_simple_nodes):
self.start_node(i)
for mn_info in self.mninfo:
self.start_masternode(mn_info)

# reconnect: simple nodes to node 0, MNs to node 0 only.
# Quorum connections between MNs must be re-established automatically
# via InitializeCurrentBlockTip → EnsureQuorumConnections, NOT via
# manual connect_nodes between MN pairs.
for i in range(1, num_simple_nodes):
self.connect_nodes(i, 0)
for mn_info in self.mninfo:
self.connect_nodes(mn_info.nodeIdx, 0)
for i in range(num_simple_nodes):
force_finish_mnsync(self.nodes[i])

# bump past WAIT_FOR_ISLOCK_TIMEOUT so txFirstSeenTime loss doesn't
# block chainlock signing for TXs mined before restart
self.bump_mocktime(10 * 60 + 1)
self.sync_blocks()

# Verify that MNs formed quorum connections to other MNs after restart.
# InitializeCurrentBlockTip → EnsureQuorumConnections must populate
# masternodeQuorumNodes so ThreadOpenMasternodeConnections establishes
# MN-to-MN links beyond the manual connections to node 0.
self.log.info("Verifying MN-to-MN quorum connections formed after restart")
for mn_info in self.mninfo:
mn_node = self.nodes[mn_info.nodeIdx]

def check_mn_peers(node=mn_node, my_hash=mn_info.proTxHash):
peers = node.getpeerinfo()
mn_peers = set(p['verified_proregtx_hash'] for p in peers
if p.get('verified_proregtx_hash', '') != '')
other_mn_peers = mn_peers - {my_hash}
return len(other_mn_peers) > 0
self.wait_until(check_mn_peers, timeout=30)

# re-grab references after restart
sender = self.nodes[self.sender_idx]
receiver = self.nodes[self.receiver_idx]

# send a TX — needs IS lock from all restarted MNs, no new blocks mined
is_id = sender.sendtoaddress(receiver_addr, 0.5)
self.bump_mocktime(30)
self.sync_mempools()
for node in self.nodes:
self.wait_for_instantlock(is_id, node)
self.log.info("InstantSend lock succeeded after full restart")
Comment on lines +215 to +219
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should drop bump_mocktime/sync-mempools when https://github.com/dashpay/dash/pull/7241/files is merged


# clean up
receiver.sendtoaddress(self.nodes[0].getnewaddress(), 0.5, "", "", True)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: instead "", "" consider using named arguments here

self.bump_mocktime(30)
self.sync_mempools()
self.generate(self.nodes[0], 2)

if __name__ == '__main__':
InstantSendTest().main()
Loading