test: fix and re-enable (hash/raw)recoveredsig zmq test#6721
test: fix and re-enable (hash/raw)recoveredsig zmq test#6721PastaPastaPasta merged 1 commit intodashpay:developfrom
Conversation
WalkthroughThe test function ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
test/functional/interface_zmq_dash.py (1)
150-154: 🛠️ Refactor suggestionReplace arbitrary
sleep(1)with an explicit readiness / polling checkHard-coding a 1-second delay is fragile – network or CI variance can easily make the test flaky.
Instead of sleeping, poll for the first recovered-sig CL message (or any other deterministic condition) before callingtest_recovered_signature_publishers().- # Wait a moment to avoid subscribing to recovered sig in the test before the one from the chainlock - # has been sent which leads to test failure. - time.sleep(1) + # Wait until the CL recovered-sig has actually been published to ZMQ so that we + # do not miss it when subscribing in the next test. + self.wait_for_recovered_sig( + self.nodes[0].getbestchainlock()["requestId"], + self.nodes[0].getbestblockhash(), + )This removes the race while keeping the intent intact.
🧹 Nitpick comments (2)
test/functional/interface_zmq_dash.py (2)
194-196: Avoid the magic number100– introduce a named constant for the quorum type
100is repeated and its meaning is opaque. Define it once (LLMQ_TEST_V17,LLMQ_100_67, etc.) and reuse the symbol – this improves readability and guards against typos.- rpc_recovered_sig = self.mninfo[0].get_node(self).quorum('getrecsig', 100, request_id, msg_hash) + QUORUM_TYPE_REC_SIG = 100 # LLMQ_100_67 + rpc_recovered_sig = self.mninfo[0].get_node(self).quorum( + 'getrecsig', QUORUM_TYPE_REC_SIG, request_id, msg_hash + )The same constant can then be reused further down (see lines 224-226).
224-226: Deduplicate quorum-type literals and keep logic self-contained
100appears three times in the new signing block. After introducingQUORUM_TYPE_REC_SIG(previous comment), factor it in here and avoid accidental divergence.- quorumHash = self.nodes[0].quorum("selectquorum", 100, sign_id)["quorumHash"] - for mn in self.get_quorum_masternodes(quorumHash): # type: MasternodeInfo - mn.get_node(self).quorum("sign", 100, sign_id, sign_msg_hash) + quorumHash = self.nodes[0].quorum( + "selectquorum", QUORUM_TYPE_REC_SIG, sign_id + )["quorumHash"] + + for mn in self.get_quorum_masternodes(quorumHash): # type: MasternodeInfo + mn.get_node(self).quorum("sign", QUORUM_TYPE_REC_SIG, sign_id, sign_msg_hash)This keeps the quorum-type consistent and self-documenting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/interface_zmq_dash.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: x86_64-apple-darwin / Build depends
3 participants
Issue being fixed or feature implemented
The issue with zmq test was discovered by @coderabbitai while reviewing #6718, see #6718 (comment)
What was done?
Use correct quorum type and hash, re-enable
test_recovered_signature_publishers()How Has This Been Tested?
Run
interface_zmq_dash.pyBreaking Changes
n/a
Checklist: