fix(node): retry attestation submission only until the next resubmission tick - #4281
Conversation
|
On hold until #3746 is addressed, as requested in #4280 (comment). Removing |
|
#3746 is merged, so this should be unblocked. There are conflicts to resolve though |
…tion-submission-retry Resolve the conflict in the attestation submission module in favour of main: #4282 removed monitor_attestation_removal and moved the tick abstraction into its own module, superseding this branch's changes there.
…ion tick A submission could be retried for up to 12 hours, so a late success stored a quote that old even though the periodic task had generated fresher ones in the meantime. Cap the retry window at the resubmission interval instead: once it elapses, the next tick supersedes the attempt with a freshly generated attestation. generate_and_submit no longer reports whether it reached the contract; its only consumer was monitor_attestation_removal, removed in #4282. Closes #4280 Closes #3747
…tion-submission-retry
…l after it starts The retry window was one interval measured from the moment the submission started, so attestation generation time was added on top of it. A slow PCCS collateral fetch pushed the retry past the tick it belonged to, and a late success stored a correspondingly older quote. The ticker now reports when the next round falls due, derived from the interval's own period, and that instant bounds the submission directly. Generation stays outside it, so a generation failure still waits for the next tick.
…tion-submission-retry
The acceptance criterion "a generation failure just waits for the next hourly tick" was unverified: attestation generation could not be faulted in a test, because TeeAuthority::Local always succeeds and only the Dstack variant does fallible I/O. Inject generation through a GenerateAttestation trait, mirroring the ReadAttestationExpiry seam next to it but generic rather than boxed, so a test double can fail every attempt and count them. The new test drives the loop with a single scheduled round and shows it runs exactly one, submits nothing, and starts the next only once the ticker yields another.
Pull request overviewReplaces the fixed 12-hour attestation-submission retry window with a deadline derived from the resubmission cadence, so a failing round stops retrying exactly when the next round falls due instead of overrunning eleven ticks. Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
Nothing to flag on secrets, panics in production paths, or the |
haiyuechen-nearone
left a comment
haiyuechen-nearone
left a comment
There was a problem hiding this comment.
@pbeza please take a look at the blocking finding from Claude. I will be happy to approve if we added a hard stop at the outer layer to capture the whole process.
I suggest that we add a timeout option for the dstack rpc call as well, but that is not blocking as it is adjacent to the issue fixed.
gilcu3
left a comment
gilcu3
left a comment
There was a problem hiding this comment.
Added some comments, and requested a change in line with @haiyuechen-nearone's review, as apparently the generation path can take forever, which would affect what this PR is trying to achieve.
Design docs record what was decided at the time and are not kept in sync with the code once the design has shipped, so this branch's edits to the retry window and the attestation-removal references are reverted.
…tion-submission-retry
|
PR title type suggestion: The phrase "only until the next resubmission tick" suggests a behavioral change to retry logic rather than a pure restructuring. Consider using |
Only the submission was bounded, so a round could run past its deadline in generation and, because tokio reports the instant a tick was scheduled for rather than the current time, a round overrunning by more than one period was handed a deadline already in the past. The submission then gave up without a single retry while still logging that it had exhausted its attempts. Derive the deadline from the tick's own start instead, and apply it to generation and to the pre-submit expiry read as well, so every await in a round shares one absolute deadline. Generation timeouts get their own metric label, since they need a different response than a generation error. The submission keeps bounding itself rather than being wrapped from outside: a timeout there would drop the future, stranding a signed transaction that the sender has already handed to a detached task, and skipping the counter that records the failure.
pbeza
changed the title
…threading a deadline The loop now cancels a round with one timeout instead of passing a deadline into generate_and_submit, and the Tick trait returns to being a pure tick. The round publishes the stage it is in through a watch channel, so a timed-out round is logged as a warning and counted in mpc_tee_attestation_round_timeouts_total under the stage that was running when the round was cut off.
…tion-submission-retry
…in the loop generate_and_submit is split into the three stage methods and the loop drives them directly, each bounded by timeout_at against one loop-local deadline. The stage enum, the watch channel, and the round timeout parameter are gone; each timeout site names its own stage label.
|
@gilcu3 I followed your suggestion (thx for it!). It feels more idiomatic now. Let me know if this is how you pictured it, and feel free to re-review when you get a chance. Also, @haiyuechen-nearone PTAL when you get a chance. Thanks, both! |
4 participants
Closes #4280
Closes #3747