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

Different bytecode produced via IR for some of OpenZeppelin's contracts when extra contracts are included in the input #15134

Open
cameel opened this issue May 23, 2024 · 2 comments
Assignees
Labels
bug 🐛 medium effort Default level of effort medium impact Default level of impact must have Something we consider an essential part of Solidity 1.0.

Comments

@cameel
Copy link
Member

cameel commented May 23, 2024

Description

This looks like yet another bug where different AST IDs lead to differences in the generated bytecode. I found it when experimenting with parallel compilation of OpenZeppelin.

When each source file is compiled in isolation by providing only that single file as input (rather than providing all of them and only selecting one via outputSelection), three contracts produce different bytecode than when all contracts are compiled together:

  • test/metatx/ERC2771Forwarder.t.sol:ERC2771ForwarderTest
  • test/token/ERC20/extensions/ERC4626.t.sol:ERC4626StdTest
  • test/utils/structs/Checkpoints.t.sol:CheckpointsTrace224Test

Environment

  • Compiler version: 0.8.26
  • Target EVM version (as per compiler settings): cancun

Steps to Reproduce

git clone --depth=1 https://github.com/OpenZeppelin/openzeppelin-contracts --branch v5.0.2
cd openzeppelin-contracts/
forge install

cat <<EOF > input.json
{
    "language": "Solidity",
    "sources": {
        "contracts/mocks/token/ERC4626Mock.sol":     {"urls": ["contracts/mocks/token/ERC4626Mock.sol"]},
        "test/token/ERC20/extensions/ERC4626.t.sol": {"urls": ["test/token/ERC20/extensions/ERC4626.t.sol"]}
    },
    "settings": {
        "remappings": [
            "@openzeppelin/contracts/=contracts/",
            "ds-test/=lib/forge-std/lib/ds-test/src/",
            "erc4626-tests/=lib/erc4626-tests/",
            "forge-std/=lib/forge-std/src/"
        ],
        "metadata": {"appendCBOR": false},
        "outputSelection": {"*": {"*": ["evm.bytecode"]}},
        "viaIR": true
    }
}
EOF

cat input.json \
    | solc --standard-json > output-select2.json
cat input.json \
    | jq 'del(.sources."contracts/mocks/token/ERC4626Mock.sol")' \
    | solc --standard-json > output-select1.json

diff --color --unified \
    <(cat output-select2.json \
        | jq '.contracts."test/token/ERC20/extensions/ERC4626.t.sol"."ERC4626StdTest".evm.bytecode.object' \
        | fold --width 160 \
    ) \
    <(cat output-select1.json \
        | jq '.contracts."test/token/ERC20/extensions/ERC4626.t.sol"."ERC4626StdTest".evm.bytecode.object' \
        | fold --width 160 \
    )

Note: This is a minimized example that's enough to reproduce one of the bytecode differences. With the full input there are more of them and they may or may not have the same cause. To make sure, when the bug is fixed, verify with the full original input: openzeppelin-5.0.2-full-input.json. It should produce the same bytecode for the three contracts when compiled as is and when you include only the source files containing those contracts.

@cameel cameel added bug 🐛 medium effort Default level of effort medium impact Default level of impact must have Something we consider an essential part of Solidity 1.0. labels May 23, 2024
@hedgar2017
Copy link

hedgar2017 commented May 23, 2024

Also reported to me as a presumably ZKsync toolchain bug with the EAS project:
zkSync-Community-Hub/zksync-developers#507

Please let me know if you need any help with reproducing.

@cameel
Copy link
Member Author

cameel commented Jun 7, 2024

@hedgar2017 Is it the same bug though? Unless it's happening in code they import from OpenZeppelin, it may very well be a distinct bug producing similar effects. We've had quite a few bugs of this kind and we're still finding new ones. We've been fixing them one by one, but they're really hard to avoid. Which is why @clonker is currently reworking the way optimizer handles names to eliminate any possibility of AST IDs influencing its operation.

If you can reproduce that error, it would be useful for confirming that we actually fixed it properly when we're done with it. Also in case the change we're preparing does not work out for some reason, it would allow us to still address this case individually. Hopefully we won't have to though and the fix will address all of them generically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 medium effort Default level of effort medium impact Default level of impact must have Something we consider an essential part of Solidity 1.0.
Projects
None yet
Development

No branches or pull requests

3 participants