test: stabilize asset locks functional test#7411
Conversation
|
✅ Review complete (commit 0b993eb) |
|
![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: 0b993eb9df
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expected_quorum_hash = self.nodes[0].quorum('list')['llmq_test_platform'][-1] | ||
| # Quorum selection depends on the request ID. Scan 100 candidates to | ||
| # avoid missing the target oldest quorum by chance. | ||
| for index in range(start_index, start_index + 100): |
There was a problem hiding this comment.
When this helper is called with start_index=501 or 601, the widened + 100 scan includes the hard-coded indexes reused later in the same test (520-522 and 620-622). If the first candidate that maps to the oldest quorum lands on one of those values, create_assetunlock() has already produced a recovered signature for that request ID; the later unlock with the same index but a different withdrawal has a different msgHash, so platformsign refuses it as a conflicting vote and the test times out/fails. Keep this search range disjoint from the later indexes or reserve the selected index.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
same as #7409 (comment): is there a reason the range was bumped this much? nvm, I get it now
WalkthroughThis PR refactors functional test code for asset-unlock quorum handling. In feature_asset_locks.py, new helper methods compute asset-unlock request IDs and locate the oldest matching platform quorum deterministically, replacing prior retry loops; generate_batch() gains an optional sync callback. In test_framework.py, a new node_has_quorum_commitment() helper is extracted from wait_for_quorum_commitment() and reused, and mine_quorum() now waits on a single node's commitment instead of all online masternodes. Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)Skipped: changes are test-only refactors of existing control flow (replacing retry loops with deterministic lookups, extracting a helper function) without new multi-component feature interactions. Estimated code review effort3 (Moderate) | ~20 minutes Related issues: None specified in the provided context. Related PRs: None specified in the provided context. Suggested labels: tests, refactor Suggested reviewers: None specified in the provided context. 🐰 A quorum once chased in a loop, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/test_framework/test_framework.py`:
- Around line 2203-2206: The quorum setup in mine_quorum still waits for every
mninfos_online node to receive the commitment before checking the miner, which
keeps the mining path gated on full DKG propagation. Remove the retained
all-masternode wait and make the flow in mine_quorum wait only for the mining
node’s commitment, using node_has_quorum_commitment on self.nodes[0] before
proceeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6507e04b-79b1-4d3a-aad8-7cf3aee42fec
📒 Files selected for processing (2)
test/functional/feature_asset_locks.pytest/functional/test_framework/test_framework.py
| self.wait_for_quorum_commitment(q, mninfos_online, llmq_type=llmq_type) | ||
|
|
||
| self.log.info("Waiting final commitment on mining node") | ||
| self.wait_until(lambda: self.node_has_quorum_commitment(self.nodes[0], q, llmq_type), timeout=15) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the retained all-masternode commitment wait.
Line 2203 still blocks on every mninfos_online node before Line 2206 checks the miner, so mine_quorum() remains gated on non-mining-node DKG propagation. For this stabilization path, wait only for the node that will build the commitment block.
Proposed fix
- self.log.info("Waiting final commitment")
- self.wait_for_quorum_commitment(q, mninfos_online, llmq_type=llmq_type)
-
self.log.info("Waiting final commitment on mining node")
self.wait_until(lambda: self.node_has_quorum_commitment(self.nodes[0], q, llmq_type), timeout=15)📝 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.
| self.wait_for_quorum_commitment(q, mninfos_online, llmq_type=llmq_type) | |
| self.log.info("Waiting final commitment on mining node") | |
| self.wait_until(lambda: self.node_has_quorum_commitment(self.nodes[0], q, llmq_type), timeout=15) | |
| self.log.info("Waiting final commitment on mining node") | |
| self.wait_until(lambda: self.node_has_quorum_commitment(self.nodes[0], q, llmq_type), timeout=15) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/functional/test_framework/test_framework.py` around lines 2203 - 2206,
The quorum setup in mine_quorum still waits for every mninfos_online node to
receive the commitment before checking the miner, which keeps the mining path
gated on full DKG propagation. Remove the retained all-masternode wait and make
the flow in mine_quorum wait only for the mining node’s commitment, using
node_has_quorum_commitment on self.nodes[0] before proceeding.
There was a problem hiding this comment.
Code Review
Source: reviewer lanes: opus (Claude general and dash-core-commit-history failed before JSON), gpt-5.5 (Codex general and dash-core-commit-history); verifier: gpt-5.5 (Codex, recovered after opus verifier failed).
Verified the PR at 0b993eb against upstream/develop. The changes are scoped to functional test stabilization and the added helpers preserve the existing asset-unlock request ID, quorum selection, and DKG commitment checks without introducing an in-scope defect.
| inode.invalidateblock(block_asset_unlock) | ||
| self.validate_credit_pool_balance(locked) | ||
| self.generate_batch(25) | ||
| self.generate_batch(25, sync_fun=lambda: self.sync_blocks()) |
There was a problem hiding this comment.
why this line is updated? how sync-mempools + sync-blocks is worse than just sync-blocks?
Is it for better performance? Or any other reason?
There was a problem hiding this comment.
Good question. This is intentional for the flake tracked in #7310: after invalidating block_asset_unlock, the disconnected asset-unlock txs can legitimately sit in different node mempools. This section only needs the replacement chain to advance and assert the credit-pool state stays at locked; mempool equality is not part of the invariant here.
The old default generate_batch() did sync_all(), and one of the recorded #7310 failures timed out exactly here in sync_mempools() with one node having txs and the others empty. Passing sync_blocks() keeps the chain-state synchronization needed by this reorg path without gating on nondeterministic mempool convergence.
There was a problem hiding this comment.
one of the recorded #7310 failures timed out exactly here in sync_mempools()
Passing sync_blocks() keeps the chain-state synchronization needed by this reorg path without gating on nondeterministic mempool convergence.
Failed sync of mempool probably points out to some broken connections between masternodes or consensus diversification ; otherwise mempools will be synced. And removing sync_mempools on this stage would just cause it hang on the later steps at some point.
I'd suggest to revert this change.
Otherwise, PR looks fine for me.
There was a problem hiding this comment.
I dug into this more, and I think the block-only sync here is intentional.
The important details are:
-
Right before this invalidation path, the test already does a normal
generate(...), which defaults tosync_all()(sync_blocks()+sync_mempools()). So the nodes should already be normally synced beforeinvalidateblock(). -
This invalidation is deep:
block_asset_unlockis roughly 46 blocks below the tip (HEIGHT_DIFF_EXPIRINGis 48, and the asset-unlock block is mined about two blocks after that height is recorded). -
Core only tries to resurrect disconnected transactions into the mempool for the first 10 disconnected blocks:
(++disconnected <= 10)The nearby comment says that for deep invalidation, keeping the mempool up to date is probably futile.
-
invalidateblock()is a local RPC path. Any tx resurrection is localAcceptToMemoryPool(...)cleanup; it is not a relay step that guarantees all peers converge to the same mempool.
So the thing this part of the test needs to prove is: after replacing the invalidated chain, all nodes accept the same blocks and keep the expected credit-pool state. sync_blocks() plus validate_credit_pool_balance(...) checks that. Requiring exact mempool equality immediately after a deep invalidation is asserting something Core does not guarantee.
The test does return to normal synchronization after reconsiderblock(): the later generate(...) uses default sync_all(), so mempool sync is not being skipped forever, only in the deep-invalidation window where it is not a reliable invariant.
I think the best follow-up is to keep this sync_blocks() call and add a short code comment explaining the deep-invalidation/mempool-resurrection reason.
4 participants
Issue being fixed or feature implemented
Fixes #7310.
feature_asset_locks.pycould fail intermittently under high concurrency while waiting for a freshly minedllmq_test_platformquorum or while syncing mempools during an asset-unlock reorg path.What was done?
mine_quorum().generate_batch()to use block-only sync for the asset-unlock invalidation padding where mempool equality is not part of the assertion.How Has This Been Tested?
upstream/developwithmake -j8.feature_asset_locks.pyruns.python3.9 test/functional/feature_asset_locks.py --tmpdir=/private/tmp/dash_assetlocks_fixed_solo2 --portseed=301.feature_asset_locks.pyinstances using seeds 100-119 with--timeout-factor=4; all 20 passed.Breaking Changes
None.
Checklist:
Supersedes closed PR #7409, which was accidentally closed during review-followup branch recovery.
This pull request was created by Codex.