Repository navigation
Commit bf94de7
authored
fix(l2): make batch-commit resends able to replace, and de-flake the shared-bridge test (#7151)
**Motivation**
`Integration Test Shared Bridge - Normal` fails intermittently with
`Receiver balance did not increase correctly` at
`test/tests/l2/shared_bridge.rs:409`. It hit `main` on Aug 3
(`0cc4572c5`) and Aug 13 (`f2998963d`), and `release/v24.0.0` on Aug 12
(`8f3d2ca05`), always with the same message hash `0x66867250…5be951` —
so the test is not wrong about what it expects, the message genuinely
never arrives. In the failing runs L2B never commits batch 1, its L2→L2
message never becomes pending on the router, and L2A's watcher logs `L2
message not found in pending messages` 153 times with 0 verified.
Three separate defects combine to produce that:
**1. Both sequencers commit from the same L1 account.**
`crates/l2/docker-compose.yaml` gives `ethrex_l2`
`--committer.l1-private-key 0x385c…` and the shared-bridge override gave
`ethrex_l2_b` the same key (likewise the proof-coordinator key). They
post to one L1 on the same 15 s timer, so their commit loops race for
the same nonce and the loser is rejected — first as an underpriced
replacement, then as a too-low nonce.
**2. The loser cannot recover, which is a bug beyond CI.**
`bump_gas_generic_tx` computed the blob fee as `1 + (bump_percentage /
100) * 10` and then divided by ten. For the only bump either call site
passes, `30`, that evaluates to `fee * 1 / 10`: **every retry lowered
`max_fee_per_blob_gas` by 90%**, so a batch commitment could never
re-price itself — on CI or on a real L1 during a fee spike. Blob
replacements also require the fee to *double*
(`DEFAULT_BLOB_PRICE_BUMP_PERCENT`), and that threshold applies to the
fee cap and the tip as well, so bumping those by 30% could not have
replaced a blob transaction either. Separately, the retry loop never
noticed when the nonce had become unreachable, so one `NonceTooLow`
burned all ten attempts in ~3.5 s and reported "max retries reached",
hiding the cause.
**3. The test slept a fixed 180 s and then asserted**, which made the
result depend on whatever else the job was doing. The first failure in
this job's history is the run of #7029, which aligned the `cargo test
--no-run` prebuild's features with the run step so the test binary is no
longer rebuilt inside it. Job step timestamps show the window between
"Start Sequencer L2B" and the assertion collapsing from **2 m 35 s to 3
s**, moving the assertion onto the sequencers' cold start — where batch
1 carries all 192 privileged deposit mints and the two co-nonced
committers collide from their first commit. Worth noting the direction:
the passing Jul 22 run had *more* nonce collisions (72 underpriced + 76
too-low) than any failing run, because they happened before the test
looked.
**Description**
- **L2B gets its own committer and proof-coordinator accounts, and its
deployment authorizes them.** Changing the keys alone is not enough:
`Timelock.commitBatch` is `onlyRole(SEQUENCER)`, and the role is granted
from the deployer's `--committer.l1-address` /
`--proof-sender.l1-address`, which both default to L2A's accounts.
Without the matching addresses every L2B `commitBatch` reverts at gas
estimation as an unauthorized caller — the same end symptom, reached a
different way. Neither option is forwarded to the deployer container by
`docker-compose.yaml`, so they are set on the override's
`contract_deployer`, leaving L2A's deployment on the defaults.
- **`bump_gas_generic_tx` raises every fee dimension** by a bump that
clears the mempool's replacement threshold for the transaction's
category — `BLOB_FEE_BUMP_PERCENT` (100) for blob-carrying transactions,
`FEE_BUMP_PERCENT` (30) otherwise — using saturating arithmetic so an
absurd starting fee cannot overflow.
- **The retry loop stops spending attempts that cannot succeed:** it
abandons the transaction with `UnreachableNonce` once the chain's nonce
has passed it (checked against the chain, not against the rejection
text, which differs between clients), and reports the reason when the
`maximum_allowed_max_fee_per_gas` / `_blob_gas` clamps leave a bid the
mempool already rejected.
- **Every shared-bridge assertion now waits for its own condition**
instead of sharing one fixed sleep, with a timeout more generous than
the 180 s it replaces. The L1 deposits accounting is waited on
separately rather than assumed settled once the L2B credit lands: the
two are updated by different transactions on different chains and
nothing orders them. The forced-inclusion test's expiry and verification
waits are deliberately left as sleeps — they are waiting for something
*not* to happen.
- **Hardening:** a test pins L2B's private keys to the addresses its
deployment authorizes, so drift between the two blocks of the override
file fails there instead of surfacing at runtime as an ABI-undecodable
`AccessControlUnauthorizedAccount` revert and a chain that silently
stops committing. Unit tests cover both replacement thresholds,
monotonicity across ten retries, saturation, and — so they cannot
silently stop discriminating — that the previous blob formula fails
them.
This is deliberately **not** a revert of #7029. That PR removed an
accidental delay which had been masking defects 1 and 2 since the
shared-bridge test landed; restoring the delay would hide them again.
**Also worth knowing, not changed here:** this job is gated
`github.event_name != 'merge_group'`, so the merge queue never runs it
and a PR can land without it ever executing against the merged result.
Combined with the paths filter, it effectively only runs on pushes to
main — which is why a ~3% flake sat unnoticed for weeks.
**Checklist**
- [x] Updated `STORE_SCHEMA_VERSION` (crates/storage/lib.rs) if the PR
includes breaking changes to the `Store` requiring a re-sync. — n/a, no
`Store` changes1 parent ac8eb46 commit bf94de7
7 files changed
Lines changed: 506 additions & 40 deletions
File tree
- crates/l2
- sdk
- src
- test/tests/l2
- tooling
- zkevm_bench
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
13 | 22 | | |
14 | 23 | | |
15 | 24 | | |
| |||
62 | 71 | | |
63 | 72 | | |
64 | 73 | | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
65 | 87 | | |
66 | 88 | | |
67 | 89 | | |
68 | 90 | | |
69 | 91 | | |
70 | 92 | | |
71 | 93 | | |
72 | | - | |
73 | | - | |
| 94 | + | |
| 95 | + | |
74 | 96 | | |
75 | 97 | | |
76 | 98 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
| 13 | + | |
13 | 14 | | |
14 | 15 | | |
15 | 16 | | |
| |||
0 commit comments