Update HeaderWithProof with latest spec changes
#1672
Conversation
njgheorghita
force-pushed
the
hwp-remove-none
branch
from
1a1427c to
47e6285
Compare
njgheorghita
requested review from
KolbyML,
morph-dev and
ogenev
and removed request for
KolbyML
|
Ready for an intial review on this pr. The failing tests pass locally when I use ethereum/portal-spec-tests#34 as a source for test vectors. But I'm happy to wait until that's merged to merge this one through |
I am not aware of a reason why you would need to |
There was a problem hiding this comment.
PR looks good. I am a little confused by this statement But I'm happy to wait until that's merged to merge this one through, because I thought it was assumed we wait for CI passing to merge, so the message is making me second guess myself, but I will ignore that feeling.
| @@ -40,21 +46,26 @@ impl ssz::Encode for HeaderWithProof { | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone, PartialEq, Decode, Encode, Deserialize)] | |||
| #[ssz(enum_behaviour = "union")] | |||
| #[ssz(enum_behaviour = "transparent")] | |||
There was a problem hiding this comment.
We will need both old type (with union and SszNone) until we are done with migration. Otherwise, I won't be able to do the migration (meaning, I won't be able to successfully decode content from the db).
I would also remove Decode from derive, because now that it's no longer union, we can't decode it (we need header timestamp). Technically, this is not accurate as format is different enough (I believe) so that only one would decode correctly. But we shouldn't try to decode this object directly, so better not to implement the trait.
There was a problem hiding this comment.
Thinking more about this, I think that old type should remain the same, and the new type should have new name
There was a problem hiding this comment.
I checked the spec and it seems that ExecutionBlockProof is different for BlockProofHistoricalRoots and BlockProofHistoricalSummaries, while we still use the some type.
Are all updated specs tests passing? Do we cover all these types in them?
| let header: Header = Decodable::decode(&mut header_rlp.as_slice()).map_err(|_| { | ||
| ssz::DecodeError::BytesInvalid("Unable to decode bytes into header.".to_string()) | ||
| })?; | ||
|
|
||
| let proof = if header.timestamp <= MERGE_TIMESTAMP { |
There was a problem hiding this comment.
Should this be <= or <? At least intuitively for me, MERGE_TIMESTAMP should be the timestamp of the first post-merge block (but I don't know if that's try in practice).
Either way, I think we should have tests to cover these corner cases, meaning have a test case for the last pre-merge and first post-merge blocks (also shanghai)
| HistoricalRootsBlockProof(HistoricalRootsBlockProof), | ||
| HistoricalSummariesBlockProof(HistoricalSummariesBlockProof), | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct PreMergeAccumulatorProof { | ||
| pub struct HistoricalHashesAccumulatorProof { |
There was a problem hiding this comment.
Not strictly part of this PR, but can't we replace this structure with:
pub type HistoricalHashesAccumulatorProof = FixedVector<B256, typenum::U15>;
|
I applied some of my suggestions in my local work: It's not polished at all, so feel free to take a look and take some or all of it. A lot of the changes are refactoring not really related to this PR/work (I might extract them into separate PR if I get to it. |
What was wrong?
Specs have been updated to the
HeaderWithProoftypeNonetype for the proofHow was it fixed?
HeaderWithProoftypePreMergeAccumulatorProof->HistoricalHashesAccumulatorProofto better match the specsportal-bridge/ some test utils have a temporary invalid proof. The next step is to update the bridge to generate valid proofs, but I didn't want to combine these two prs otherwise it would get pretty large. Next, I'll work on updating the bridge and then update all the invalid proofs to be validTo-Do