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

script regressions #7834

Open
2 tasks done
sakulstra opened this issue May 2, 2024 · 5 comments
Open
2 tasks done

script regressions #7834

sakulstra opened this issue May 2, 2024 · 5 comments
Assignees
Labels
T-bug Type: bug

Comments

@sakulstra
Copy link
Contributor

sakulstra commented May 2, 2024

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 (cafc260 2024-05-02T00:22:11.188280000Z)

What command(s) is the bug in?

forge script

Operating System

macOS (Apple Silicon)

Describe the bug

There seems to be a few regressions on scripts introduced in the last few weeks.

  1. when running a script it will no longer properly show the number of txns
==========================

Chain 1

Estimated gas price: 28.900907541 gwei

Estimated total gas used for script: 11250722

Estimated amount required: 0.325156076291494602 ETH

==========================
##
Sending transactions [0 - 0]. // in earlier versions this would have shown 0-4 (given there are 5txns in that script)
  1. when running a script with --resume it will no longer show any gas information, but immediately start with:
##
Sending transactions [0 - 0].

omitting info like estimated amount required.

  1. when running a script with --resume after doing some txns the script will fail as the pre-calculated nonce changed.
Context:
- EOA nonce changed unexpectedly while sending transactions. Expected 107 got 108 from provider.

Not sure if that ever was not an issue, but I guess it should be solved anyways as it's quite common (run a script, gas spikes and a txn is not executed, swap some assets, try to resume).
The issue appears to be that the script will try to use the hardcoded nonce from run-latest. I guess the "proper" way would be the recalculate the execution path based on txns that already have a hash.

@sakulstra sakulstra added the T-bug Type: bug label May 2, 2024
@klkvr
Copy link
Member

klkvr commented May 2, 2024

when running a script it will no longer properly show the number of txns

Are you running the script with --slow flag when this occurs?

@sakulstra
Copy link
Contributor Author

@klkvr yes, forge script scripts/Deploy.s.sol:Deploy --rpc-url mainnet --broadcast --ledger --mnemonic-indexes 1 --sender <redacted> --verify -vvvv --slow was the full command

@klkvr
Copy link
Member

klkvr commented May 2, 2024

Not sure if that ever was not an issue, but I guess it should be solved anyways as it's quite common (run a script, gas spikes and a txn is not executed, swap some assets, try to resume).

So the issue occurs only when you firstly run scirpt, then do some transactions from script sender which change sender nonce, and then resume script? Or it is the case even when script is just resumed immidiately without sender nonce being changed?

@sakulstra
Copy link
Contributor Author

@klkvr only when resuming after doing some txns elsewhere(at least didn't face it before).

@klkvr
Copy link
Member

klkvr commented May 2, 2024

I don't think this is something we should/can do.

Currently --resume assumes that nonce did not change, thus it does not re-execute script and just sends transactions which were not sent before.

If we'd account for nonce change after --resume, this would require us to at least partialy re-execute script as new nonce might result in other contract addresses -> other transactions. Such re-execution is not really trivial as we'd probably have to firstly execute script from the beginning, then determine once we've reached the tx we stopped broadcasting at, then apply state changeset between this tx and current state, and then continue executing and collecting new txses. And this would probably get trickier with multiple senders

Also, such --resume functionality might result in footguns for scripts like this which precompute addreses for contracts dependent on each-other:

uint256 nonce = vm.getNonce(sender);
address expectedContract2 = vm.computeCreateAddress(sender, nonce + 1);
address contract1 = address(new Contract1(expectedContract2));
// arbitrary txs sent here
new Contract2(contract1); // contract2 has different address than the one Contract1 saw in constructor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
None yet
Development

No branches or pull requests

2 participants