Skip to content

fix: ensure test_verify_tee_expired_attestation_triggers_resharing is not flaky#1939

Merged
gilcu3 merged 1 commit intomainfrom
1900-test_verify_tee_expired_attestation_triggers_resharing-is-flaky
Feb 3, 2026
Merged

fix: ensure test_verify_tee_expired_attestation_triggers_resharing is not flaky#1939
gilcu3 merged 1 commit intomainfrom
1900-test_verify_tee_expired_attestation_triggers_resharing-is-flaky

Conversation

@gilcu3
Copy link
Contributor

@gilcu3 gilcu3 commented Feb 3, 2026

Closes #1900

@gilcu3 gilcu3 linked an issue Feb 3, 2026 that may be closed by this pull request
Copy link
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment on lines +598 to +599
// Add 100 blocks margin to account for block time variance and ensure attestation is
// reliably expired. This assumed that 100 blocks takes always more than `ATTESTATION_EXPIRY_SECONDS` seconds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we coming up with these estimations and how do they reflect reality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the de-facto estimation is 1 second per block, but the test was failing sometimes, probably due to exactly that. So here I am making sure we give it more time, and if that's not the case the failure should point that out.

Comment on lines +642 to +646
let block_info = worker.view_block().await?;
let current_timestamp = block_info.timestamp() / 1_000_000_000;
// Putting this assertion here such that if the test fails for this reason
// we already know
assert!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to bother, but could you explain to me what is happening so I can learn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test was failing, probably due to the blocks being executed much faster than expected. Now I test explicitly if what we expected, that the expiry time passed after a number of blocks, is correct. If in the future the test fails, but in another place, then we know that time was not the problem

Copy link
Contributor

@SimonRastikian SimonRastikian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@gilcu3 gilcu3 added this pull request to the merge queue Feb 3, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 3, 2026
@gilcu3
Copy link
Contributor Author

gilcu3 commented Feb 3, 2026

CI failed above, but it was another test tests::resharing::test_key_resharing_simple::case_4 so all "good"

@gilcu3 gilcu3 added this pull request to the merge queue Feb 3, 2026
Merged via the queue into main with commit f5bb280 Feb 3, 2026
8 checks passed
@gilcu3 gilcu3 deleted the 1900-test_verify_tee_expired_attestation_triggers_resharing-is-flaky branch February 3, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test_verify_tee_expired_attestation_triggers_resharing is flaky

3 participants