improve: add a vkey check to helios finalizer
#2575
Conversation
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
src/finalizer/utils/helios.ts
Outdated
| */ | ||
| async function ensureVkeysMatch(apiBaseUrl: string, sp1Helios: ethers.Contract): Promise<void> { | ||
| const [apiResp, contractVkeyRaw] = await Promise.all([ | ||
| axios.get<VkeyResponse>(`${apiBaseUrl}/v1/api/vkey`), |
There was a problem hiding this comment.
For future, we probably want 1 - 2 retries on this to avoid sporadic issues.
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
grasphoper
requested review from
bmzig,
dohaki,
james-a-morris,
mrice32 and
nicholaspai
as code owners
|
I actually changed my mind on having this Ok done here e1fb73a. There's a bit of boilerplate but I think it's better than bringing in a new dependency. |
Signed-off-by: Ihor Farion <[email protected]>
| * allowing to request a proof with an incorrect(stale) ELF vkey. | ||
| */ | ||
| async function ensureVkeysMatch(apiBaseUrl: string, sp1Helios: ethers.Contract): Promise<void> { | ||
| const [apiResp, contractVkeyRaw] = await Promise.all([getVkeyWithRetries(apiBaseUrl), sp1Helios.heliosProgramVkey()]); |
There was a problem hiding this comment.
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 redis cache within this function so we don't need to pass it by input
There was a problem hiding this comment.
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 vkey change, the upgrade process is:
- send txs to upgrade all chains(contracts) to the new
vkey(repoint Spokes to the newSP1Helioswith new vkey) - we need to make sure that every chain had the proof generated for it and upgraded. This will happen at around the same time for all the chains, but perhaps not at exactly the same time
- while some chain has already new
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
| } | ||
|
|
||
| // Exponential backoff with jitter and a cap | ||
| export function backoffWithJitter(retry: number, baseDelayMs = 50, backoffExponentBase = 2, maxDelayMs = 5000) { |
There was a problem hiding this comment.
+1 now we can use this throughout the repo
| for (;;) { | ||
| try { | ||
| const response = await axios.get(`${apiBaseUrl}/v1/api/proofs/${proofId}`); | ||
| const proofState: ProofStateResponse = create(response.data, ProofStateResponseSS); |
There was a problem hiding this comment.
Should we move this create outside the try-catch block?
There was a problem hiding this comment.
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 .create being inside doesn't create any issues, that's a way to retry on another type of network failure
| for (;;) { | ||
| try { | ||
| const response = await axios.get(`${apiBaseUrl}/v1/api/vkey`); | ||
| const vkeyResponse: VkeyResponse = create(response.data, VkeyResponseSS); |
There was a problem hiding this comment.
There's no functionality to erase a proof from the ZK Api if it was requested and didn't error. To prevent incidental manual cleanup required on a wrong proof request, we compare vkeys between an API and contract to only generate matching proofs
Integration tests done for chain 999: https://hyperevmscan.io//tx/0x1a7aa90f2e11ce283d429cc70e3967eb59170201218d8b630e718c83c4b2b399