Conversation
c78ed9f to
319b3ab
Compare
319b3ab to
3e7dcd9
Compare
6cf6a1f to
eb6ff32
Compare
eb6ff32 to
ab13906
Compare
| // Sign represents the interface for signing things via eth_sign. | ||
| func (s *SignerClient) Sign(ctx context.Context, address common.Address, data []byte) ([]byte, error) { | ||
| var result hexutil.Bytes | ||
| if err := s.client.CallContext(ctx, &result, "eth_sign", address, data); err != nil { |
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
Jean is going to look and respond here as he did this work.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Hi @jjeangal, I hadn't realised this was talking to the kms signer.
So yes, agreed on the followups 👍
05288e9 to
78e33c9
Compare
|
Re #459 (comment)
|
Two comments point at a flag that was never registered. The note in espresso/cli.go claims op-batcher/flags/flags.go registers --espresso.fallback-auth-lead-time, and initEspresso's doc claims the fallback batcher reads a FallbackAuthLeadTime field on BatcherConfig. Neither the flag nor the field exists on any branch; both comments date from the TEE flag import and describe a knob the fallback batcher landed without.
The test's per-endpoint call counters, the two restarted-server flags and the recorded shutdown error are all written by httptest handler goroutines (or by throttlingLoop, for closeApp) and read directly by the test goroutine, so the package could not run under -race at all: five reports on every run. Make each of them atomic. The slices.Contains check over the counters becomes a small anyUncalled helper, since atomic.Int64 elements can't be compared by value.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
A few smaller cleanups, none of them blocking:
|
* espresso: delete dead supporting code Remove code with no callers: espresso/ethclient.go (duplicates batcherL1Adapter; FetchEspressoBatcherAddress belongs to the caff node in another repo), opcrypto.Verify and its test, and the AllowEmptyAttestationService escape hatch, which nothing sets, so Check() now requires the attestation service URL unconditionally. Also drop a doc reference to DebouncingHandler, which does not exist. * espresso: gofmt cli.go after removing the config field
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
espresso: replace hand-copied bindings with generated op-service/bindings (#513) * Delete espresso/bindings, an abigen copy that had drifted from the contracts. * Add op-service/bindings: ABI-only bindings for BatchAuthenticator and SystemConfig, one package per contract, generated from the contracts-bedrock forge artifacts by gen.sh and kept current by the check-bindings CI job. * Add CI job to detect BatchAuthenticator + SystemConfig bindings drifts Closes #504.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12fe8b1c28
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| case Skip: | ||
| s.numInFlightJobs.Add(-1) | ||
| continue |
There was a problem hiding this comment.
When Espresso returns ErrPermanent, this marks the asynchronous job complete without notifying BlockLoader. EnqueueBlocks has already appended the corresponding L2 block to queuedBlocks, so subsequent ranges begin after it and never resubmit it; if that block never entered Espresso, the streamer cannot produce a contiguous chain and L1 publishing stalls. Permanent submission and receipt-verification failures should reset/fail the loader or terminate the batcher rather than being silently discarded.
Useful? React with 👍 / 👎.
| case RetrySubmission: | ||
| s.submitJobQueue <- jobResp.job | ||
| continue |
There was a problem hiding this comment.
When an Espresso endpoint returns a transient error immediately, this requeues the job with no delay, so the four submission workers continuously issue requests as fast as the endpoint can reject them. During an outage or rate-limit response this creates a tight request loop across every in-flight block, potentially worsening throttling and consuming CPU; apply a cancellation-aware retry delay or backoff before requeueing.
Useful? React with 👍 / 👎.
9f35429 to
12fe8b1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f35429bf8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| _, err = l.Txmgr.Send(ctx, candidate) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to send registerBatcher transaction: %w", err) | ||
| } |
There was a problem hiding this comment.
When registerSigner passes gas estimation but reverts after an intervening verifier/governance state change, SimpleTxManager.Send returns the mined receipt with a nil error regardless of its status (op-service/txmgr/txmgr.go:354-359). Discarding that receipt here reports successful registration and starts the Espresso loops even though the TEE signer was not registered, causing every subsequent authenticateBatchInfo transaction to revert and batch publication to stall; reject a nil or failed-status receipt before continuing startup.
Useful? React with 👍 / 👎.
| if c.EspressoAttestationService == "" { | ||
| return fmt.Errorf("attestation service URL is required when Espresso is enabled") | ||
| } |
There was a problem hiding this comment.
When Espresso runs outside Nitro with --espresso.testing-batcher-private-key, initEspresso deliberately selects the pre-approved key and leaves Attestation empty, and registerBatcher consequently skips both proof generation and registration before ever using the service URL. Requiring the URL unconditionally here therefore rejects an otherwise supported devnet/test configuration unless operators provide a meaningless dummy endpoint; make this requirement conditional on the enclave-attestation path.
Useful? React with 👍 / 👎.
6 participants
Based on #448
Pulls in the Espresso/TEE batcher .
op-batcher/enclave/attestation.goand modifies the driver to add an Espresso path. Bulk of the changes is inespresso_-prefixed files.This is #447, re-hosted from an in-repo branch (now properly stacked).