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

Document InstructionResult #7463

Closed
2 tasks done
PatrickAlphaC opened this issue Mar 20, 2024 · 8 comments
Closed
2 tasks done

Document InstructionResult #7463

PatrickAlphaC opened this issue Mar 20, 2024 · 8 comments
Assignees
Labels
A-docs Area: docs good first issue Good for newcomers T-feature Type: feature
Milestone

Comments

@PatrickAlphaC
Copy link

Component

Forge

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

  • Foundry
  • 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 PatrickAlphaC added the T-bug Type: bug label Mar 20, 2024
@DaniPopes
Copy link
Member

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

@PatrickAlphaC
Copy link
Author

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

@DaniPopes
Copy link
Member

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

@PatrickAlphaC
Copy link
Author

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
Copy link
Author

PatrickAlphaC commented Mar 23, 2024

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.

@DaniPopes DaniPopes added A-docs Area: docs T-feature Type: feature and removed T-bug Type: bug labels May 24, 2024
@DaniPopes DaniPopes changed the title Called contract seemingly randomly exits Document InstructionResult May 24, 2024
@zerosnacks zerosnacks added the good first issue Good for newcomers label Jul 12, 2024
@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@leovct
Copy link
Contributor

leovct commented Jul 31, 2024

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?

@zerosnacks
Copy link
Member

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.

@leovct
Copy link
Contributor

leovct commented Aug 5, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: docs good first issue Good for newcomers T-feature Type: feature
Projects
None yet
Development

No branches or pull requests

4 participants