forked from ethereum/go-ethereum
-
Notifications
You must be signed in to change notification settings - Fork 1
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
mralj
wants to merge
28
commits into
master
Choose a base branch
from
core/rip/7728
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
0ae7e7b
dial L1 RPC client passed via required flag
mralj 1974d92
added L1SLoad sekelton
mralj 99ccaf7
bugfix
mralj a24608e
added ability to activate rollup precompiles from eth/internal and et…
mralj 204ef24
added example how code in overrides would change
mralj 759dda7
added L1SLoad sekelton
mralj f0dd217
implements L1SLOAD contract
mralj c4b24af
added rpc call timeout strategy
mralj bea23a3
added batch call for StoragesAt as well as tests
mralj e9a5c28
added test for too long input edgecase
mralj 56c1f67
added missing mocks for tests
mralj 5f039c5
Merge pull request #4 from NethermindEth/core/rip/7728-precompile-impl
mralj 6a98534
added L1SLoad sekelton
mralj ef91bcd
implements L1SLOAD contract
mralj 2a7b7d7
cmd - rollup specific files
mralj 0f74390
cleaned up code & created more rollup specific files
mralj 1ccbc95
simplified the code
mralj b72098e
added missing "," - fixed comptime bug
mralj 76a2339
internal/ethapi and tracers use pre-existing function call
mralj bd56bdc
code cleanup after trying to merge into arb/op-geth
mralj bdd7b7d
ethclient moved to node.Node
mralj d409ef8
bugfixes - l1rpc activated at proper point and precompile address
mralj ee58cfe
missed cleanup of ActivePrecompiles
mralj 128b120
removed unused import - popped up after rebasing
mralj 42855ae
concurrent map r/w bugfix
mralj d4cd646
rollup precompile config is glob. variable
mralj 237b78f
removed unnecessary call to vm.SetVmL1RpcClient
mralj ae96b7b
Merge pull request #5 from NethermindEth/feat/rollup-specific-files
mralj File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
package utils | ||
|
||
import ( | ||
"github.com/ethereum/go-ethereum/core/vm" | ||
"github.com/ethereum/go-ethereum/internal/flags" | ||
"github.com/ethereum/go-ethereum/log" | ||
"github.com/ethereum/go-ethereum/node" | ||
"github.com/urfave/cli/v2" | ||
) | ||
|
||
var L1NodeRPCEndpointFlag = &cli.StringFlag{ | ||
Name: "rollup.l1.rpc_endpoint", | ||
Usage: "L1 node RPC endpoint eg. http://0.0.0.0:8545", | ||
Category: flags.RollupCategory, | ||
} | ||
|
||
var RollupFlags = []cli.Flag{ | ||
L1NodeRPCEndpointFlag, | ||
} | ||
|
||
// TODO: when we have clearer picture of how do we want rollup "features" (EIPs/RIPs) to be activated | ||
// make this "rule" activated (ie. if not "rule activated" then L1 client can simply be nil) | ||
func ActivateL1RPCEndpoint(ctx *cli.Context, stack *node.Node) { | ||
if !ctx.IsSet(L1NodeRPCEndpointFlag.Name) { | ||
log.Error("L1 node RPC endpoint URL not set", "flag", L1NodeRPCEndpointFlag.Name) | ||
return | ||
} | ||
|
||
l1RPCEndpoint := ctx.String(L1NodeRPCEndpointFlag.Name) | ||
stack.RegisterEthClient(l1RPCEndpoint) | ||
vm.SetVmL1RpcClient(stack.EthClient()) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,146 @@ | ||
//[rollup-geth] | ||
// These are rollup-geth specific precompiled contracts | ||
|
||
package vm | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"math/big" | ||
"time" | ||
|
||
"github.com/ethereum/go-ethereum/common" | ||
"github.com/ethereum/go-ethereum/log" | ||
"github.com/ethereum/go-ethereum/params" | ||
) | ||
|
||
type RollupPrecompileActivationConfig struct { | ||
L1SLoad | ||
} | ||
|
||
type L1RpcClient interface { | ||
StoragesAt(ctx context.Context, account common.Address, keys []common.Hash, blockNumber *big.Int) ([]byte, error) | ||
} | ||
|
||
var ( | ||
rollupL1SloadAddress = common.BytesToAddress([]byte{0x01, 0x01}) | ||
precompiledContractsRollupR0 = PrecompiledContracts{ | ||
rollupL1SloadAddress: &L1SLoad{}, | ||
} | ||
) | ||
|
||
func activeRollupPrecompiledContracts(rules params.Rules) PrecompiledContracts { | ||
switch rules.IsR0 { | ||
case rules.IsR0: | ||
return precompiledContractsRollupR0 | ||
default: | ||
return nil | ||
} | ||
} | ||
|
||
// ActivateRollupPrecompiledContracts activates rollup-specific precompiles | ||
func (pc PrecompiledContracts) ActivateRollupPrecompiledContracts(rules params.Rules, config RollupPrecompileActivationConfig) { | ||
activeRollupPrecompiles := activeRollupPrecompiledContracts(rules) | ||
for k, v := range activeRollupPrecompiles { | ||
pc[k] = v | ||
} | ||
|
||
// NOTE: if L1SLoad was not activated via chain rules this is no-op | ||
pc.activateL1SLoad(config.L1RpcClient, config.GetLatestL1BlockNumber) | ||
} | ||
|
||
func ActivePrecompilesIncludingRollups(rules params.Rules) []common.Address { | ||
activePrecompiles := ActivePrecompiles(rules) | ||
activeRollupPrecompiles := activeRollupPrecompiledContracts(rules) | ||
|
||
for k := range activeRollupPrecompiles { | ||
activePrecompiles = append(activePrecompiles, k) | ||
} | ||
|
||
return activePrecompiles | ||
} | ||
|
||
//INPUT SPECS: | ||
//Byte range Name Description | ||
//------------------------------------------------------------ | ||
//[0: 19] (20 bytes) address The contract address | ||
//[20: 51] (32 bytes) key1 The storage key | ||
//... ... ... | ||
//[k*32-12: k*32+19] (32 bytes)key_k The storage key | ||
|
||
type L1SLoad struct { | ||
L1RpcClient L1RpcClient | ||
GetLatestL1BlockNumber func() *big.Int | ||
} | ||
|
||
func (c *L1SLoad) RequiredGas(input []byte) uint64 { | ||
storageSlotsToLoad := len(input[common.AddressLength-1:]) / common.HashLength | ||
storageSlotsToLoad = min(storageSlotsToLoad, params.L1SLoadMaxNumStorageSlots) | ||
|
||
return params.L1SLoadBaseGas + uint64(storageSlotsToLoad)*params.L1SLoadPerLoadGas | ||
} | ||
|
||
func (c *L1SLoad) Run(input []byte) ([]byte, error) { | ||
if !c.isL1SLoadActive() { | ||
log.Error("L1SLOAD called, but not activated", "client", c.L1RpcClient, "and latest block number function", c.GetLatestL1BlockNumber) | ||
return nil, errors.New("L1SLOAD precompile not active") | ||
} | ||
|
||
if len(input) < common.AddressLength+common.HashLength { | ||
return nil, errors.New("L1SLOAD input too short") | ||
} | ||
|
||
countOfStorageKeysToRead := (len(input) - common.AddressLength) / common.HashLength | ||
thereIsAtLeast1StorageKeyToRead := countOfStorageKeysToRead > 0 | ||
allStorageKeysAreExactly32Bytes := countOfStorageKeysToRead*common.HashLength == len(input)-common.AddressLength | ||
|
||
if inputIsInvalid := !(thereIsAtLeast1StorageKeyToRead && allStorageKeysAreExactly32Bytes); inputIsInvalid { | ||
return nil, errors.New("L1SLOAD input is malformed") | ||
} | ||
|
||
contractAddress := common.BytesToAddress(input[:common.AddressLength]) | ||
input = input[common.AddressLength-1:] | ||
contractStorageKeys := make([]common.Hash, countOfStorageKeysToRead) | ||
for k := 0; k < countOfStorageKeysToRead; k++ { | ||
contractStorageKeys[k] = common.BytesToHash(input[k*common.HashLength : (k+1)*common.HashLength]) | ||
} | ||
|
||
var ctx context.Context | ||
if params.L1SLoadRPCTimeoutInSec > 0 { | ||
c, cancel := context.WithTimeout(context.Background(), time.Second*time.Duration(params.L1SLoadRPCTimeoutInSec)) | ||
ctx = c | ||
defer cancel() | ||
} else { | ||
ctx = context.Background() | ||
} | ||
|
||
res, err := c.L1RpcClient.StoragesAt(ctx, contractAddress, contractStorageKeys, c.GetLatestL1BlockNumber()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return res, nil | ||
} | ||
|
||
func (c *L1SLoad) isL1SLoadActive() bool { | ||
return c.GetLatestL1BlockNumber != nil && c.L1RpcClient != nil | ||
} | ||
|
||
func (pc PrecompiledContracts) activateL1SLoad(l1RpcClient L1RpcClient, getLatestL1BlockNumber func() *big.Int) { | ||
if precompileNotRuleActivated := pc[rollupL1SloadAddress] == nil; precompileNotRuleActivated { | ||
return | ||
} | ||
|
||
if rpcClientNotOverridenUseDefaultOne := l1RpcClient == nil; rpcClientNotOverridenUseDefaultOne { | ||
l1RpcClient = defaultRollupPrecompilesConfig.L1RpcClient | ||
} | ||
|
||
if latestBlockGetterNotOverridenUseDefaultOne := getLatestL1BlockNumber == nil; latestBlockGetterNotOverridenUseDefaultOne { | ||
getLatestL1BlockNumber = defaultRollupPrecompilesConfig.GetLatestL1BlockNumber | ||
} | ||
|
||
pc[rollupL1SloadAddress] = &L1SLoad{ | ||
L1RpcClient: l1RpcClient, | ||
GetLatestL1BlockNumber: getLatestL1BlockNumber, | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
//[rollup-geth] | ||
// These are rollup-geth specific precompiled contracts | ||
|
||
package vm | ||
|
||
import ( | ||
"math/big" | ||
) | ||
|
||
var defaultRollupPrecompilesConfig RollupPrecompileActivationConfig = RollupPrecompileActivationConfig{ | ||
L1SLoad: L1SLoad{ | ||
GetLatestL1BlockNumber: LetRPCDecideLatestL1Number, | ||
}, | ||
} | ||
|
||
func SetVmL1RpcClient(c L1RpcClient) { | ||
defaultRollupPrecompilesConfig.L1RpcClient = c | ||
} | ||
|
||
// generateRollupPrecompiledContractsOverrides generates rollup precompile config including L2 specific overrides | ||
func generateRollupPrecompiledContractsOverrides(evm *EVM) RollupPrecompileActivationConfig { | ||
return RollupPrecompileActivationConfig{ | ||
L1SLoad{ | ||
L1RpcClient: evm.Config.L1RpcClient, | ||
GetLatestL1BlockNumber: LetRPCDecideLatestL1Number, | ||
}, | ||
} | ||
} | ||
|
||
// [OVERRIDE] LetRPCDecideLatestL1Number | ||
// Each rollup should override this function so that it returns | ||
// correct latest L1 block number | ||
func LetRPCDecideLatestL1Number() *big.Int { | ||
return nil | ||
} | ||
|
||
// [OVERRIDE] getLatestL1BlockNumber | ||
// Each rollup should override this function so that it returns | ||
// correct latest L1 block number | ||
// | ||
// EXAMPLE 2 | ||
// func GetLatestL1BlockNumber(state *state.StateDB) func() *big.Int { | ||
// return func() *big.Int { | ||
// addressOfL1BlockContract := common.Address{} | ||
// slotInContractRepresentingL1BlockNumber := common.Hash{} | ||
// return state.GetState(addressOfL1BlockContract, slotInContractRepresentingL1BlockNumber).Big() | ||
// } | ||
// } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
package vm | ||
|
||
import ( | ||
"context" | ||
"math/big" | ||
"testing" | ||
|
||
"github.com/ethereum/go-ethereum/common" | ||
) | ||
|
||
type MockL1RPCClient struct{} | ||
|
||
func (m MockL1RPCClient) StoragesAt(ctx context.Context, account common.Address, keys []common.Hash, blockNumber *big.Int) ([]byte, error) { | ||
// testcase is in format "abab", this makes output lenght 2 bytes | ||
const mockedRespValueSize = 2 | ||
mockResp := make([]byte, mockedRespValueSize*len(keys)) | ||
for i := range keys { | ||
copy(mockResp[mockedRespValueSize*i:], common.Hex2Bytes("abab")) | ||
} | ||
|
||
return mockResp, nil | ||
} | ||
|
||
func TestPrecompiledL1SLOAD(t *testing.T) { | ||
mockL1RPCClient := MockL1RPCClient{} | ||
|
||
allPrecompiles[rollupL1SloadAddress] = &L1SLoad{} | ||
allPrecompiles.activateL1SLoad(mockL1RPCClient, func() *big.Int { return big1 }) | ||
|
||
testJson("l1sload", rollupL1SloadAddress.Hex(), t) | ||
testJsonFail("l1sload", rollupL1SloadAddress.Hex(), t) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, and high latency would also affect sequencer throughput. So a timeout (or some more sophisticated heuristic) is needed for sequencer nodes.
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.
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.
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.
There was a problem hiding this comment.
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.