Support time-based forking

[Rjected]

Describe the feature

Shanghai and future forks will fork based on timestamp rather than block, and we currently only use a BlockNumber to distinguish between forks:

reth/crates/primitives/src/chain_spec.rs

Lines 79 to 82 in 2a77e08

	/// The active hard forks and their block numbers
	pub hardforks: BTreeMap<Hardfork, BlockNumber>,

This could be changed to another type such as:

enum ForkKind {
    /// A fork's block number
    Block(BlockNumber),
    
    /// The unix timestamp of a fork
    Time(u64),
}

Additional context

The geth implementation of this has been merged in go-ethereum#25878

[leruaa]

I would like to take this on

[leruaa]

@Rjected Should the forks field key be changed to ForkKind too in ForkFilter?

reth/crates/primitives/src/forkid.rs

Lines 110 to 116 in 115e623

	pub struct ForkFilter {
	forks: BTreeMap<BlockNumber, ForkHash>,
	head: BlockNumber,
	cache: Cache,
	}

Same question for ForkId::next?

reth/crates/primitives/src/forkid.rs

Lines 77 to 82 in 115e623

	pub struct ForkId {
	/// CRC32 checksum of the all fork blocks from genesis.
	pub hash: ForkHash,
	/// Next upcoming fork block number, 0 if not yet known.
	pub next: BlockNumber,
	}

[Rjected]

@leruaa I think so, that's how I've interpreted the geth change etc

[onbjerg]

I don't understand, is this about the fork filter or the hardfork abstraction as a whole? We have special handling for Paris and Shanghai because they are not block number based, rolling it into a single abstraction makes it super hard to use. You would essentially have to replace

reth/crates/primitives/src/chain_spec.rs

Line 120 in 2a77e08

pub fn fork_active(&self, fork: Hardfork, current_block: BlockNumber) -> bool {

with something like fork_active(fork: Hardfork) -> ForkKind and do a match statement everywhere you need to do this check even though you know it's only going to be one of the variants.

Alternatively you would need it to take a block header instead of a block number, but then you end up having to construct dummy headers everywhere that doesn't have direct access to a header

[leruaa]

@onbjerg For fork_active() maybe we could keep the current signature and compute if the given fork is active using merge timestamp and slot duration (those 2 being stored on the chain spec)?

[Rjected]

@onbjerg this is most important in the fork filter / forkid calculation, and it might not be necessary to change the entire hardfork abstraction to support time-based fork activation. Forks shanghai and onwards will not be block number based, so I think it makes sense to at least include this kind of abstraction in the fork filter. Open minded to it existing / not existing elsewhere.

Given that shanghai will not be activated by a block number though, how can the current signature of fork_active be correct? Maybe instead it should be fork_active(fork: Hardfork, current_block: ForkKind) -> bool? Hypothetically if we're at the tip, and are passing activation, we won't have a block number to refer to when checking if it's active yet

[onbjerg]

For fork_active() maybe we could keep the current signature and compute if the given fork is active using merge timestamp and slot duration (those 2 being stored on the chain spec)?

This won't work for Paris since that is used in PoW-land and it won't work for other networks since the offset in terms of timestamp will be different

[onbjerg]

Forks shanghai and onwards will not be block number based, so I think it makes sense to at least include this kind of abstraction in the fork filter. Open minded to it existing / not existing elsewhere.

Ah ok, I was unaware that this is going to be the standard now. Perhaps we have a TTD/Number/Time enum as you suggested and the fork_active function could take some struct e.g.:

struct ForkCondition {
  // current block number
  number: BlockNumber,
  total_difficulty: usize,
  timestamp: usize
}

And then you would use it like: fork_active(Hardfork::Shanghai, ForkCondition::time(1234))? The name kind of sucks but I hope the idea makes sense. This would also allow us to remove the Paris and Shanghai special handling we have now.

[Rjected]

Ah ok, I was unaware that this is going to be the standard now. Perhaps we have a TTD/Number/Time enum as you suggested and the fork_active function could take some struct e.g.:
...
And then you would use it like: fork_active(Hardfork::Shanghai, ForkCondition::time(1234))? The name kind of sucks but I hope the idea makes sense. This would also allow us to remove the Paris and Shanghai special handling we have now.

Yeah, I think this could work, and is probably the best way to do it since paris is activated by ttd, not block num or timestamp

Some more context on timestamp forking being the standard for now: go-ethereum#26481

[leruaa]

I'm not sure to understand the diif between the ForkKind enum and the ForkCondition struct. Given that the latter can be used like that: ForkCondition::time(1234), I guess all the fields are Option? Why not just have:

fn fork_active(fork: Hardfork, kind: ForkKind) -> bool

[leruaa]

Oh I think I get it: ForkCondition is used to provide all infos needed by fork_active() to find out if the given fork is active