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
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughAdded three helper methods to test/functional/feature_llmq_is_retroactive.py: check_no_is, sleep_and_check_no_is, and create_fund_sign_tx. Replaced direct InstantLock waits with sleep_and_check_no_is across test flows. Consolidated two session-timeout tests into a single test_session_timeout(do_cycle_llmqs) that handles both all-nodes and single-node transaction propagation; removed cycle_llmqs, test_all_nodes_session_timeout, and test_single_node_session_timeout. Replaced cycle_llmqs calls with mine_cycle_quorum and timing adjustments; final block assertions now require both txid_all_nodes and txid_single_node. Updated BitcoinTestFramework.wait_for_instantlock signature to remove the expected parameter (now wait_for_instantlock(self, txid, node, timeout=60)). Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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.
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)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
test/functional/feature_llmq_is_retroactive.py (5)
27-29: Assert-based check is good; add a message to aid debugging on failureProviding an assertion message makes failures easier to triage in CI logs.
- assert not node.getrawtransaction(txid, True)["instantlock"] + assert not node.getrawtransaction(txid, True)["instantlock"], f"InstantLock unexpectedly present for {txid}"
31-33: Prefer mocktime bump over real sleep to reduce test runtimeAdvance mocktime to trigger schedulers instead of sleeping wall-clock time. Keeps tests fast and less flaky.
- time.sleep(sleep) - self.check_no_is(txid, node) + self.bump_mocktime(sleep) + self.check_no_is(txid, node)Note: If you’ve observed any behavior here that strictly requires wall-clock time (e.g., network backoffs not tied to scheduler), keep the sleep — otherwise mocktime is usually sufficient.
149-158: Avoid overwriting txid_all_nodes; assert both broadcasts produce the same txidOverwriting the variable obscures intent. Capture both txids (they should match) and assert equality for clarity.
- txid_all_nodes = self.nodes[0].sendrawtransaction(rawtx) - txid_all_nodes = self.nodes[3].sendrawtransaction(rawtx) + txid_all_nodes = self.nodes[0].sendrawtransaction(rawtx) + txid_all_nodes_3 = self.nodes[3].sendrawtransaction(rawtx) + assert txid_all_nodes_3 == txid_all_nodes
173-173: Optionally assert the returned txid to match txid_single_nodeNot strictly required, but helps catch accidental payload mismatches.
- self.nodes[0].sendrawtransaction(rawtx_1) + # Should be the same txid as seen by node 3 + assert self.nodes[0].sendrawtransaction(rawtx_1) == txid_single_nodeCaveat: If sendrawtransaction ever returns an error when the tx is already in mempool, keep as-is (or wrap in try/except and assert via getrawtransaction).
187-193: Pass is_first=False to avoid re-mining 24*3 extra blocks per cycle; prefer mocktime over sleepmine_cycle_quorum adds 24*3 blocks only for the first cycle (is_first=True). Subsequent calls here should set is_first=False to avoid the performance hit, which aligns with the PR’s goal of reducing test time. Also, mocktime is preferable to wall-clock sleep.
- self.mine_cycle_quorum() - self.mine_cycle_quorum() + self.mine_cycle_quorum(is_first=False) + self.mine_cycle_quorum(is_first=False) self.wait_for_chainlocked_block_all_nodes(self.nodes[0].getbestblockhash(), timeout=30) - - time.sleep(5) + self.bump_mocktime(5) self.check_no_is(txid_all_nodes, self.nodes[0]) self.check_no_is(txid_single_node, self.nodes[0])Reference: test_framework.mine_cycle_quorum has extra_blocks = 24 * 3 if is_first else 0; calling with default True repeatedly reintroduces the initial overhead.
📜 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 (1)
test/functional/feature_llmq_is_retroactive.py(7 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/feature_llmq_is_retroactive.py
🧠 Learnings (3)
📓 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.
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.
📚 Learning: 2025-08-19T18:53:02.819Z
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.
Applied to files:
test/functional/feature_llmq_is_retroactive.py
📚 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/feature_llmq_is_retroactive.py
🧬 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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (7)
test/functional/feature_llmq_is_retroactive.py (7)
45-49: Helper looks correct and improves readabilityCleanly encapsulates fund+sign flow using node0’s wallet. Good reuse later in the test.
67-67: Correct use of the negative IS helpersCalls correctly verify absence of InstantLock at the right points. Argument order is correct now.
Also applies to: 105-105, 136-136
143-146: Nice consolidation of the two session-timeout flowsRunning both paths via a single entry point reduces duplication while keeping semantics clear.
161-163: Propagation checks look goodWaiting for the “all nodes” tx on nodes 1 and 2 before proceeding avoids races.
176-177: Good: validating propagation of the “single-node” tx after reconnectEnsures the second path is covered deterministically.
181-185: LGTM: negative assertions enforce the “no IS after timeout” expectationThis properly fails on regressions since check_no_is asserts internally.
197-198: Final block assertions are preciseExplicitly checking that both txs made it into the block is the right verification for the retroactive path.
knst
force-pushed
the
test-is-retroactive
branch
from
5be6fe2 to
7336573
Compare
Force-pushed to remove one accidental commit used for testing (3e3c5ad) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/functional/feature_llmq_is_retroactive.py (1)
183-185: Good: negative post-timeout checks now enforcedThese calls enforce the “no IS after signing session timeout” expectation via the internal assert—addresses the earlier concern about ignored return values.
🧹 Nitpick comments (5)
test/functional/feature_llmq_is_retroactive.py (5)
31-33: Nit: rename parameter from “sleep” to “delay_secs” (or similar)Minor readability tweak to avoid shadowing the concept of “sleep” and to better convey units. No behavior change.
Apply this minimal rename:
-def sleep_and_check_no_is(self, txid, node, sleep): - time.sleep(sleep) +def sleep_and_check_no_is(self, txid, node, delay_secs): + time.sleep(delay_secs) self.check_no_is(txid, node)
152-155: Avoid reassigning txid_all_nodes; send to node 3 without overwriting for clarityReassigning the same variable can confuse readers into thinking a different txid is intended. Keep the original txid and just submit the raw tx on node 3.
Apply:
rawtx = self.create_fund_sign_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) +# Ensure node 3 also has the TX while isolated (same txid) +self.nodes[3].sendrawtransaction(rawtx)Optionally, add a sanity check:
assert txid_all_nodes == self.nodes[3].sendrawtransaction(rawtx)
161-163: Confirm intent: does “all nodes” need to include node 4 (high minrelay)?You verify propagation to nodes 1 and 2. If the “all nodes” path is meant to include the high-min-relay node 4, consider waiting for it as well or clarify naming/comments.
181-182: Optional: replace fixed sleep with a condition where possibleIf this ever flakes, consider waiting on a concrete condition (e.g., mempool presence across nodes or a specific RPC state) instead of a fixed 5s wall-clock sleep. Not blocking.
36-44: Catch specific RPC error instead of bare except in wait_for_txBare except can hide programming errors; prefer JSONRPCException (and only that) here. This keeps scope tight and aligns with prior guidance.
Add the import near the top of this file:
from test_framework.authproxy import JSONRPCExceptionThen narrow the exception:
def wait_for_tx(self, txid, node, expected=True, timeout=60): def check_tx(): try: return node.getrawtransaction(txid) - except: + except JSONRPCException: return False
📜 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 (1)
test/functional/feature_llmq_is_retroactive.py(7 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/feature_llmq_is_retroactive.py
🧠 Learnings (3)
📓 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.
Learnt from: knst
PR: dashpay/dash#6824
File: test/functional/feature_llmq_is_retroactive.py:34-37
Timestamp: 2025-08-19T18:53:02.863Z
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.
📚 Learning: 2025-08-19T18:53:02.863Z
Learnt from: knst
PR: dashpay/dash#6824
File: test/functional/feature_llmq_is_retroactive.py:34-37
Timestamp: 2025-08-19T18:53:02.863Z
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.
Applied to files:
test/functional/feature_llmq_is_retroactive.py
📚 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/feature_llmq_is_retroactive.py
🧬 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(1957-1959)isolate_node(777-779)mine_cycle_quorum(2199-2323)test/functional/test_framework/util.py (1)
set_node_times(520-523)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (9)
test/functional/feature_llmq_is_retroactive.py (9)
27-29: Assert-based “no IS” check is correct and conciseThis aligns with the earlier suggestion to assert directly on the InstantLock flag. Keeps failures crisp and avoids masking unexpected exceptions.
45-49: Helper to create/fund/sign TX looks goodSelf-contained, wallet-backed TX construction keeps the test logic clear.
67-67: Good: enforcing the “no IS before block” expectation via helperThese calls meaningfully exercise the negative path and will fail on regressions due to the internal assert.
Also applies to: 105-105, 136-136
114-114: LGTM: reusing the new create_fund_sign_tx helperReduces duplication and ensures consistent fee/serialization.
143-147: Consolidation into test_session_timeout() is a clear improvementRunning the “single node” and “all nodes” paths via one entry point reduces duplicate waits/sleeps and improves test time.
149-149: LGTM: deterministic setup inside test_session_timeoutCalling set_node_times() upfront helps keep mocktime in sync after earlier tests.
173-177: LGTM: delayed broadcast of the single-node TX to the rest of the networkMatches the scenario where a previously isolated TX becomes known post-reconnect; subsequent waits ensure proper propagation.
187-193: LGTM: double quorum cycle and re-checksCycling LLMQs twice and reasserting “no IS” is a solid guard against retroactive signing sneaking in after rotation.
197-198: Final block assertions are preciseAsserting both txids are in the mined block makes the test outcomes unambiguous.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
test/functional/feature_llmq_is_retroactive.py (5)
38-41: Avoid bare except in wait_for_tx; catch JSONRPCException explicitly.Catching all exceptions can hide real programming errors unrelated to “tx not found.” Narrow the exception and add the missing import to improve debuggability.
Apply this diff near the imports and in wait_for_tx:
@@ -import time +import time +from test_framework.authproxy import JSONRPCException @@ - def check_tx(): - try: - return node.getrawtransaction(txid) - except: - return False + def check_tx(): + try: + return node.getrawtransaction(txid) + except JSONRPCException: + return False
149-155: Don’t reassign txid_all_nodes on the second broadcast; it’s the same tx.Reassigning the same variable from node 3 is confusing and risks masking subtle mismatches. Keep the first assignment and just ensure node 3 also accepts the 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) + # also put it into node3's mempool while it's isolated + self.nodes[3].sendrawtransaction(rawtx)
173-173: Make sendrawtransaction idempotent wrt “already in mempool/known”.After reconnection, tx relay might race you; sendrawtransaction can error if the tx is already known. Tolerate that specific case to avoid flaky failures.
- self.nodes[0].sendrawtransaction(rawtx_1) + try: + self.nodes[0].sendrawtransaction(rawtx_1) + except JSONRPCException as e: + # tolerate duplicates if propagation raced us + if "already in mempool" not in str(e) and "transaction already known" not in str(e): + raise
164-165: Prefer wait_until/bump_mocktime over fixed sleeps to reduce flakiness.Fixed sleeps can be either too short (flaky) or longer than needed (slow). Where feasible, prefer:
- wait_until with a concrete predicate (e.g., all expected signers have voted or mempool contains tx on specific nodes), or
- self.bump_mocktime(...) which also advances schedulers in this framework.
Given the PR goal to speed up tests, this could shave a few seconds without altering semantics.
Also applies to: 167-167, 181-181, 191-191
187-190: Minor: replace two mine_cycle_quorum() calls with a small loop.Keeps intention obvious and avoids copy-paste.
- self.mine_cycle_quorum() - self.mine_cycle_quorum() + for _ in range(2): + self.mine_cycle_quorum()
📜 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 (1)
test/functional/feature_llmq_is_retroactive.py(7 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/feature_llmq_is_retroactive.py
🧠 Learnings (3)
📓 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.
Learnt from: knst
PR: dashpay/dash#6824
File: test/functional/feature_llmq_is_retroactive.py:34-37
Timestamp: 2025-08-19T18:53:02.863Z
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.
📚 Learning: 2025-08-19T18:53:02.863Z
Learnt from: knst
PR: dashpay/dash#6824
File: test/functional/feature_llmq_is_retroactive.py:34-37
Timestamp: 2025-08-19T18:53:02.863Z
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.
Applied to files:
test/functional/feature_llmq_is_retroactive.py
📚 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/feature_llmq_is_retroactive.py
🧬 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(1957-1959)isolate_node(777-779)mine_cycle_quorum(2199-2323)test/functional/test_framework/util.py (1)
set_node_times(520-523)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: predict_conflicts
- GitHub Check: check_merge
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (5)
test/functional/feature_llmq_is_retroactive.py (5)
27-34: Solid assert-style helpers; contract is clear and consistent.The assert-first design for check_no_is and the convenience wrapper sleep_and_check_no_is (with a sensible default) look good and align with the earlier discussion that these helpers should not return values.
45-49: Helper to create/fund/sign tx reduces duplication.Good consolidation. This keeps the test flow readable and avoids copy-pasting RPC sequences.
67-67: Consistent use of sleep_and_check_no_is at call sites.These call sites now correctly rely on the helper’s internal assertion, which will fail the test if an IS lock appears unexpectedly.
Also applies to: 105-105, 136-136
143-147: Nice consolidation: single test_session_timeout toggled with do_cycle_llmqs.This removes duplication while keeping both scenarios covered. The logging makes it easy to spot which path failed.
197-198: Good: assert both txids are included in the block.This enforces the intended coverage for both “all nodes” and “single node” cases in the unified test.
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: