feat: eth_sendRawTransactionConditional

hamdiallam commented 3 weeks ago

Closes ethereum-optimism/ecopod#1024

Sequencer implementation of eth_sendRawTransactionConditional described in this design doc, refreshed from @tynes initial draft

Design Doc Divergence

Rather than exposing a separate port to expose the sequencer rpcs, we instead expand on the --rollup configuration to specify whether or not this endpoint should be enabled. The default, --rollup.sequencerenabletxconditional is false, thus retaining the desired outcome that this endpoint must be explicitly enabled by the operator

Open Questions

--rollup.sequencerhttp is the public rpc for tx submission when running in verifier mode. Similarly should we enable the forwarding of sendRawTransactionConditional? My intuition says no as this is not a user-facing endpoint and bundlers could directly use the public endpoints for a chain
The conditional attached to the tx is gossiped to peers by adding a backwards-compatible envelope to the encoded TransactionPacket. It is only backwards compatible when reading the prior format. If a peer isn't running a version of op-geth with this patch, it will fail to decode the message. Alternatively, instead of the envelope we can choose to introduce a new p2p message TransactionConditionalsMsg = 0xff and propagate these txs seperately internally to broadcastTransactions()

Metrics

Not all the metrics in the design doc are implemented here. In op-geth we primarily track mempool time and rejection. In the proxy, we'll track the request breakdown by authenticated caller and top-level success rate

The mempool latency is tracked by attaching the submission time to the conditional and recording the elapsed time when the transaction is mined or demoted from the txpool

protolambda commented 1 day ago

Reviewed the implementation details and some DoS risks. Would also be helpful if a test plan is shared, and if someone from the node team can review also, because of the size of the PR.

And request: can you add a 4337 category to the fork.yaml file, and describe the changes? That way the op-geth.optimism.io website accurately represents the geth dfiff, and documents the 4337 work nicely. You can run the tool locally, to generate a local index.html, see https://github.com/protolambda/forkdiff/ (make sure to pull latest upstream commits in local repository checkout)

tynes commented 1 day ago

Thoughts on squashing this into a single commit before merge? Or we just do squash merges over here?

ethereum-optimism / op-geth