feat: eth_sendRawTransactionConditional

[hamdiallam]

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

Extensions

Transactions with an attached conditional are bound to the block builder it was submitted to. This means that these transactions must be excluded from being gossiped to any configured peers via the p2p protocol.

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]

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]

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

[hamdiallam]

thanks for the feedback! Addressed most of them and generating the forkdiff

In regards to the changes related to the transactionEnvelope that would require the specs PR there are two options

1. Make it a formal spec such that all EL clients need to do this.
2. In the proxy use the op-conductor api to determine the active sequencer and route the transaction directly. We would filter these transactions from being gossiped to avoid reverted txs in the event there's an active sequencer change and the conditional is erased and included when gossiped to the new active replica (causing a potential revert)

cc @tynes the change was needed since we rely on tx gossip between the sequencer replicas

Curious which option you guys think makes the most sense. I think (2) so that things are completely out of protocol and we an enshrine later. This does open DoS concerns with the active sequencer but we have the rate limits in place at both the proxy layer and in op-geth with @protolambda ’s suggestion

[hamdiallam]

Okay so ended up going with (2). Conditional transactions are bound to the block builder they were submitted to.

Removed all the rlp encoding logic and reverted the change to TransactionPacket. Added an check to the broadcast logic that filters out transactions with conditionals from being broadcasted.

For an architecture where different replicas can be the active sequencer, we can have the proxy simply broadcast the conditional transaction to all the replicas instead of an EL change to gossip these txs

[hamdiallam]

soft bump @protolambda @tynes

[tynes]

I am more open to the idea of sending the conditional requests via p2p even tho there is no way to make sure they are not malleated

[tynes]

Curious what you think @protolambda about the P2P part of this that was previously implemented

[tynes]

Curious what you think @protolambda about the P2P part of this that was previously implemented

Looks like we don't need it due to changes to proxyd where it can forward txs to multiple endpoints

[hamdiallam]

soft bump @tynes @protolambda

[protolambda]

Reviewed everything except the test-cases, will revisit those tomorrow.

[anacrolix]

Seems good to me. I dug into the RPC API to check some of the assumptions.

I don't think the rate limiting is quite right, but I don't know if the caller should be handling their allowed rate, or if it should be blocking in the RPC (I guess the former).

There's a lot of use of atomics which can mask concurrency bugs but that seems to be widespread in op-geth.

[axelKingsley]

I looked over this and I think we should prioritize shipping it soon. There are some open questions, and some resolved ones that I closed.

@protolambda - please confirm this is not blocking: https://github.com/ethereum-optimism/op-geth/pull/330/files#r1751043303

@tynes and @anacrolix - I see we've got an approval from Matt, and then some further comments, but none of them appeared blocking. Is there anything critical we need done before merging this?

Looks like the API interface for this comes into txproxy here: ethereum-optimism/infra#42

And there's a monorepo E2E test for this feature: https://github.com/ethereum-optimism/optimism/pull/11671/files

Given the E2E test passes for @hamdiallam , and it's ready for its own review, I feel comfortable with the changes.

If there's still a lot of activity here because we believe this feature isn't ready to land, I want to make sure we know which conversations are critical. Since I don't see any, I want to ask for some 👍 emojis so we can merge this.