foundry icon indicating copy to clipboard operation
foundry copied to clipboard

Document `InstructionResult`

Open PatrickAlphaC opened this issue 1 year ago • 5 comments

Component

Forge

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

  • [X] Foundry
  • [X] Foundryup

What version of Foundry are you on?

forge 0.2.0 (1a4960d 2024-03-20T00:28:07.727577000Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Intel)

Describe the bug

I hate that I'm doing this, because the reproducibility is... weird. Here is my test contract, you can copy paste the code directly into a blank foundry template created from forge init.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

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

contract CounterTest is Test {
    function setUp() public {}

    function testWeirdness() public {
        address addr = deployStuff();
        bool response = sendCalldata(addr);
        assert(response);
    }

    function deployStuff() public returns (address addr) {
        bytes memory bytecode =
            hex"60a88060093d393df3602035335f540114156100a4575f3560e01c8063e18d4afd1461003e57806390949f111461005757806308949a76146100715780637aa9a7f91461008b575b5f545f1461004a575f5ffd5b60015f5560075f5260205ff35b5f54600114610064575f5ffd5b60025f5560075f5260205ff35b5f5460021461007e575f5ffd5b60035f5560075f5260205ff35b5f54600314610098575f5ffd5b5f5f5560075f5260205ff35b5f5ffd";

        assembly {
            // s11Helper := create(0, add(bytecode, 0x20), mload(bytecode))
            addr := create(callvalue(), add(bytecode, 0x20), mload(bytecode))
        }

        require(addr != address(0), "Failed to deploy S11Helper");
    }

    function sendCalldata(address addr) public returns (bool) {
        // Data doesn't really matter
        bytes memory data = abi.encode(uint256(0), uint256(1), uint256(2));
        (bool success,) = addr.call(data);
        if (!success) {
            return false;
        }
        return true;
    }
}

Running the following command gets an expected fail:

forge test --mt testWeirdness

However why it fails is a mystery to me, it seems foundry just "cuts out" the call. Here is an image of calling the bytecode deployed contract using the debug flag:

This line: (bool success,) = addr.call(data); so inside the addr contract.

forge test --debug testWeirdness
Screenshot 2024-03-20 at 6 04 00 PM

We see the call ends with a PUSH0 opcode. However, looking at the deployed bytecode, there should at LEAST be another SLOAD opcode after the call. the RETURN opcode from the contract deployment code is f3 so we can remove60a88060093d393df3 from the bytecode, and we are left with the runtime code. The first few opcodes there are at least:

PUSH1 0x20
CALLDATALOAD
CALLER
PUSH0
SLOAD
ADD

So why is the call showing kicking the bucket after PUSH0?

What I've tried

According to the EVM, there are a few reasons why a CALL opcode will fail:

1. Not enough gas.
2. Not enough values on the stack.
3. The current execution context is from a [STATICCALL](https://www.evm.codes/#FA) and the [value](https://www.evm.codes/#34) (stack index 2) is not 0 (since Byzantium fork).

I tried running with {gas: 100} on the call, and saw no difference, but running with {gas: 6} seemed to correctly limit the opcodes to just:

PUSH1 0x20
CALLDATALOAD
CALLER

So adding more gas probably isn't the issue, since removing gas works fine.

2. Not enough values on the stack.: This doesn't make sense since we are in a new call context And 3 doesn't make sense since we are not in a staticcall.

So I'm not sure what's going on.

PatrickAlphaC avatar Mar 20 '24 22:03 PatrickAlphaC

It fails on the PUSH0 opcode with NotActivated:

                        trace: CallTrace {
                            depth: 1,
                            success: false,
                            caller: 0x7fa9385be102ac3eac297483dd6233d62b3e1496,
                            address: 0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f,
                            maybe_precompile: None,
                            selfdestruct_refund_target: None,
                            kind: Call,
                            value: 0x0_U256,
                            data: 0x000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000002,
                            output: 0x,
                            gas_used: 9079256848778772073,
                            gas_limit: 9079256848778772073,
                            status: NotActivated,
                            call_context: None,
                            steps: [],
                        },

Will open a PR to always display the call result in traces because I've encountered this before

DaniPopes avatar Mar 21 '24 07:03 DaniPopes

Where would we see why we are getting NotActivated? Will a PUSH0 opcode fail because something wasn't activated?

PatrickAlphaC avatar Mar 21 '24 15:03 PatrickAlphaC

It means the opcode is not supported in the current EVM version

DaniPopes avatar Mar 22 '24 03:03 DaniPopes

ahhhhhhhhhhhhhhh. I see! I forgot to add evm_version = 'shanghai' in the foundry.toml file.

I think we may start seeing stuff like this more... Maybe we can flesh out that error some more? Instead of NotActivated perhaps it can say OpcodeNotSupported?

And/or maybe a line in the foundry docs to say what NotActivated means?

PatrickAlphaC avatar Mar 23 '24 19:03 PatrickAlphaC

https://ethereum.stackexchange.com/questions/161891/received-notactivated-error-when-debugging-my-foundry-test/161892#161892

I've added this here for web crawler to pick up for others who may run into this issue.

PatrickAlphaC avatar Mar 23 '24 19:03 PatrickAlphaC

Hi @zerosnacks, I'd like to work on this one 👍

The "Understanding Traces" section in the Foundry book's "Forge Overview" would be an appropriate place to include information about trace interpretation. I think this section could be expanded to provide more detailed explanations of trace outputs and their meanings.

Regarding the approach to explaining the InstructionResult variants, there are two main options to consider:

  • Hardcoded explanation: This approach would involve manually documenting each InstructionResult variant within the Foundry book. It would provide a static, comprehensive reference for users but may require regular updates to stay in sync with any changes in the revm library.

  • Dynamic documentation generation: This method would involve generating the documentation on the fly, directly from the revm source code. This approach would ensure that the documentation always reflects the current state of the InstructionResult enum in revm.

I prefer the dynamic approach, but it requires several steps:

  • We need to improve the InstructionResult docs directly in revm.
  • We also need to develop a script capable of parsing Rust code and dynamically generating the InstructionResult documentation. Or perhaps you've already set up something similar?

What do you think?

leovct avatar Jul 31 '24 07:07 leovct

Hi @leovct thanks!

I think dynamic documentation generation is beyond the scope of this specific ticket. It is sufficient to document only the non-obvious ones that users face regularly (such as the OOG shorthand, NotActivated or InvalidFEOpcode in relation to new cheatcodes being used in an out-of-date Foundry version) and point them to possible causes. The book is largely focused on end-users whereas crate documentation is meant for implementers with higher context.

If you see any improvements in terms of clarifying specific instructions that can be upstreamed to REVM I'm sure they will be appreciative of it as well.

zerosnacks avatar Jul 31 '24 08:07 zerosnacks

The PR has been merged into the foundry book, I think we can close the problem now :)

leovct avatar Aug 05 '24 09:08 leovct