test: adds QSENDRECSIGS functional tests for watchquorum nodes#7301
test: adds QSENDRECSIGS functional tests for watchquorum nodes#7301knst wants to merge 1 commit intodashpay:developfrom
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 3a4fea7) |
![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)
WalkthroughThese changes add test infrastructure for verifying InstantSend lock propagation to configured masternodes. A new wire-message type Sequence DiagramsequenceDiagram
participant Test as Test Case
participant MN as Masternode
participant Obs as RecSigsObserver
participant Net as P2P Network
Test->>Test: Create RecSigsObserver instances
Test->>Obs: send_qsendrecsigs(on=True)
Obs->>MN: Enable qsendrecsigs message
Test->>Test: Trigger InstantSend transaction
MN->>Net: Broadcast MSG_ISDLOCK inv
Net->>Obs: on_inv(MSG_ISDLOCK message)
Obs->>Obs: Record MSG_ISDLOCK received
Test->>Test: Sync P2P observers
Test->>Obs: Assert lock observed
Obs-->>Test: Confirmation received
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)
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']
2 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: