test: reduce waste quorum generation and duplicated sleep in feature_llmq_is_retroactive.py #6824
Conversation
Test has never worked as expected. It is expected instant send quorum; helper mine_cycle_quorum should be used here
…in feature_llmq_is_retroactive.py
… is ready This flag is easy to forget and if it is forgotten, everything works as expected, but extra 24 * 3 useless blocks are generated for each
…e quorum is ready" This reverts commit 32d35b1.
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 5
🧹 Nitpick comments (3)
test/functional/test_framework/test_framework.py (1)
2199-2323: mine_cycle_quorum() refactor: internalized first-cycle advancement and removed param.
- Correctly uses cycle_quorum_is_ready to decide extra 3×24 blocks on the first call.
- The DKG phase sequencing and waits mirror the previous helper logic, now specific to DIP0024 (type 103).
- Logs and return values unchanged in spirit; consumers remain compatible.
Minor nitpick: skip_count is always applied (even when at cycle boundary), which intentionally advances to the next cycle start. This matches prior behavior; just highlighting for awareness.
test/functional/feature_llmq_is_retroactive.py (2)
152-161: Nit: duplicate sendrawtransaction assignment; consider asserting equality instead.You assign txid_all_nodes twice with the same tx. It works, but obscures intent. Prefer sending from one node and relaying, or assert same txid for clarity.
- rawtx = self.create_fund_sign_tx() - txid_all_nodes = self.nodes[0].sendrawtransaction(rawtx) - txid_all_nodes = self.nodes[3].sendrawtransaction(rawtx) + rawtx = self.create_fund_sign_tx() + txid_all_nodes = self.nodes[0].sendrawtransaction(rawtx) + txid_from_node3 = self.nodes[3].sendrawtransaction(rawtx) + assert txid_all_nodes == txid_from_node3
190-197: Optional: post-cycle recheck should assert negative as well.After cycling LLMQs twice you re-check absence of IS but don’t assert. Suggest asserting to make the intent testable.
- time.sleep(5) - self.check_no_is(txid_all_nodes, self.nodes[0]) - self.check_no_is(txid_single_node, self.nodes[0]) + time.sleep(5) + assert not self.check_no_is(txid_all_nodes, self.nodes[0]) + assert not self.check_no_is(txid_single_node, self.nodes[0])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
test/functional/feature_llmq_connections.py(2 hunks)test/functional/feature_llmq_is_retroactive.py(7 hunks)test/functional/feature_llmq_rotation.py(4 hunks)test/functional/rpc_verifyislock.py(2 hunks)test/functional/test_framework/test_framework.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/rpc_verifyislock.pytest/functional/feature_llmq_rotation.pytest/functional/feature_llmq_connections.pytest/functional/feature_llmq_is_retroactive.pytest/functional/test_framework/test_framework.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
test/functional/test_framework/test_framework.py
🧬 Code Graph Analysis (4)
test/functional/rpc_verifyislock.py (1)
test/functional/test_framework/test_framework.py (1)
mine_cycle_quorum(2199-2323)
test/functional/feature_llmq_rotation.py (1)
test/functional/test_framework/test_framework.py (1)
mine_cycle_quorum(2199-2323)
test/functional/feature_llmq_connections.py (1)
test/functional/test_framework/test_framework.py (1)
mine_cycle_quorum(2199-2323)
test/functional/feature_llmq_is_retroactive.py (1)
test/functional/test_framework/test_framework.py (1)
mine_cycle_quorum(2199-2323)
🪛 Ruff (0.12.2)
test/functional/feature_llmq_is_retroactive.py
31-31: Do not use bare except
(E722)
🪛 Flake8 (7.2.0)
test/functional/feature_llmq_is_retroactive.py
[error] 31-31: do not use bare 'except'
(E722)
🔇 Additional comments (14)
test/functional/feature_llmq_connections.py (2)
91-91: API migration looks good (mine_cycle_quorum without params).Switching to the new no-arg helper aligns with the refactor in the test framework. No behavioral issues spotted.
106-106: Second call site updated correctly.Consistent usage of the simplified helper; matches the new internal readiness flag behavior.
test/functional/rpc_verifyislock.py (2)
84-84: Second call updated properly.No issues; consistent with the new helper semantics.
61-61: No lingering old mine_cycle_quorum() usages foundI ran the suggested grep across the repo and only the method definition in
test_framework.pywas matched—no call sites still pass arguments. The updated zero-arg calls are consistent everywhere.test/functional/feature_llmq_rotation.py (4)
138-138: Good: explicitly marking cycle_quorum_is_ready before first mine_cycle_quorum.This ensures the subsequent mine_cycle_quorum() doesn’t pay the 3×24 “first-time” cost in this test, which is exactly what the readiness flag was introduced for.
154-154: API migration: mine_cycle_quorum() call updated.Looks correct and consistent with the readiness handling introduced above.
176-176: API migration: mine_cycle_quorum() (second usage).Consistent and correct.
210-210: API migration: mine_cycle_quorum() (third usage).Consistent and correct.
test/functional/test_framework/test_framework.py (2)
1532-1534: Introduced cycle_quorum_is_ready flag: good stateful design for first-cycle behavior.Initializing the internal readiness flag in set_dash_test_params centralizes first-cycle logic and reduces API surface.
1935-1945: wait_for_instantlock signature change verified
No remaining call sites pass the removedexpectedparameter—invocations across the repo have been updated to the new(txid, node, timeout)form.test/functional/feature_llmq_is_retroactive.py (4)
48-53: Helper create_fund_sign_tx(): LGTM.Compact, deterministic funding and signing flow using node0. No issues spotted.
165-166: Good: wait for network propagation on nodes 1 and 2.Ensures a stable baseline before timing out the initial IS session.
200-202: Nice: explicit validation both transactions were mined retroactively.Asserting presence of both txids in the block makes the combined scenario robust.
139-140: Assert absence of IS after reconnection (second scenario).Same as above.
- self.sleep_and_check_no_is(txid, self.nodes[0], 5) + assert not self.sleep_and_check_no_is(txid, self.nodes[0], 5)Likely an incorrect or invalid review comment.
| def check_no_is(self, txid, node): | ||
| try: | ||
| self.log.info(f"Expecting no InstantLock for {txid}") | ||
| return node.getrawtransaction(txid, True)["instantlock"] | ||
| except: | ||
| return False | ||
|
|
There was a problem hiding this comment.
Avoid bare except; catch specific RPC/lookup errors.
Bare except masks programming errors (e.g., the swapped-args bug above). Catch JSONRPCException (RPC “tx not found”) and KeyError for missing fields, and let unexpected exceptions surface.
- def check_no_is(self, txid, node):
- try:
- self.log.info(f"Expecting no InstantLock for {txid}")
- return node.getrawtransaction(txid, True)["instantlock"]
- except:
- return False
+ def check_no_is(self, txid, node):
+ try:
+ self.log.info(f"Expecting no InstantLock for {txid}")
+ return node.getrawtransaction(txid, True)["instantlock"]
+ except (JSONRPCException, KeyError):
+ return FalseAdd the missing import at the top of this file:
from test_framework.authproxy import JSONRPCException🧰 Tools
🪛 Ruff (0.12.2)
31-31: Do not use bare except
(E722)
🪛 Flake8 (7.2.0)
[error] 31-31: do not use bare 'except'
(E722)
🤖 Prompt for AI Agents
In test/functional/feature_llmq_is_retroactive.py around lines 27 to 33, the
check_no_is function uses a bare except which hides programming errors; modify
it to import JSONRPCException from test_framework.authproxy at the top and
change the except to explicitly catch JSONRPCException and KeyError, returning
False for these cases while allowing other exceptions to propagate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
test/functional/feature_llmq_is_retroactive.py (6)
27-33: Replace bare except and internal asserts with explicit exception handling and a boolean return.
- Bare except hides programming errors and fails lint (E722).
assert Falseis discouraged in tests and can be optimized away (B011).- Make this helper return a boolean so callers can assert explicitly.
Apply this diff:
- def check_no_is(self, txid, node): - try: - self.log.info(f"Expecting no InstantLock for {txid}") - assert not node.getrawtransaction(txid, True)["instantlock"] - except: - assert False + def check_no_is(self, txid, node): + self.log.info(f"Expecting no InstantLock for {txid}") + try: + # Return True if InstantLock is present, False otherwise + return node.getrawtransaction(txid, True).get("instantlock", False) + except (JSONRPCException, KeyError): + # If TX is not found or field is absent, treat as "no IS yet" + return FalseAdd the missing import near the other imports:
from test_framework.authproxy import JSONRPCException
70-70: Assert the “no IS before block” expectation.Currently the result is ignored; assert explicitly so regressions fail the test.
- self.sleep_and_check_no_is(txid, self.nodes[0], 5) + assert not self.sleep_and_check_no_is(txid, self.nodes[0], 5)
108-108: Assert absence of IS after reconnection (partially known TX).Make the intent testable.
- self.sleep_and_check_no_is(txid, self.nodes[0], 5) + assert not self.sleep_and_check_no_is(txid, self.nodes[0], 5)
139-139: Assert absence of IS after reconnection (second partially known TX case).- self.sleep_and_check_no_is(txid, self.nodes[0], 5) + assert not self.sleep_and_check_no_is(txid, self.nodes[0], 5)
187-188: Enforce “no IS after session timeout” with assertions.Make the conditions fail-fast.
- self.check_no_is(txid_all_nodes, self.nodes[0]) - self.check_no_is(txid_single_node, self.nodes[0]) + assert not self.check_no_is(txid_all_nodes, self.nodes[0]) + assert not self.check_no_is(txid_single_node, self.nodes[0])
195-196: Assert the post-cycle “no IS” expectation as well.- time.sleep(5) - self.check_no_is(txid_all_nodes, self.nodes[0]) - self.check_no_is(txid_single_node, self.nodes[0]) + time.sleep(5) + assert not self.check_no_is(txid_all_nodes, self.nodes[0]) + assert not self.check_no_is(txid_single_node, self.nodes[0])
🧹 Nitpick comments (2)
test/functional/feature_llmq_is_retroactive.py (2)
156-158: Avoid overwriting txid_all_nodes; validate same txid on both broadcasts.Overwriting obscures intent. Assert equality to document that both nodes see the same tx.
- txid_all_nodes = self.nodes[0].sendrawtransaction(rawtx) - txid_all_nodes = self.nodes[3].sendrawtransaction(rawtx) + txid_all_nodes = self.nodes[0].sendrawtransaction(rawtx) + assert txid_all_nodes == self.nodes[3].sendrawtransaction(rawtx)
190-193: Pass is_first=False on the second mine_cycle_quorum to avoid re-mining 24*3 blocks.Calling twice with the default (True) repeatedly mines the expensive initial offset. Aligns with the PR’s performance goal.
- self.mine_cycle_quorum() - self.mine_cycle_quorum() + self.mine_cycle_quorum() + self.mine_cycle_quorum(is_first=False) self.wait_for_chainlocked_block_all_nodes(self.nodes[0].getbestblockhash(), timeout=30)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
test/functional/feature_llmq_is_retroactive.py(7 hunks)test/functional/test_framework/test_framework.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/functional/test_framework/test_framework.py
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_llmq_is_retroactive.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
🧬 Code Graph Analysis (1)
test/functional/feature_llmq_is_retroactive.py (2)
test/functional/test_framework/test_framework.py (6)
bump_mocktime(877-894)generate(803-806)no_op(800-801)wait_for_chainlocked_block_all_nodes(1955-1957)isolate_node(777-779)mine_cycle_quorum(2197-2319)test/functional/test_framework/util.py (1)
set_node_times(520-523)
🪛 Ruff (0.12.2)
test/functional/feature_llmq_is_retroactive.py
31-31: Do not use bare except
(E722)
32-32: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🪛 Flake8 (7.2.0)
test/functional/feature_llmq_is_retroactive.py
[error] 31-31: do not use bare 'except'
(E722)
| def sleep_and_check_no_is(self, txid, node, sleep): | ||
| time.sleep(sleep) | ||
| self.check_no_is(txid, node) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return the boolean result from sleep_and_check_no_is.
Propagate the helper’s result so call sites can assert on it.
def sleep_and_check_no_is(self, txid, node, sleep):
time.sleep(sleep)
- self.check_no_is(txid, node)
+ return self.check_no_is(txid, node)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def sleep_and_check_no_is(self, txid, node, sleep): | |
| time.sleep(sleep) | |
| self.check_no_is(txid, node) | |
| def sleep_and_check_no_is(self, txid, node, sleep): | |
| time.sleep(sleep) | |
| return self.check_no_is(txid, node) |
🤖 Prompt for AI Agents
In test/functional/feature_llmq_is_retroactive.py around lines 34 to 37, the
helper sleep_and_check_no_is currently sleeps then calls self.check_no_is but
does not return its result; change the function to return the boolean result of
self.check_no_is(txid, node) after the sleep so callers can assert on it, and
update any call sites to use the returned value (e.g., assertTrue/False or
assign and assert) instead of relying on side effects.
There was a problem hiding this comment.
check_no_is only doing assert, no more ret value
There was a problem hiding this comment.
@knst You're absolutely right! I see now that check_no_is is designed to use assertions for validation rather than returning a boolean value. The method either succeeds silently or raises an AssertionError, which is the typical pattern for test helper methods.
My suggestion to return a value from sleep_and_check_no_is was incorrect since check_no_is doesn't return anything - it just asserts. Thank you for the clarification!
✏️ Learnings added
Learnt from: knst
PR: dashpay/dash#6824
File: test/functional/feature_llmq_is_retroactive.py:34-37
Timestamp: 2025-08-19T18:53:02.819Z
Learning: The `check_no_is` method in `test/functional/feature_llmq_is_retroactive.py` uses assertions for validation and doesn't return any value - it either succeeds silently or raises an AssertionError when the InstantLock assertion fails.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
| try: | ||
| self.log.info(f"Expecting no InstantLock for {txid}") | ||
| assert not node.getrawtransaction(txid, True)["instantlock"] | ||
| except: | ||
| assert False |
There was a problem hiding this comment.
can't this be simply
| try: | |
| self.log.info(f"Expecting no InstantLock for {txid}") | |
| assert not node.getrawtransaction(txid, True)["instantlock"] | |
| except: | |
| assert False | |
| self.log.info(f"Expecting no InstantLock for {txid}") | |
| assert not node.getrawtransaction(txid, True)["instantlock"] |
?
Issue being fixed or feature implemented
feature_llmq_is_retroactive.pyis one of the slowest functional test and it has bunch of duplicated logic for "single node" and "all nodes" logic.What was done?
Refactored case of "single node" and "all nodes" to test simultaneously instead of doing 1-by-1
It reduced amount of code and extra delays and improvement performance.
This PR fixes feature_llmq_is_retroactive.py test by generating real extra quorum for instant send (previously wrong type of quorum has been generated). Issue has been introduced by refactor: deprecate non-deterministic IS support #5553 when only 1 helper mine_quorum has been replaced to proper rotation quorum.
This PR introduces a minor refactoring
mine_cycle_quorumto prevent a perf bug when 24*3 blocks generated again and again for sub-sequent quorums (they are needed only once) when this helper is misused.How Has This Been Tested?
Run 20 parallel jobs. Median time for
feature_llmq_is_retroactive.pyis decreased from 134s to just 103s on my localhost.Median time for
interface_zmq_dash.pygot 3seconds boost (54s -> 51s) as expected, see relatedmine_cycle_quorumhelper.Breaking Changes
N/A
Checklist: