-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(proof-data-handler): exclude batches without object file in GCS #2980
base: main
Are you sure you want to change the base?
feat(proof-data-handler): exclude batches without object file in GCS #2980
Conversation
f1b8ad3
to
65cc26e
Compare
@popzxc, I remember you mentioned not to ask for code reviews this wave, but you're probably the most familiar with this code (along with @slowli). So, if you could make an exception this time, I’d really appreciate it. If you're busy, no worries – feel free to ignore, and I’ll ask @RomanBrodetski to find someone else. Thanks! |
Kindly ping @slowli @RomanBrodetski. I need a reviewer. |
core/lib/object_store/src/retries.rs
Outdated
skip(f), // output request and store as a part of structured logs | ||
fields(retries) // Will be recorded before returning from the function | ||
)] | ||
async fn retry_optional<T, Fut, F>( |
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.
AFAICT, this function is unused. It's not detected as such because of the instrument
attribute.
core/lib/types/src/tee_types.rs
Outdated
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.
Dumb question: Why was this module moved to zksync_types
? It has a lot of deps which are not usually necessary.
core/lib/types/src/tee_types.rs
Outdated
@@ -17,6 +19,17 @@ impl fmt::Display for TeeType { | |||
} | |||
} | |||
|
|||
/// Representation of a locked batch. Used in DAL to fetch details about the locked batch to |
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.
If this belongs to the DAL domain, it makes sense to define this type in DAL. AFAICT, there are some TEE-related types defined there already (like TeeProofGenerationJobStatus
).
}; | ||
let datetime_utc = Utc.from_utc_datetime(&locked_batch.created_at); | ||
let duration = Utc::now().signed_duration_since(datetime_utc); | ||
let status = if duration > ChronoDuration::days(10) { |
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.
Nit: It may make sense to extract this value into a constant so that it's more visible.
let datetime_utc = Utc.from_utc_datetime(&locked_batch.created_at); | ||
let duration = Utc::now().signed_duration_since(datetime_utc); | ||
let status = if duration > ChronoDuration::days(10) { | ||
TeeProofGenerationJobStatus::PermanentlyIgnored |
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.
Nit: Maybe it makes sense to log this status assignment, so that it is easier to debug?
Some((start, _)) => Some((start, batch_number)), | ||
None => Some((batch_number, batch_number)), | ||
}; | ||
let datetime_utc = Utc.from_utc_datetime(&locked_batch.created_at); |
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.
It may make sense to store the timestamp as DateTime<Utc>
in LockedBatch
.
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.
@pbeza to be honest I don't fully follow this solution. I understand what we are trying to do (mark older unresolved jobs as skipped), but I'm not sure I understand the Why
here. We can discuss over a huddle or async
@@ -0,0 +1 @@ | |||
UPDATE tee_proof_generation_details SET status = 'permanently_ignore' WHERE status = 'permanently_ignored'; |
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.
I don't think we need any logic in down
migration. The current logic does not precisely rollback it either. I think for this migration I'd just keep down empty
core/lib/types/src/tee_types.rs
Outdated
@@ -17,6 +19,17 @@ impl fmt::Display for TeeType { | |||
} | |||
} | |||
|
|||
/// Representation of a locked batch. Used in DAL to fetch details about the locked batch to | |||
/// determine whether it should be flagged as permanently ignored if it has no corresponding file in | |||
/// the object store for an extended period. |
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.
thanks for adding a comment, but I still don't understand what a "locked" batch is. Please elaborate
4ee505b
to
bfeddc9
Compare
/tee/proof_inputs endpoint no longer returns batches that have no corresponding object file in Google Cloud Storage for an extended period. Since the recent `mainnet`'s `24.25.0` redeployment, we've been [flooded with warnings][warnings] for the `proof-data-handler` on `mainnet` (the warnings are actually _not_ fatal in this context): ``` Failed request with a fatal error (...) Blobs for batch numbers 490520 to 490555 not found in the object store. Marked as unpicked. ``` The issue was caused [by the code][code] behind the `/tee/proof_inputs` [endpoint][endpoint_proof_inputs] (which is equivalent to the `/proof_generation_data` [endpoint][endpoint_proof_generation_data]) – it finds the next batch to send to the [requesting][requesting] `tee-prover` by looking for the first batch that has a corresponding object in the Google object store. As it skips over batches that don’t have the objects, [it logs][logging] `Failed request with a fatal error` for each one (unless the skipped batch was successfully proven, in which case it doesn’t log the error). This happens with every [request][request] the `tee-prover` sends, which is why we were getting so much noise in the logs. One possible solution was to manually flag the problematic batches as `permanently_ignored`, like Thomas [did before][Thomas] on `mainnet`. It was a quick and dirty workaround, but now we have a more automated solution. [warnings]: https://grafana.matterlabs.dev/goto/TjlaXQgHg?orgId=1 [code]: https://github.com/matter-labs/zksync-era/blob/3f406c7d0c0e76d798c2d838abde57ca692822c0/core/node/proof_data_handler/src/tee_request_processor.rs#L35-L79 [endpoint_proof_inputs]: https://github.com/matter-labs/zksync-era/blob/3f406c7d0c0e76d798c2d838abde57ca692822c0/core/node/proof_data_handler/src/lib.rs#L96 [endpoint_proof_generation_data]: https://github.com/matter-labs/zksync-era/blob/3f406c7d0c0e76d798c2d838abde57ca692822c0/core/node/proof_data_handler/src/lib.rs#L67 [requesting]: https://github.com/matter-labs/zksync-era/blob/3f406c7d0c0e76d798c2d838abde57ca692822c0/core/bin/zksync_tee_prover/src/tee_prover.rs#L93 [logging]: https://github.com/matter-labs/zksync-era/blob/3f406c7d0c0e76d798c2d838abde57ca692822c0/core/lib/object_store/src/retries.rs#L56 [Thomas]: https://matter-labs-workspace.slack.com/archives/C05ANUCGCKV/p1725284962312929
bfeddc9
to
cf2cf1d
Compare
What ❔
/tee/proof_inputs
endpoint no longer returns batches that have no corresponding object file in Google Cloud Storage for an extended period.Why ❔
TEE's
proof-data-handler
onmainnet
was flooded with warnings.Since the recent
mainnet
's24.25.0
redeployment, we've been flooded with warnings for theproof-data-handler
onmainnet
(the warnings are actually not fatal in this context):The issue is caused by the code behind the
/tee/proof_inputs
endpoint (which is equivalent to the/proof_generation_data
endpoint) – it finds the next batch to send to the requestingtee-prover
by looking for the first batch that has a corresponding object in the Google object store. As it skips over batches that don’t have the objects, it logsFailed request with a fatal error
for each one (unless the skipped batch was successfully proven, in which case it doesn’t log the error). This happens with every request thetee-prover
sends, which is why we're getting so much noise in the logs.One possible solution is to flag the problematic batches as
permanently_ignored
, like Thomas did before onmainnet
.Checklist
zk fmt
andzk lint
.