feat(l1): implement eth-config #4625
Conversation
Lines of code reportTotal lines added: Detailed view |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
crates/networking/rpc/rpc.rs
Outdated
| "cancun": { "target": 3, "max": 6, "baseFeeUpdateFraction": 3338477 }, | ||
| "prague": { "target": 6, "max": 9, "baseFeeUpdateFraction": 5007716 }, | ||
| "osaka": { "target": 6, "max": 9, "baseFeeUpdateFraction": 5007716 }, | ||
| "cancun": { "baseFeeUpdateFraction": 3338477, "max": 6, "target": 3 }, | ||
| "prague": { "baseFeeUpdateFraction": 5007716, "max": 9, "target": 6 }, | ||
| "osaka": { "baseFeeUpdateFraction": 5007716, "max": 9, "target": 6 }, |
There was a problem hiding this comment.
Related to the reordering of fields
| pub target: u32, | ||
| pub max: u32, | ||
| pub base_fee_update_fraction: u64, | ||
| pub max: u32, | ||
| pub target: u32, |
There was a problem hiding this comment.
I reordered the fields so they'd get serialized in alphabetical order, since the spec for the methods requires them to be so
crates/vm/backends/revm/helpers.rs
Outdated
| Fork::Cancun => SpecId::CANCUN, | ||
| Fork::Prague => SpecId::PRAGUE, | ||
| Fork::Osaka => SpecId::OSAKA, | ||
| _ => SpecId::LATEST, |
There was a problem hiding this comment.
I wasn't sure what to do with the BPO forks given this version of REVM doesn't account for them, so I included this as a placeholder. If anyone has a better idea please let me know
There was a problem hiding this comment.
We can do the same as in #4643 (comment) and map it to the "real" fork right before BPO forks (in this case Osaka).
There was a problem hiding this comment.
Might be worth to just wait for that PR to be merged before this one TBH, since the share a bunch of changes
| let block_number = context.storage.get_latest_block_number().await?; | ||
| let fork_id = if let Some(timestamp) = activation_time { | ||
| ForkId::new(chain_config, genesis_header, timestamp, block_number).fork_hash |
There was a problem hiding this comment.
Does passing the latest block number here work? I thought the timestamp and block number need to be for the block which we want the fork ID from.
There was a problem hiding this comment.
I only meant for this to work properly with post-merge forks to be fair; since the EIP only requires the method to work form Cancun onwards. Probably should add a proper check and return an error for pre-Merge forks though (the EIP doesn't require the method to work for Paris and Shanghai either, but it's simple enough to return something sensible for them)
There was a problem hiding this comment.
Returning an error for pre-Paris forks is OK. We should also add a comment here, mentioning the block number is not really correct, but being enough for post-merge forks.
crates/networking/rpc/eth/client.rs
Outdated
| .storage | ||
| .get_block_by_number(context.storage.get_latest_block_number().await?) | ||
| .await? | ||
| .unwrap_or_default() |
There was a problem hiding this comment.
what is the default here?
There was a problem hiding this comment.
Both blockheader and blockbody implement default, which I assume sets the default value for their respective fields (0 for numeric ones, None for options, etc). I'm not sure if that was the best way to handle the unwrap though. Would you say it's better to just use expect? Since getting the latest block number but that block number being missing shouldn't ever really happen
There was a problem hiding this comment.
Yes, I don't think .unwrap_or_default() is the correct way of dealing with this. You should return an RpcError if there is no block
There was a problem hiding this comment.
Sure, fixed it here!
crates/networking/rpc/eth/client.rs
Outdated
|
|
||
| async fn handle(&self, context: RpcApiContext) -> Result<Value, RpcErr> { | ||
| let chain_config = context.storage.get_chain_config()?; | ||
| let latest_block_timestamp = context |
There was a problem hiding this comment.
are you sure you need the latest block timestamp and not the current time?
There was a problem hiding this comment.
Wouldn't the current time for configuration purposes be the timestamp of the latest block?
There was a problem hiding this comment.
not necesarily, you might be in a transition between forks. A block with the newest fork hasn't arrived yet, but the time for the old fork has passed.
I would verify how Geth does it
There was a problem hiding this comment.
From what I can see in this file, Geth also gets the latest block header. The relevant lines start at 1207, and what it does is
var (
c = api.b.ChainConfig()
t = api.b.CurrentHeader().Time
)
resp := configResponse{
Next: assemble(c, c.Timestamp(c.LatestFork(t)+1)),
Current: assemble(c, c.Timestamp(c.LatestFork(t))),
Last: assemble(c, c.Timestamp(c.LatestFork(^uint64(0)))),
}
CurrentHeader() just fetches the latest blockheader, assemble is the function that builds the eth-config response object
| pub fn next_fork(&self, block_timestamp: u64) -> Option<Fork> { | ||
| let next = if self.is_bpo5_activated(block_timestamp) { | ||
| None | ||
| } else if self.is_bpo4_activated(block_timestamp) && self.bpo5_time.is_some() { |
There was a problem hiding this comment.
It seems we need to implement ordering of forks, then this would be simpler
There was a problem hiding this comment.
I don't think just ordering of forks would suffice; unless there's an approach I'm missing. One, because just ordering wouldn't be enough to ask a fork what the fork after it would be; we'd still need to implement from for Fork or ahave an array with all the enum variants for Fork indexed by its number (or use a crate like strum. Two, because we'd also need to know if the fork is activated, which could only be done if the ChainConfig struct had an array for the fork timestamps we were indexing with the number assigned to the fork.
I still think doing the necessary refactors for that would be worth it, for the sake of future maintainability reasons too; I decided not to do it to not make this PR too big since it seemed a bit out of scope.
There was a problem hiding this comment.
Ok, please create a follow up ticket. Not planning to work on this right now, but it's good to have one
SDartayet
force-pushed
the
eth-config-method
branch
from
3a6360c to
677920a
Compare
Motivation
Achieving full Fusaka implementation before launch.
Description
This PR implements EIP-7910. To summarize, it consists of a new eth method, that returns the fork ID, chain ID, activation time, active precompiles and system contracts for the currently activated fork, and when applicable, the next and latest fork's the client has in its configuration.
As part of implementing the precompiles side of things, the logic of how we check an address is a precompile was refactored, introducing a method that returns the precompiles per fork so as to more closely tie the addresses we return in the
eth-configmethod and the way we check a precompile is active when executing it.To test this PR, you can follow the instructions in this page. Keep in mind the tests require to have whichever chain they're being run on fully synced.
Closes #3711