matter-labs / foundry-zksync

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

Unable to run invariant tests using handler pattern #565

Closed brotherlymite closed 1 month ago

brotherlymite commented 2 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 (a69c479 2024-09-06T00:32:22.954024000Z)

What command(s) is the bug in?

forge test --zksync

Operating System

macOS (Apple Silicon)

Describe the bug

When running the invariant test, the test seems to be passing but strangely the functions in the handler contract are not being called for some reason.

To replicate, run forge test --match-contract PoolInvariants -vv --show-progress on this branch and you would see the following output in the terminal:

forge test --match-contract PoolInvariants -vv --show-progress

No files changed, compilation skipped
tests/core/pool-invariant/PoolInvariants.t.sol:PoolInvariants
  ↪ Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 23.04s (44.22s CPU time)

Ran 2 tests for tests/core/pool-invariant/PoolInvariants.t.sol:PoolInvariants
[PASS] invariant_checkHealthFactor() (runs: 25, calls: 25000, reverts: 0)
[PASS] invariant_summary() (runs: 25, calls: 25000, reverts: 0)
Logs:
  Supply Success Calls USDX: 71
  Borrow Success Calls USDX: 62
  Borrow Fail Calls USDX: 43
  Withdraw Success Calls USDX: 70
  Withdraw Fail Calls USDX: 47
  Repay Success Calls USDX: 85
  Repay Fail Calls USDX: 91

  Supply Success Calls WETH: 51
  Borrow Success Calls WETH: 55
  Borrow Fail Calls WETH: 22
  Withdraw Success Calls WETH: 47
  Withdraw Fail Calls WETH: 22
  Repay Success Calls WETH: 55
  Repay Fail Calls WETH: 38

  Supply Success Calls WBTC: 55
  Borrow Success Calls WBTC: 34
  Borrow Fail Calls WBTC: 14
  Withdraw Success Calls WBTC: 29
  Withdraw Fail Calls WBTC: 27
  Repay Success Calls WBTC: 43
  Repay Fail Calls WBTC: 39

Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 23.04s (44.22s CPU time)

When the same test is being run in zksync, we see that the handler functions are not being called. To run the same test in zksync and replicate: checkout to this branch, copy .env.examples to .env and run FOUNDRY_PROFILE=zksync forge test --zksync --match-contract PoolInvariants --rpc-url https://mainnet.era.zksync.io -vv --show-progress. This is the output we are getting when running the same test in zksync:

FOUNDRY_PROFILE=zksync forge test --zksync --match-contract PoolInvariants --rpc-url https://mainnet.era.zksync.io -vv --show-progress

[⠃] Using zksolc-1.5.3

No files changed, compilation skipped
tests/core/pool-invariant/PoolInvariants.t.sol:PoolInvariants
  ↪ Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 1160.67s (21.18s CPU time)

Ran 2 tests for tests/core/pool-invariant/PoolInvariants.t.sol:PoolInvariants
[PASS] invariant_checkHealthFactor() (runs: 1, calls: 2, reverts: 0)
[PASS] invariant_summary() (runs: 1, calls: 2, reverts: 0)
Logs:
  Supply Success Calls USDX: 0
  Borrow Success Calls USDX: 0
  Borrow Fail Calls USDX: 0
  Withdraw Success Calls USDX: 0
  Withdraw Fail Calls USDX: 0
  Repay Success Calls USDX: 0
  Repay Fail Calls USDX: 0

  Supply Success Calls WETH: 0
  Borrow Success Calls WETH: 0
  Borrow Fail Calls WETH: 0
  Withdraw Success Calls WETH: 0
  Withdraw Fail Calls WETH: 0
  Repay Success Calls WETH: 0
  Repay Fail Calls WETH: 0

  Supply Success Calls WBTC: 0
  Borrow Success Calls WBTC: 0
  Borrow Fail Calls WBTC: 0
  Withdraw Success Calls WBTC: 0
  Withdraw Fail Calls WBTC: 0
  Repay Success Calls WBTC: 0
  Repay Fail Calls WBTC: 0

Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 1160.67s (21.18s CPU time)

We see that none of the actions of the invariant test, including Supply, Borrow, Withdraw, Repay are being called for some reason but the same test in normal foundry works fine.

Karrq commented 2 months ago

Hey @brotherlymite, thanks for the report! We investigated the issue and found that this behavior can be observed when using the handler pattern. In particular, when the handler is called it is done thru the zkVM, thus when the handler makes use of cheatcodes, it causes the unexpected behavior you are observing (in this case, the execution is equivalent to a no-op). EDIT: see next comment

There's no way for us to determine if the target selectors are handler contracts or not, thus it's not possible to determine whether to run it in EVM or zkVM automatically.

We are working on a new feature (#569) which will make calling cheatcodes in external contracts a lot easier to work with, without having to switch zkvm on or off manually.


Unfortunately, there's no workaround available to make it work today, but if you wish to maintain this type of test I can suggest migrating to a fuzz test instead, for example:

struct SummaryInput {
  uint8 tokenIndex;
  uint128 amount;
  uint8 opIdx;
}

/// forge-config: zksync.fuzz.runs = 25
/// forge-config: zksync.fuzz.show-logs = true
function testFuzz_summary(SummaryInput[1000] memory inputs) external {
  address(vm).call(abi.encodeWithSignature("zkVm(bool)", false));

  FuzzSelector memory pool = targetSelectors()[0];

  for (uint i = 0; i < inputs.length; i++) {
    SummaryInput memory input = inputs[i];

    uint8 idx = (input.opIdx % uint8(pool.selectors.length));
    bytes4 op = pool.selectors[idx];

    address(s_poolHandler).call(abi.encodeWithSelector(op, input.tokenIndex, input.amount));

  }

  s_poolHandler.summary();
}

You can tweak the runs paramters like usual, and the depth is represented by the array length. In the snippet above I made it match the original (non-zksync) test configuration you reported (please note that show-logs might be undesirable).

Note that the calls to PoolHandler will happen in EVM, thus zkVM must be switched on inside it for the contract under test to be ran inside zkVM. Additionally, it's important to ensure that PoolHandler is either deployed in EVM or be made persistent, otherwise its deployment in setUp will happen in zkVM only, not allowing it to be called from within the test.

Karrq commented 2 months ago

Hey @brotherlymite, upon further review we actually determined the issue to be a bit different. The handler is actually being called via EVM, so cheatcodes would be fine, but the deployment happens in zkEVM (during setUp), thus the contract wouldn't be present in EVM and cause this behavior.

The fix in this case would be to deactivate zkVm before doing the deployment of the handler, which resolves the issue above, and allows invariant tests to run as normal. Unfortunately, this means we have the opposite problem: non-handler pattern is not working as intended.

Additionally, in the version you originally encountered the issue there's still a bug, in particular when the deployment happening in EVM instead of zkVM, it wouldn't be detected for the invariant executor, but this has been resolved as of #572, therefore an upgrade is required to run invariant tests with the handler pattern.


There are a few more adjustments that need to be made to make the invariant test to run successfully, but these are relatively easy and are just to account for zkVM specifics:

  1. use profile.zksync.invariant.no_zksync_reserved_addresses, this prevents reserved addresses to be used for fuzzed senders
  2. deal ETH to the user to allow the transaction to be ran, otherwise the bootloader will error out because the balance is insufficient
  3. ensure that tx.origin is set correctly - in particular startPrank(user) should change to startPrank(user, user), otherwise the tx originator will be set to the caller of PoolHandler, instead of the pranked sender. If the fuzzed sender is the originator, you'll need to deal ETH to it, for the same reason as dealing to user

Please let me know if you have any more questions about this issue! Thank you

Karrq commented 1 month ago

Hey @brotherlymite! We have fixed the issue with invariant testing as of #581! We also added 2 tests and improved an existing one to ensure this stays the case in the future, you could make use of them as reference, in particular the reproduction test for this issue! Please let me know if you still encounter issues!