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.
check_mempool_size() compared getmempoolinfo()['size'] against self.mempool_size, which only models the transactions the test itself submits. Masternodes submit the one-shot V24 MnEHF signal transaction on their own as soon as a quorum able to sign it exists (src/llmq/ehf_signals.cpp:118), at a moment the test does not control. When that lands between the test's last generate() and a check expecting an empty mempool, the test fails with 'not(1 == 0)'. This is the failure mode still reported on #7310 after #7411 closed it, and it still reproduces on current develop. Count only the transactions this test submits. Deliberately not a wait_until on the mempool size: that would also pass if an asset unlock transaction wrongly lingered, which is what the assertion exists to catch.
…omous MnEHF signal txs 03ff1ff test: ignore MnEHF signal txs in asset lock mempool accounting (pasta) 1cea97f test: wait for every quorum commitment on the mining node (pasta) Pull request description: ## Issue being fixed or feature implemented Follow-up to #7411, which closed #7310. The same test kept failing after that closure, so the fix was only partial. Sorting the post-closure reports on #7310 by the tree they actually ran on splits them in two: - The `wait_for_quorum_list()` timeouts in `test_v24_fork` -> `mine_quorum_2_nodes` all ran on PR heads that predate #7411. #7422's head `9f030351b` and #7350's head do not contain #7411's merge `cbab2549bc4`, and the #7418 report says so explicitly. That failure mode is fixed. - The exact-mempool-count failure — `test_asset_unlocks` -> `check_mempool_result` -> `check_mempool_size` -> `AssertionError: not(1 == 0)` — still reproduces on current `develop` with #7411 present. This PR fixes the second one and the race that lets it happen. ## What was done? Two related changes. **1. `mine_quorum()` only waited for the commitment of the type it was mining.** #7411 added a wait for the mining node to hold the final commitment, but only for `llmq_type`, the type the caller asked for. The block `mine_quorum()` then generates carries a commitment for *every* LLMQ type whose mining window is open, and in regtest all test types share `dkgInterval = 24`, so they all finalize on the same block. A type whose real commitment has not reached the mining node yet is mined as a **null** commitment (`src/llmq/blockprocessor.cpp:842-846`); null commitments are accepted without being recorded as mined (`src/llmq/blockprocessor.cpp:322-331`), so that quorum is silently skipped for the whole cycle. Captured from a failing run, at final-commitment block 178, for the undriven `llmq_test`: ```text GetMineableCommitments cf height[178] content: { created nversion[3] quorumIndex[0] } ProcessCommitment -- processing commitment for block height=178, type=100, quorumHash=1ffd275a..., signers=0, validMembers=0, quorumPublicKey=000000000000...0000 ``` `created` rather than `cached` is the mining node synthesising a null commitment because it held no real one. `mine_quorum()` now waits for the mining node to hold every commitment the masternodes actually produced for that quorum hash, not just the driven type. Only commitments the masternodes already have are awaited, so a type whose DKG legitimately produced nothing cannot hold a test up. **2. `check_mempool_size()` asserted a global mempool count.** regtest's `llmqTypeMnhf` is `LLMQ_TEST` (`src/chainparams.cpp:929`), so a skipped `llmq_test` quorum defers the one-shot V24 MnEHF signal transaction to a later cycle. `CEHFSignalsHandler` submits it from the masternodes on their own (`src/llmq/ehf_signals.cpp:118`) at a moment the test does not control, while `check_mempool_size()` compared `getmempoolinfo()['size']` against `self.mempool_size`, which only ever modelled the test's own transactions: ```text node2 ... Special EHF TX is created hash=1feb94ce... node2 ... IsValidMNActivation: set MnEHF for bit=12 is valid (bit 12 = DEPLOYMENT_V24) node0 ... accepted 1feb94ce... (poolsz 1 txn, 0 kB) test ... AssertionError: not(1 == 0) ``` The count now excludes MnEHF signal transactions, so the assertion still says exactly what it said before about the transactions this test submits, and no longer depends on when the masternodes submit theirs. Deliberately not a `wait_until` on the mempool size: that would also pass if an asset-unlock transaction wrongly lingered, which is what the assertion exists to catch. `mine_cycle_quorum()` has the same gap and does not wait for commitments at all, but it drives rotated (dip0024) quorums and no failure in this family was traced to it, so it is left alone. ## How Has This Been Tested? Built from `upstream/develop` at `efe6dec7b9a` on macOS arm64, `configure --prefix=depends/aarch64-apple-darwin25.3.0 --disable-bench`, `make -j13`. Sequential control: `feature_asset_locks.py` passes in 167 s, matching CI's passing runtime. The natural rate of this flake is only ~3% locally, and it is gated by a discrete precursor rather than a timing window, so raising `--jobs` does not amplify it — at `-j20` the machine slows uniformly, which gives the `llmq_test` commitment *more* time to arrive and suppressed the precursor entirely (0/20). The before/after was therefore measured against a forced precursor: an experiment-only patch, not part of this PR, that removes the "Mine block to empty mempool" `generate()` in `test_asset_unlocks` — the block that would otherwise sweep the MnEHF transaction. Same assertion, same transaction, same mechanism. | Run | Result | | --- | --- | | 15x parallel, forced precursor, without this PR | **15/15 failed**, every one the `not(1 == 0)` signature | | 15x parallel, forced precursor, with this PR | **15/15 passed** | | 15x parallel, unforced, without this PR | 1/15 failed (the natural ~3% rate) | | 15x parallel, unforced, with this PR | 30/30 passed across 2 batches; a 3rd batch is excluded, see below | A third unforced 15x batch ran while the host was under an unrelated load spike (load average 145 on a 14-core machine, from desktop applications rather than the test run) and lost 14 of 15 copies to block-sync, mempool-sync, recovered-signature and RPC timeouts. Those are host-contention failures, not this signature, and are excluded from the table rather than counted as passes. Across all 45 unforced runs with this PR there were zero `not(1 == 0)` occurrences. In the forced-and-unfixed runs the single mempool entry was confirmed to be the MnEHF transaction, e.g. `872ca79e...` created by `CEHFSignalsHandler` and accepted on node0 at `poolsz 1 txn`. Because the framework change affects every test that mines quorums, all 15 such tests were run: `feature_dip4_coinbasemerkleroots`, `feature_llmq_connections`, `feature_llmq_data_recovery`, `feature_llmq_dkg_intake`, `feature_llmq_dkgerrors`, `feature_llmq_evo`, `feature_llmq_rotation`, `feature_llmq_signing`, `feature_llmq_simplepose` (both variants), `feature_mnehf`, `feature_notifications`, `feature_protx_version`, `p2p_instantsend`, `p2p_platform_ban`, `p2p_quorum_data`. All passed except `feature_protx_version`, which failed at `test_revoke_protx` waiting for `getconnectioncount() == 0` (feature_protx_version.py:241). That is the pre-existing #6702 flake, not a regression here: it reproduces identically on unmodified `develop` (1/8 failed at `-j8` on a clean tree vs 3/8 with this PR, the same `line 241` signature in every case), and it is unrelated to quorum commitments. `test/lint/lint-python.py` passes. ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: 02358a0fc4bfcc76fed5240f288d0c278818cad1ddd01ec8181f380c7b5c8cadbdc36c2ef4559c034b15c37989ed4214987945ae5de761b32b1eacabc15cab6c
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.