Add dynamic Hardfork variant

[mattsse]

Describe the feature

Currently the hardfork type is an enum of known ethereum hardforks

reth/crates/ethereum-forks/src/hardfork.rs

Line 22 in 20aa3c4

pub enum Hardfork {

this makes it difficult to integrate L2 hardforks without introducing feature flags.

this can be changed by introducing a hardfork type that has a name because we need Ord

something like:

struct NamedHardfork {
  name: &'static str, 
}

// new Hardfork variant:
Other(NamedHardFork)

and then this would be another variant of the hardfork enum:

reth/crates/ethereum-forks/src/hardfork.rs

Line 22 in 20aa3c4

pub enum Hardfork {

TODO

investigate named hardfork integration

this will require a lot of manual impls

an alternative approach would be trait based but this would complicate things a lot

@rakita we could try to do something similar with SpecId in revm

Additional context

No response

[rakita]

Ref: bluealloy/revm#918

We are already trait bases in Revm. But i need to thinker a little to find best option. Last thing that i had in my mind is to use u64 as SpecId and every standard fork is a one bit,this would allow setting externaly bits without disruption common ones.

How would you handle if you have two consecutive external forks?

[supernovahs]

@rakita you mean this approach right?

struct HardforkManager {
    hardforks: Vec<u64>, 
}

impl HardforkManager {
    // Creates a new HardforkManager with enough space to handle a given number of hardforks
    fn new(total_hardforks: usize) -> Self {
        let num_u64s = (total_hardforks + 63) / 64;  // Calculate how many u64s are needed
        HardforkManager {
            hardforks: vec![0; num_u64s],
        }
    }

    // Activate a hardfork by setting the corresponding bit
    fn activate_hardfork(&mut self, index: usize) {
        let array_index = index / 64;  // Determine which u64 in the array
        let bit_index = index % 64;    // Determine which bit in the u64
        self.hardforks[array_index] |= 1 << bit_index;
    }

    // Deactivate a hardfork by clearing the corresponding bit
    fn deactivate_hardfork(&mut self, index: usize) {
        let array_index = index / 64;
        let bit_index = index % 64;
        self.hardforks[array_index] &= !(1 << bit_index);
    }

    // Check if a hardfork is active
    fn is_hardfork_active(&self, index: usize) -> bool {
        let array_index = index / 64;
        let bit_index = index % 64;
        (self.hardforks[array_index] & (1 << bit_index)) != 0
    }
}

[KyrylR]

May I?

cc @mattsse

[KyrylR]

Hello, I have started my research on this topic

If my understanding is correct, we would like to have the ability to introduce new hardforks without the need to add new fields to the enum, am I right?

In my opinion, the introduction of Other(NamedHardFork) could create a mess inside the codebase in the long term

An alternative would be to introduce additional enums for different chains, but this should still be carefully considered

So right now we are choosing between explicit or implicit (through the NamedHardFork) options

What do you think? Or am I going in the wrong direction?

[mattsse]

If my understanding is correct, we would like to have the ability to introduce new hardforks without the need to add new fields to the enum, am I right?

yes, non ethereum native hardfork should then be constants

In my opinion, the introduction of Other(NamedHardFork) could create a mess inside the codebase in the long term

hmm, I need to think about this, maybe we also need to abstract the chainspec away, then rolling separate hardfork enums wouldn't be an issue

we almost never use hardfork enums directly, instead the chainspec uses them to check activation

So right now we are choosing between explicit or implicit (through the NamedHardFork) options

yeah

I guess I need to think about this a bit more because this could impact a lot of things

[KyrylR]

Hello, I am continuing to dive into the issue

What do you think if I create a Draft PR with an implementation that was originally suggested? I believe it should be similar to how it is implemented for ErrorData enum -- Custom field

What do you think?

[KyrylR]

cc @mattsse

[mattsse]

What do you think if I create a Draft PR with an implementation that was originally suggested?

that'd be great!

I'm leaning more and more towards this solution actually, because this would make things a lot easier

[KyrylR]

Got it, will do

[KyrylR]

Hello, so I am little bit stuck with this

There are a few options

1. As was suggested, I created following structure:

#[cfg_attr(feature = "serde", derive(Serialize))]
#[derive(Debug, Copy, Clone, Eq, PartialEq, PartialOrd, Ord, Hash)]
pub struct HardforkData {
    pub name: &'static str,
    pub block: u64,
    pub timestamp: u64,
}

But lifetimes are tricky to deal with, so I had to implement Deserialize on my own:

#[cfg(feature = "serde")]
impl<'de> Deserialize<'de> for HardforkData {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
        where
            D: serde::Deserializer<'de>,
    {
        #[derive(Deserialize)]
        struct Helper<'a> {
            name: &'a str,
            block: u64,
            timestamp: u64,
        }

        let helper = Helper::deserialize(deserializer)?;
        Ok(HardforkData {
            name: helper.name.to_owned().leak(),
            block: helper.block,
            timestamp: helper.timestamp,
        })
    }
}

Which uses .leak() thing, it says for itself (kinda memory leak😅)

2. Second option is to use String, but then we will need to drop Copy trait for Hardfork enum

3. Another option is to use fixed size array of bytes (also could be tricky during conversions)

4. Maybe use something like this: https://crates.io/crates/fixedstr. But it's yet another dep, and I'm not sure that's what we want

[mattsse]

I think we only need this part:

pub struct HardforkData {
    pub name: &'static str,

because the other values are configured separately

[KyrylR]

Ok(HardforkData {
    name: helper.name.to_owned().leak(),

Is it ok to leave it like this?

[mattsse]

ah this is for deserialize impl?

we can do Cow<'static, str>

[KyrylR]

we can do Cow<'static, str>

It won't work because of the Copy trait

cc @mattsse

[joshieDo]

#9065