Espresso 3b: TEE batcher (re-hosted)

[QuentinI]

Based on #448

Pulls in the Espresso/TEE batcher .

• In op-node: adds EspressoBatch type and marshaling logic for it. This is the datastructure that ends up posted to Espresso.
• In espresso package: adds the CLI flags and interfaces for the streamer.
• In the batcher: adds NSM helper in op-batcher/enclave/attestation.go and modifies the driver to add an Espresso path. Bulk of the changes is in espresso_-prefixed files.

This is #447, re-hosted from an in-repo branch (now properly stacked).

[piersy]

This eth_sign call can't work against op-signer, and I don't think op-signer should be extended to make it work either.

op-signer doesn't serve eth_sign. Its server registers only two namespaces — eth (eth_signTransaction) and opsigner (signBlockPayload, signBlockPayloadV2): https://github.com/ethereum-optimism/infra/blob/main/op-signer/service/service.go#L73-L82. There is no arbitrary-data signing method. And this SignerClient can only talk to op-signer in the first place: NewSignerClient dials with op-signer's mutual-TLS and then handshakes with a health_status ping before returning, so pointing it at a plain geth or another HSM front-end fails at construction. So the call here errors method-not-found at runtime against a real op-signer.

The reason op-signer has no such method is deliberate, and it's why I'd argue against adding one. An HSM-backed signer must never sign raw bytes the caller hands it. If it did, a compromised batcher could pass a 32-byte value that is really the sighash of an L1 transaction spending the funded key, or a block payload for equivocation, and the HSM would sign it. That's why every op-signer method reconstructs the thing being signed server-side from typed arguments and binds a domain tag and chain id into the hash — see BlockPayloadArgs (domain, chainId, payloadBytes) and Message().ToSigningHash(). The client never sends a bare hash. Adding an eth_sign that signs any digest would remove exactly that protection for a key that also signs L1 transactions.

There's a second, backend-independent problem: eth_sign applies the EIP-191 prefix ("\x19Ethereum Signed Message:\n32" || hash), but the verify side recovers over the raw digest (crypto.SigToPub(batchHash, sig) in op-node/rollup/derive/espresso_batch.go):

optimism/op-node/rollup/derive/espresso_batch.go

Lines 105 to 107 in 014f88d

	batchHash := crypto.Keccak256(batchData)
	signerKey, err := crypto.SigToPub(batchHash, signatureData)

So even a signer that did serve eth_sign would recover the wrong address and every batch would be rejected.

If remote HSM signing of Espresso batches is a requirement, the right shape is a purpose-built op-signer method modeled on signBlockPayload: the client sends typed args (a fixed domain tag, the L2 chain id / namespace, and the batch commitment), op-signer reconstructs the domain-separated digest and signs it with the HSM key, and op-node verifies the same digest. That also resolves the separate domain-separation gap (the batch digest is currently a bare keccak256(rlp(batch)) with no namespace binding). It is a change in the op-signer repo, so it can't land from this PR alone — until it exists, only the local private-key ChainSigner actually works. I'd suggest dropping this eth_sign helper and the clientSigner branch here rather than shipping a path that can't sign or verify.

[lukeiannucci]

Jean is going to look and respond here as he did this work.

[jjeangal]

Thanks for the thorough writeup. The analysis is right about op-signer, and the fact that it reads as targeting op-signer at all is a documentation gap on our side, so let me fill in the missing context first.

This Sign call doesn't talk to op-signer. The --signer.endpoint it's deployed against is espresso-kms-signer (https://github.com/EspressoSystems/espresso-kms-signer), a small AWS KMS signing sidecar we built specifically to speak the signer protocol this batcher uses: health_status, eth_signTransaction, and eth_sign (https://github.com/EspressoSystems/espresso-kms-signer/blob/758e4d0/src/rpc.rs#L26-L46). It runs as an ECS sidecar next to op-batcher-tee and has done full batch-posting cycles on our kms test devnets (batches accepted on L1 and on HotShot). On the client side, NewSignerClient isn't actually op-signer-specific: mTLS only kicks in when tlsConfig.Enabled is set (https://github.com/celo-org/optimism/blob/c828d61cd9/op-service/signer/client.go#L31-L65) (plain HTTP otherwise), and the handshake is just a health_status call into a Go string, which the sidecar answers. That genericness is what let us add KMS signing with no batcher code changes.

On EIP-191: agreed that a standard eth_sign would break recovery, but the sidecar's eth_sign is deliberately non-standard; it signs the raw 32-byte digest with no message prefix and returns r||s||v with v ∈ {0,1}, (i.e. go-ethereum's crypto). Sign convention, which makes it semantically identical to the privateKeySigner path in this PR (crypto.Sign(hash, privKey) (https://github.com/celo-org/optimism/blob/c828d61cd9/op-service/crypto/espresso.go#L161)). Both verify the same way under SigToPub. And it's pinned rather than hoped-for: the sidecar's fixture generator (https://github.com/EspressoSystems/espresso-kms-signer/blob/758e4d0/tests/fixtures/gen/main.go#L158-L178) is a Go program that imports op-service/signer itself and records the exact JSON-RPC params bytes geth's RPC client marshals (including the base64 []byte encoding), which CI replays against the production handler; there's also a localstack test that runs the real KMS path end-to-end and asserts the recovered address. That said, you're completely right that calling a method eth_sign while breaking eth_sign semantics is asking for exactly this confusion. I'd be happy to rename it in a follow-up, and we'll add a doc comment on SignerClient.Sign pointing at the sidecar and its semantics either way. (Small note: the espresso_batch.go verify code you linked has since moved into espresso-streamers digest recovery (https://github.com/EspressoSystems/espresso-streamers/blob/1884a718fbf7/op/derivation/espresso_batch.go#L102-L104).)

On "an HSM signer should never sign raw caller-supplied bytes", no pushback on the principle, and I'd rather be precise about what it costs us here. A raw-digest endpoint does mean the sidecar's eth_signTransaction guards (chainId, from, to-allowlist) only protect against a buggy caller, not a malicious one; anyone who can reach the endpoint can get a signature over an arbitrary digest, including an L1 tx sighash. What bounds the damage is that this key was never a batch-eligibility authority: batch acceptance in TEE mode requires an EIP-712 commitment signature from the ephemeral key generated inside the Nitro enclave and verified on-chain via the TEE verifier; the sidecar never touches that key. So a compromised sidecar (or its host) can spend the batcher address's gas funds and inject noise into our own HotShot namespace, but it can't make derivation accept a batch the enclave didn't produce. That's our documented trust model: the sidecar is trusted for availability, not integrity.

You're also right about the missing domain separation, and that one stands on its own: the namespace lives in the transaction envelope outside the signed bytes, so the signature binds neither chain nor namespace, under the local key just as much as the remote one. Fixing it means changing the digest every verifier reconstructs, so it's a coordinated change across the batcher and espresso-streamers with a migration story for payloads already in the stream, and we'll file it as a tracked issue rather than fold it into this PR.

Where I'd push back is on the remedy. A typed, domain-separated signing method modeled on signBlockPayload is the right end state, but it belongs in espresso-kms-signer (op-signer isn't in this deployment), and it should land together with the digest-scheme change since both alter what verifiers recover over. Dropping clientSigner in the meantime wouldn't remove the capability this comment worries about; it would move the key from KMS hardware into batcher memory, which is a strict downgrade for the same attack surface. So my proposal: keep clientSigner/Sign as-is here, add a comment linking the sidecar and its non-standard semantics, and file two linked follow-ups, one for the typed domain-separated method (including the eth_sign rename/retirement) and one for the digest-scheme migration (which I will discuss with the team). Happy to talk through the typed-method design if you have opinions on the shape!

[jjeangal]

Any thoughts or remarks @piersy?

[piersy]

Hi @jjeangal, I hadn't realised this was talking to the kms signer.

So yes, agreed on the followups 👍

[palango]

I did another review round on this, together with the integration spec. The recent hardening commits addressed the right things, but I found two issues I'd consider blocking, plus a few more we
should handle before merging.

The big one: every restart re-scans HotShot history from the static --espresso.origin-height-espresso. setupEspressoStreamer (espresso_driver.go:148) passes the CLI flag as the streamer's
HotShot cursor on every start, and the v2 streamer never fast-forwards it — fallbackHotShotPos is computed from the light client but never applied to hotShotPos. The v1 streamer's Refresh()
did exactly that fast-forward, so this looks like it got dropped in the v2 wiring. Practical effect: a restart months after activation re-fetches the entire HotShot range since the caffeination
point at 100 blocks per fetch pair, and until the backfill reaches the tip, Peek yields nothing, so no batches are posted and the safe head stalls — plausibly for hours, growing with chain age.
The spec requires the streamer to avoid exactly this ("avoiding both re-scanning from genesis and missing unprocessed batches", §4.3.1), and it breaks the stateless-restart criterion in §2.4.
Manually bumping the flag before every restart isn't something we can rely on operationally. Related: the flag defaults to 0 with no validation and the usage string doesn't mention the scan-start
semantics (espresso/cli.go:94), so omitting it silently scans from HotShot genesis.

Second: the TEE publish path ignores the derivation-side enforcement grace window from #468. During [EspressoTime, EspressoTime + BatchAuthEnforcementDelaySecs) derivation still applies
sender-based auth against batcherHash — the fallback key. The TEE batcher signs its L1 txs with the KMS key, so any batch it lands in that window is permanently rejected by every verifier; auth
events aren't even scanned yet. If we activate with activeIsEspresso=true from the start (which the deploy script allows), the fallback skips publishing while the TEE batcher burns L1 fees on
batches nobody accepts — a 20–40 minute safe-head stall. Minimum fix is a documented ops constraint that the fallback stays active through the window; nicer would be a gate in the publish path.

Then three timeout/wedge issues, all the same family:

• The Espresso SDK calls in the submit/verify workers run without per-call deadlines (espresso.go:660, espresso.go:737, espresso.go:776). The SDK uses http.DefaultClient, which has no
  timeout, so a black-holed connection permanently consumes a worker; with all four wedged, submission stays stopped even after the endpoint recovers.
• Same pattern in the loading loop: Peek (espresso.go:918) does raw L1 RPCs via checkBatch with the loop's shutdown context. A hung L1 RPC silently stops frame publication, no error logs.
• On a stop/start cycle, l.espressoStreamer is never nil'ed, so the next StartBatchSubmitting runs clearState against the stale streamer, and its re-anchor gate retries every 5s with no
  deadline while holding the start mutex (espresso_driver.go:334 via driver.go:206). If the op-node is resyncing at that point, a later stop or SIGTERM blocks forever — the same start-mutex-hang
  pattern already fixed for waitForLocalSafeHead and registration.

Given networkTimeoutCtx already exists and is used elsewhere in these files, I'd enforce the timeout once at a client-wrapper seam instead of per call site.

Last area: test coverage. Most of the new safety-critical logic is pure functions, and most of it is untested:

• The active-batcher gate has zero coverage — isBatcherActive, the batcherHash low-20-byte slicing, shouldSkipPublishForActiveSeq (espresso_active.go). This is the switch that decides
  whether a batcher publishes at all during a handoff; an inverted mode check compiles and passes CI. The mock-backend pattern in espresso_fallback_gate_test.go already covers the sibling half of
  the same gate, so extending it should be cheap.
• nextBlockRange's reset/reorg/prune branches (espresso.go:1104) — only the zero-guard and happy path are tested. The safe-chain-reorg hash check is the sole defense against re-queueing
  orphaned blocks to Espresso.
• EnqueueBlocks reorg detection and the requestClearState/performClearState CAS handshake (espresso.go:1014) — including the earlier data-race fix, which has no regression test.
• evaluateVerification (espresso.go:459) — both submitter mocks fail FetchLatestBlockHeight, so startHeight stays 0 and the re-submission timeout logic is never exercised.
• rollbackFailedStart and waitForLocalSafeHead (espresso_driver.go:234) — untested; waitForLocalSafeHead hardcodes real-clock constants, so it can't be tested as written. Injecting the
  timing would fix that and cover the caffeination gate.

I also checked the eth_sign digest question from the earlier thread against espresso-kms-signer: its eth_sign signs the raw digest, which matches the streamer's recovery, so no bug there —
the typed domain-separated signing method stands as a follow-up.

There are a handful of low-severity leftovers as well; I'll file those separately.

[palango]

Following up on my review with some design-level feedback. Beyond the bugs, I looked at where the new code could be simpler, and I'd group it into things I'd like to see in this PR, and bigger
restructurings we should discuss rather than block on.

Things I'd like addressed in this PR — each is small, low-risk, and closes one of the review findings structurally instead of point-fixing it:

• Enforce timeouts at one seam instead of per call site. The submitter uses 3 of the SDK's 14 methods (SubmitTransaction, FetchTransactionByHash, FetchLatestBlockHeight); a thin wrapper
  applying NetworkTimeout inside those, plus the same bound inside batcherL1Adapter/batcherL2Adapter (which the streamer's reads already go through, so Peek gets covered too), fixes the whole
  unbounded-context class from my review and means a future call site can't forget the timeout. The ad-hoc networkTimeoutCtx blocks at individual call sites can then go.
• Own the per-start state in one place. l.espressoStreamer/l.espressoSubmitter survive StopBatchSubmitting and rollbackFailedStart, which is what makes the stale-streamer clearState wedge
  possible. A small session struct (streamer + submitter + resolved verifier address) created on start and nil'ed on stop/rollback makes that state unrepresentable, instead of handled.
• One BatchAuthenticator reader, bound once. registerBatcher, resolveTEEVerifierAddress, and isBatcherActive each construct their own binding from the same address, and isBatcherActive
  re-does a CodeAt probe per publish tick plus fetches the SystemConfig address via BatchAuthenticator.SystemConfig() even though the batcher already has l.RollupConfig.L1SystemConfigAddress.
  One reader object, code-checked once at startup, cuts the gate to at most 2 L1 calls per tick and makes it unit-testable with the mock-backend pattern already used in
  espresso_fallback_gate_test.go.
• Deduplicate the generated bindings. espresso/bindings/batch_authenticator.go (2,277 lines) duplicates the binding already shipped by the imported espresso-streamers module — and the ABIs
  differ: the module's copy declares the BatcherChangedThisBlock custom error, so the vendored copy is a contract revision behind, which affects revert decoding. Let's pick one canonical binding.
  Similarly, espresso/bindings/system_config.go is 2,640 lines for exactly one call (BatcherHash()); a minimal one-function binding would do.
• Shrink the txmgr seam. Since the batcher builds the txmgr config itself, it can capture the ChainSigner right there — then op-service/txmgr/espresso.go (the new
  SimpleTxManager.Sign/SignTransaction methods) and the runtime type assertion in initChainSigner can be deleted, and txmgr's public API stays identical to upstream, which matters for our
  rebases. Related: ChainSignerFactoryFromConfig is a near-verbatim copy of SignerFactoryFromConfig, which now has no production callers — we should keep exactly one factory.

Bigger candidates I'd like to discuss rather than request outright, since they're substantial churn in code you own:

• The submit/verify worker-pool machinery (espresso.go:49-803) is 6 struct types, 6 channels, and 13+ goroutines with a hand-balanced in-flight counter — for a throughput of one L2 block per 1–2
  s. A per-transaction submitAndConfirm retry loop under errgroup.SetLimit (the same construct driver.go already uses for DA requests) would be roughly a quarter of the code, would make
  evaluateSubmission/evaluateVerification pure and testable, and actually matches the spec's §4.1.3 wording ("for each block, spawns a goroutine that submits the batch and waits for
  finalization") more literally. If that's too much, a minimal version — dropping the two scheduler goroutines and the chan-of-chans worker queues in favor of workers reading a shared buffered
  channel — is behavior-identical and already a big win.
• The two tick loops each poll getSyncStatus and coordinate through the clearStateRequested CAS mailbox because neither owns all the state. Merging them into one loop (keeping both tickers, so
  cadence and observable behavior are unchanged) lets reset() call clearState directly and deletes the mailbox protocol entirely.
• The proper fix for the restart-rescan issue from my review: the v2 streamer still tracks fallbackHotShotPos but never applies it. Re-enabling the v1-style light-client fast-forward (or seeding
  the cursor from the light client's finalized height at the anchor's origin) would turn --espresso.origin-height-espresso into a lower-bound floor like the L2 flag, instead of the live cursor.
  That touches the streamer module's loop semantics, so I'd like to align on the design with you.

There's also a list of smaller cleanups (a publishMode enum instead of the three interacting booleans, the triplicated zeroed-sync-status guard, clock injection for the timing-dependent startup
functions, the two field-by-field config copy layers, dead code in espresso/ethclient.go, and the double-prefixed --espresso.espresso-attestation-service flag name). Happy to file those as
individual comments or a follow-up issue, whatever is easier for you to work through.

[philippecamacho]

Re #459 (comment)
@palango Thank you very much for the feedback.

• The big one: every restart re-scans HotShot history from the static ... espresso: --espresso.origin-height-espresso defaults to 0 with no validation and undocumented scan-start semantics #495 (+ associated PR )

• Second: the TEE publish path ignores the derivation-side enforcement grace window .. TEE batcher publishes during the batch-auth grace window while derivation still enforces sender-based auth #492 (+ associated PR )

• Then three timeout/wedge issues, all the same family: ... espresso: batcher wedges on hung network calls — no per-call deadlines, and the re-anchor gate holds the start mutex forever #493 (+ associated PR )

• Last area: test coverage. Most of the new safety-critical logic is pure functions ... espresso: safety-critical batcher logic is untested (active gate, reorg branches, clearState handshake, verification timeout) #496 (+ associated PR)

[philippecamacho]

Re #459 (comment)

@palango Thank you. Tickets #501, #502, #503, #504, #505 #506, #507, #508 have been created.

Happy to file those as individual comments or a follow-up issue, whatever is easier for you to work through.

Yes please.

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.

[palango]

A few smaller cleanups, none of them blocking:

• The publish mode is the interaction of three booleans (Espresso.Enabled, isFallbackAuthRequired, activeIsEspresso), reasoned about pairwise in two places. I'd compute a single mode
  enum once; that also drops a per-tx L1 RPC after the fork.
• The zeroed-sync-status guard exists three times in three shapes (espresso.go:1062, espresso_driver.go:346, and again inside computeSyncActions), so a fix to one copy will miss the
  others at some point. Extracting it has a nice side effect: nextBlockRange becomes a pure function.
• waitForLocalSafeHead and the submitter retry timing read the real clock, which is why their timeout behavior has no tests. op-service/clock exists for this.
• Two config copy layers copy fields one by one (CLIConfig → EspressoBatcherConfig, Service → DriverSetup); one already silently renames a field. Embedding removes both. Also,
  ChainSigner is embedded in BatcherService and promotes signing methods onto the whole service, better a named field.
• The submitter constructor: 9 exported names, functional options, a panic on nil client, one production caller. A plain config struct is enough here.
• Dead code that can just go: espresso/ethclient.go (duplicates batcherL1Adapter), opcrypto.Verify plus its test, AllowEmptyAttestationService (contradicts Check()), and a stale
  DebouncingHandler mention in a doc comment.
• --espresso.l1-url dials a second L1 client for a split-endpoint setup nobody runs, and Check() contradicts the flag's default. Drop it until someone actually needs it.

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.