Repository navigation
fix(l2): make batch-commit resends able to replace, and de-flake the shared-bridge test - #7151
Conversation
…oordinator accounts instead of reusing L2A's. Both sequencers post to the same L1, so the two committers were racing for the same nonce, and the loser could not recover: its send was rejected as an underpriced replacement, then as a too-low nonce until it exhausted its retries, after which it re-wedged on the same batch on every cycle. Whenever the two commit loops stayed phase-locked, L2B never committed a batch, its L2->L2 message never became pending on the router, and test_shared_bridge failed with an unchanged destination balance. The new keys are rich accounts from fixtures/keys/private_keys_l1.txt, funded in fixtures/genesis/l1.json. Their only other L1 use is the deployer's rich deposit, sent from each rich account itself, which completes before either sequencer starts.
… committer and proof-coordinator keys is not enough on its own: the Timelock is initialized with the deployer's committer and proof-sender address options as its authorized sequencers, and both default to the accounts ethrex_l2 uses. With only the keys changed, every commitBatch from L2B reverted at gas estimation as an unauthorized caller, so L2B committed nothing at all -- the same end symptom as the nonce race, reached a different way. Neither option is forwarded to the deployer container by docker-compose.yaml, so exporting them from the workflow would not have reached it; they are set on the override's contract_deployer instead, leaving L2A's deployment on the defaults.
…d stop retrying when it cannot. The blob branch of bump_gas_generic_tx computed `1 + (bump / 100) * 10` and then divided by ten. For the only bump either call site passed, 30, that evaluates to `fee * 1 / 10`: every retry *lowered* max_fee_per_blob_gas by 90%. A batch commitment could therefore never re-price itself, on CI or on a real L1 during a fee spike. Blob transactions also need a far larger bump than regular ones -- peer clients and ethrex's own DEFAULT_BLOB_PRICE_BUMP_PERCENT require the fee to double -- and the threshold applies to the fee cap and the tip as well, not only to the blob fee, so a 30% bump on those dimensions could not have replaced a blob transaction either. Two ways the retry budget was being spent on attempts that could not succeed: - Once the account has moved past the transaction's nonce, no fee gets it included; all ten attempts fail identically and the caller is told "max retries reached", which hides the cause. The nonce is now checked against the chain -- not against the rejection text, which differs between clients -- and the attempt is abandoned with UnreachableNonce. - The maximum_allowed_max_fee_per_gas / _blob_gas clamps can cancel out the bump, leaving a bid the mempool already rejected. That is now detected and reported instead of resent. Fees are raised with saturating arithmetic so an absurd starting fee cannot overflow. Unit tests cover both thresholds, monotonicity across ten retries, saturation, and -- so the tests cannot silently stop discriminating -- that the previous formula fails them.
…utes. Each assertion now polls for the state it is about to check, with a timeout more generous than the sleep it replaces. A fixed sleep asserts on whatever state the system happens to be in when the timer fires, so a change in how long the rest of the job takes flips the result without anything about the chains changing -- which is exactly how this test started failing: a CI change removed a rebuild that used to sit between starting the sequencers and starting the test, and the assertion window moved onto the sequencers' cold start. The L1 deposits accounting is waited on separately rather than assumed to be settled once the L2B credit lands: the two are updated by different transactions on different chains and nothing orders them. Also pins L2B's sequencer keys to the addresses its deployment authorizes as SEQUENCER. They live in separate blocks of the same override file, and a mismatch is invisible until runtime, where it surfaces as a commitBatch that reverts at gas estimation with an ABI-undecodable AccessControlUnauthorizedAccount and an L2 that silently stops committing.
|
🤖 Codex Code Review
The blob-fee regression fix itself and the shared-bridge test improvements look directionally correct. I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewThis is a high-quality PR addressing real operational issues in the L2 shared bridge. The changes are well-documented, thoroughly tested, and fix critical bugs in transaction fee bumping and test reliability. Critical Fixes (Correct)1. Blob Fee Calculation Bug (sdk.rs:1028-1033) 2. Nonce Exhaustion Detection (sdk.rs:944-956) Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
MegaRedHand
left a comment
MegaRedHand
left a comment
There was a problem hiding this comment.
Do we need to update documentation?
| client.send_raw_transaction(encoded_tx.as_slice()).await | ||
| } | ||
|
|
||
| pub async fn send_tx_bump_gas_exponential_backoff( |
There was a problem hiding this comment.
This logic bumps the fee on any error, even transient ones. Also, the 100% bump with 10 retries can result in 1024x the fee on blob transactions in the worst case, with no user-configurable bump.
We should revisit this in the future.
MegaRedHand
left a comment
MegaRedHand
left a comment
There was a problem hiding this comment.
LGTM. The previous comments are improvements that can be added in other PRs
Bump by adding the increment instead of scaling by `100 + bump`: the product saturates on a fee near the type's maximum, and dividing a saturated product by 100 lands two orders of magnitude below the input, so an absurd starting fee was bumped *downwards* -- the same shape as the blob-fee defect this branch fixes. The unit test had asserted that decrease as expected behaviour; it now pins the invariant that matters, that a bump never returns a lower fee. Also floor the increment at one wei. A percentage alone rounds to zero below `100 / bump_percent` wei, so the resend would carry the pooled transaction's exact fee and be rejected as an underpriced replacement. This goes beyond the review comment but is the same failure mode at the other end of the range. Take both bump percentages from the mempool's own `DEFAULT_PRICE_BUMP_PERCENT` and `DEFAULT_BLOB_PRICE_BUMP_PERCENT` rather than restating them, so the client and the admission rule cannot drift apart, and read the same constants in the tests instead of duplicated literals. `ethrex-blockchain` is already in this crate's dependency graph through `ethrex-rpc`, so this adds no compile units. Note this lowers the non-blob bump from 30% to 10%, which still clears the threshold exactly and reduces how far fees compound across ten retries. Finally, a consumed nonce no longer implies the send failed: if an earlier attempt of ours was accepted and has since been mined, return that hash instead of `UnreachableNonce`, so a transaction that is on chain is never reported as an error.
`tooling` and `tooling/zkevm_bench` both resolve ethrex-sdk (the latter through ethrex-l2), so declaring ethrex-blockchain on it leaves their lockfiles stale and `make check-cargo-lock` fails under --locked. Regenerated with `make update-cargo-lock`; the only change in either file is the new dependency entry.
2 participants
Motivation
Integration Test Shared Bridge - Normalfails intermittently withReceiver balance did not increase correctlyattest/tests/l2/shared_bridge.rs:409. It hitmainon Aug 3 (0cc4572c5) and Aug 13 (f2998963d), andrelease/v24.0.0on Aug 12 (8f3d2ca05), always with the same message hash0x66867250…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 logsL2 message not found in pending messages153 times with 0 verified.Three separate defects combine to produce that:
1. Both sequencers commit from the same L1 account.
crates/l2/docker-compose.yamlgivesethrex_l2--committer.l1-private-key 0x385c…and the shared-bridge override gaveethrex_l2_bthe 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_txcomputed the blob fee as1 + (bump_percentage / 100) * 10and then divided by ten. For the only bump either call site passes,30, that evaluates tofee * 1 / 10: every retry loweredmax_fee_per_blob_gasby 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 oneNonceTooLowburned 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-runprebuild'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
Timelock.commitBatchisonlyRole(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 L2BcommitBatchreverts at gas estimation as an unauthorized caller — the same end symptom, reached a different way. Neither option is forwarded to the deployer container bydocker-compose.yaml, so they are set on the override'scontract_deployer, leaving L2A's deployment on the defaults.bump_gas_generic_txraises 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.UnreachableNonceonce 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 themaximum_allowed_max_fee_per_gas/_blob_gasclamps leave a bid the mempool already rejected.AccessControlUnauthorizedAccountrevert 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
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync. — n/a, noStorechanges