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

fix(anvil): use header.number not best_number #9151

Merged
merged 3 commits into from
Oct 21, 2024
Merged

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Oct 21, 2024

Motivation

Closes #9143

Solution

While applying arb chain specifics in convert_block, we use the self.best_number() to set header.number. This is wrong as best_number keeps increasing as the chain progresses.

Hence, RPC call such as eth_getBlockByNumber would return the incorrect number field.

Note that now header.number and l1BlockNumber in case of Arbitrum will be the same.

@@ -1917,7 +1917,7 @@ impl Backend {
) = NamedChain::try_from(self.env.read().env.cfg.chain_id)
{
// Block number is the best number.
block.header.number = self.best_number();
block.header.number = number;
Copy link
Collaborator

@grandizzy grandizzy Oct 21, 2024

Choose a reason for hiding this comment

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

I think the proper fix would be

            if self.is_fork() {
                // Block number is the best number.
                block.header.number = self.best_number();
            }

and a test for could be

// <https://github.com/foundry-rs/foundry/issues/9143>
#[tokio::test(flavor = "multi_thread")]
async fn test_arbitrum_non_fork_block_number() {
    let (api, _) =
        spawn(NodeConfig::test().with_chain_id(42161u64.into())).await;
    // Mine blocks and check properly returned by `eth_getBlockByNumber` API.
    api.mine_one().await;
    api.mine_one().await;
    let first_block = api.block_by_number(1.into()).await.unwrap().unwrap();
    assert_eq!(1, first_block.header.number);
    let latest_block = api.block_by_number(BlockNumberOrTag::Latest).await.unwrap().unwrap();
    assert_eq!(2, latest_block.header.number);
}

Copy link
Member Author

@yash-atreya yash-atreya Oct 21, 2024

Choose a reason for hiding this comment

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

Hmm. Wouldn't that cause the same issue in fork mode?

E.g:
If anvil was forked at X block number and then two blocks were mined i.e self.best_number() == X + 2.

We call eth_getBlockByNumber(X), the resultant block.number field would be X + 2 whereas it should be X.

Copy link
Member Author

Choose a reason for hiding this comment

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

Am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, I guess you're right, should be the same

@yash-atreya
Copy link
Member Author

There's a deeper issue with how we handle Arbitrum in anvil, which is causing the test_arbitrum_fork_block_number to fail.

On arbitrum env.block.number is l1BlockNumber. See

// on arbitrum `block.number` is the L1 block which is included in the

So when we fork off of arbitrum the env.block.number is set to l1BlockNumber that we get from get_block(fork_block_number).

However, upon mining a new block we incrementenv.block.number. See

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

This BlockEnv is then used in the TransactionExecutor which results in the block being mined with the incorrect block number (i.e l1BlockNumber + 1), instead of fork_block_number + 1.

Fix for this is a bit hacky:

In do_mine_block, for Arb specific chain_id's; temporarily set the env.block.number to storage.best_number (which is being correctly maintained). Use this modified BlockEnv in TransactionExecutor. After tx execution is complete and block is mined. Flip env.block.number back to maintain l1BlockNumber consistency.

@mattsse wdyt?

@yash-atreya
Copy link
Member Author

Ignoring test_arbitrum_fork_block_number for now. The solution is a bit hacky and non-trivial. Fixing in a separate PR.

@yash-atreya yash-atreya enabled auto-merge (squash) October 21, 2024 10:21
@yash-atreya yash-atreya requested a review from grandizzy October 21, 2024 12:22
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

This makes sense, would be nice to provide a quick fix for #9152 too so we won't break the other use case

@yash-atreya yash-atreya merged commit 6d9951f into master Oct 21, 2024
21 checks passed
@yash-atreya yash-atreya deleted the yash/fix-arb-blk branch October 21, 2024 12:29
rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
* fix(`anvil`): use header.number not best_number

* test

* ignore test_arbitrum_fork_block_number
@grandizzy grandizzy added T-bug Type: bug C-anvil Command: anvil labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-anvil Command: anvil T-bug Type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Anvil eth_getBlockByNumber returns incorrect block number for Arbitrum chain ID with no forking
2 participants