[Optimism][Integrate]RIP-7728

[mralj]

Integrates RIP-7728 implemented in #2 to op-geth.

RIP-7728

This proposal introduces a new precompiled contract, L1SLOAD, that loads several storage slots from L1, requiring a contract address, storage keys, and RPC connection to L1.

Implementation details

To leverage this precompile node runner on L2; it must have the following:

1. Some Notion of latest L1 block
2. RPC connection to L1 node, preferably "fast one" since we are making L1 RPC call during EVM precompile execution.

Our rollup-geth has a flag --rollup.l1.rpc_endpoint from which it reads the URL of the required L1 RPC. This flag should be set when starting execution (op-geth) client.

Overriding the default config

Out of the box, rollup-geth provides the following method for "dealing" with the latest L1 block:

var defaultRollupPrecompilesConfig RollupPrecompileActivationConfig = RollupPrecompileActivationConfig{
	L1SLoad: L1SLoad{
		GetLatestL1BlockNumber: LetRPCDecideLatestL1Number, // CHANGE THIS METHOD
	},
}

The code is located in core/vm/contracts_rollup_overrides. We should pass into the GetLatestL1BlockNumber appropriate method for providing the latest L1 block on the op-stack blockchains.

Example:

var defaultRollupPrecompilesConfig RollupPrecompileActivationConfig = RollupPrecompileActivationConfig{
	L1SLoad: L1SLoad{
		GetLatestL1BlockNumber: GetLatestL1BlockNumber
	},
}

func GetLatestL1BlockNumber(state *state.StateDB) func() *big.Int {
	return func() *big.Int {
		addressOfL1BlockContract := common.Address{}
		slotInContractRepresentingL1BlockNumber := common.Hash{}
		return state.GetState(addressOfL1BlockContract, slotInContractRepresentingL1BlockNumber).Big()
	}
}

[jmederosalvarado]

lgtm

[protolambda]

This is not an endorsement of this particular RIP, just some early quick feedback for the process of building these type of L2-oriented diffs.

About the specific RIP now that we are here and thinking about it: I am very cautious with it, and not as familiar with it, but from a quick look I have several serious concerns:

• State-root based L1 state access from a L2 makes L1 archive nodes a requirement to fully sync history, this is very centralizing.
• Makes RPC calls a synchronous part of the EVM, this is not good for performance
• Affects proving of the rollup: the withdrawal proof now includes additional L1 inputs.
• The introduction of Verkle on L1 will completely break the storage proving (even if improving, it's still a breaking change to a proving system).
• The gas cost seems way too cheap for a blocking RPC call in the EVM. This can grind a L2 sequencer to a halt if the gas limit is high.
• The RIP makes no comment on speculative execution of a tx: this now involves synchronous RPC calls, and makes it costly to pre-validate a tx behavior. Like with EIP-4337 for txs that pay their own fee, or future op-stack work, this execution cost is of great concern.
• Has no versioning with the precompile input to do things different in the future.

The diff might be small, but the effect on security and sync-ability of a L2 is quite deep, and thus gives a bad perspective of the change.

Feedback on the general diff aspect:

• Diffs should include testing. I don't see coverage of the new functionality except a few JSON files. Does the L1 get mocked? It's difficult to review the testing.
• Documentation of the changes is useful; document what was changed, and why, in a change to the fork.yaml like the op-geth codebase does is helpful.

Additional tips, to make the diff more usable / maintainable:

• Split new functionality into separate files as much as possible
• Make it easy to understand the feature-flag aspect; how does this new functionality active? When does it run?
• More doc-comments would be helpful
• Avoid spurious formatting changes. If the L1 formatting is awkward we keep it, to avoid unnecessary merge conflicts.
• You can reduce Go-fmt changes by encapsulating the code better. E.g. separating the new struct fields with a blank line, to avoid it affecting the name/field alignment of other fields. And a comment about the below struct-fields being part of a particular rollup change is helpful too.

[tynes]

The way to solve the archive node requirements for RIP-7728 is to operate the precompile as a key/value store in 2 modes. Block building mode is backed by an RPC and it is expected that each call to the precompile also appends its returndata and a witness as a EIP-7685 Request to the block. In full node mode, the requests are validated before the state transition function and populated into precompile.

[mralj]

@protolambda thank you very much for taking the time and effort for providing the feedback, it's much appreciated!

This is not an endorsement of this particular RIP, just some early quick feedback for the process of building these type of L2-oriented diffs.

About the specific RIP now that we are here and thinking about it: I am very cautious with it, and not as familiar with it, but from a quick look I have several serious concerns:

• State-root based L1 state access from a L2 makes L1 archive nodes a requirement to fully sync history, this is very centralizing.
• Makes RPC calls a synchronous part of the EVM, this is not good for performance
• Affects proving of the rollup: the withdrawal proof now includes additional L1 inputs.
• The introduction of Verkle on L1 will completely break the storage proving (even if improving, it's still a breaking change to a proving system).
• The gas cost seems way too cheap for a blocking RPC call in the EVM. This can grind a L2 sequencer to a halt if the gas limit is high.
• The RIP makes no comment on speculative execution of a tx: this now involves synchronous RPC calls, and makes it costly to pre-validate a tx behavior. Like with EIP-4337 for txs that pay their own fee, or future op-stack work, this execution cost is of great concern.
• Has no versioning with the precompile input to do things different in the future.

The diff might be small, but the effect on security and sync-ability of a L2 is quite deep, and thus gives a bad perspective of the change.

Firstly I want to stress: we are not endorsing this RIP either. We decided to implement it just as an experiment, since it:

1. "Touched" different layers of the geth codebase
2. It was well defined, not too big, not too small
3. It was proposed by the Scroll team, but Optimism was in looking into adopting something similar as well.

Thus it looked like a good candidate to experiment with.
Regarding the issues you mentioned, while I agree with most of them, I don't think I'm the best person to address them :)

Feedback on the general diff aspect:

• Diffs should include testing. I don't see coverage of the new functionality except a few JSON files. Does the L1 get mocked? It's difficult to review the testing.

Testing is not in our focus too much during this "experimentation phase", but if the project is to move forwards, it's going to become extremely important.
I think we will need some help from L2s on this (specifically testing strategies of various L2s to make sure nothing breaks and everything works as expected).

That being said, this PR contains couple of "sanity check" tests, just to confirm implementation is not broken.
The added tests follow blueprint from the geth team for the other precompiles. I am relying on their testing setup that's why only a couple of json files were enough to cover these basic tests (specifically already existing testJson and testJsonFail functions).

Does the L1 get mocked?

Yeah, it does: link

Additionally, while this is by no means any proof that this was properly implemented, I have ran optimism node connected to op-geth with these changes applied locally, and the calling precompile from Solidty worked as expected and nothing was breaking. I understand this is far from proper testing but, at least it worked on my machine™️ 😅

• Documentation of the changes is useful; document what was changed, and why, in a change to the fork.yaml like the op-geth codebase does is helpful.

Additional tips, to make the diff more usable / maintainable:

• Split new functionality into separate files as much as possible
• Make it easy to understand the feature-flag aspect; how does this new functionality active? When does it run?
• More doc-comments would be helpful
• Avoid spurious formatting changes. If the L1 formatting is awkward we keep it, to avoid unnecessary merge conflicts.
• You can reduce Go-fmt changes by encapsulating the code better. E.g. separating the new struct fields with a blank line, to avoid it affecting the name/field alignment of other fields. And a comment about the below struct-fields being part of a particular rollup change is helpful too.

Regarding all above I can only say, again, thank you very much for the feeback!
We are already doing some "stuff" you suggested (e.g. separating to new files as much as possible, and all of these are suffixed with rollup, e.g. precompiles_rollup.go), but certainly, as you noticed there is room for improvement.