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

createSelectFork does not fork latest block number if you sleep #6616

Closed
1 of 2 tasks
maa105 opened this issue Dec 18, 2023 · 7 comments · Fixed by #7087
Closed
1 of 2 tasks

createSelectFork does not fork latest block number if you sleep #6616

maa105 opened this issue Dec 18, 2023 · 7 comments · Fixed by #7087
Assignees
Labels
good first issue Good for newcomers T-bug Type: bug

Comments

@maa105
Copy link

maa105 commented Dec 18, 2023

Component

Forge

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

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (68124b5 2023-11-24T16:01:13.759990613Z)

What command(s) is the bug in?

createSelectFork

Operating System

Linux

Describe the bug

createSelectFork doesnt seem to check if a new block is mined check this:

pragma solidity ^0.8.21;

import {Test, console} from "forge-std/Test.sol";
import {Vm} from "forge-std/Vm.sol";

contract EntryTest is Test {
    function testStart() public {        
        vm.createSelectFork('rpc_url');
        console.log(block.number);
        for(uint i; i < 3; i++) {
            vm.sleep(15000);
            vm.createSelectFork('rpc_url');
            console.log(block.number);
        }
    }
}

if you ran this on sepolia test eth you will get the same block number printed out several times. while I expect that after 15 seconds a new block arrives and the createSelectFork must fork from that latest block

P.S. this takes ~45 seconds to finish so be patient

@maa105 maa105 added the T-bug Type: bug label Dec 18, 2023
@mattsse
Copy link
Member

mattsse commented Jan 9, 2024

catching up here

the issue here is that we're mapping missing block number to latest and then check if there's already an existing fork that we can reuse:

let fork_id = create_fork_id(&fork.url, fork.evm_opts.fork_block_number);
trace!(?fork_id, "created new forkId");
if let Some(fork) = self.forks.get_mut(&fork_id) {
fork.num_senders += 1;
let _ = sender.send(Ok((fork_id, fork.backend.clone(), fork.opts.env.clone())));
} else {

this means that we don't create a new fork in this example, but I think we want to, as you described.
we should instead check the block number for Latest first

@mattsse mattsse added the good first issue Good for newcomers label Jan 9, 2024
@bolajahmad
Copy link

Looked into this issue, should it still be open? I would like to work on it, if it's still open.

The code has been updated already and now looks like this,

        let fork_id = ForkId::new(&fork.url, fork.evm_opts.fork_block_number);
        trace!(?fork_id, "created new forkId");


        if let Some(fork) = self.forks.get(&fork_id).cloned() {
            // assign a new unique fork id but reuse the existing backend
            let unique_fork_id: ForkId =
                format!("{}-{}", fork_id.as_str(), fork.num_senders()).into();
            trace!(?fork_id, ?unique_fork_id, "created new unique forkId");
            fork.inc_senders();
            let backend = fork.backend.clone();
            let env = fork.opts.env.clone();
            self.forks.insert(unique_fork_id.clone(), fork);
            let _ = sender.send(Ok((unique_fork_id, backend, env)));
        } else {
            // there could already be a task for the requested fork in progress

Does this change resolve the issue or further dev work is still needed?

@grandizzy
Copy link
Collaborator

Does this change resolve the issue or further dev work is still needed?

I had a look at this and it's still reproducible but if I am not missing something the fix is more complex than suggested. Thing is that create_fork is used for roll fork as well, where block to roll is known and passed by cheatcode user

https://github.com/foundry-rs/foundry/blob/master/crates/evm/core/src/fork/multi.rs#L295-L296

    opts.evm_opts.fork_block_number = Some(block);
    self.create_fork(opts, sender)

In case of creating a new fork the fork_block_number is always None https://github.com/foundry-rs/foundry/blob/master/crates/evm/core/src/fork/multi.rs#L286

    Request::CreateFork(fork, sender) => self.create_fork(*fork, sender),

therefore the ForkId will always match the initial created fork https://github.com/foundry-rs/foundry/blob/master/crates/evm/core/src/fork/multi.rs#L257-L261

    fn create_fork(&mut self, fork: CreateFork, sender: CreateSender) {
        let fork_id = ForkId::new(&fork.url, fork.evm_opts.fork_block_number);
        trace!(?fork_id, "created new forkId");

        if let Some(fork) = self.forks.get(&fork_id).cloned() {

I think the best way would be to be able to set latest block as fork.evm_opts.fork_block_number when creating a new fork but not sure what's the best way to accomplish this as the fork environment / provider is not yet initialized @mattsse wdyt? thanks

@mattsse
Copy link
Member

mattsse commented Feb 9, 2024

yeah I think the best solution is to always assume the fork needs to be created if the block_number is none or latest and then we check afterwards if the url@number pair already exists?

so I guess need to shift around a few self.forks.get(&fork_id) checks

@grandizzy
Copy link
Collaborator

yeah I think the best solution is to always assume the fork needs to be created if the block_number is none or latest and then we check afterwards if the url@number pair already exists?

so I guess need to shift around a few self.forks.get(&fork_id) checks

makes sense, @bolajahmad lmk if you still want to pick this up, otherwise I can craft a PR for it

@mattsse
Copy link
Member

mattsse commented Feb 9, 2024

@grandizzy feel free to take this :)

ty!

@grandizzy
Copy link
Collaborator

@grandizzy feel free to take this :)

ty!

thanks, I made a draft PR with a proposed solution #7087 please have a look when you have time

mattsse added a commit that referenced this issue Feb 19, 2024
#7087)

* - if no block provided then fork is created using latest block for fork id (url@latest_block). if block provided then fork id is url@provided_block (this means there'll no longer be a url@latest fork id but always url@block)
- after creation of fork the id is checked if in inserted forks. If it doesn't exist then it's inserted, otherwise the existing backend is reused and a new fork url@latest_block-1 recorded

CreatedFork::inc_senders increments number of senders and returns the new unique fork id. CreatedFork::num_senders was removed
MultiForkHandler::insert_new_fork added to handle fork insertion / send on channels

* Dummy test

* Add back comment, minor code style

* Consume fork ids in insert_new_fork

* add comment

---------

Co-authored-by: Matthias Seitz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers T-bug Type: bug
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants
@maa105 @mattsse @grandizzy @bolajahmad and others