test: adds QSENDRECSIGS functional tests for watchquorum nodes#7301
test: adds QSENDRECSIGS functional tests for watchquorum nodes#7301knst wants to merge 2 commits intodashpay:developfrom
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 59dec1c) |
![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: 3a4fea7360
ℹ️ 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".
| b"qpcommit": None, | ||
| b"qrinfo": None, | ||
| b"qsendrecsigs": None, | ||
| b"qsendrecsigs": msg_qsendrecsigs, |
There was a problem hiding this comment.
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 👍 / 👎.
|
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 (3)
✅ Files skipped from review due to trivial changes (2)
WalkthroughAdds a test-only wire message Sequence DiagramsequenceDiagram
participant Test as Test Case
participant MN as Masternode
participant Obs as RecSigsObserver
participant Net as P2P Network
Test->>Obs: create observer per MN
Test->>Obs: send_qsendrecsigs(wants_recsigs=True)
Obs->>MN: establish P2P connection
Test->>Test: broadcast InstantSend transaction
MN->>Net: announce MSG_ISDLOCK (inv)
Net->>Obs: deliver inv containing MSG_ISDLOCK
Obs->>Obs: on_inv() sets isdlock_inv_seen
Test->>Obs: sync_with_ping()
Test->>Test: assert at least one observer saw MSG_ISDLOCK
Test->>Obs: disconnect observers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The new functional test adds useful coverage for the QSENDRECSIGS regression fixed in #7293, but has a few notable issues: (1) mapping qsendrecsigs to a deserializer in MESSAGEMAP without adding a default on_qsendrecsigs callback creates a latent AttributeError for any P2PInterface mock that becomes a verified quorum-relay MN peer; (2) the assertion uses any() so the test passes even if 3 of 4 MNs still suppress the inv, which is exactly the partial regression it should catch; (3) the test uses raw QSENDRECSIGS rather than starting a node with -watchquorums, despite the log line claiming otherwise.
Reviewed commit: 3a4fea7
🟡 2 suggestion(s) | 💬 2 nitpick(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 `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.
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.
| assert any(o.isdlock_inv_seen for o in observers), \ | ||
| "non-MN peer with QSENDRECSIGS got no MSG_ISDLOCK inv" |
There was a problem hiding this comment.
🟡 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
| 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.
| b"qpcommit": None, | ||
| b"qrinfo": None, | ||
| b"qsendrecsigs": None, | ||
| b"qsendrecsigs": msg_qsendrecsigs, |
There was a problem hiding this comment.
🟡 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_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.
💡 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.
| 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") |
There was a problem hiding this comment.
💬 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
| 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']
| # MSG_ISDLOCK inv type, see src/protocol.h | ||
| if inv.type == 31: | ||
| self.isdlock_inv_seen = True |
There was a problem hiding this comment.
💬 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']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
No blockers from me on the current head; remaining comments are advisory suggestions/nits.
|
pls consider a1caa31 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fef1071cfa
ℹ️ 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".
…vers Match the m_wants_recsigs name used in net_processing.cpp, and explicitly disconnect the RecSigsObserver peers instead of relying on the next sub-test's restart to clean them up. +fixed mapping qsendrecsigs to msg_qsendrecsigs Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59dec1c9ab
ℹ️ 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".
| assert any(obs.isdlock_inv_seen for _, obs in observers), \ | ||
| "non-MN peer with QSENDRECSIGS got no MSG_ISDLOCK inv" |
There was a problem hiding this comment.
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 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds a regression test for QSENDRECSIGS opt-in non-MN peers receiving MSG_ISDLOCK invs, plus the framework plumbing for msg_qsendrecsigs. Logic is sound and exercises PeerReconstructsISLockFromRecsig correctly. Main concern is that the assertion uses any instead of all, weakening per-peer regression coverage. No blockers.
Reviewed commit: 59dec1c
🟡 2 suggestion(s) | 💬 1 nitpick(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 `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.
- [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.
| assert any(obs.isdlock_inv_seen for _, obs in observers), \ | ||
| "non-MN peer with QSENDRECSIGS got no MSG_ISDLOCK inv" |
There was a problem hiding this comment.
🟡 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
| 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.
| def test_isdlock_relayed_to_recsigs_observer(self): | ||
| self.log.info("Non-MN peer started with -watchquorums must still get ISDLOCK invs") | ||
| 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" | ||
|
|
||
| for node, _ in observers: | ||
| node.disconnect_p2ps() |
There was a problem hiding this comment.
🟡 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.
| for inv in message.inv: | ||
| # MSG_ISDLOCK inv type, see src/protocol.h | ||
| if inv.type == 31: | ||
| self.isdlock_inv_seen = True |
There was a problem hiding this comment.
💬 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']
3 participants
Issue being fixed or feature implemented
Addresses #7293 (comment)
What was done?
Adds functional tests for QSENDRECSIGS for watch-quorum nodes.
How Has This Been Tested?
Run on the top of #7293 and on develop with #7293 reverted.
Breaking Changes
N/A
Checklist: