Add ChainSpec::hardfork_fork_id and ChainSpec::shanghai_fork_id helper fns #4748

0xprames · 2023-09-23T13:25:55Z

should close #4745

opening this PR as draft - want to add some tests for these helpers

codecov · 2023-09-23T13:34:14Z

Codecov Report

Merging #4748 (01e3801) into main (05198e9) will increase coverage by 0.05%.
The diff coverage is 95.60%.

Files	Coverage Δ
crates/primitives/src/hardfork.rs	`100.00% <100.00%> (ø)`
crates/primitives/src/chain/spec.rs	`97.87% <95.57%> (-0.21%)`	⬇️

... and 8 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`15.45% <1.68%> (-0.06%)`	⬇️
unit-tests	`62.70% <95.60%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
reth binary	`32.28% <ø> (ø)`
blockchain tree	`80.44% <ø> (ø)`
pipeline	`88.45% <ø> (ø)`
storage (db)	`73.31% <ø> (ø)`
trie	`94.48% <ø> (-0.04%)`	⬇️
txpool	`49.03% <ø> (-0.48%)`	⬇️
networking	`76.14% <ø> (-0.05%)`	⬇️
rpc	`57.79% <ø> (-0.01%)`	⬇️
consensus	`61.06% <ø> (ø)`
revm	`28.53% <ø> (ø)`
payload builder	`8.16% <ø> (ø)`
primitives	`85.52% <95.60%> (+0.25%)`	⬆️

mattsse

ptal @Rjected

mattsse · 2023-09-24T10:43:40Z

crates/primitives/src/chain/spec.rs

+                // the timestamp check here is a bit brittle as its tied to the self.satisfy() impl
+                // returning a non-0 head.timestamp for ForkCondition::Timestamp, and a 0
+                // timestamp for the other ForkCondition types
+                // this check is required though - without it, we will drop into the else block and
+                // shortcircuit for heads valid during timestamp-based forks, leading to an
+                // incorrect ForkId for Shanghai, Cancun etc. An alternative fix
+                // here would be to modify the Head returned in self.satisfy() to
+                // include a valid blocknum for ForkCondition::Timestamp, thus not
+                // excercising the else block, and calculating an incorrect fork_hash


I really don't understand why we need this now,

this PR was supposed to add support for this on Chainspec

reth/crates/primitives/src/hardfork.rs

Lines 47 to 53 in b7a6eed

/// Get the [ForkId] for this hardfork in the given spec, if the fork is activated at any point.

pub fn fork_id(&self, spec: &ChainSpec) -> Option<ForkId> {

match spec.fork(*self) {

ForkCondition::Never => None,

_ => Some(spec.fork_id(&spec.fork(*self).satisfy())),

}

}

spec.fork_id(&spec.fork(*self).satisfy()) this line has a bug for Hardfork::Shanghai and all timestamp based HardForks - I have removed the fix above and push up the assoc. test failures.

I caught it while adding unit tests - The existing test coverage differs from the satisfy() actual implementation, and the tests I've added in this PR hook into the underlying satisfy() call.

See:

running 1 test thread 'chain::spec::tests::mainnet_hardfork_fork_ids' panicked at 'assertion failed: `(left == right)` left: `ForkId { hash: ForkHash("dce96c2d"), next: 0 }`, right: `ForkId { hash: ForkHash("fc64ec04"), next: 1150000 }`: Expected fork ID ForkId { hash: ForkHash("dce96c2d"), next: 0 }, computed fork ID ForkId { hash: ForkHash("fc64ec04"), next: 1150000 } for hardfork Shanghai', crates/primitives/src/chain/spec.rs:1179:17 stack backtrace:

Happy to sync with @Rjected on this further.

Without my change the ForkId for Shanghai and presumably all the future timestamp based forks (Cancun etc) will short-circuit the existing logic and return the ForkId for Frontier

This is probably a bug in satisfy, since to obtain the ForkId for shanghai, the fork_id method uses the block number of the Head argument to apply the block-based forks, before adding the timestamp-based forks. Satisfy does this:

reth/crates/primitives/src/chain/spec.rs

Lines 939 to 943 in 7a0781c

ForkCondition::Block(number) => Head { number, ..Default::default() },

ForkCondition::Timestamp(timestamp) => Head { timestamp, ..Default::default() },

ForkCondition::TTD { total_difficulty, .. } => {

Head { total_difficulty, ..Default::default() }

}

fork_id does this:

reth/crates/primitives/src/chain/spec.rs

Lines 488 to 498 in 7a0781c

for (_, cond) in self.forks_iter() {

// handle block based forks and the sepolia merge netsplit block edge case (TTD

// ForkCondition with Some(block))

if let ForkCondition::Block(block) |

ForkCondition::TTD { fork_block: Some(block), .. } = cond

{

if cond.active_at_head(head) {

if block != current_applied {

forkhash += block;

current_applied = block;

}

So for satisfy to work with timestamp forks, it needs to return a Head with a block and TTD greater than the highest known fork block / TTD respectively. It should also not use Default::default.

yeah exactly!

called similar out in the comments:

An alternative fix
// here would be to modify the Head returned in self.satisfy() to
// include a valid blocknum for ForkCondition::Timestamp,

I can fix it there instead of this timestamp fix, I'll push that up in the next rev

ah so... I spent a bit of time looking into this, and fixing it in satisfy() is a little bit more difficult than I thought.

That method is type ForkCondition::satisfy() and effectively only has access each enum variant's values. So in this case. satisfy() only has access to either

ForkCondition::Block(block_num) => Head { block_num, .. Default()) ForkCondition::Timestamp(timestamp) = Head {timestamp, .. Default())

so if we want to fix it in satisfy() we may need to change ForkCondition::Timestamp(u64,u64) to include both the block_num and timestamp - and change it everywhere its been declared/initalized.

It will be a larger code change - which I'm open to take on. The alternative to fixing it in satisfy() is to instead just add

if cond.active_at_head(head) || head.timestamp > 0

like it was in the first push up the prev commit to this PR.

Wdyt @Rjected - should we refactor ForkCondition::Timestamp to include a block_num in the variant vals? It feels like this may take away from the whole "timestamp based fork condition" aspect, but it may also be necessary.

Happy to implement another solution as well - if one exists.

also - it doesn't make sense to just hardcode a blocknum/ttd in that Head{} return because it varies per chain.

so mainnet's forkcondition::timestamp for Shanghai/Cancun will be different than sepolias.

I don't really want to add in block nums to timestamp forks. Maybe ForkCondition is the wrong place to put satisfy though - looking at the two places where it's used:

reth/crates/primitives/src/hardfork.rs

Lines 47 to 62 in 4603dc3

/// Get the [ForkId] for this hardfork in the given spec, if the fork is activated at any point.

pub fn fork_id(&self, spec: &ChainSpec) -> Option<ForkId> {

match spec.fork(*self) {

ForkCondition::Never => None,

_ => Some(spec.fork_id(&spec.fork(*self).satisfy())),

}

}

/// Get the [ForkFilter] for this hardfork in the given spec, if the fork is activated at any

/// point.

pub fn fork_filter(&self, spec: &ChainSpec) -> Option<ForkFilter> {

match spec.fork(*self) {

ForkCondition::Never => None,

_ => Some(spec.fork_filter(spec.fork(*self).satisfy())),

}

}

It looks like we have access to the ChainSpec - so this could be ChainSpec::satisfy instead of ForkCondition::satisfy

alright, let me take a look.

mattsse · 2023-09-24T10:44:34Z

crates/primitives/src/chain/spec.rs

+    /// Get the fork id for the given hardfork.
+    pub fn hardfork_fork_id(&self, fork: Hardfork) -> ForkId {
+        self.fork_id(&self.fork(fork).satisfy())
+    }


why is this not an option here? but on hardfork it is.

looks like this does not handle the ::Never case

ah sorry about that - added.

Rjected

I think this is a bug in satisfy, see comment for why

Rjected · 2023-09-25T17:16:27Z

crates/primitives/src/chain/spec.rs

+                // the timestamp check here is a bit brittle as its tied to the self.satisfy() impl
+                // returning a non-0 head.timestamp for ForkCondition::Timestamp, and a 0
+                // timestamp for the other ForkCondition types
+                // this check is required though - without it, we will drop into the else block and
+                // shortcircuit for heads valid during timestamp-based forks, leading to an
+                // incorrect ForkId for Shanghai, Cancun etc. An alternative fix
+                // here would be to modify the Head returned in self.satisfy() to
+                // include a valid blocknum for ForkCondition::Timestamp, thus not
+                // excercising the else block, and calculating an incorrect fork_hash


This is probably a bug in satisfy, since to obtain the ForkId for shanghai, the fork_id method uses the block number of the Head argument to apply the block-based forks, before adding the timestamp-based forks. Satisfy does this:

reth/crates/primitives/src/chain/spec.rs

Lines 939 to 943 in 7a0781c

ForkCondition::Block(number) => Head { number, ..Default::default() },

ForkCondition::Timestamp(timestamp) => Head { timestamp, ..Default::default() },

ForkCondition::TTD { total_difficulty, .. } => {

Head { total_difficulty, ..Default::default() }

}

fork_id does this:

reth/crates/primitives/src/chain/spec.rs

Lines 488 to 498 in 7a0781c

for (_, cond) in self.forks_iter() {

// handle block based forks and the sepolia merge netsplit block edge case (TTD

// ForkCondition with Some(block))

if let ForkCondition::Block(block) |

ForkCondition::TTD { fork_block: Some(block), .. } = cond

{

if cond.active_at_head(head) {

if block != current_applied {

forkhash += block;

current_applied = block;

}

So for satisfy to work with timestamp forks, it needs to return a Head with a block and TTD greater than the highest known fork block / TTD respectively. It should also not use Default::default.

0xprames · 2023-09-28T03:21:48Z

should be ready for another review @Rjected

mattsse

not familiar with how this works,
defer to @Rjected

Rjected

logic-wise this looks good, could we add some tests for the new satisfy method?

…ed fork_ids

… directly

…tamp forks

…isfy() in fork_filter

…nches

0xprames · 2023-09-30T10:25:50Z

cool, I added a relatively comprehensive unit test to cover the cases not already covered in the test_hardfork_fork_id suite across mainnet, goerli, sepolia

should cover all conditional/match arms

mattsse

lgtm, pending @Rjected

Rjected

lgtm, the tests are great!

0xprames force-pushed the chainspec-hardfork_id-helpers branch from d62d1c6 to f84c60e Compare September 23, 2023 23:20

0xprames marked this pull request as ready for review September 23, 2023 23:21

0xprames requested a review from gakonst as a code owner September 23, 2023 23:21

0xprames force-pushed the chainspec-hardfork_id-helpers branch from 6c0ad8f to 5aef0a5 Compare September 23, 2023 23:56

mattsse reviewed Sep 24, 2023

View reviewed changes

mattsse requested a review from Rjected September 24, 2023 10:45

Rjected reviewed Sep 25, 2023

View reviewed changes

mattsse requested a review from Rjected September 27, 2023 09:06

0xprames force-pushed the chainspec-hardfork_id-helpers branch from 8daa6f0 to 76285dd Compare September 27, 2023 19:12

mattsse reviewed Sep 28, 2023

View reviewed changes

Rjected reviewed Sep 29, 2023

View reviewed changes

0xprames added 11 commits September 30, 2023 15:53

(feat/fix) Add ChainSpec::hardfork_fork_id helpers, fix timestamp bas…

f84edb8

…ed fork_ids

test failure for shanghai, fixing fork_id logic

6568db0

fix fork_id logic, shanghai (timestamped fork) fork_id works w/ tests

6a12336

add unit test coverage for test_shanghai_hardfork helper

4a77dc1

clippy - unecessary let during debugging

68a9b99

review comments, comment out fix to showcase test failures

a26f167

simplify chainspec::hardfork_fork_id methods to use Hardfork::fork_id…

045a219

… directly

add ChainSpec::satisfy w/ helper to find last block fork before times…

364ebf7

…tamp forks

remove ForkCondition::satisfy() completely, migrate to ChainSpec::sat…

5a86947

…isfy() in fork_filter

make clippy happy - unecessary if let Some(_) -> .is_some()

2f0bb92

add unit test for ChainSpec::satisfy, cover all match/conditional bra…

01e3801

…nches

0xprames force-pushed the chainspec-hardfork_id-helpers branch from c347a47 to 01e3801 Compare September 30, 2023 10:23

0xprames requested a review from Rjected September 30, 2023 13:06

mattsse approved these changes Oct 2, 2023

View reviewed changes

Rjected approved these changes Oct 3, 2023

View reviewed changes

mattsse added this pull request to the merge queue Oct 3, 2023

Merged via the queue into paradigmxyz:main with commit 2cddd0f Oct 3, 2023
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ChainSpec::hardfork_fork_id and ChainSpec::shanghai_fork_id helper fns #4748

Add ChainSpec::hardfork_fork_id and ChainSpec::shanghai_fork_id helper fns #4748

0xprames commented Sep 23, 2023

codecov bot commented Sep 23, 2023 •

edited

Loading

mattsse left a comment

mattsse Sep 24, 2023

0xprames Sep 24, 2023 •

edited

Loading

0xprames Sep 24, 2023

Rjected Sep 25, 2023

0xprames Sep 25, 2023

0xprames Sep 26, 2023

0xprames Sep 26, 2023

Rjected Sep 26, 2023

0xprames Sep 27, 2023

mattsse Sep 24, 2023 •

edited

Loading

0xprames Sep 24, 2023

Rjected left a comment

Rjected Sep 25, 2023

0xprames commented Sep 28, 2023

mattsse left a comment

Rjected left a comment

0xprames commented Sep 30, 2023

mattsse left a comment

Rjected left a comment

	/// Get the [ForkId] for this hardfork in the given spec, if the fork is activated at any point.
	pub fn fork_id(&self, spec: &ChainSpec) -> Option<ForkId> {
	match spec.fork(*self) {
	ForkCondition::Never => None,
	_ => Some(spec.fork_id(&spec.fork(*self).satisfy())),
	}
	}

	ForkCondition::Block(number) => Head { number, ..Default::default() },
	ForkCondition::Timestamp(timestamp) => Head { timestamp, ..Default::default() },
	ForkCondition::TTD { total_difficulty, .. } => {
	Head { total_difficulty, ..Default::default() }
	}

	for (_, cond) in self.forks_iter() {
	// handle block based forks and the sepolia merge netsplit block edge case (TTD
	// ForkCondition with Some(block))
	if let ForkCondition::Block(block) \|
	ForkCondition::TTD { fork_block: Some(block), .. } = cond
	{
	if cond.active_at_head(head) {
	if block != current_applied {
	forkhash += block;
	current_applied = block;
	}

Add ChainSpec::hardfork_fork_id and ChainSpec::shanghai_fork_id helper fns #4748

Add ChainSpec::hardfork_fork_id and ChainSpec::shanghai_fork_id helper fns #4748

Conversation

0xprames commented Sep 23, 2023

codecov bot commented Sep 23, 2023 • edited Loading

Codecov Report

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xprames Sep 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse Sep 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rjected left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xprames commented Sep 28, 2023

mattsse left a comment

Choose a reason for hiding this comment

Rjected left a comment

Choose a reason for hiding this comment

0xprames commented Sep 30, 2023

mattsse left a comment

Choose a reason for hiding this comment

Rjected left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 23, 2023 •

edited

Loading

0xprames Sep 24, 2023 •

edited

Loading

mattsse Sep 24, 2023 •

edited

Loading