Skip to content

eth_blockNumber returns L1 block for Arbitrum fork #6749

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

Closed
2 tasks done
ian-eulith opened this issue Jan 9, 2024 · 12 comments · Fixed by #7360
Closed
2 tasks done

eth_blockNumber returns L1 block for Arbitrum fork #6749

ian-eulith opened this issue Jan 9, 2024 · 12 comments · Fixed by #7360
Labels
first issue A good way to start contributing T-bug Type: bug

Comments

@ian-eulith
Copy link

ian-eulith commented Jan 9, 2024

Component

Anvil

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (6defdce 2023-05-17T00:04:50.286076000Z)

What command(s) is the bug in?

anvil

Operating System

macOS (Apple Silicon)

Describe the bug

$ anvil --chain-id 42161 --fork-url https://arb1.arbitrum.io/rpc &>/dev/null &
$ cast block-number --rpc-url https://arb1.arbitrum.io/rpc
168785678
$ cast block-number --rpc-url http://127.0.0.1:8545
18971606

An Anvil fork of Arbitrum returns the L1 block number over JSON-RPC. Based on the Arbitrum docs and the behavior of other providers, I expected it to return the L2 block number.

@ian-eulith ian-eulith added the T-bug Type: bug label Jan 9, 2024
@gakonst gakonst added this to Foundry Jan 9, 2024
@github-project-automation github-project-automation bot moved this to Todo in Foundry Jan 9, 2024
@ian-eulith
Copy link
Author

I realized I was running an old version of Anvil. I updated to the latest:

anvil 0.2.0 (71d8ea5 2024-01-09T15:10:35.401468000Z)

Same result.

@mattsse
Copy link
Member

mattsse commented Jan 10, 2024

I see, this makes me think calling this when we instantiate the fork is wrong:

// on arbitrum `block.number` is the L1 block which is included in the
// `l1BlockNumber` field
if let Some(l1_block_number) = block.other.get("l1BlockNumber").cloned() {
if let Ok(l1_block_number) = serde_json::from_value::<U256>(l1_block_number) {
env.block.number = l1_block_number.to_alloy();
}
}

but in solidity block.number is still the L1 block, right?

so this is a bit horrible. We want to fork off the L2, what should happen with the block.number variable over time?

@mds1
Copy link
Collaborator

mds1 commented Jan 10, 2024

Partly related to #748, you can see the arbitrum block.number case is called out there. @mattsse you are right that in solidity block.number returns L1 block values as described in https://docs.arbitrum.io/for-devs/concepts/differences-between-arbitrum-ethereum/block-numbers-and-time

@mattsse
Copy link
Member

mattsse commented Jan 11, 2024

hmm, okay so the right fix for abitrum would be to not use the l1blocknumber for the fork target block number (L2), but unclear what we should do to evm.blocknumber once new blocks are mined, simply increase by one as well or with some multiplier (L2 blocktime vs mainnet blocktime?)

@mds1
Copy link
Collaborator

mds1 commented Jan 11, 2024

hmm, okay so the right fix for abitrum would be to not use the l1blocknumber for the fork target block number (L2)

I believe this is correct, since the RPC returns the L2 block number, we want to use L2 block numbers as the fork blocks

but unclear what we should do to evm.blocknumber once new blocks are mined, simply increase by one as well or with some multiplier (L2 blocktime vs mainnet blocktime?)

Paging @gzeoneth to help confirm and decide the best course of action. Both of these options seem ok but I'm not sure what's best for users, but I think I'd avoid a 1:1 relation between and L1 and L2 block numbers since that will likely never occur in practice

@mattsse
Copy link
Member

mattsse commented Jan 15, 2024

okay, I thought about this for a bit and I think we need to track the best block separately in a dedicated variable and not using the evm.blocknumber since this is different for arb

mind taking this on @Evalir

/// Returns the current best number of the chain
pub fn best_number(&self) -> u64 {
self.env.read().block.number.try_into().unwrap_or(u64::MAX)
}
/// Sets the block number
pub fn set_block_number(&self, number: U256) {
let mut env = self.env.write();
env.block.number = number;
}

env.block.number = env.block.number.saturating_add(rU256::from(1));

@Evalir Evalir self-assigned this Jan 15, 2024
@gzeoneth
Copy link

hmm, okay so the right fix for abitrum would be to not use the l1blocknumber for the fork target block number (L2)

I believe this is correct, since the RPC returns the L2 block number, we want to use L2 block numbers as the fork blocks

but unclear what we should do to evm.blocknumber once new blocks are mined, simply increase by one as well or with some multiplier (L2 blocktime vs mainnet blocktime?)

Paging @gzeoneth to help confirm and decide the best course of action. Both of these options seem ok but I'm not sure what's best for users, but I think I'd avoid a 1:1 relation between and L1 and L2 block numbers since that will likely never occur in practice

Yes, the ArbBlockNumber (L2 block number) will need to be tracked separately. I think having evm.blocknumber being static (last known value) might make the most sense here, since it technically is unrelated to L2 blocks. It should still be able to advance evm.blocknumber with vm.roll but no need to make it correlate with other activity.

@gpxl-dev
Copy link

Ok I see my issue was closed as a dupe, but does add some important context - this issue goes away if you do not specify ----chain-id 42161

@grandizzy
Copy link
Collaborator

grandizzy commented Mar 9, 2024

okay, I thought about this for a bit and I think we need to track the best block separately in a dedicated variable and not using the evm.blocknumber since this is different for arb

isn't simpler to reuse storage best number e.g.

     /// Returns the current best number of the chain
     pub fn best_number(&self) -> u64 {
-        self.env.read().block.number.try_into().unwrap_or(u64::MAX)
+        self.blockchain.storage.read().best_number.try_into().unwrap_or(u64::MAX)
     }

@mattsse
Copy link
Member

mattsse commented Mar 9, 2024

@grandizzy hmm I think you're right,

I'm not sure what we should do with the env block number once we mined new blocks, also increase by 1?

@grandizzy
Copy link
Collaborator

@mattsse IMO that would be perfectly fine, the only thing to make sure is we don't get best block overwritten when mining new blocks, so line here

https://github.com/foundry-rs/foundry/blob/master/crates/anvil/src/eth/backend/mem/mod.rs#L926

should probably be

let block_number: U64 = storage.best_number.saturating_add(U64::from(1));

I will do a PR with proposed changes

@grandizzy
Copy link
Collaborator

Ok I see my issue was closed as a dupe, but does add some important context - this issue goes away if you do not specify ----chain-id 42161

looks like a different issue and will address in same PR - specifics of chain (like the fact that on Arbitrum block.number is the L1 block) are set based on the value of --chain-id, so when not passed chain id defaults to 31337 and Arbitrum specifics are not applied

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first issue A good way to start contributing T-bug Type: bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants