ithacaxyz / odyssey

A testnet open-source Layer 2 from the future, co-designed with the developer tools stack.
https://www.ithaca.xyz/updates/odyssey
Apache License 2.0
200 stars 49 forks source link

refactor `revm_spec` Function for Better Maintainability and Performance #29

Closed malik672 closed 1 month ago

malik672 commented 1 month ago

The revm_spec function currently uses a long chain of if-else statements to determine the appropriate SpecId based on the active fork. I think the forks should be separated into a new file and should be refactored to use mappings instead of the multiple if/esle statements

something like this:


use once_cell::sync::Lazy;
use reth_chainspec::EthereumHardfork;
use reth_optimism_forks::OptimismHardfork;
use reth_revm::primitives::SpecId;

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]

/// Represents a fork in either the Ethereum or Optimism network.
pub enum Fork {
    /// An Ethereum network fork.
    Ethereum(EthereumHardfork),
    /// An Optimism network fork.
    Optimism(OptimismHardfork),
}

/// A list of all forks in reverse chronological order (newest first).
pub static FORKS: Lazy<Vec<Fork>> = Lazy::new(|| {
    vec![
        // Optimism forks (from newest to old for efficient lookup)
        Fork::Optimism(OptimismHardfork::Granite),
        Fork::Optimism(OptimismHardfork::Fjord),
        Fork::Optimism(OptimismHardfork::Ecotone),
        Fork::Optimism(OptimismHardfork::Canyon),
        Fork::Optimism(OptimismHardfork::Regolith),
        Fork::Optimism(OptimismHardfork::Bedrock),

        // Ethereum forks (from newest to old for efficient lookup)
        Fork::Ethereum(EthereumHardfork::Prague),
        Fork::Ethereum(EthereumHardfork::Cancun),
        Fork::Ethereum(EthereumHardfork::Shanghai),
        Fork::Ethereum(EthereumHardfork::Paris),
        Fork::Ethereum(EthereumHardfork::London),
        Fork::Ethereum(EthereumHardfork::Berlin),
        Fork::Ethereum(EthereumHardfork::Istanbul),
        Fork::Ethereum(EthereumHardfork::Petersburg),
        Fork::Ethereum(EthereumHardfork::Byzantium),
        Fork::Ethereum(EthereumHardfork::SpuriousDragon),
        Fork::Ethereum(EthereumHardfork::Tangerine),
        Fork::Ethereum(EthereumHardfork::Homestead),
        Fork::Ethereum(EthereumHardfork::Frontier),
    ]
});

/// Maps each fork to its corresponding EVM specification ID.
pub static FORK_SPEC_MAP: Lazy<BTreeMap<Fork, SpecId>> = Lazy::new(|| {
    let mut map = BTreeMap::new();
    map.insert(Fork::Optimism(OptimismHardfork::Granite), SpecId::GRANITE);
    map.insert(Fork::Optimism(OptimismHardfork::Fjord), SpecId::FJORD);
    map.insert(Fork::Optimism(OptimismHardfork::Ecotone), SpecId::ECOTONE);
    map.insert(Fork::Optimism(OptimismHardfork::Canyon), SpecId::CANYON);
    map.insert(Fork::Optimism(OptimismHardfork::Regolith), SpecId::REGOLITH);
    map.insert(Fork::Optimism(OptimismHardfork::Bedrock), SpecId::BEDROCK);
    map.insert(Fork::Ethereum(EthereumHardfork::Prague), SpecId::PRAGUE_EOF);
    map.insert(Fork::Ethereum(EthereumHardfork::Cancun), SpecId::CANCUN);
    map.insert(Fork::Ethereum(EthereumHardfork::Shanghai), SpecId::SHANGHAI);
    map.insert(Fork::Ethereum(EthereumHardfork::Paris), SpecId::MERGE);
    map.insert(Fork::Ethereum(EthereumHardfork::London), SpecId::LONDON);
    map.insert(Fork::Ethereum(EthereumHardfork::Berlin), SpecId::BERLIN);
    map.insert(Fork::Ethereum(EthereumHardfork::Istanbul), SpecId::ISTANBUL);
    map.insert(Fork::Ethereum(EthereumHardfork::Petersburg), SpecId::PETERSBURG);
    map.insert(Fork::Ethereum(EthereumHardfork::Byzantium), SpecId::BYZANTIUM);
    map.insert(Fork::Ethereum(EthereumHardfork::SpuriousDragon), SpecId::SPURIOUS_DRAGON);
    map.insert(Fork::Ethereum(EthereumHardfork::Tangerine), SpecId::TANGERINE);
    map.insert(Fork::Ethereum(EthereumHardfork::Homestead), SpecId::HOMESTEAD);
    map.insert(Fork::Ethereum(EthereumHardfork::Frontier), SpecId::FRONTIER);
    map
});
onbjerg commented 1 month ago

I fail to see how a map is more performant than an if statement. It's also fine that it's an if statement, since the forks are inherently sequential, so we don't gain much in terms of code organization. In fact, this would require us to add forks in multiple places. Your proposed interface would also require us to first do a linear search in a vec, and then do a binary search in a map to do the fork -> specid mapping

Thank you for your suggestion:)