fix(tee): fix race condition in batch locking #3342
Conversation
pbeza
force-pushed
the
tee/fix/atomic-batch-locking
branch
from
a86fc98 to
e95cb27
Compare
pbeza
force-pushed
the
tee/fix/atomic-batch-locking
branch
from
e95cb27 to
46dcfde
Compare
After [scaling][1] [zksync-tee-prover][2] to two instances/replicas on Azure for azure-stage2, azure-testnet2, and azure-mainnet2, we started experiencing [duplicated proving for some batches][3]. While this is not an erroneous situation, it is wasteful from a resource perspective. This was due to a race condition in batch locking. This PR fixes the issue by adding atomic batch locking. [1]: https://github.com/matter-labs/gitops-kubernetes/pull/7033/files [2]: https://github.com/matter-labs/zksync-era/blob/aaca32b6ab411d5cdc1234c20af8b5c1092195d7/core/bin/zksync_tee_prover/src/main.rs [3]: https://grafana.matterlabs.dev/goto/M1I_Bq7HR?orgId=1
pbeza
force-pushed
the
tee/fix/atomic-batch-locking
branch
from
46dcfde to
7d96c1c
Compare
There was a problem hiding this comment.
Dumb question: How is the locking made atomic in this PR? AFAIU, the first SELECT statement, if queried concurrently, can still return the same L1 batch number unless some kind of row-level locking is implemented (cf. SELECT FOR UPDATE SKIP LOCKED in this contract verifier query). I'm not even sure the UPDATE query will fail for the transaction committed last in case of a race (maybe it would with serialization isolation level, but I'd argue that erroring is not the best cause of action here; row-level locks seem to work better).
Not a dumb question at all! The dumb one here was me! ;P I totally misunderstood what SQL transactions can actually handle in this context. Had to brush up on the finer details of SQL locking. Thanks for steering me in the right direction! These two links were super helpful: |
|
@slowli, I’ve addressed your code review comments. Take a look when you get a chance. It’s kinda hard to test properly without deploying it to |
|
@slowli, @haraldh suggested locking the entire Let me know if this more fine-grained locking approach still works for you, or if we’re missing something – or maybe there’s an easier way we haven’t considered. |
This reverts commit a1f99ab.
Commit a7dc0ed (PR #3342) was supposed to fix a race condition in batch locking by introducing SQL row-locking, but it didn't work as expected. Now we are switching back to coarser-grained table-level locking as [originally suggested][1] by Harald. The original fix was hard to test unless deployed to `stage` due to the undeterministic nature of the problem, so we needed to merge it to the `main` branch to properly test it. [1]: #3342 (comment)
…3358) ## What ❔ Commit a7dc0ed (PR #3342) was supposed to fix a race condition in batch locking by introducing SQL row-locking, but it [didn't work][2] as expected.  Now we are switching back to coarser-grained table-level locking as [originally suggested][1] by Harald. The original fix was hard to test unless deployed to `stage` due to the undeterministic nature of the problem, so we needed to merge it to the `main` branch to properly test it. [1]: #3342 (comment) [2]: https://grafana.matterlabs.dev/goto/AhEd5FVNg?orgId=1 ## Why ❔ To fix the bug that only activates after running `zksync-tee-prover` on multiple instances. ## Checklist - [x] 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. - [x] Code has been formatted via `zkstack dev fmt` and `zkstack dev lint`.
🤖 I have created a release *beep* *boop* --- ## [25.3.0](core-v25.2.0...core-v25.3.0) (2024-12-11) ### Features * change seal criteria for gateway ([#3320](#3320)) ([a0a74aa](a0a74aa)) * **contract-verifier:** Download compilers from GH automatically ([#3291](#3291)) ([a10c4ba](a10c4ba)) * integrate gateway changes for some components ([#3274](#3274)) ([cbc91e3](cbc91e3)) * **proof-data-handler:** exclude batches without object file in GCS ([#2980](#2980)) ([3e309e0](3e309e0)) * **pruning:** Record L1 batch root hash in pruning logs ([#3266](#3266)) ([7b6e590](7b6e590)) * **state-keeper:** mempool io opens batch if there is protocol upgrade tx ([#3360](#3360)) ([f6422cd](f6422cd)) * **tee:** add error handling for unstable_getTeeProofs API endpoint ([#3321](#3321)) ([26f630c](26f630c)) * **zksync_cli:** Health checkpoint improvements ([#3193](#3193)) ([440fe8d](440fe8d)) ### Bug Fixes * **api:** batch fee input scaling for `debug_traceCall` ([#3344](#3344)) ([7ace594](7ace594)) * **tee:** correct previous fix for race condition in batch locking ([#3358](#3358)) ([b12da8d](b12da8d)) * **tee:** fix race condition in batch locking ([#3342](#3342)) ([a7dc0ed](a7dc0ed)) * **tracer:** adds vm error to flatCallTracer error field if exists ([#3374](#3374)) ([5d77727](5d77727)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: zksync-era-bot <[email protected]>
What ❔
After scaling
zksync-tee-proverto two instances/replicas on Azure forazure-stage2,azure-testnet2, andazure-mainnet2, we started experiencing duplicated proving for some batches.While this is not an erroneous situation, it is wasteful from a resource perspective. This was due to a race condition in batch locking. This PR fixes the issue by adding atomic batch locking.
Why ❔
To fix the bug that only activates after running
zksync-tee-proveron multiple instances.Checklist
zkstack dev fmtandzkstack dev lint.