test: wait delays for IS, CL just 0.05s #6751
Conversation
It bumps mocktime properly for all instantsend lock
Dash Core sends transactions after random delay for privacy reason.
tx_relay->m_next_inv_send_time = GetExponentialRand(current_time, OUTBOUND_INVENTORY_BROADCAST_INTERVAL)
This expential random delay once in 1000 tries is longer than 2s * 10 = 20 seconds
To be sure that helper wait_for_instantsend is reliable need to bump mocktime for 20+ second,
or even better 30 seconds
… feature_llmq_is_retroactive
WalkthroughThis set of changes updates several functional test files to modify the handling of simulated time progression during test execution. Across multiple test cases, calls to ✨ 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
test/functional/feature_asset_locks.py(1 hunks)test/functional/feature_llmq_is_cl_conflicts.py(2 hunks)test/functional/feature_llmq_is_retroactive.py(7 hunks)test/functional/feature_llmq_singlenode.py(2 hunks)test/functional/feature_notifications.py(1 hunks)test/functional/interface_zmq_dash.py(3 hunks)test/functional/p2p_instantsend.py(4 hunks)test/functional/rpc_verifyislock.py(1 hunks)test/functional/test_framework/test_framework.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (8)
📓 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: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.331Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be implemented in src/test/, src/wallet/test/, and src/qt/test/ using Boost::Test or Qt 5 for GUI tests
test/functional/rpc_verifyislock.py (1)
Learnt from: kwvg
PR: dashpay/dash#6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
test/functional/feature_notifications.py (1)
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.
test/functional/feature_llmq_singlenode.py (1)
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.
test/functional/p2p_instantsend.py (3)
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: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
test/functional/feature_llmq_is_cl_conflicts.py (2)
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: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
test/functional/feature_llmq_is_retroactive.py (1)
Learnt from: kwvg
PR: dashpay/dash#6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
test/functional/test_framework/test_framework.py (1)
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 (5)
test/functional/rpc_verifyislock.py (1)
test/functional/test_framework/test_framework.py (1)
bump_mocktime(872-889)
test/functional/feature_asset_locks.py (1)
test/functional/test_framework/test_framework.py (1)
bump_mocktime(872-889)
test/functional/feature_llmq_singlenode.py (1)
test/functional/test_framework/test_framework.py (1)
bump_mocktime(872-889)
test/functional/feature_llmq_is_cl_conflicts.py (1)
test/functional/test_framework/test_framework.py (1)
bump_mocktime(872-889)
test/functional/interface_zmq_dash.py (1)
test/functional/test_framework/test_framework.py (1)
bump_mocktime(872-889)
🪛 Ruff (0.11.9)
test/functional/feature_llmq_is_retroactive.py
32-32: Do not use bare except
(E722)
test/functional/test_framework/test_framework.py
1936-1936: Do not use bare except
(E722)
🪛 Flake8 (7.2.0)
test/functional/feature_llmq_is_retroactive.py
[error] 32-32: do not use bare 'except'
(E722)
test/functional/test_framework/test_framework.py
[error] 1936-1936: do not use bare 'except'
(E722)
🔇 Additional comments (14)
test/functional/test_framework/test_framework.py (3)
1928-1931: Good documentation of the timing behavior change.The comment clearly explains the rationale behind the change and provides guidance on the recommended approach. This will help test developers understand why they need to manually bump mocktime before waiting for instant locks.
1943-1952: Approve the removal of sleep parameter from wait_for_chainlocked_block.The change simplifies the method by using the default sleep interval from
wait_until, which is consistent with the overall goal of standardizing wait behavior in the test framework.
1958-1959: Approve the removal of sleep parameter from wait_for_best_chainlock.Similar to the other wait methods, this change standardizes the sleep behavior and removes the explicit sleep parameter, making the method more consistent with the framework's default behavior.
test/functional/rpc_verifyislock.py (1)
41-41: LGTM: Proper timing adjustment for InstantSend testing.The addition of
bump_mocktime(30)after sending the transaction and before waiting for instantlock aligns with the PR objective to reduce random delays and improve test reliability. This change ensures deterministic timing behavior for InstantSend lock verification tests.test/functional/feature_llmq_singlenode.py (2)
107-107: LGTM: Consistent timing adjustment for InstantSend.The addition of
bump_mocktime(30)after sending funds and before waiting for instantlock ensures deterministic timing behavior in the single node quorum test. This change is consistent with the PR's objective to standardize InstantSend timing in tests.
185-185: LGTM: Proper timing for chainlock and InstantSend test.The
bump_mocktime(30)addition in the chainlock and InstantSend test section maintains consistency with the timing adjustments made throughout the codebase. This ensures reliable test execution when testing both chainlocks and InstantSend together.test/functional/feature_llmq_is_cl_conflicts.py (2)
99-99: LGTM: Proper timing for conflict handling test.The
bump_mocktime(30)addition after sending rawtx4 and before mempool synchronization ensures deterministic timing behavior in the chainlock override test. This change aligns with the PR's objective to standardize InstantSend timing across all test scenarios.
160-160: LGTM: Consistent timing adjustment for chainlock validation.The addition of
bump_mocktime(30)after sending rawtx5 maintains timing consistency in the chainlock override validation phase. This ensures reliable test execution when validating chainlock behavior with conflicting transactions.test/functional/feature_notifications.py (2)
135-136: LGTM: Timing adjustment and node selection for notification test.The addition of
bump_mocktime(30)ensures deterministic timing for InstantSend processing. The change fromself.nodes[1]toself.nodes[0]in thewait_for_instantlockcall appears intentional - waiting on the sender node while the receiver (node1) is configured with instantsendnotify. This aligns with the notification test setup.
139-139: LGTM: Additional timing for notification file processing.The
bump_mocktime(30)before waiting for notification files ensures sufficient time passes for both InstantSend processing and file system notification operations. This improves test reliability by providing adequate time for the notification mechanism to complete.test/functional/feature_asset_locks.py (1)
380-380: LGTM: Proper timing adjustment for asset unlock InstantSend test.The addition of
bump_mocktime(30)after sending the asset unlock transaction and before waiting for instantlock ensures deterministic timing behavior in the asset unlock test. This change is consistent with the PR's objective to standardize InstantSend timing across all test scenarios, including asset lock functionality.test/functional/p2p_instantsend.py (1)
45-45: LGTM: Standardized mock time advances for InstantSend testingThe addition of
bump_mocktime(30)calls before waiting for InstantLock confirmations aligns with the PR objective of making tests more deterministic and faster. This standardizes the simulation of network delays and ensures proper synchronization before assertions.Also applies to: 61-61, 105-105, 128-128, 135-135
test/functional/interface_zmq_dash.py (1)
296-296: LGTM: Consistent mock time advances for ZMQ InstantSend testingThe 30-second mock time bumps before waiting for InstantLock confirmations are consistent with the broader pattern across the test suite. This improves test determinism and synchronization with network delays.
Also applies to: 349-349, 383-383
test/functional/feature_llmq_is_retroactive.py (1)
52-52: LGTM: Consistent mock time advances for retroactive signing testsThe 30-second mock time bumps before waiting for transactions and InstantLock confirmations improve test determinism and align with the PR objective of standardizing timing in InstantSend tests.
Also applies to: 73-73, 83-83, 116-116, 155-155, 195-195, 198-198
|
|
||
| def check_instantlock(): | ||
| self.bump_mocktime(1) | ||
| try: | ||
| return node.getrawtransaction(txid, True)["instantlock"] | ||
| except: | ||
| return False | ||
|
|
||
| self.log.info(f"Expecting InstantLock for {txid}") | ||
| if self.wait_until(check_instantlock, timeout=timeout, sleep=1, do_assert=expected) and not expected: | ||
| if self.wait_until(check_instantlock, timeout=timeout, do_assert=expected) and not expected: | ||
| raise AssertionError("waiting unexpectedly succeeded") | ||
|
|
There was a problem hiding this comment.
Approve the simplified wait_for_instantlock method with one improvement needed.
The removal of internal mocktime bumping aligns with the PR objective to make tests more deterministic. However, the bare except clause should be made more specific for better error handling.
Apply this diff to make the exception handling more specific:
- except:
+ except Exception:Or even better, catch the specific exception you expect:
- except:
+ except JSONRPCException:📝 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 check_instantlock(): | |
| self.bump_mocktime(1) | |
| try: | |
| return node.getrawtransaction(txid, True)["instantlock"] | |
| except: | |
| return False | |
| self.log.info(f"Expecting InstantLock for {txid}") | |
| if self.wait_until(check_instantlock, timeout=timeout, sleep=1, do_assert=expected) and not expected: | |
| if self.wait_until(check_instantlock, timeout=timeout, do_assert=expected) and not expected: | |
| raise AssertionError("waiting unexpectedly succeeded") | |
| def check_instantlock(): | |
| try: | |
| return node.getrawtransaction(txid, True)["instantlock"] | |
| except Exception: | |
| return False |
🧰 Tools
🪛 Ruff (0.11.9)
1936-1936: Do not use bare except
(E722)
🪛 Flake8 (7.2.0)
[error] 1936-1936: do not use bare 'except'
(E722)
🤖 Prompt for AI Agents
In test/functional/test_framework/test_framework.py around lines 1932 to 1942,
the bare except clause in the check_instantlock function should be replaced with
a more specific exception to improve error handling. Identify the specific
exception type that node.getrawtransaction might raise when the transaction is
not found or invalid, and catch only that exception instead of a bare except.
This will make the code more robust and avoid masking unexpected errors.
| # random delay before tx is actually send by network could take up to 30 seconds | ||
| def wait_for_tx(self, txid, node, expected=True, timeout=60): | ||
| def check_tx(): | ||
| try: | ||
| return node.getrawtransaction(txid) | ||
| except: | ||
| return False | ||
| if self.wait_until(check_tx, timeout=timeout, do_assert=expected) and not expected: | ||
| raise AssertionError("waiting unexpectedly succeeded") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the bare except clause in the wait_for_tx helper method
The bare except clause should be replaced with a more specific exception to improve code quality and debugging.
Apply this diff to fix the exception handling:
- def wait_for_tx(self, txid, node, expected=True, timeout=60):
- def check_tx():
- try:
- return node.getrawtransaction(txid)
- except:
- return False
- if self.wait_until(check_tx, timeout=timeout, do_assert=expected) and not expected:
- raise AssertionError("waiting unexpectedly succeeded")
+ def wait_for_tx(self, txid, node, expected=True, timeout=60):
+ def check_tx():
+ try:
+ return node.getrawtransaction(txid)
+ except Exception:
+ return False
+ if self.wait_until(check_tx, timeout=timeout, do_assert=expected) and not expected:
+ raise AssertionError("waiting unexpectedly succeeded")📝 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.
| # random delay before tx is actually send by network could take up to 30 seconds | |
| def wait_for_tx(self, txid, node, expected=True, timeout=60): | |
| def check_tx(): | |
| try: | |
| return node.getrawtransaction(txid) | |
| except: | |
| return False | |
| if self.wait_until(check_tx, timeout=timeout, do_assert=expected) and not expected: | |
| raise AssertionError("waiting unexpectedly succeeded") | |
| # random delay before tx is actually send by network could take up to 30 seconds | |
| def wait_for_tx(self, txid, node, expected=True, timeout=60): | |
| def check_tx(): | |
| try: | |
| return node.getrawtransaction(txid) | |
| except Exception: | |
| return False | |
| if self.wait_until(check_tx, timeout=timeout, do_assert=expected) and not expected: | |
| raise AssertionError("waiting unexpectedly succeeded") |
🧰 Tools
🪛 Ruff (0.11.9)
32-32: Do not use bare except
(E722)
🪛 Flake8 (7.2.0)
[error] 32-32: do not use bare 'except'
(E722)
🤖 Prompt for AI Agents
In test/functional/feature_llmq_is_retroactive.py around lines 27 to 35, replace
the bare except clause in the wait_for_tx method with a specific exception, such
as catching the exception type raised by node.getrawtransaction when the
transaction is not found (e.g., JSONRPCException or a relevant exception). This
improves error handling clarity and debugging by avoiding catching all
exceptions indiscriminately.
|
Very nice! I like this find. |
3 participants
Issue being fixed or feature implemented
Dash Core sends transactions after random delay for privacy reason.
This expential random delay could be long as 2s * 10 = 20 seconds or even longer sometimes.
What was done?
Prior work: #6631
To be sure that helper wait_for_instantsend is reliable need to bump mocktime for 20+ second, or even better 30 seconds.
It fixes potential instability in functional tests and make them more deterministic and faster to run.
Decreased delay for ChainLocks also.
How Has This Been Tested?
Run functional tests several times; no new failures are noticed.
Minor performance improvement for functional tests that use
wait_for_instantlock:Breaking Changes
N/A
Checklist: