-
Notifications
You must be signed in to change notification settings - Fork 96
improve: add a vkey check to helios finalizer
#2575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
a58558c
b061107
b2ac53b
503c1d3
daf7cea
2e09999
33f767b
e1fb73a
600cdad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,3 +15,12 @@ export function retryAsync<T, U extends unknown[]>( | |
| } | ||
| return ret; | ||
| } | ||
|
|
||
| // Exponential backoff with jitter and a cap | ||
| export function backoffWithJitter(retry: number, baseDelayMs = 50, backoffExponentBase = 2, maxDelayMs = 5000) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 now we can use this throughout the repo |
||
| const baseDelay = baseDelayMs * backoffExponentBase ** retry; | ||
| const jitter = (0.5 - Math.random()) * baseDelay; | ||
| const delay = baseDelay + jitter; | ||
| const base = Math.min(delay, maxDelayMs); | ||
| return base + jitter; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,15 @@ | ||
| import { BigNumber, ethers } from "."; | ||
| import { ApiProofRequest, PROOF_OUTPUTS_ABI_TUPLE, ProofOutputs } from "../interfaces/ZkApi"; | ||
| import axios from "axios"; | ||
| import { backoffWithJitter, BigNumber, ethers } from "."; | ||
| import { | ||
| ApiProofRequest, | ||
| PROOF_OUTPUTS_ABI_TUPLE, | ||
| ProofOutputs, | ||
| ProofStateResponse, | ||
| ProofStateResponseSS, | ||
| VkeyResponse, | ||
| VkeyResponseSS, | ||
| } from "../interfaces/ZkApi"; | ||
| import { create } from "superstruct"; | ||
|
|
||
| /** | ||
| * Calculates the deterministic Proof ID based on the request parameters. | ||
|
|
@@ -52,3 +62,69 @@ export function decodeProofOutputs(publicValuesBytes: string): ProofOutputs { | |
| })), | ||
| }; | ||
| } | ||
|
|
||
| export async function getProofStateWithRetries( | ||
| apiBaseUrl: string, | ||
| proofId: string, | ||
| maxAttempts = 3 | ||
| ): Promise<ProofStateResponse | 404> { | ||
| let attempt = 0; | ||
| for (;;) { | ||
| try { | ||
| const response = await axios.get(`${apiBaseUrl}/v1/api/proofs/${proofId}`); | ||
| const proofState: ProofStateResponse = create(response.data, ProofStateResponseSS); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we move this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thinking is: if e.g. the API "meant to send correct bytes", but something was wrong with the network communication, we won't be able to parse the response with Superstruct so we can just retry. The idea here is that retries on Superstruct throw should be rare but we still want to retry those errors. That said, if superstruct is throwing because suddenly our formats on ZK API <> Finalizer don't agree, then we'll notice it in the pages and go fix it. So this |
||
| return proofState; | ||
| } catch (e: any) { | ||
| // 404 is a valid/expected response: proof not yet created | ||
| if (axios.isAxiosError(e) && e.response?.status === 404) { | ||
| return 404; | ||
| } | ||
|
|
||
| attempt++; | ||
| if (attempt >= maxAttempts) { | ||
| throw e; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, backoffWithJitter(attempt))); | ||
| // todo: consider adding a logger log here .onRetry with `datadog = true` to monitor the error rates | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export async function requestProofWithRetries( | ||
| apiBaseUrl: string, | ||
| request: ApiProofRequest, | ||
| maxAttempts = 3 | ||
| ): Promise<void> { | ||
| let attempt = 0; | ||
| for (;;) { | ||
| try { | ||
| await axios.post(`${apiBaseUrl}/v1/api/proofs`, request); | ||
| return; | ||
| } catch (e: any) { | ||
| attempt++; | ||
| if (attempt >= maxAttempts) { | ||
| throw e; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, backoffWithJitter(attempt))); | ||
| // todo: consider adding a logger log here .onRetry with `datadog = true` to monitor the error rates | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export async function getVkeyWithRetries(apiBaseUrl: string, maxAttempts = 3): Promise<VkeyResponse> { | ||
| let attempt = 0; | ||
| for (;;) { | ||
| try { | ||
| const response = await axios.get(`${apiBaseUrl}/v1/api/vkey`); | ||
| const vkeyResponse: VkeyResponse = create(response.data, VkeyResponseSS); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| return vkeyResponse; | ||
| } catch (e) { | ||
| attempt++; | ||
| if (attempt >= maxAttempts) { | ||
| throw e; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, backoffWithJitter(attempt))); | ||
| // todo: consider adding a logger log here .onRetry with `datadog = true` to monitor the error rates | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these likely to change frequently? Should we cache this in redis to avoid extra queries on every finalizer run? We can follow a pattern like this where we load the
rediscache within this function so we don't need to pass it by inputThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These won't be updated frequently, but we do need the non-cached value for this logic to make sense.
This logic is to be used to harden our upgrade process.
Context here is that currently we don't have logic to remove proofs from the ZK API.
So with this
vkeychange, the upgrade process is:vkey(repoint Spokes to the newSP1Helioswith new vkey)vkey, while the API still has the old one, this check prevents the finalizer from requesting a proof with an incorrectvkey(so we don't have to go clean that up manually)So I do think we have to stick with the raw request here