fix(l1): fork_id validation #1729
Conversation
|
crates/common/types/fork_id.rs
Outdated
| // decide if our head is block or timestamp based. | ||
| let mut head = head_timestamp; | ||
| if let Some(last_block_number_based_fork) = block_number_based_forks.last() { | ||
| if *last_block_number_based_fork > latest_block_number { |
There was a problem hiding this comment.
I don't understand this condition. The idea is that if we are on a fork that is block-number based we should use the block number, but this condition does not ensure we are on a block-based fork.
For example if we were on genesis on a homestead block with block number 0 and homestead block 0 with no other forks we would be on a block-based fork but the condition would be false. I think we could check that there are no timestamp-based forks <= to our timestamp instead
There was a problem hiding this comment.
Addressed in c7f4ef8!
crates/common/types/fork_id.rs
Outdated
| // See https://eips.ethereum.org/EIPS/eip-2124#validation-rules. | ||
| pub fn is_valid( | ||
| &self, | ||
| incoming: Self, |
There was a problem hiding this comment.
nit: maybe we can call this one remote instead so it matches the eip.
There was a problem hiding this comment.
Addressed in d8f81cb.
crates/common/types/fork_id.rs
Outdated
| let mut hasher = Hasher::new(); | ||
| hasher.update(genesis_hash.as_bytes()); | ||
| for activation in forks { | ||
| if activation != last_activation { |
There was a problem hiding this comment.
nit: A comment here with "skip forks with repeated block numbers or timestamps" would help.
Optionally, de-nesting into the following might help too, but that one might be personal taste instead of good reason:
if activation == last_activation {
continue;
}There was a problem hiding this comment.
Agreed! Addressed in d8f81cb.
| } | ||
| } | ||
|
|
||
| fn get_all_fork_id_combinations(forks: Vec<u64>, genesis_hash: BlockHash) -> Vec<(H32, u64)> { |
There was a problem hiding this comment.
Definitely not for this PR, but we should probably pre-compute/cache this and have it available in all connections without recomputing.
There was a problem hiding this comment.
Created #1761!
| ]; | ||
|
|
||
| assert_test_cases(test_cases, genesis.config, genesis_hash); | ||
| } |
There was a problem hiding this comment.
Can we add some non-happy test cases? We have, for instance, the cases in the spec. We can create some arbitrary A,B and C fork hashes and do the combinations.
There was a problem hiding this comment.
Good idea! Added in e037d4a!
Co-authored-by: fmoletta <[email protected]>
Co-authored-by: Tomás Arjovsky <[email protected]>
Co-authored-by: Tomás Arjovsky <[email protected]>
Co-authored-by: Tomás Arjovsky <[email protected]>
Motivation
We are currently checking whether the received fork ID is exactly the same as ours. Instead, we should follow the validation rules specified in https://eips.ethereum.org/EIPS/eip-2124.
Description
This PR adds a proper validation to the received fork id.
Closes #1685