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

vm.roll() effect delayed when via_ir = true in solidity >=0.8.15 #4934

Closed
2 tasks done
hananbeer opened this issue May 12, 2023 · 5 comments
Closed
2 tasks done

vm.roll() effect delayed when via_ir = true in solidity >=0.8.15 #4934

hananbeer opened this issue May 12, 2023 · 5 comments
Labels
T-bug Type: bug

Comments

@hananbeer
Copy link

Component

Forge

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

  • Foundry
  • Foundryup

What version of Foundry are you on?

Windows: forge 0.2.0 (bb62e3c 2023-03-15T09:18:40.7502879Z), Linux: forge 0.2.0 (f3c20d5 2023-05-12T00:06:03.466532062Z)

What command(s) is the bug in?

forge test

Operating System

Windows

Describe the bug

vm.roll() is expected to set the block number such that subsequent reads of block.number are immediately updated.

however, strangely only with via_ir = true AND only for solidity >=0.8.15, the effect takes place only after calling a contract.

PoC:

// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.15;

import "forge-std/Test.sol";

contract Bug {
    function check() public {
        console.log("[contract   ] block: %d, time: %d, hash: %x", block.number, block.timestamp, uint256(blockhash(block.number - 1)));
    }
}

contract BugTest is Test {
    function testBug() public {
        Bug bug = new Bug();

        console.log("[before roll] block: %d, time: %d, hash: %x", block.number, block.timestamp, uint256(blockhash(block.number - 1)));
        vm.roll(block.number + 5);
        console.log("[after  roll] block: %d, time: %d, hash: %x", block.number, block.timestamp, uint256(blockhash(block.number - 1)));
        bug.check();
    }
}

Output:

  [before roll] block: 1, time: 1, hash: 0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563
  [after  roll] block: 6, time: 1, hash: 0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563 <-- wrong
  [contract   ] block: 6, time: 1, hash: 0x36b6384b5eca791c62761152d0c79bb0604c104a5fb6f4eb0703f3154bb3db0 <-- somehow this is correct

notice block 6 returns two different hashes in different contexts.

Expected output:

  [before roll] block: 1, time: 1, hash: 0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563
  [after  roll] block: 6, time: 1, hash: 0x36b6384b5eca791c62761152d0c79bb0604c104a5fb6f4eb0703f3154bb3db0
  [contract   ] block: 6, time: 1, hash: 0x36b6384b5eca791c62761152d0c79bb0604c104a5fb6f4eb0703f3154bb3db0

I have tested this on Windows and on Linux with fresh forge install.

very strange bug indeed.

@hananbeer hananbeer added the T-bug Type: bug label May 12, 2023
@hrkrshnn
Copy link

hrkrshnn commented May 12, 2023

I believe this is a known issue. The problem is that according to Solidity, block.number is a constant across the transaction. So a new reference to it can just dup the value instead of calling the same opcode. Does that explain the issue?

But there should be a way to fix these issues without creating further restrictions in the compiler. I'll make an issue in solidity on it

@hananbeer
Copy link
Author

hananbeer commented May 13, 2023

hmm. so it's technically not a bug in neither forge nor solc and yet my code reverts.
I thought it might be related to solc but it roll the block eventually so I was thrown off, but honestly I should've thought it just dups from the stack or something like that...

I think the best move would be if forge test could somehow detect vm.roll + via_ir (assuming that only happens in via_ir?) if that's possible. probably for timestamp and the likes as well. but I understand if that's not trivially achievable and just wallow in my misery and that some poor soul will encounter this in the future.

edit: come to think about it, I think it could make most sense to create a cheatcode such as vm.blockhash. just the call to this contract would be enough to workaround this stack dup thing.

@mds1
Copy link
Collaborator

mds1 commented May 13, 2023

Just linking some relevant issues here since this has come up before around block timestamp:

@hrkrshnn
Copy link

Initially I thought the compiler was dup-ing block.number twice (which is bad since duping is 1 gas more expensive). But turns out it was actually duping the bigger expression. Which is cheaper.

  contract Test {
      function f() external {
      }
  }
  contract C {
      bytes32 x;
      bytes32 y;
      Test immutable test = new Test();
      function f() external {
          x = blockhash(block.number - 1);
          test.f();
          y = blockhash(block.number - 1);
      }
  }

If you look at the optimized IR for this, --ir-optimized --optimize --debug-info none, you can see that the compiler will just cache the first expression blockhash(block.number - 1) and then dup it for the second sstore.

If you change this to

contract Test {
    function f() external {
    }
}
contract C {
    uint x;
    uint y;
    Test immutable test = new Test();
    function f() external {
        x = block.number;
        test.f();
        y = block.number;
    }
}

You'll see that there are no dups, and each sstore is has number as the second argument. (dup is 1 gas more expensive).

@hananbeer
Copy link
Author

hananbeer commented May 13, 2023

that's right. thanks for the info.
I also tried scoping the first blockhash() with { } but it leaks outside the scope. I wonder if that might be a problem elsewhere... but it is not technically a bug still.

for now I just workaround it with the external call, but I think foundry should consider adding this to Cheatcodes.sol and the docs to warn people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
No open projects
Archived in project
Development

No branches or pull requests

3 participants