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.25k stars 1.73k forks source link

bug(`forge script`): tx succeeds during simulation but fails on any regular function call that occurs in a script after contract creation #2002

Open chad-js opened 2 years ago

chad-js commented 2 years ago

Component

Anvil

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

What version of Foundry are you on?

forge 0.2.0 (0962fd3 2022-06-16T18:19:05.497315Z)

What command(s) is the bug in?

No response

Operating System

macOS (Apple Silicon)

Describe the bug

Executing a contract func that updates storage on a contract that doesn't have a constructor fails on a forked instance of optimism or arbitrum (using anvil). On mainnet and polygon forks, there's no issue.

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

contract Contract {
  address public test;

  function initialize(address _test) external {
    test = _test;
  }
}
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Script.sol";

import {Contract} from "src/Contract.sol";

contract ContractScript is Script {

    Contract public testContract;

    function run() public {
        vm.broadcast();
        testContract = new Contract();

        vm.broadcast();
        testContract.initialize(address(1));
    }
}

What's interesting is the simulation is successful, but the actual broadcast tx fails:

$ forge script script/Contract.s.sol --rpc-url "http://127.0.0.1:8545" --private-key $ANVIL_ACCT_9 --broadcast -vvvv                                                                                            [14:41:16]
[⠒] Compiling...
Nothing to compile
Traces:
  [93261] ContractScript::run() 
    ├─ [0] VM::broadcast() 
    │   └─ ← ()
    ├─ [49699] → new Contract@0x700b6a60ce7eaaea56f065753d8dcb9653dbad35
    │   └─ ← 248 bytes of code
    ├─ [0] VM::broadcast() 
    │   └─ ← ()
    ├─ [2502] Contract::initialize(0x0000000000000000000000000000000000000000) 
    │   └─ ← ()
    └─ ← ()

Script ran successfully.
Gas used: 93261
==========================
Simulated On-chain Traces:

  [106971] → new Contract@0x700b6a60ce7eaaea56f065753d8dcb9653dbad35
    └─ ← 248 bytes of code

  [23694] Contract::initialize(0x0000000000000000000000000000000000000000) 
    └─ ← ()

==========================

Estimated total gas used for script: 131151

Amount required: 0.000262302 ETH

==========================

###
Finding wallets for all the necessary addresses...
##
Sending transactions [0 - 1].
⠉ [00:00:00] [#################################################################################################################################################################################################################################################] 2/2 txes (0.0s)
Transactions saved to: broadcast/Contract.s.sol/10/run-latest.json

##
Waiting for receipts.
⠙ [00:00:07] [#############################################################################################################################################################################################################################################] 2/2 receipts (0.0s)
#####
✅ Hash: 0x616898701db8efc33e972bfb36467311b28cfc49ed863f70dfe2f4a1d48a4c71
Contract Address: 0x700b6a60ce7eaaea56f065753d8dcb9653dbad35
Block: 12064970
Paid: 0.000213942 ETH (106971 gas * 2 gwei)

#####
❌ Hash: 0x03db02d9ff00fb3f4e1c6550670aa6c57e8a9361d1e406a34e391e0de5ee0b21
Block: 12064971
Paid: 0.0000469 ETH (23450 gas * 2 gwei)

Transactions saved to: broadcast/Contract.s.sol/10/run-latest.json

Error: 
   0: ["Transaction Failure: 0x03db…0b21"]

Location:
   cli/src/cmd/forge/script/receipts.rs:75

If you remove logic that updates any storage state, the tx succeeds.

chad-js commented 2 years ago

I just gave the script I put above a try on prod optimism, the tx for the initialize call fails there as well:

$ forge script script/Contract.s.sol --rpc-url $OPTIMISM_RPC_URL --private-key <prvKey> --broadcast --slow -vvvv                                                                                             [16:33:43]
[⠢] Compiling...
Nothing to compile
Traces:
  [113155] ContractScript::run() 
    ├─ [0] VM::broadcast() 
    │   └─ ← ()
    ├─ [49699] → new Contract@0x8eb4600bbab2286a46103f4923866de9653b737b
    │   └─ ← 248 bytes of code
    ├─ [0] VM::broadcast() 
    │   └─ ← ()
    ├─ [22402] Contract::initialize(0x0000000000000000000000000000000000000001) 
    │   └─ ← ()
    └─ ← ()

Script ran successfully.
Gas used: 113155
==========================
Simulated On-chain Traces:

  [106971] → new Contract@0x8eb4600bbab2286a46103f4923866de9653b737b
    └─ ← 248 bytes of code

  [43606] Contract::initialize(0x0000000000000000000000000000000000000001) 
    └─ ← ()

==========================

Estimated total gas used for script: 128175

Amount required: 0.000000128175 ETH

==========================

###
Finding wallets for all the necessary addresses...
##
Sending transactions [0 - 1].
⠁ [00:00:00] [########################################################################################################################>------------------------------------------------------------------------------------------------------------------------] 1/2 txes (0.4s)⠉ [00:00:07] [#############################################################################################################################################################################################################################################] 1/1 receipts (0.0s)
#####
✅ Hash: 0x1a972cf266cbfd61a028e1fdd694686130ce4b6dc0c7202bd21212ef775c8589
Contract Address: 0x8eb4600bbab2286a46103f4923866de9653b737b
Block: 12073464
Gas Used: 106971

⠉ [00:00:07] [#################################################################################################################################################################################################################################################] 2/2 txes (0.0s)⠉ [00:00:07] [#############################################################################################################################################################################################################################################] 1/1 receipts (0.0s)
#####
❌ Hash: 0x1b07ca2cce8b7302dbdb0e4a257c07d19689647c50de8df8a3549a2eedd1a3ff
Block: 12073469
Gas Used: 21204

Transactions saved to: broadcast/Contract.s.sol/10/run-latest.json

Error: 
   0: ["Transaction Failure: 0x1b07…a3ff"]

But, notice that executing a tx with the same args but with cast and a gas limit of 75000 results in success:

$ cast send 0x8eb4600bbab2286a46103f4923866de9653b737b "initialize(address)"  0x0000000000000000000000000000000000000001 --gas 75000 --rpc-url $OPTIMISM_RPC_URL --private-key <prvKey>                     [16:41:41]

blockHash               0xf6c411ec971b21c1ec5c41487929b814bad5be151f4831de7b07d0e36c6a44c0
blockNumber             12074683
contractAddress         
cumulativeGasUsed       43606
effectiveGasPrice       
gasUsed                 43606
logs                    []
logsBloom               0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
root                    
status                  1
transactionHash         0x1493c477c18629ab75f6759f95bed118517f6a4e66602460a2c3643c93486801
transactionIndex        0
type                    

So, seems like this is a gas estimation issue

gakonst commented 2 years ago

Per @mds1 suggestion let's go with the following 2 steps:

1 Add a broadcast overload that lets you specify the gas limit for the tx, vm.broadcast(uint256 gasLimit) and vm.broadcast(address sender, uint256 gasLimit)

  1. add bespoke logic for each L2, determined by chainId, this will need to be fork-aware when #1715 is merged

cc @joshieDo

joshieDo commented 2 years ago

We should probably add those broadcast interfaces. However, this is actually a result from another issue.

Since Arbitrum & Optimism* have different gas calculations, we discard the gas estimated locally, and request a new one from the RPC. To support batching, they're happening at the same time for tx1 and tx2. However, tx2 needs tx1 to be submitted and accepted first in this case, for the estimation to be a valid one.

We should probably force --slow on Arbitrum & Optimism* or any other transaction that requires an estimation coming from the RPC. In this case, even with --slow it won't work, since we estimate everything before we start sending, which needs to be adjusted as well.

joshieDo commented 2 years ago

I was under the impression Arbitrum and Optimism had different gas metering, but it seems that only Arbitrum has.

mds1 commented 2 years ago

Just summarizing what we ended up on as the path forward:

  1. Remove Optimism from the has_different_gas_calc function

https://github.com/foundry-rs/foundry/blob/112bd440c7cc83480612c5e05d71229ceeba7178/cli/src/cmd/forge/script/broadcast.rs#L275-L277 https://github.com/foundry-rs/foundry/blob/123ad0a427dc938a0a911eae3332de6576370a51/cli/src/cmd/utils.rs#L199-L205

  1. Force --slow on Arbitrum and Optimism. Both of don't have mempools like L1's so if you send multiple txs they get rejected with nonce too low.

  2. Move RPC gas estimation right before sending the intended tx, and add a note to the output indicating the "total gas/ETH cost estimates" shown beforehand aren't guaranteed to be accurate (since we don't estimate Optimism costs correctly)

  3. This one wasn't settled on, but I think we should change the broadcast overloads to specify margin on the gas estimate, instead of absolute limits. So if I'm estimating gas for a uniswap v2 trade where cost varies by block position, I can do vm.broadcast(20) to mean "add 20% margin to the gas estimate". IMO this is better UX than needing to harcode gas limits directly.

tynes commented 2 years ago
  1. add bespoke logic for each L2, determined by chainId, this will need to be fork-aware when #1715 is merged

For accurate fee estimation on Optimism, the L2 (execution) fee and the L1 (availability) fee needs to be taken into account. See here for how to do so. If special logic is added that is chain aware, perhaps handling this could just be abstracted away from users. See https://community.optimism.io/docs/developers/build/transaction-fees/#estimating-the-l1-data-fee for a higher level explanation

onbjerg commented 2 years ago

Was this fixed in #2046 @joshieDo?

mds1 commented 2 years ago

Of the 4 items listed in my above comment, I believe 1 and 2 are implemented, 3 and 4 are not.

gakonst commented 2 years ago

This one wasn't settled on, but I think we should change the broadcast overloads to specify margin on the gas estimate, instead of absolute limits. So if I'm estimating gas for a uniswap v2 trade where cost varies by block position, I can do vm.broadcast(20) to mean "add 20% margin to the gas estimate". IMO this is better UX than needing to harcode gas limits directly.

I love this idea, just re-read it.

devanoneth commented 2 years ago

Just a small note on all of this, recently stumped me and had to use the -g flag.

Could we also get a better error message if the transaction runs out of gas? Transaction Failure: 0x1b07…a3ff is not the most obvious. I'm down to implement this myself, just curious if others would agree or there's some reason we don't currently do that.

mds1 commented 2 years ago

+1 on @devanonon's comment, haven't gotten around to opening issues yet but here's a related repro with other issues too: https://github.com/mds1/forge-trace-and-script-issue

simplyoptimistic commented 8 months ago

This issue still exists, and I believe it applies to any regular function call that occurs in a script after contract creation (i.e. not necessarily limited to an initialization call). I used --with-gas-multiplier to resolve it.

zerosnacks commented 2 months ago

Optimistically marking this ticket as resolved. We've since moved to Alloy and https://github.com/foundry-rs/foundry/issues/4903#issuecomment-2168194556 indicated a similar issue has been resolved.

Feel free to re-open if there are still outstanding issues or alternatively open a new ticket that is specific to your situation.

captnseagraves commented 1 month ago

Can confirm this is still an issue. My instance occurred while running a script aimed at optimism sepolia. I second that it does apply to any regular function call that occurs in a script after contract creation as @simplyoptimistic offered. I also used --with-gas-multiplier to resolve the issue.