foundry-rs / foundry

Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.
https://getfoundry.sh
Apache License 2.0
8.35k stars 1.77k forks source link

block.timestamp bugs when using skip #6180

Closed 77abe77 closed 11 months ago

77abe77 commented 1 year ago

Component

Forge

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

What version of Foundry are you on?

forge 0.2.0 (619f3c5 2023-10-22T00:19:08.867786000Z)

What command(s) is the bug in?

forge test -vvvv --mc "ForgeBug"

Operating System

None

Describe the bug

Two bugs, easy to reproduce:

Consists of 2 contracts ForgeBug and ForgeBug1

Common bug in both contracts:

Actual Behavior:

duration (end - start) is always 0. Implying that start is getting erroneously mutated along the way.

Expected Behavior:

duration (end - start) should be non zero and that start should be caching the first value of block.timestamp

contract ForgeBug is Test {
    function test_timebug() external {
        uint start = block.timestamp;
        console2.log("start: %d", start);

        for (uint i; i < 4; i++) {
            skip(15);
            console2.log("skip 15 sec: %d", block.timestamp);
        }

        uint end = block.timestamp;
        console2.log("start: %d", start);
        console2.log("end: %d", end);
        console2.log("duration: %d", end - start);
    }

}

ForgeBug Contract Additional Bug

Actual Behavior:

Logs:
  start: 1
  skip 15 sec: 16
  skip 15 sec: 16
  skip 15 sec: 16
  skip 15 sec: 16
  start: 16
  end: 16
  duration: 0

Expected Behavior:

Logs:
  start: 1
  skip 15 sec: 16
  skip 15 sec: 31
  skip 15 sec: 46
  skip 15 sec: 61
  start: 61
  end: 61
  duration: 0

ForgeTest1 has an unrolled for loop version of the test included and causes correct behavior (minus duration bug)

contract ForgeBug1 is Test {
    function test_timebug1() external {
        uint start = block.timestamp;
        console2.log("start: %d", start);

        skip(15);
        console2.log("skip 15 sec: %d", block.timestamp);
        skip(15);
        console2.log("skip 15 sec: %d", block.timestamp);
        skip(15);
        console2.log("skip 15 sec: %d", block.timestamp);
        skip(15);
        console2.log("skip 15 sec: %d", block.timestamp);

        uint end = block.timestamp;
        console2.log("start: %d", start);
        console2.log("end: %d", end);
        console2.log("duration: %d", end - start);
    }

    function test_timebug2() external {
        uint start = block.timestamp;
        console2.log("start: %d", start);

        for (uint i; i < 4; i++) {
            skip(15);
            console2.log("skip 15 sec: %d", block.timestamp);
        }

        uint end = block.timestamp;
        console2.log("start: %d", start);
        console2.log("end: %d", end);
    }

}

Logs:

[PASS] test_timebug1() (gas: 12642)
Logs:
  start: 1
  skip 15 sec: 16
  skip 15 sec: 31
  skip 15 sec: 46
  skip 15 sec: 61
  start: 61
  end: 61
  duration: 0

[PASS] test_timebug2() (gas: 12066)
Logs:
  start: 1
  skip 15 sec: 16
  skip 15 sec: 31
  skip 15 sec: 46
  skip 15 sec: 61
  start: 61
  end: 61
mattsse commented 1 year ago

unable to reproduce:

Running 1 test for test/Timestamp.t.sol:ForgeBug
[PASS] test_timebug() (gas: 13628)
Logs:
  start: 1
  skip 15 sec: 16
  skip 15 sec: 31
  skip 15 sec: 46
  skip 15 sec: 61
  start: 1
  end: 61
  duration: 60
77abe77 commented 1 year ago

@mattsse what forge version are you running?

mds1 commented 1 year ago

@77abe77 are you compiling with via-ir? If so see the convo and links in https://github.com/foundry-rs/foundry/issues/4934#issuecomment-1546667682

77abe77 commented 1 year ago

@mds1 good catch, yes I am! will take a look

brockelmore commented 1 year ago

@mds1 - maybe we should add vm.blockTimestamp() and vm.blockHeight() cheatcodes (as non-pure fns)

Effectively this prevents the optimizer from doing this. Alternatively we could add a vm.blackbox(...) that just echos the input data but overloading it would be a pain. So forge specific timestamps/block heights may make sense? Ergonomics of this aren't great, but at least we can point people to it 🤷

mds1 commented 1 year ago

vm.blockTimestamp() and vm.blockNumber() are nice workarounds, I like that idea. Agreed it’s a bit clunky, but the native cheat is still simpler than users having to implement a workaround themselves, so supportive 👍