Conversation
6b0b640 to
1e29d62
Compare
The publish gate predicts whether authenticateBatchInfo would revert, so it must resolve the fallback batcher through the same SystemConfig the contract does. Reading batcherHash from RollupConfig.L1SystemConfigAddress leaves the two free to disagree: a batcher that predicts "authorized" against one SystemConfig loops on UnauthorizedFallbackBatcher, and one that predicts "unauthorized" stops publishing and reports an expected address the contract never asked for. Read systemConfig() from the contract instead, once, and keep the binding. The probe already latches this way, so the steady-state gate still costs two eth_calls. A zero address means the proxy holds code but has not been initialized, and is not kept, so the reader resolves again afterwards. The mock now serves each ABI only at its own address, which is what makes a read from the wrong SystemConfig fail the tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The address becomes the EIP-712 verifying contract every batch authentication is signed against. initialize rejects address(0), so a contract reporting one holds code but no state, and a batcher that accepts it signs every authentication against a domain no verifier will accept. Fail startup instead, where the cause is still visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three call sites test the reader for nil while isFallbackAuthRequired tested the rollup config address, so the same question had two answers and nothing kept them agreeing. Make the constructor return no reader for a zero address and have every site read that nil. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate is evaluated inside publishStateToL1's loop, so it runs once per batch transaction rather than once per tick. And a lazy deployment probe only saves the fallback batcher: the Espresso batcher probes the same contract through registerBatcher, which fails startup when there is no code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The batcher that is not the active one takes a skip branch on every evaluation, and the gate runs before each batch transaction, so the same two warnings repeat for as long as the node stands by. Route them through degradedLog, which the batcher already uses for tick-driven warnings, and clear each state when it recovers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ActiveIsEspresso, EspressoBatcher, EspressoTEEVerifier and the batcherHash read spelled the same probe, timeout and error wrap four times, under three different verbs, and the (opts, cancel) pair hid the timeout from vet's lostcancel check. One generic readContract does it once, and callOpts goes. State the zero-address rule the reader follows: reject one where the value is kept, since an uninitialized contract answers every address getter with zero and a latched zero never recovers, and pass one through where the caller compares and discards it. Two of four reads guard it and nothing said why. Drop Address(): its one caller builds the registration transaction from RollupConfig four lines later, so the accessor only added a second way to spell the address. The mock now dispatches by ABI per address, and the TEE verifier test keeps only the assertion the probe tests do not already make. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Split the long sentences, lead each paragraph with its cause, and drop "latching" in favour of plain wording. Name the two cached addresses that reject a zero and say which address the batcher identities are compared against. Also note what the reader's mutex guards. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…cator An Espresso-enabled batcher whose rollup config has a zero BatchAuthenticatorAddress started cleanly: config.Check does not see the rollup config, setupEspressoStreamer handed the zero address to NewStreamer, and registerBatcher and resolveTEEVerifierAddress both returned early on the nil reader. It then signed every commitment against a zero verifying contract and sent every authenticateBatchInfo to the zero address, failing silently on each batch -- while the same zero read *from* a deployed contract already failed startup hard. Add checkEspressoBatchAuthenticator beside checkEspressoDataAvailability and checkFallbackAuthConfirmations, the family of checks that validate the CLI config against the loaded rollup config and return an error. It is the first point both facts are in hand, and it fails before the tx manager is built rather than three steps into StartBatchSubmitting. The fallback batcher is deliberately not covered. Pre-fork it runs as a vanilla upstream batcher with no BatchAuthenticator coupling, so a zero address is legitimate there, and checkFallbackAuthConfirmations keeps reading that zero as "no contract, bound does not apply". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Open the reader's zero-address paragraph with which reads reject a zero and which do not, then give the reason and the two cases. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
97c897a to
bd1fd72
Compare
palango
left a comment
There was a problem hiding this comment.
No functional bugs, I'd approve once the comment and test bits below are in. Build, go vet and go test -race ./op-batcher/batcher/ pass on 51bb935.
Reading SystemConfig from the BatchAuthenticator is right, despite #503: authenticateBatchInfo checks its own stored systemConfig.batcherHash() (BatchAuthenticator.sol:174), and there's no setter, so caching it is fine too.
- The cost comment (
espresso_active.go:22-26) is off. The gate runs before everypublishTxToL1call, including the trailingio.EOFone (driver.go:889), and a wrong mode costs 1 call while the first fallback check costs 3 plus aCodeAt. "At most two in steady state" would be accurate. batcherKeyUnauthorizedis only cleared on success (espresso_active.go:69). Flip the switch away and back with the key still wrong and there's no fresh Warn, and the eventual Info reports adurationcovering the other mode. Clear it in the mode-mismatch branch too.shouldSkipPublishForActiveSeqis changed here but still untested: flip eitherreturn true(espresso_driver.go:309,:320) and the package stays green.- The wrong-mode rows in
TestIsBatcherActive(batch_authenticator_test.go:263-264) would fail the identity check anyway, so onlywantCallscatches a missing mode check. Add a row where one key holds both roles. - Nothing asserts the throttling, and four subtests print unchecked WARNs.
testlog.CaptureLoggerwould fix both. - Error paths in
isBatcherActive,resolveTEEVerifierAddressandregisterBatcheraren't covered.
ensureDeployed/haveCode can go. The bindings already call CodeAt when an eth_call returns empty and give bind.ErrNoCode (op-geth accounts/abi/bind/v2/base.go:238-249), so the latched probe adds nothing for reads. registerBatcher sends a tx, so it should keep a plain CodeAt. systemConfigCaller can then use readContract instead of its own timeout.
Nits: an uninitialized proxy reverts rather than returning zero, and the deploy scripts initialize atomically, so the rationale at batch_authenticator.go:32-37 doesn't hold and the zero checks are unreachable (keep or drop, your call). service.go:316 overstates it: auth txs to 0x0 succeed as no-ops, so it's wasted gas, not failed batches. "serialized today" (:44) is a temporal comment.
|
Two follow-ups, not for this PR:
|
Closes #503.
This PR:
Key places to review: