test: fix remaining feature_asset_locks flake from autonomous MnEHF signal txs - #7509
test: fix remaining feature_asset_locks flake from autonomous MnEHF signal txs#7509PastaPastaPasta wants to merge 2 commits into
Conversation
mine_quorum() waited for the mining node to hold the final commitment only for the llmq type it was asked to mine. The block it 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 in time 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. Seen at block 178 of a failing feature_asset_locks.py run, where llmq_test got signers=0, validMembers=0, quorumPublicKey=0...0 while the driven llmq_test_instantsend commitment was mined correctly. Wait for the mining node to hold every commitment the masternodes actually produced. Types whose DKG legitimately produced nothing are not awaited, so this cannot hold up a test that expects a type not to form.
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 dashpay#7310 after dashpay#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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe asset-lock test adds Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Masternodes
participant DashTestFramework
participant MiningNode
Masternodes->>DashTestFramework: Provide commitment types
DashTestFramework->>MiningNode: Check quorum commitment types
MiningNode-->>DashTestFramework: Return non-null types
DashTestFramework->>DashTestFramework: Wait for all produced types
DashTestFramework->>MiningNode: Mine quorum after synchronization
🚥 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 |
|
⛔ Blockers found — Sonnet deferred (commit 03ff1ff) |
![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: 03ff1ffec9
ℹ️ 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 = set() | ||
| for mn in mninfos: | ||
| expected |= self.node_quorum_commitment_types(mn.get_node(self), quorum_hash) | ||
| return expected <= self.node_quorum_commitment_types(self.nodes[0], quorum_hash) |
There was a problem hiding this comment.
When an undriven LLMQ type's independent phase-handler thread is slower than the driven type, none of the queried masternodes may advertise that auxiliary commitment on the first poll. In that case expected contains only the driven commitment—which the miner already has—so this subset check succeeds immediately; the following block can still be mined before the slower handler adds and relays its real commitment, recreating the null-commitment/skipped-cycle behavior this change is intended to prevent. Establish that the auxiliary handlers have finished, or otherwise stabilize the expected type set, before accepting this condition.
AGENTS.md reference: AGENTS.md:L163-L165
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The MnEHF filtering correctly preserves asset-lock mempool accounting, but the new miner synchronization helper does not reliably discover every auxiliary commitment. Because auxiliary LLMQ handlers finalize independently, the predicate can succeed before a slower handler publishes its commitment, leaving the skipped-cycle race this PR is intended to fix.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/functional/test_framework/test_framework.py`:
- [BLOCKING] test/functional/test_framework/test_framework.py:2177-2181: Stabilize auxiliary commitment discovery before mining
This predicate can succeed before an auxiliary LLMQ handler has finalized. The preceding `wait_for_quorum_commitment()` only waits for the explicitly driven type, so the first poll here may find that type alone on every masternode and immediately confirm that the miner has it. Each LLMQ type runs in an independent phase-handler thread (`src/llmq/net_dkg.cpp:605-610`), and reaching phase 6 does not close the race: `WaitForNextPhase()` publishes the phase before control returns to `FinalizeCommitments()` and `AddMineableCommitment()` at `src/llmq/net_dkg.cpp:788-792`. A slower auxiliary handler can therefore add its real commitment just after this subset check returns, while the following block has already synthesized a null commitment for that type. Establish that all relevant auxiliary handlers have completed finalization before deriving the expected set, while retaining an explicit way to represent a completed DKG that legitimately produced no commitment.
| def check_miner_commitments(): | ||
| expected = set() | ||
| for mn in mninfos: | ||
| expected |= self.node_quorum_commitment_types(mn.get_node(self), quorum_hash) | ||
| return expected <= self.node_quorum_commitment_types(self.nodes[0], quorum_hash) |
There was a problem hiding this comment.
🔴 Blocking: Stabilize auxiliary commitment discovery before mining
This predicate can succeed before an auxiliary LLMQ handler has finalized. The preceding wait_for_quorum_commitment() only waits for the explicitly driven type, so the first poll here may find that type alone on every masternode and immediately confirm that the miner has it. Each LLMQ type runs in an independent phase-handler thread (src/llmq/net_dkg.cpp:605-610), and reaching phase 6 does not close the race: WaitForNextPhase() publishes the phase before control returns to FinalizeCommitments() and AddMineableCommitment() at src/llmq/net_dkg.cpp:788-792. A slower auxiliary handler can therefore add its real commitment just after this subset check returns, while the following block has already synthesized a null commitment for that type. Establish that all relevant auxiliary handlers have completed finalization before deriving the expected set, while retaining an explicit way to represent a completed DKG that legitimately produced no commitment.
source: ['codex']
2 participants
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:
wait_for_quorum_list()timeouts intest_v24_fork->mine_quorum_2_nodesall ran on PR heads that predate test: stabilize asset locks functional test #7411. ci: make conflict prediction advisory #7422's head
9f030351band backport: Merge bitcoin/bitcoin#26326: net: don't lock cs_main while reading blocks #7350'shead do not contain test: stabilize asset locks functional test #7411's merge
cbab2549bc4, and the fix(net): bound signing message vector intake #7418 report says soexplicitly. That failure mode is fixed.
test_asset_unlocks->check_mempool_result->
check_mempool_size->AssertionError: not(1 == 0)— still reproduces oncurrent
developwith test: stabilize asset locks functional test #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 blockmine_quorum()thengenerates carries a commitment for every LLMQ type whose mining window is
open, and in regtest all test types share
dkgInterval = 24, so they allfinalize 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 withoutbeing recorded as mined (
src/llmq/blockprocessor.cpp:322-331), so that quorumis silently skipped for the whole cycle.
Captured from a failing run, at final-commitment block 178, for the undriven
llmq_test:createdrather thancachedis the mining node synthesising a null commitmentbecause it held no real one.
mine_quorum()now waits for the mining node to hold every commitment themasternodes 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
llmqTypeMnhfisLLMQ_TEST(src/chainparams.cpp:929), so a skippedllmq_testquorum defers the one-shot V24 MnEHF signal transaction to a latercycle.
CEHFSignalsHandlersubmits it from the masternodes on their own(
src/llmq/ehf_signals.cpp:118) at a moment the test does not control, whilecheck_mempool_size()comparedgetmempoolinfo()['size']againstself.mempool_size, which only ever modelled the test's own transactions: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_untilon the mempool size: that would also pass if an asset-unlocktransaction 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/developatefe6dec7b9aon macOS arm64,configure --prefix=depends/aarch64-apple-darwin25.3.0 --disable-bench,make -j13.Sequential control:
feature_asset_locks.pypasses in 167 s, matching CI'spassing 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
--jobsdoes not amplify it —at
-j20the machine slows uniformly, which gives thellmq_testcommitmentmore 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()intest_asset_unlocks— the block that would otherwisesweep the MnEHF transaction. Same assertion, same transaction, same mechanism.
not(1 == 0)signatureA 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 byCEHFSignalsHandlerandaccepted 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 exceptfeature_protx_version,which failed at
test_revoke_protxwaiting forgetconnectioncount() == 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 failedat
-j8on a clean tree vs 3/8 with this PR, the sameline 241signature inevery case), and it is unrelated to quorum commitments.
test/lint/lint-python.pypasses.Breaking Changes
None. Test-only change.
Checklist: