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

Add dynamic Hardfork variant #7679

Open
mattsse opened this issue Apr 16, 2024 · 14 comments · May be fixed by #8111
Open

Add dynamic Hardfork variant #7679

mattsse opened this issue Apr 16, 2024 · 14 comments · May be fixed by #8111
Assignees
Labels
C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started

Comments

@mattsse
Copy link
Collaborator

mattsse commented Apr 16, 2024

Describe the feature

Currently the hardfork type is an enum of known ethereum hardforks

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:

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

@mattsse mattsse added C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started labels Apr 16, 2024
@rakita
Copy link
Collaborator

rakita commented Apr 16, 2024

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
Copy link
Contributor

@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
Copy link
Contributor

KyrylR commented Apr 23, 2024

May I?

cc @mattsse

@KyrylR
Copy link
Contributor

KyrylR commented Apr 24, 2024

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
Copy link
Collaborator Author

mattsse commented Apr 24, 2024

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
Copy link
Contributor

KyrylR commented Apr 25, 2024

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
Copy link
Contributor

KyrylR commented Apr 26, 2024

cc @mattsse

@mattsse
Copy link
Collaborator Author

mattsse commented Apr 27, 2024

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
Copy link
Contributor

KyrylR commented Apr 27, 2024

Got it, will do

@KyrylR
Copy link
Contributor

KyrylR commented Apr 27, 2024

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😅)

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

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

  3. 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
Copy link
Collaborator Author

mattsse commented Apr 28, 2024

I think we only need this part:

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

because the other values are configured separately

@KyrylR
Copy link
Contributor

KyrylR commented Apr 28, 2024

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

Is it ok to leave it like this?

@mattsse
Copy link
Collaborator Author

mattsse commented Apr 29, 2024

ah this is for deserialize impl?

we can do Cow<'static, str>

@KyrylR
Copy link
Contributor

KyrylR commented Apr 29, 2024

we can do Cow<'static, str>

It won't work because of the Copy trait

cc @mattsse

@KyrylR KyrylR linked a pull request May 5, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

4 participants