Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements RIP-7728 #2

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Implements RIP-7728 #2

wants to merge 28 commits into from

Conversation

mralj
Copy link
Collaborator

@mralj mralj commented Sep 25, 2024

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.

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 L2 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()
	}
}

@mralj mralj changed the title Implements RIP-7728 [P1] Implements RIP-7728 Sep 26, 2024
@mralj mralj marked this pull request as ready for review September 26, 2024 18:32
@mralj mralj self-assigned this Sep 26, 2024
mralj pushed a commit that referenced this pull request Oct 1, 2024
@mralj mralj changed the base branch from core/eip/7708 to master October 4, 2024 09:00
@mralj mralj changed the title [P1] Implements RIP-7728 Implements RIP-7728 Oct 8, 2024
mralj and others added 4 commits October 9, 2024 10:10
I decided to implement it this way after trying to integrate code with Arbitrum and having a better understanding of the calls that are made to the NewEvm
This approach makes it easier to both override the default config, and to have the option to "not to think about it"
Making rollup-geth easier to sync with both upstream and L2s
}

var ctx context.Context
if params.L1SLoadRPCTimeoutInSec > 0 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follower nodes cannot allow timeout when processing blocks, they must retry if the request failed or timed out. Is the idea here that sequencer nodes would enable params.L1SLoadRPCTimeoutInSec, but follower nodes would not?

Copy link
Collaborator Author

@mralj mralj Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!. I should've documented this, good catch.
The intent behind this was that, I was "afraid" that if no timeout mechanism was available to the Sequencers , that, even though improbable, this could lead to situation (either caused by some weird bug or even by an "attack") where Sequencer is stalled for a long period of time. And since most (all?) L2s have centralised sequencers this seemed "pretty bad".

And I agree, since followers are "just" processing blocks it makes no sense that they create "local forks" in case of the timeouts.

they must retry if the request failed or timed out

Agreed, I should've added some retry mechanism it makes sense that followers have one.


Thinking about this one bit more, I realise the question remains: What if "follower" cannot validate block because it cannot get response from it's RPC (eg. RPC "dies") or just for some reason errors? Do you have some thoughts about this one?

In scenarios where we just wait for RPC or "constantly" retry, the follower will just get stuck here and fall behind.
From UX perspective does it make sense to crash node in this scenario? I'm thinking that people running followers are more likely to notice "dead" node than one fallen behind (which I guess will have to be restarted anyways)?

Alternative is that we can "allow" (by eg. erroring after N retires or even introducing some longer timeout) follower to create "private fork" in hope it will "heal itself" eventually.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sequencer is stalled for a long period of time [...] this seemed "pretty bad"

Exactly, and high latency would also affect sequencer throughput. So a timeout (or some more sophisticated heuristic) is needed for sequencer nodes.

What if "follower" cannot validate block because it cannot get response from it's RPC (eg. RPC "dies") or just for some reason errors?

Follower nodes need reliable access to an L1 node anyway. They should independently verify deposits. Technically this is optional, but not verifying deposits is dangerous as it would allow the sequencer to mint a large amount of tokens and use it with exchanges/apps that don't wait for finalization.

So I think it's reasonable to assume that follower nodes have a reliable L1 RPC connection.

From UX perspective does it make sense to crash node in this scenario?

Yeah, interesting. I think retrying a few times (with maybe exponential backoff) would make sense, but I agree we shouldn't retry forever, better to crash after some tries.

Alternative is that we can "allow" (by eg. erroring after N retires or even introducing some longer timeout) follower to create "private fork" in hope it will "heal itself" eventually.

Hm... generally follower is not authorized to sign blocks, so I'm not sure this would work. And the follower's private chain would not become the canonical, finalized chain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Thanks for the feedback :)
I've created the issue for this so that I don't forget it, as I'm currently working on other things. You can check it out here. Given your feedback and this discussion, I think it is a reasonable starting point, and if need be, further improvements can be made later on.

@mralj mralj mentioned this pull request Dec 10, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants