fix(host): bound preimage hint retries instead of looping forever#2597
fix(host): bound preimage hint retries instead of looping forever#2597BrianBland wants to merge 1 commit intomainfrom
Conversation
🟡 Heimdall Review Status
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
| INITIAL_HINT_RETRY_BACKOFF, MAX_HINT_ATTEMPTS, MAX_HINT_RETRY_BACKOFF, OfflineHostBackend, | ||
| OnlineHostBackend, |
There was a problem hiding this comment.
These constants (MAX_HINT_ATTEMPTS, INITIAL_HINT_RETRY_BACKOFF, MAX_HINT_RETRY_BACKOFF) are only used internally by OnlineHostBackend::get_preimage. Re-exporting them from the crate root makes them part of the public API, which means changing a retry tuning parameter becomes a semver-breaking change.
Consider keeping them pub only within the module (or pub(crate)) and removing them from the re-exports here and in backend/mod.rs, unless there's a downstream consumer that needs to reference them (e.g. for configuration or testing from another crate).
c89fd6f to
c1833db
Compare
| if preimage.is_some() { | ||
| return preimage.ok_or(PreimageOracleError::KeyNotFound); |
There was a problem hiding this comment.
Nit: We just confirmed preimage.is_some() on line 90, so the ok_or(KeyNotFound) on line 91 is dead code — it can never produce Err. Same pattern on line 119-120 inside the retry loop.
Consider using Ok(preimage.expect("checked is_some")) or restructuring with if let Some(data) = preimage { return Ok(data); } to make the intent clearer and avoid surfacing a misleading KeyNotFound variant in a path that can never fire.
OnlineHostBackend::get_preimage's previous loop had no retry cap, no backoff, and no error path: a persistently-failing handle_hint pinned the PreimageServer task indefinitely, and a missing hint with the key absent from the KV produced an empty spin. Cap retries at MAX_HINT_ATTEMPTS with exponential backoff between attempts, propagate the last error after exhaustion, and fail fast with a clear error when no hint has been routed at all. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c1833db to
688500a
Compare
Review SummaryThe fix correctly addresses both infinite-loop paths in
No issues found. The implementation is clean:
|
Summary
OnlineHostBackend::get_preimagehad two infinite-loop paths: acontinueafter everyhandle_hintfailure with no cap or backoff, and an empty spin when no hint was routed and the key was absent from the KV. Either pinned thePreimageServertask forever, hangingHost::build_witnessfor the fullPROVER_TIMEOUTand burning CPU + RPC budget.MAX_HINT_ATTEMPTS = 5with exponential backoff (50 ms → 1 s), propagate the last error after exhaustion, and fail fast with a clear error when no hint has been routed.