Skip to content
Open
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
42 changes: 42 additions & 0 deletions test/functional/p2p_instantsend.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

from test_framework.messages import msg_qsendrecsigs
from test_framework.p2p import P2PInterface
from test_framework.test_framework import DashTestFramework
from test_framework.util import assert_equal, assert_raises_rpc_error, force_finish_mnsync

Expand All @@ -12,6 +14,24 @@
Tests InstantSend functionality (prevent doublespend for unconfirmed transactions)
'''

class RecSigsObserver(P2PInterface):
"""Non-MN peer that opts in to recsigs and records every ISDLOCK inv it sees."""

def __init__(self):
super().__init__()
self.isdlock_inv_seen = False

def send_qsendrecsigs(self, wants_recsigs=True):
self.send_message(msg_qsendrecsigs(wants_recsigs))

def on_inv(self, message):
for inv in message.inv:
# MSG_ISDLOCK inv type, see src/protocol.h
if inv.type == 31:
self.isdlock_inv_seen = True
Comment on lines +29 to +31
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💬 Nitpick: Prefer a named constant for MSG_ISDLOCK type id

MSG_TX (1) and MSG_BLOCK (2) are defined as named constants in test_framework/messages.py and used elsewhere in the framework. The bare literal 31 with an explanatory comment works, but adding MSG_ISDLOCK = 31 to messages.py and importing it would match the rest of the framework and survive a future renumber gracefully. The corresponding C++ enum value lives in src/protocol.h.

source: ['claude']

Comment on lines +28 to +31
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💬 Nitpick: Define MSG_ISDLOCK constant in messages.py instead of literal 31

Other inv types in test_framework/messages.py are exposed as named constants (MSG_TX=1, MSG_BLOCK=2, etc.) and their numeric values mirror src/protocol.h. The literal 31 with an inline comment diverges from that convention and breaks silently if the value is renumbered. Add MSG_ISDLOCK = 31 to messages.py and import it here.

source: ['claude']

super().on_inv(message)


class InstantSendTest(DashTestFramework):
def add_options(self, parser):
self.add_wallet_options(parser)
Expand All @@ -36,6 +56,7 @@ def run_test(self):

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

def test_block_doublespend(self):
Expand Down Expand Up @@ -131,6 +152,27 @@ def test_mempool_doublespend(self):
# mine more blocks
self.generate(self.nodes[0], 2)

def test_isdlock_relayed_to_recsigs_observer(self):
self.log.info("Non-MN peer started with -watchquorums must still get ISDLOCK invs")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💬 Nitpick: Test mentions -watchquorums but never sets the flag — either fix the log or actually exercise the watcher path

The log line claims "Non-MN peer started with -watchquorums must still get ISDLOCK invs", but set_dash_test_params(8, 4) doesn't pass extra_args=["-watchquorums"] to any node — the test instead attaches P2PInterface mocks and sends QSENDRECSIGS manually. That does cover the generic m_wants_recsigs && !verified_masternode server branch, which is the same code path #7293 fixed, but the production scenario reported in the bug was a full node running with -watchquorums. Either (a) rephrase the log to match what the test actually does ("Non-MN peer that opted in via QSENDRECSIGS must still get ISDLOCK invs"), or (b) start a real node with -watchquorums to exercise the watcher init path end-to-end.

💡 Suggested change
Suggested change
self.log.info("Non-MN peer started with -watchquorums must still get ISDLOCK invs")
self.log.info("Non-MN peer that opted in via QSENDRECSIGS must still get ISDLOCK invs")

source: ['claude', 'codex']

observers = []
for mn in self.mninfo:
node = mn.get_node(self)
obs = node.add_p2p_connection(RecSigsObserver())
obs.send_qsendrecsigs(True)
obs.sync_with_ping()
observers.append((node, obs))

txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
self.wait_for_instantlock(txid)
for _, obs in observers:
obs.sync_with_ping()

assert any(obs.isdlock_inv_seen for _, obs in observers), \
"non-MN peer with QSENDRECSIGS got no MSG_ISDLOCK inv"
Comment on lines +170 to +171
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Require every observer to receive the ISDLOCK inv

This assertion only checks any(...), so the test passes even if most watchquorum masternodes fail to relay MSG_ISDLOCK to opted-in non-MN peers. Because this test is meant to validate relay behavior across the watchquorum nodes set up just above, using any can hide partial regressions and let quorum-relay breakages slip through CI undetected.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

smth like 75b876f should do it

Comment on lines +170 to +171
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: Use all(...) instead of any(...) — the bug being guarded is per-peer state

PeerReconstructsISLockFromRecsig() is evaluated per peer at every ISDLOCK relay site (src/net_processing.cpp:1201, 2547, 2577, 6363) against peer.m_wants_recsigs, which is per-CNode. The test attaches one opted-in observer to each masternode, but any(...) only requires one observer to see the inv. A regression where some watch-quorum peers incorrectly suppress MSG_ISDLOCK would still pass as long as a single MN delivers it. Tighten to all(...) so each per-peer ISDLOCK relay decision is exercised, and a partial regression is caught with a diagnosable failure.

💡 Suggested change
Suggested change
assert any(obs.isdlock_inv_seen for _, obs in observers), \
"non-MN peer with QSENDRECSIGS got no MSG_ISDLOCK inv"
assert all(obs.isdlock_inv_seen for _, obs in observers), \
"each non-MN peer with QSENDRECSIGS must get a MSG_ISDLOCK inv"

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 `test/functional/p2p_instantsend.py`:
- [SUGGESTION] lines 170-171: Use `all(...)` instead of `any(...)` — the bug being guarded is per-peer state
  `PeerReconstructsISLockFromRecsig()` is evaluated per peer at every ISDLOCK relay site (src/net_processing.cpp:1201, 2547, 2577, 6363) against `peer.m_wants_recsigs`, which is per-CNode. The test attaches one opted-in observer to each masternode, but `any(...)` only requires one observer to see the inv. A regression where some watch-quorum peers incorrectly suppress MSG_ISDLOCK would still pass as long as a single MN delivers it. Tighten to `all(...)` so each per-peer ISDLOCK relay decision is exercised, and a partial regression is caught with a diagnosable failure.


for node, _ in observers:
node.disconnect_p2ps()
Comment on lines +155 to +174
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: Test framing references -watchquorums but no node is started with it

The log line says Non-MN peer started with -watchquorums must still get ISDLOCK invs, but the test attaches a generic non-MN P2PInterface that manually sends QSENDRECSIGS=true. That correctly exercises the protocol-level surface in PeerReconstructsISLockFromRecsig, but does not cover the end-to-end path where a real node started with -watchquorums issues QSENDRECSIGS at handshake (see src/net.cpp / src/evo/mnauth.cpp). Either rename the log/test to reflect what is actually tested (e.g. non-MN peer that opts into recsigs), or add a second variant that spins up a node with -watchquorums to also cover the integration path.

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 `test/functional/p2p_instantsend.py`:
- [SUGGESTION] lines 155-174: Test framing references `-watchquorums` but no node is started with it
  The log line says `Non-MN peer started with -watchquorums must still get ISDLOCK invs`, but the test attaches a generic non-MN P2PInterface that manually sends QSENDRECSIGS=true. That correctly exercises the protocol-level surface in PeerReconstructsISLockFromRecsig, but does not cover the end-to-end path where a real node started with `-watchquorums` issues QSENDRECSIGS at handshake (see src/net.cpp / src/evo/mnauth.cpp). Either rename the log/test to reflect what is actually tested (e.g. `non-MN peer that opts into recsigs`), or add a second variant that spins up a node with `-watchquorums` to also cover the integration path.


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

Expand Down
17 changes: 17 additions & 0 deletions test/functional/test_framework/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -2447,6 +2447,23 @@ def __repr__(self):
return "msg_qsigshare(sigShares=%d)" % (len(self.sig_shares))


class msg_qsendrecsigs:
__slots__ = ("wants_recsigs",)
msgtype = b"qsendrecsigs"

def __init__(self, wants_recsigs=True):
self.wants_recsigs = wants_recsigs

def deserialize(self, f):
self.wants_recsigs = bool(struct.unpack("<?", f.read(1))[0])

def serialize(self):
return struct.pack("<?", self.wants_recsigs)

def __repr__(self):
return f"msg_qsendrecsigs(wants_recsigs={self.wants_recsigs})"


class msg_qwatch:
__slots__ = ()
msgtype = b"qwatch"
Expand Down
4 changes: 3 additions & 1 deletion test/functional/test_framework/p2p.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
msg_pong,
msg_qdata,
msg_qgetdata,
msg_qsendrecsigs,
msg_sendaddrv2,
msg_sendcmpct,
msg_sendheaders,
Expand Down Expand Up @@ -177,7 +178,7 @@
b"qjustify": None,
b"qpcommit": None,
b"qrinfo": None,
b"qsendrecsigs": None,
b"qsendrecsigs": msg_qsendrecsigs,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep qsendrecsigs ignored until callback exists

Changing MESSAGEMAP to decode qsendrecsigs makes P2PInterface.on_message() dispatch to on_qsendrecsigs, but P2PInterface has no such callback method, so receiving this message raises AttributeError and tears down the test peer. This is reachable when nodes send QSENDRECSIGS to authenticated quorum-relay peers (see the send paths in CMNAuth::ProcessMessage / CConnman::SetMasternodeQuorumRelayMembers), so this mapping change can break functional tests that use default P2PInterface on those connections.

Useful? React with 👍 / 👎.

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: Add a default on_qsendrecsigs callback to P2PInterface

Previously MESSAGEMAP[b"qsendrecsigs"] = None caused incoming QSENDRECSIGS messages to be silently dropped at p2p.py:414. After this change, the message is deserialized and dispatched via on_messagegetattr(self, 'on_qsendrecsigs')(message) (p2p.py:562). No default on_qsendrecsigs is defined alongside the other Dash defaults at lines 617–625. Today this is latent: p2p_quorum_data.py calls the mnauth RPC on P2P mocks, but its fake proRegTxHashes don't match real relay quorum members so mnauth.cpp:161-167 doesn't push QSENDRECSIGS back. Any future test that performs MNAUTH with a real MN proRegTxHash that is also in a relay quorum will receive QSENDRECSIGS and crash with AttributeError. The fix is one line.

💡 Suggested change
Suggested change
b"qsendrecsigs": msg_qsendrecsigs,
def on_qsendrecsigs(self, message): pass
def on_mnlistdiff(self, message): pass
def on_clsig(self, message): pass
def on_islock(self, message): pass
def on_isdlock(self, message): pass

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 `test/functional/test_framework/p2p.py`:
- [SUGGESTION] line 181: Add a default `on_qsendrecsigs` callback to P2PInterface
  Previously `MESSAGEMAP[b"qsendrecsigs"] = None` caused incoming QSENDRECSIGS messages to be silently dropped at p2p.py:414. After this change, the message is deserialized and dispatched via `on_message` → `getattr(self, 'on_qsendrecsigs')(message)` (p2p.py:562). No default `on_qsendrecsigs` is defined alongside the other Dash defaults at lines 617–625. Today this is latent: `p2p_quorum_data.py` calls the `mnauth` RPC on P2P mocks, but its fake proRegTxHashes don't match real relay quorum members so `mnauth.cpp:161-167` doesn't push QSENDRECSIGS back. Any future test that performs MNAUTH with a real MN proRegTxHash that is also in a relay quorum will receive QSENDRECSIGS and crash with AttributeError. The fix is one line.

Comment thread
UdjinM6 marked this conversation as resolved.
b"qsigrec": None,
b"qsigsesann": None,
b"qsigshare": None,
Expand Down Expand Up @@ -621,6 +622,7 @@ def on_isdlock(self, message): pass
def on_platformban(self, message): pass
def on_qgetdata(self, message): pass
def on_qdata(self, message): pass
def on_qsendrecsigs(self, message): pass
def on_qwatch(self, message): pass

def on_verack(self, message): pass
Expand Down
Loading