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

Improved error message when forking with non archive node #5131

Merged
merged 8 commits into from
Jun 20, 2023
Merged

Improved error message when forking with non archive node #5131

merged 8 commits into from
Jun 20, 2023

Conversation

agostbiro
Copy link
Contributor

Motivation

Improve error message when a user forks from an older block with a non-archive node. Motivated by #4962

Solution

The solution is to log the following message when an RPC error was caused with high certainty by forking from a non-archive node:

It looks like you're trying to fork from an older block with a non-archive node which is not
supported. Please try to change your RPC url to an archive node if the issue persists.

We can detect this if we get an RPC error message that includes "missing trie node", e.g.:

(code: -32000, message: missing trie node 5e96223edf7c35fa98934f77cb7788297b0f89af912caae197623ce5250029c7 (path ), data: None)

Or if we try to fetch an older block and get_block returns null.

I chose error log level in the cli create instead of warning as warnings don't seem to be logged by default and I didn't want to change to default log levels. I put the warning message into the panic message in the anvil crate as error level logs don't seem to be logged by default in anvil.

As an alternative solution I explored creating a separate error type for forking from a non-archive node, but ultimately decided against it for the following reasons:

  1. We cannot be 100% certain that the above RPC errors are caused by connecting to a non-archive node.
  2. Revm panics in certain cases before we could convert into a "non-archive node error".
  3. Anvil exits with panic immediately when initialization fails due to a potential non-archive node error.

Testing

A repro for #4962 can be found in this repository: https://github.com/agostbiro/foundry-fork-error-repro/

I tested the changes locally by running forge test on the repro and by running anvil in fork mode with an older block using a non-archive node.

Reviewing

Many of the changes in anvil/src/config.rs are an unfortunate consequence of rustfmt deciding on a different formatting for the file after I changed a few lines. I isolated the formatting changes in a separate commit.

@mattsse mattsse requested a review from Evalir June 12, 2023 11:58
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

I like where this is going! I have a few nits, mainly:

  • slimming down abstractions and the complexity and doing things in a most straightforward manner
  • Checking if we can actually fork recent blocks with non-archive nodes, as I strongly believe this is the case (fwiw most RPC urls used are not archive nodes and work perfectly for tests, we shouldn't block people from using them)

anvil/src/config.rs Outdated Show resolved Hide resolved
anvil/src/config.rs Outdated Show resolved Hide resolved
anvil/src/config.rs Outdated Show resolved Hide resolved
common/src/constants.rs Show resolved Hide resolved
evm/src/executor/fork/backend.rs Outdated Show resolved Hide resolved
evm/src/executor/fork/init.rs Outdated Show resolved Hide resolved
@agostbiro
Copy link
Contributor Author

  • Checking if we can actually fork recent blocks with non-archive nodes, as I strongly believe this is the case (fwiw most RPC urls used are not archive nodes and work perfectly for tests, we shouldn't block people from using them)

Hey, thanks for the review! Just to clarify: the warning is not displayed if a node successfully returns older blocks, so I don't think it should a problem when forking from a recent block with a non-archive node. I'm addressing the other issues.

@Evalir Evalir requested review from Evalir and mattsse June 19, 2023 15:21
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

aight this seems correct now! @mattsse would love your eyes on this if possible

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice, ty, big QoL improvement and should hopefully prevent new foundry issues for non archive nodes

Comment on lines +835 to +836
// the block, and the block number is less than equal the latest block, then
// the user is forking from a non-archive node with an older block number.
Copy link
Member

Choose a reason for hiding this comment

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

this sounds correct

@Evalir Evalir merged commit 794f831 into foundry-rs:master Jun 20, 2023
17 checks passed
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.

None yet

4 participants