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

Incorrect Default EVM Version for Solidity Compiler 0.4.21-0.5.4 #189

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

ZhangZhuoSJTU
Copy link
Contributor

The default EVM version for Solidity compilers ranging from 0.4.21 to 0.5.4 is incorrectly implemented. Specifically, although the Constantinople hard fork had been introduced during this period, the default EVM version remains as Byzantium. After version 0.5.5, the default EVM version shifts to Petersburg.

That is, Constantinople is never used as the default EVM version for Solidity compilers.

This can be confirmed by running solc --help, with the output indicating the default EVM version as follows:

$ solc --help
solc, the Solidity commandline compiler.

...

Allowed options:
  --help               Show help message and exit.
  --version            Show version and exit.
  --license            Show licensing information and exit.
  --evm-version version
                       Select desired EVM version. Either homestead,
                       tangerineWhistle, spuriousDragon, byzantium (default) or
                       constantinople.
  ...

$ solc --version
solc, the Solidity commandline interface
Version: 0.4.24+commit.e67f0147.Linux.g++

@ZhangZhuoSJTU
Copy link
Contributor Author

Additionally, in Solidity 0.8.24, Cancun is not set as the default (0.8.25 does so). Reference

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

pending @klkvr

crates/core/src/utils.rs Outdated Show resolved Hide resolved
@DaniPopes
Copy link
Member

I dont think we were aiming for "solc's default" but rather the latest evm version supported at the given cersion?

@ZhangZhuoSJTU
Copy link
Contributor Author

I dont think we were aiming for "solc's default" but rather the latest evm version supported at the given cersion?

@DaniPopes I think based on our usage of the normalize_version_solc function, it seems like it represents "solc's default." Is that correct, or am I misunderstanding something?

https://github.com/foundry-rs/block-explorers/blob/d44051a4e5fd4a522739b3c2be6df6aeebca849b/crates/block-explorers/src/contract.rs#L279-L281

    /// Returns the contract's compiler settings.
    #[cfg(feature = "foundry-compilers")]
    pub fn settings(&self) -> Result<Settings> {
        let mut settings = self.source_code.settings()?.unwrap_or_default();

        if self.optimization_used == 1 && !settings.optimizer.enabled.unwrap_or_default() {
            settings.optimizer.enable();
            settings.optimizer.runs(self.runs as usize);
        }

        settings.evm_version = self.evm_version()?;

        Ok(settings)
    }
    
    ...

    /// Parses the EVM version.
    #[cfg(feature = "foundry-compilers")]
    pub fn evm_version(&self) -> Result<Option<EvmVersion>> {
        match self.evm_version.to_lowercase().as_str() {
            "" | "default" => {
                Ok(EvmVersion::default().normalize_version_solc(&self.compiler_version()?))
            }
            _ => {
                let evm_version = self
                    .evm_version
                    .parse()
                    .map_err(|e| EtherscanError::Unknown(format!("bad evm version: {e}")))?;
                Ok(Some(evm_version))
            }
        }
    }

pub fn normalize_version_solc(self, version: &Version) -> Option<Self> {
// The EVM version flag was only added in 0.4.21; we work our way backwards
if *version >= BYZANTIUM_SOLC {
// If the Solc version is the latest, it supports all EVM versions.
// For all other cases, cap at the at-the-time highest possible fork.
let normalized = if *version >= PRAGUE_SOLC {
self
} else if self >= Self::Cancun && *version >= CANCUN_SOLC {
Self::Cancun
} else if self >= Self::Shanghai && *version >= SHANGHAI_SOLC {
Self::Shanghai
} else if self >= Self::Paris && *version >= PARIS_SOLC {
Self::Paris
} else if self >= Self::London && *version >= LONDON_SOLC {
Self::London
} else if self >= Self::Berlin && *version >= BERLIN_SOLC {
Self::Berlin
} else if self >= Self::Istanbul && *version >= ISTANBUL_SOLC {
Self::Istanbul
} else if self >= Self::Petersburg && *version >= PETERSBURG_SOLC {
Self::Petersburg
} else if self >= Self::Constantinople && *version >= CONSTANTINOPLE_SOLC {
Self::Constantinople
} else if self >= Self::Byzantium {
Self::Byzantium
} else {
self
};
Some(normalized)
} else {
None
}
}

@DaniPopes
Copy link
Member

DaniPopes commented Aug 22, 2024

the comment in the function itself suggests otherwise, "cap at the highest possible fork"

basically if a user specifies cancun, but the solc version only knows up to shanghai (even if the default is paris), then we'll cap to shanghai

@ZhangZhuoSJTU
Copy link
Contributor Author

@DaniPopes, thank you for your prompt response! I apologize if I'm misunderstanding something, but I have a couple of questions for clarification:

  1. In the block-explorer repo, it seems the purpose of the normalization process is to determine the EVM version when the configuaration specifies it as "default" or even does not provided. To this end, finding the "solc's default" EVM version appears particularly useful when building a local or project from Etherscan. Did I get that right? It seems like this might be a more common use case (comparing to "capping at the highest possible fork").

  2. Additionally, could you please provide any concrete use cases for "capping at the highest possible fork"? Is there any code reference available that illustrates this? According to the solidity documentation, it is not always a good idea to use a version that is "not actually deployed to the Ethereum mainnet" but the highest possible fork.

@ZhangZhuoSJTU
Copy link
Contributor Author

ZhangZhuoSJTU commented Aug 22, 2024

Or, maybe it is the block-explorer repo that misuses the API.

In that case, I guess a proper patch is to introduce a new API in the compilers repo, and then change the block-explorer repo.

@klkvr
Copy link
Member

klkvr commented Aug 22, 2024

  1. In the block-explorer repo, it seems the purpose of the normalization process is to determine the EVM version when the configuaration specifies it as "default" or even does not provided. To this end, finding the "solc's default" EVM version appears particularly useful when building a local or project from Etherscan. Did I get that right? It seems like this might be a more common use case (comparing to "capping at the highest possible fork").

Yeah, this logic in block-explorers is not great, we should probably keep a similar mapping from solc version to default EVM version for it.

  1. Additionally, could you please provide any concrete use cases for "capping at the highest possible fork"? Is there any code reference available that illustrates this? According to the solidity documentation, it is not always a good idea to use a version that is "not actually deployed to the Ethereum mainnet" but the highest possible fork.

Primary purpose of this is to support multi-version projects. E.g. if some of user contracts are getting compiled with 0.8.25, and some with 0.8.18, and he has evm_version = "cancun" in foundry.toml, then 0.8.25 would get called with "cancun" evm version, and for 0.8.18 we will normalize it to "paris"

@ZhangZhuoSJTU
Copy link
Contributor Author

@DaniPopes @klkvr I've updated a new patch, where we introduce a new function named default_version_solc to support the functionality of finding the default version.

If it is a proper patch, I will also change the code in block-explorers accordingly.

Thanks again!

@ZhangZhuoSJTU
Copy link
Contributor Author

@DaniPopes @klkvr @mattsse kindly pinging for your review when you have a moment. Thanks!

Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

this is a bit hacky, but still works better than current approach for determining default EVM version.

if this turns out not great, we can always add a similar mapping from solc version to default EVM version

@mattsse mattsse merged commit f2b260f into foundry-rs:main Aug 26, 2024
15 checks passed
@ZhangZhuoSJTU ZhangZhuoSJTU deleted the fix/evm_version branch August 27, 2024 01:05
mattsse pushed a commit to foundry-rs/block-explorers that referenced this pull request Aug 27, 2024
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.

4 participants