feat(proof-data-handler): exclude batches without object file in GCS #2980

pbeza · 2024-09-27T17:36:43Z

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 on mainnet was flooded with warnings.

Since the recent mainnet's 24.25.0 redeployment, we've been flooded with 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 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 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 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 the tee-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 on mainnet.

Checklist

PR title corresponds to the body of PR (we generate changelog entries from PRs).
Tests for the changes have been added / updated.
Documentation comments have been added / updated.
Code has been formatted via zk fmt and zk lint.

pbeza · 2024-09-30T17:54:47Z

@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!

pbeza · 2024-10-01T19:53:01Z

Kindly ping @slowli @RomanBrodetski. I need a reviewer.

slowli · 2024-10-02T12:10:49Z

core/lib/object_store/src/retries.rs

+        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>(


AFAICT, this function is unused. It's not detected as such because of the instrument attribute.

slowli · 2024-10-02T12:12:25Z

core/lib/types/src/tee_types.rs

Dumb question: Why was this module moved to zksync_types? It has a lot of deps which are not usually necessary.

slowli · 2024-10-02T12:13:55Z

core/lib/types/src/tee_types.rs

@@ -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


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).

slowli · 2024-10-02T12:15:16Z

core/node/proof_data_handler/src/tee_request_processor.rs

+                    };
+                    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) {


Nit: It may make sense to extract this value into a constant so that it's more visible.

slowli · 2024-10-02T12:16:43Z

core/node/proof_data_handler/src/tee_request_processor.rs

+                    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


Nit: Maybe it makes sense to log this status assignment, so that it is easier to debug?

slowli · 2024-10-02T12:17:49Z

core/node/proof_data_handler/src/tee_request_processor.rs

+                        Some((start, _)) => Some((start, batch_number)),
+                        None => Some((batch_number, batch_number)),
+                    };
+                    let datetime_utc = Utc.from_utc_datetime(&locked_batch.created_at);


It may make sense to store the timestamp as DateTime<Utc> in LockedBatch.

RomanBrodetski

@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

RomanBrodetski · 2024-10-02T10:25:28Z

core/lib/dal/migrations/20240930110000_tee_add_permanently_ignored_state.down.sql

@@ -0,0 +1 @@
+UPDATE tee_proof_generation_details SET status = 'permanently_ignore' WHERE status = 'permanently_ignored';


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

RomanBrodetski · 2024-10-02T10:26:56Z

core/lib/types/src/tee_types.rs

@@ -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.


thanks for adding a comment, but I still don't understand what a "locked" batch is. Please elaborate

pbeza · 2024-10-08T13:07:31Z

JFYI: This PR is on hold because the code it is based on was recently radically redesigned/refactored here: #3017. This PR may be cherry-picked/revisited once #3017 is merged into main.

/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

pbeza force-pushed the tee/flag-old-batches-as-permanently-ignored-automatically branch 3 times, most recently from f1b8ad3 to 65cc26e Compare September 30, 2024 11:22

pbeza marked this pull request as ready for review September 30, 2024 12:02

pbeza requested review from slowli, haraldh, RomanBrodetski and popzxc September 30, 2024 12:04

slowli reviewed Oct 2, 2024

View reviewed changes

RomanBrodetski reviewed Oct 2, 2024

View reviewed changes

pbeza force-pushed the tee/flag-old-batches-as-permanently-ignored-automatically branch 17 times, most recently from 4ee505b to bfeddc9 Compare October 31, 2024 18:29

pbeza force-pushed the tee/flag-old-batches-as-permanently-ignored-automatically branch from bfeddc9 to cf2cf1d Compare October 31, 2024 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(proof-data-handler): exclude batches without object file in GCS #2980

feat(proof-data-handler): exclude batches without object file in GCS #2980

pbeza commented Sep 27, 2024 •

edited

Loading

pbeza commented Sep 30, 2024

pbeza commented Oct 1, 2024

slowli Oct 2, 2024

slowli Oct 2, 2024

slowli Oct 2, 2024

slowli Oct 2, 2024

slowli Oct 2, 2024

slowli Oct 2, 2024

RomanBrodetski left a comment

RomanBrodetski Oct 2, 2024

RomanBrodetski Oct 2, 2024

pbeza commented Oct 8, 2024 •

edited

Loading

		@@ -0,0 +1 @@
		UPDATE tee_proof_generation_details SET status = 'permanently_ignore' WHERE status = 'permanently_ignored';

feat(proof-data-handler): exclude batches without object file in GCS #2980

Are you sure you want to change the base?

feat(proof-data-handler): exclude batches without object file in GCS #2980

Conversation

pbeza commented Sep 27, 2024 • edited Loading

What ❔

Why ❔

Checklist

pbeza commented Sep 30, 2024

pbeza commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanBrodetski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbeza commented Oct 8, 2024 • edited Loading

pbeza commented Sep 27, 2024 •

edited

Loading

pbeza commented Oct 8, 2024 •

edited

Loading