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

Fix sender_nonce shadowing #6589

Merged
merged 2 commits into from
Dec 13, 2023
Merged

Fix sender_nonce shadowing #6589

merged 2 commits into from
Dec 13, 2023

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Dec 12, 2023

Motivation

I have accidentally shadowed sender_nonce parameter while implementing #6300

Ref #6571

cc @mds1

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's aight—thanks for the fix!

@mds1
Copy link
Collaborator

mds1 commented Dec 12, 2023

Just tested, and it looks like this does not fix the bug in #6571. If we don't see any other bugs in #6300, it's possible I bisected wrong (but we should probably merge this fix anyway)

@klkvr
Copy link
Member Author

klkvr commented Dec 13, 2023

I have bisected it too and came to the same conclusion (#6300)

This fix seemed to work for me, I will now check once again

@klkvr
Copy link
Member Author

klkvr commented Dec 13, 2023

@mds1 I have checked again and it works on my machine

@mds1
Copy link
Collaborator

mds1 commented Dec 13, 2023

Ah, ok let me re-test and report back :)

@mds1
Copy link
Collaborator

mds1 commented Dec 13, 2023

Hm, I'm still not able to show this as fixing the issue, and I've confirmed this with @clabby also

@klkvr
Copy link
Member Author

klkvr commented Dec 13, 2023

that's strange. how do you perform the testing? are you using foundryup to load and build this PR's branch?

I am testing by replacing all forge mentions in Optimism's repo scripts with /path_to_foundry/target/debug/forge

@mds1
Copy link
Collaborator

mds1 commented Dec 13, 2023

I tried both foundryup -C 0c74334 and foundryup -P 6589 to replace my global forge installation with this version

@klkvr
Copy link
Member Author

klkvr commented Dec 13, 2023

might be some issue with foundryup, I will try it once I'll get to my laptop

@clabby
Copy link
Contributor

clabby commented Dec 13, 2023

might be some issue with foundryup, I will try it once I'll get to my laptop

On my end, built directly on your branch through cargo build --release --bin forge - forge 0.2.0 (0c74334 2023-12-13T16:16:05.059695000Z)

@klkvr
Copy link
Member Author

klkvr commented Dec 13, 2023

oh, I've patched it in the wrong place at first, now it should work correctly, sorry for wasting your time on re-testing guys

The #6300 updated two files - one used for tests runner and the second for scripts. The code there is very similar but the shadowing happened only in case of scripts. I have probably patched both of them while debugging and then removed the patch from the wrong file

Copy link
Collaborator

@mds1 mds1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed it is now working, thank you very much!

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet!

@Evalir Evalir merged commit b6e6ce3 into foundry-rs:master Dec 13, 2023
19 checks passed
@pcaversaccio
Copy link
Contributor

Hey guys, I don't have a clue whether this PR triggers that error, but something happened over the last 2 days.

Example test: https://github.com/pcaversaccio/snekmate/blob/main/test/utils/Create2Address.t.sol#L27-L55

image

Those tests have been passing over a year and I haven't touched them, and the run successfully until 2 days ago. Now they fail, and it appears the CREATE2 contract creation is off in forge
image

@pcaversaccio
Copy link
Contributor

pcaversaccio commented Dec 14, 2023

Example log:

Logs:
  Error: a == b not satisfied [address]
        Left: 0x7DfcbfF9850aeb999a21010a0F052aFfA8Ad132B
       Right: 0xd092F93aDEC47Fd511d6D6515cb95F93EC79ABd4

Traces:
  [562604] Create2AddressTest::testComputeAddress()
    ├─ [0] VM::addr(115313734405729647875071022194542362875491702418537149851887407440565438981951 [1.153e77]) [staticcall]
    │   └─ ← initialAccount: [0x6a4b82A9171B19ac9970Bd2A48797358e5afEfe0]
    ├─ [0] VM::label(initialAccount: [0x6a4b82A9171B19ac9970Bd2A48797358e5afEfe0], "initialAccount")
    │   └─ ← ()
    ├─ [0] VM::getCode("ERC20Mock.sol:ERC20Mock") [staticcall]
    │   └─ ← 0x608060405260405162000c6238038062000c62833981016040819052620000269162000294565b83836003620000368382620003ab565b506004620000458282620003ab565b5050506200005a82826200006460201b60201c565b505050506200049d565b6001600160a01b038216620000935760405163ec442f0560e01b81525f60048201526024015b60405180910390fd5b620000a05f8383620000a4565b5050565b6001600160a01b038316620000d2578060025f828254620000c6919062000477565b90915550620001449050565b6001600160a01b0383165f9081526020819052604090205481811015620001265760405163391434e360e21b81526001600160a01b038516600482015260248101829052604481018390526064016200008a565b6001600160a01b0384165f9081526020819052604090209082900390555b6001600160a01b038216620001625760028054829003905562000180565b6001600160a01b0382165f9081526020819052604090208054820190555b816001600160a01b0316836001600160a01b03167fddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef83604051620001c691815260200190565b60405180910390a3505050565b634e487b7160e01b5f52604160045260245ffd5b5f82601f830112620001f7575f80fd5b81516001600160401b0380821115620002145762000214620001d3565b604051601f8301601f19908116603f011681019082821181831017156200023f576200023f620001d3565b81604052838152602092508660208588010111156200025c575f80fd5b5f91505b838210156200027f578582018301518183018401529082019062000260565b5f602085830101528094505050505092915050565b5f805f8060808587031215620002a8575f80fd5b84516001600160401b0380821115620002bf575f80fd5b620002cd88838901620001e7565b95506020870151915080821115620002e3575f80fd5b50620002f287828801620001e7565b604087015190945090506001600160a01b038116811462000311575f80fd5b6060959095015193969295505050565b600181811c908216806200033657607f821691505b6020821081036200035557634e487b7160e01b5f52602260045260245ffd5b50919050565b601f821115620003a657805f5260205f20601f840160051c81016020851015620003825750805b601f840160051c820191505b81811015620003a3575f81556001016200038e565b50505b505050565b81516001600160401b03811115620003c757620003c7620001d3565b620003df81620003d8845462000321565b846200035b565b602080601f83116001811462000415575f8415620003fd5750858301515b5f19600386901b1c1916600185901b1785556200046f565b5f85815260208120601f198616915b82811015620004455788860151825594840194600190910190840162000424565b50858210156200046357878501515f19600388901b60f8161c191681555b505060018460011b0185555b505050505050565b808201808211156200049757634e487b7160e01b5f52601160045260245ffd5b92915050565b6107b780620004ab5f395ff3fe608060405234801561000f575f80fd5b50600436106100a6575f3560e01c806340c10f191161006e57806340c10f191461011f57806370a082311461013457806395d89b411461015c5780639dc29fac14610164578063a9059cbb14610177578063dd62ed3e1461018a575f80fd5b806306fdde03146100aa578063095ea7b3146100c857806318160ddd146100eb57806323b872dd146100fd578063313ce56714610110575b5f80fd5b6100b26101c2565b6040516100bf9190610611565b60405180910390f35b6100db6100d6366004610678565b610252565b60405190151581526020016100bf565b6002545b6040519081526020016100bf565b6100db61010b3660046106a0565b61026b565b604051601281526020016100bf565b61013261012d366004610678565b61028e565b005b6100ef6101423660046106d9565b6001600160a01b03165f9081526020819052604090205490565b6100b261029c565b610132610172366004610678565b6102ab565b6100db610185366004610678565b6102b5565b6100ef6101983660046106f9565b6001600160a01b039182165f90815260016020908152604080832093909416825291909152205490565b6060600380546101d19061072a565b80601f01602080910402602001604051908101604052809291908181526020018280546101fd9061072a565b80156102485780601f1061021f57610100808354040283529160200191610248565b820191905f5260205f20905b81548152906001019060200180831161022b57829003601f168201915b5050505050905090565b5f3361025f8185856102c2565b60019150505b92915050565b5f336102788582856102d4565b610283858585610354565b506001949350505050565b61029882826103b1565b5050565b6060600480546101d19061072a565b61029882826103e5565b5f3361025f818585610354565b6102cf8383836001610419565b505050565b6001600160a01b038381165f908152600160209081526040808320938616835292905220545f19811461034e578181101561034057604051637dc7a0d960e11b81526001600160a01b038416600482015260248101829052604481018390526064015b60405180910390fd5b61034e84848484035f610419565b50505050565b6001600160a01b03831661037d57604051634b637e8f60e11b81525f6004820152602401610337565b6001600160a01b0382166103a65760405163ec442f0560e01b81525f6004820152602401610337565b6102cf8383836104eb565b6001600160a01b0382166103da5760405163ec442f0560e01b81525f6004820152602401610337565b6102985f83836104eb565b6001600160a01b03821661040e57604051634b637e8f60e11b81525f6004820152602401610337565b610298825f836104eb565b6001600160a01b0384166104425760405163e602df0560e01b81525f6004820152602401610337565b6001600160a01b03831661046b57604051634a1406b160e11b81525f6004820152602401610337565b6001600160a01b038085165f908152600160209081526040808320938716835292905220829055801561034e57826001600160a01b0316846001600160a01b03167f8c5be1e5ebec7d5bd14f71427d1e84f3dd0314c0f7b2291e5b200ac8c7c3b925846040516104dd91815260200190565b60405180910390a350505050565b6001600160a01b038316610515578060025f82825461050a9190610762565b909155506105859050565b6001600160a01b0383165f90815260208190526040902054818110156105675760405163391434e360e21b81526001600160a01b03851660048201526024810182905260448101839052606401610337565b6001600160a01b0384165f9081526020819052604090209082900390555b6001600160a01b0382166105a1576002805482900390556105bf565b6001600160a01b0382165f9081526020819052604090208054820190555b816001600160a01b0316836001600160a01b03167fddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef8360405161060491815260200190565b60405180910390a3505050565b5f602080835283518060208501525f5b8181101561063d57858101830151858201604001528201610621565b505f604082860101526040601f19601f8301168501019250505092915050565b80356001600160a01b0381168114610673575f80fd5b919050565b5f8060408385031215610689575f80fd5b6106928361065d565b946020939093013593505050565b5f805f606084860312156106b2575f80fd5b6106bb8461065d565b92506106c96020850161065d565b9150604084013590509250925092565b5f602082840312156106e9575f80fd5b6106f28261065d565b9392505050565b5f806040838503121561070a575f80fd5b6107138361065d565b91506107216020840161065d565b90509250929050565b600181811c9082168061073e57607f821691505b60208210810361075c57634e487b7160e01b5f52602260045260245ffd5b50919050565b8082018082111561026557634e487b7160e01b5f52601160045260245ffdfea2646970667358221220868bd415ccbb347b8d2ab86ae06487b928ec3d158bbfe95cec56ef3ce04a339864736f6c63430008170033
    ├─ [569] 0x104fBc016F4bb334D775a19E8A6510109AC63E00::compute_address(0x4d4611212780fbfb51f4726eeb465cdcca956a7f506696bf7df93f05dc82a241, 0x180607c4972614544b4e7b2897c2a408042b9b6cba03371cadc095be3dbaa5c9, Create2AddressTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) [staticcall]
    │   └─ ← 0x7DfcbfF9850aeb999a21010a0F052aFfA8Ad132B
    ├─ [488093] → new ERC20Mock@0xd092F93aDEC47Fd511d6D6515cb95F93EC79ABd4
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: initialAccount: [0x6a4b82A9171B19ac9970Bd2A48797358e5afEfe0], value: 100)
    │   └─ ← 1975 bytes of code
    ├─ emit log(val: "Error: a == b not satisfied [address]")
    ├─ emit log_named_address(key: "      Left", val: 0x7DfcbfF9850aeb999a21010a0F052aFfA8Ad132B)
    ├─ emit log_named_address(key: "     Right", val: ERC20Mock: [0xd092F93aDEC47Fd511d6D6515cb95F93EC79ABd4])
    ├─ [0] VM::store(VM: [0x7109709ECfa91a80626fF3989D68f67F5b1DD12D], 0x6661696c65640000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000001)
    │   └─ ← ()
    └─ ← ()

To me it looks like either:

bytes memory args = abi.encode(arg1, arg2, arg3, arg4);
bytes memory bytecode = abi.encodePacked(
    vm.getCode("ERC20Mock.sol:ERC20Mock"),
    args
);

is not working properly anymore, or, the CREATE2 creation is broken...

@klkvr
Copy link
Member Author

klkvr commented Dec 14, 2023

hey @pcaversaccio can you confirm that your forge version is forge 0.2.0 (53b15e6 2023-12-14T00:20:31.556940000Z)?

@Evalir
Copy link
Member

Evalir commented Dec 14, 2023

@pcaversaccio ack, could you give steps to reproduce this locally?

@pcaversaccio
Copy link
Contributor

hey @pcaversaccio can you confirm that your forge version is forge 0.2.0 (53b15e6 2023-12-14T00:20:31.556940000Z)?

Confirmed it's forge 0.2.0 (53b15e6 2023-12-14T00:17:09.405850859Z).

@pcaversaccio
Copy link
Contributor

@pcaversaccio ack, could you give steps to reproduce this locally?

Clone snekmate, run foundryup and invoke e.g. forge snapshot.

@klkvr
Copy link
Member Author

klkvr commented Dec 14, 2023

image

Can't reproduce it locally, am I doing something different?

@pcaversaccio
Copy link
Contributor

Can't reproduce it locally, am I doing something different?

Hmm, let me push it and make a PR with a CI run. I always hit an error locally with the latest version.

@pcaversaccio
Copy link
Contributor

Hmm, I just re-forced the compilation, and now it seems gone...

@Evalir
Copy link
Member

Evalir commented Dec 14, 2023

@pcaversaccio glad to hear—could you please double/triple check this to make sure this is gone?

@klkvr
Copy link
Member Author

klkvr commented Dec 14, 2023

Hmm, I just re-forced the compilation, and now it seems gone...

This might be related to compiler not invalidating build cache properly. It seems that sometimes not all contracts are getting recompiled automatically after metadata/bytecode changes somewhere.

I sometimes have to re-force compilation for verification to complete successfully too, will try to do some investigation on that later, but it doesn't seem related to this PR anyway

@pcaversaccio
Copy link
Contributor

Can confirm it passes also in the CI: https://github.com/pcaversaccio/snekmate/actions/runs/7212725690/job/19651033390. Sorry for the false alarm, but I was like wtf :) and again it's a cache issue...

@Evalir
Copy link
Member

Evalir commented Dec 14, 2023

yep no worries @pcaversaccio — please keep it going if you ever find these, they're of huge help. Glad this was just a false alarm 🙏

@pcaversaccio
Copy link
Contributor

yep no worries @pcaversaccio — please keep it going if you ever find these, they're of huge help. Glad this was just a false alarm 🙏

Will def do!

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

Successfully merging this pull request may close these issues.

5 participants