matter-labs / foundry-zksync

Fork of Foundry tailored for zkSync environment
Apache License 2.0
299 stars 130 forks source link

mysterious test failures, inability to check changes in state #438

Closed 0xBrian closed 4 months ago

0xBrian commented 4 months ago

Component

Forge

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

What version of Foundry are you on?

forge 0.0.2 (0989cdc 2024-06-20T00:20:02.912238865Z)

What command(s) is the bug in?

forge test --zksync -vvvv --use 0.8.20

Operating System

Linux

Describe the bug

pragma solidity >=0.8 <0.9.0;

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

interface IMiningContract {
    function multiMint_SameAddress(address mintToAddress, uint256[] memory nonces) external;
    function usedCombinations(bytes32) external view returns (bool);
}

bytes32 constant DIGEST = 0x000000000000a8d299d120846e9c48444fbe330d9407a8c005848c491b2556f5;
address constant MINER = address(0xFaf20E5cA7E39d43A3aabc450602b4147c3aA62E);
uint256 constant NONCE = 0x29d48001900813c807e0aff4bbdc0ff11ff9b81c9e789bd91c3907e0000095c5;
address constant MINING_CONTRACT_ADDRESS = address(0x366d17aDB24A7654DbE82e79F85F9Cb03c03cD0D);
uint constant BLOCKNUM = 36_852_529;

contract Test1 is Test {
    function test1() external {
        vm.createSelectFork("zksync_era", BLOCKNUM - 2);
        IMiningContract mc = IMiningContract(MINING_CONTRACT_ADDRESS);
        uint256[] memory nonces = new uint256[](1);
        nonces[0] = NONCE;
        vm.startPrank(MINER);
        assertFalse(mc.usedCombinations(DIGEST), "entry in mapping, shouldn't be");
        mc.multiMint_SameAddress(MINER, nonces);
        assertTrue(mc.usedCombinations(DIGEST), "entry not in mapping, should be");
        vm.stopPrank();
    }
}

contract Blah {
    IMiningContract public mc;
    constructor(address a) {
        mc = IMiningContract(address(a));
    }
    function doit(address miner, uint256[] memory nonces) external {
        console.log("address of mining contract: %s", address(mc));
        mc.multiMint_SameAddress(miner, nonces);
    }
}

contract Test2 is Test {
    function test2() external {
        vm.createSelectFork("zksync_era", BLOCKNUM - 2);
        Blah blah = new Blah(MINING_CONTRACT_ADDRESS);
        uint256[] memory nonces = new uint256[](1);
        nonces[0] = NONCE;
        vm.startPrank(MINER);
        assertFalse(blah.mc().usedCombinations(DIGEST), "entry in mapping, shouldn't be");
        blah.doit(MINER, nonces);
        assertTrue(blah.mc().usedCombinations(DIGEST), "entry not in mapping, should be");
        vm.stopPrank();
    }
}

Above is test/Whatever.sol in a stock forge init project, except that I added the following to foundry.toml:

[rpc_endpoints]
zksync_era = "https://mainnet.era.zksync.io"

I'm testing with forge test --zksync -vvvv --use 0.8.20.

This code is about replaying this transaction: https://era.zksync.network/tx/0x14f963c64f1800aa2d8b938556ba662daecf55801869d96903398639d7a888a8

That tx was submitted in block 36_852_529 by 0xFaf20E5cA7E39d43A3aabc450602b4147c3aA62E. Without getting into the weeds about what the destination contract does, after the call to multiMint_SameAddress, there was an observable change, an additional entry in the map called usedCombinations. So after that call, usedCombinations(0x000000000000a8d299d120846e9c48444fbe330d9407a8c005848c491b2556f5) became true. It's still true, which we can see at https://era.zksync.network/address/0x366d17adb24a7654dbe82e79f85f9cb03c03cd0d#readContract#F47 .

Test1 / test1 represents my first attempt to replay the tx. It works like I expect. I go back to a block or two before the tx and do exactly what it did. Before the call, the entry is not in the map, and after the call, the entry is in the map. OK.

Test2 / test2 is a contrived attempt to show what happens when I start taking simplistic caveman code and splitting it up into sensical parts -- it stops working. It seems to stop being able to see the changes in the contract I'm testing. Can you help me understand why test1 works and test2 doesn't? Thanks.

Karrq commented 4 months ago

Thanks for the report! Could you let us know exactly the error you are getting? Is is the last assertion that fails or is there some other error message?

0xBrian commented 4 months ago

It's just the assertion that doesn't pass, no error message. I think test1 and test2 should either both pass, or both fail. Thanks.

Karrq commented 4 months ago

Hey @0xBrian, I investigated the issue. Thanks for the reproducible example, it was really helpful!

I actually tried fetching the source and luckily the contract was verified on the explorer so I could take a look at how it worked, and noticed that the DIGEST is computed using a few items from the contract state, but also the msg.sender.

Unfortunately, the answer to your issue is rather bland: prank simply doesn't set the msg.sender for nested calls! So, in test2, the msg.sender would end up being the address where Blah was deployed, and not MINER, as it would be in the test1.

I hope this helps!

0xBrian commented 4 months ago

OK, thank you for investigating. We figured it was something like that. But does it work this way in normal Foundry, or is it something related to the changes required for ZKSync? To me it seems like this behavior would prevent anyone from writing realistic tests.

Karrq commented 4 months ago

Yes, that's intended foundry behaviour, we can also see this in the test suite here.

What we do require for ZKSync is that any contract running inside zkVM isn't able to use cheatcodes (in this case Blah), but you can work around that by disabling zkVM before the call to the external contract, run your cheatcodes there, and re-enable zkVM afterwards.

For example:

vm.zkVm(false);
blah.doit(...);

and inside doit:

vm.zkVm(true); //here or after prank is fine either way
// the important part is that `doit` isn't run inside zkVM
vm.prank(MINER);
...

As far as realistic test, I'm not sure I understand what you mean. If instead of Blah you had another contract, on chain, which handled the calls to MINING_CONTRACT_ADDRESS somehow, shouldn't you be calling tha contract instead then? And if instead it was some other kind of program off chain, wouldn't that basically be Test1?

You can always create other functions in the Test1 contract to group functionality, and as long as it's not prefixed with test (or a few others like invariant) then it won't be executed as a standalone test.

I'll go ahead and close the issue since we clarified it's intended behaviour, but if you have any more questions you can just comment here on the issue and I'll still reply :)