Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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']

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

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:
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 thread
knst marked this conversation as resolved.
Outdated
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 +172
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