Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
41 changes: 41 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, on=True):
self.send_message(msg_qsendrecsigs(on))

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']

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: Hardcoded inv type 31 instead of named constant

inv.type == 31 uses a magic number with only a comment pointing at src/protocol.h. test_framework/messages.py already exposes inv-type constants (MSG_TX = 1, MSG_BLOCK = 2, MSG_GOVERNANCE_OBJECT = 17, MSG_CMPCT_BLOCK = 20). Adding MSG_ISDLOCK = 31 there and importing it would keep Python and C++ inv-type definitions discoverable together and avoid drift.

source: ['claude']

super().on_inv(message)
Comment on lines +27 to +32
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: Only positive direction of recsigs gate is exercised

The test asserts that after QSENDRECSIGS true an observer receives an ISDLOCK inv. It does not exercise the negative direction — that an observer which never opts in receives no ISDLOCK invs, or that one which toggles off via QSENDRECSIGS false stops receiving them. Adding the negative assertion would fully lock in the gate behavior of m_wants_recsigs. Optional follow-up; the surrounding plumbing is already in place.

source: ['claude']



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,26 @@ 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']

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: Log message references -watchquorums but test does not use that flag

The log says "Non-MN peer started with -watchquorums must still get ISDLOCK invs", but RecSigsObserver is a plain P2PInterface that opts in via QSENDRECSIGS — no -watchquorums startup flag is used. The C++ relay gate keys off m_wants_recsigs && verified ProTx, which is what the test actually exercises, so the test is correct, but the wording can mislead future readers. Rephrase to e.g. "Non-MN peers that opt in to recsigs (mirroring -watchquorums behavior) must still get ISDLOCK invs."

source: ['claude']

observers = []
for mn in self.mninfo:
obs = mn.get_node(self).add_p2p_connection(RecSigsObserver())
obs.send_qsendrecsigs(True)
obs.sync_with_ping()
observers.append(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(o.isdlock_inv_seen for o in observers), \
"non-MN peer with QSENDRECSIGS got no MSG_ISDLOCK inv"
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: any() makes the regression check too weak — use all()

PR #7293 fixed every masternode peer to stop suppressing MSG_ISDLOCK for non-MN recsig consumers. The server-side filter at net_processing.cpp:1201-1204 (PeerReconstructsISLockFromRecsig) only suppresses the inv when the peer is a verified MN, so every one of the 4 MNs is expected to relay the inv to its attached non-MN observer. Asserting any(...) lets the test pass even if 3 out of 4 MNs are still broken — i.e., a partial regression of #7293 would slip through, which is precisely what this test should catch. Tighten to all(...), ideally wrapped in a wait_until to give propagation room.

💡 Suggested change
Suggested change
assert any(o.isdlock_inv_seen for o in observers), \
"non-MN peer with QSENDRECSIGS got no MSG_ISDLOCK inv"
self.wait_until(lambda: all(o.isdlock_inv_seen for o in observers),
timeout=10)
assert all(o.isdlock_inv_seen for o in observers), \
"some non-MN peers with QSENDRECSIGS got no 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 169-170: `any()` makes the regression check too weak — use `all()`
  PR #7293 fixed every masternode peer to stop suppressing `MSG_ISDLOCK` for non-MN recsig consumers. The server-side filter at `net_processing.cpp:1201-1204` (`PeerReconstructsISLockFromRecsig`) only suppresses the inv when the peer is a verified MN, so every one of the 4 MNs is expected to relay the inv to its attached non-MN observer. Asserting `any(...)` lets the test pass even if 3 out of 4 MNs are still broken — i.e., a partial regression of #7293 would slip through, which is precisely what this test should catch. Tighten to `all(...)`, ideally wrapped in a `wait_until` to give propagation room.


# Skip disconnect_p2ps(): the next sub-test "test_instantsend_after_restart()"
# restarts all nodes and tears down these P2P connections anyway.

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__ = ("on",)
msgtype = b"qsendrecsigs"

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

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

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

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


class msg_qwatch:
__slots__ = ()
msgtype = b"qwatch"
Expand Down
3 changes: 2 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
Loading