fix: deflake //rs/tests/consensus/tecdsa:tecdsa_key_rotation_test#9674
fix: deflake //rs/tests/consensus/tecdsa:tecdsa_key_rotation_test#9674basvandijk wants to merge 1 commit intomasterfrom
Conversation
The test was flaky because setting max_parallel_pre_signature_transcripts_in_creation to 0 could create a race condition: if a key rotation occurred before the config change propagated, the execution layer's stash would contain pre-signatures with the new key transcript, but no new pre-signatures would ever be delivered (since creation was disabled). This prevented the stale-key-transcript purge mechanism from firing, leaving the stash stuck at 5 instead of draining to 0. Fix by setting max_parallel to 1 and pre_signatures_to_create_in_advance to 1. This keeps creation active so new pre-signatures with the rotated key transcript are delivered, triggering the purge of stale entries. The test now awaits a stash size of 1 (matching the new target) instead of 0.
There was a problem hiding this comment.
Pull request overview
Deflakes the //rs/tests/consensus/tecdsa:tecdsa_key_rotation_test by avoiding a configuration state that can prevent post-rotation pre-signature deliveries (and therefore prevent stale-stash purging), which previously could cause the test to hang until timeout.
Changes:
- Adjusts chain-key pre-signature config during the test to keep creation enabled at a low rate (
max_parallel=1) and reduce the target stash size (pre_signatures_to_create_in_advance=1). - Updates the post-rotation assertion to expect a stash size of
1instead of0. - Updates inline comments to explain the rationale behind the new configuration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -56,13 +56,15 @@ fn test(test_env: TestEnv) { | |||
| .await; | |||
| // Stash size should be 5 before the roation | |||
There was a problem hiding this comment.
Typo in comment: "roation" should be "rotation" (also consider updating the comment to reflect the current test intent, since the stash size is later reduced).
| // Stash size should be 5 before the roation | |
| // Stash size should initially be 5 before key rotation |
| set_pre_signature_stash_size( | ||
| &governance, | ||
| app_subnet.subnet_id, | ||
| key_ids.as_slice(), | ||
| /* max_parallel_pre_signatures */ 0, | ||
| /* max_stash_size */ 5, | ||
| /* max_parallel_pre_signatures */ 1, |
There was a problem hiding this comment.
After updating the chain key config via set_pre_signature_stash_size, the test proceeds without waiting for the new target stash size/config to take effect. This can make the test more timing-dependent (config propagation vs. key rotation). Consider awaiting the stash size to reach 1 immediately after the proposal (similar to pre_signature_stash_management_test.rs) before continuing with the rotation assertions.
Problem
The test
//rs/tests/consensus/tecdsa:tecdsa_key_rotation_testwas flaky, timing out at 600s. The stash was stuck at 5 instead of draining to 0.Root Cause
The test set
max_parallel_pre_signature_transcripts_in_creation=0to stop pre-signature creation, then expected the stash to drain to 0 after key rotation. However, this created a race condition:max_parallel=0, no new pre-signatures would ever be delivered.Fix
max_parallel_pre_signature_transcripts_in_creation=1(instead of 0) to keep creation active at a low ratepre_signatures_to_create_in_advance=1(instead of 5) to reduce the target stash sizeThis ensures that after key rotation, new pre-signatures with the rotated key transcript are created and delivered, triggering the purge of stale entries. The test still validates that key rotation works correctly and the stash stabilizes at the configured target.
Validation
Both
tecdsa_key_rotation_testandtecdsa_key_rotation_test_head_nnspass locally.