Update HeaderWithProof with latest spec changes

[njgheorghita]

What was wrong?

Specs have been updated to the HeaderWithProof type

• Removing the None type for the proof
• Removes the ssz union

How was it fixed?

• Updated the HeaderWithProof type
• Renamed PreMergeAccumulatorProof -> HistoricalHashesAccumulatorProof to better match the specs
• I tested these locally against the test vectors in Update the header_with_proof test vectors after SSZ Union removal portal-spec-tests#34 but this pr will have failing tests until that is merged
• portal-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 valid

To-Do

• Clean up commit history and use conventional commits.

[njgheorghita]

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

[KolbyML]

• portal-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 valid

I am not aware of a reason why you would need to but I didn't want to combine these two prs otherwise if you updated the bridge to generate valid proofs first. They could be separate PR's regardless, so I am a little confused by the claim, that this PR was made first so it was possible to make 2 separate PR's. I am fine with this order, I am just a little confused with the wording I guess.

[KolbyML]

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.

[morph-dev]

Left some high level comments.

I plan to work on top of this PR, and I might suggest some changes if they are blocking my work.

[morph-dev]

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.

[morph-dev]

Thinking more about this, I think that old type should remain the same, and the new type should have new name

[morph-dev]

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?

[njgheorghita]

All the updated specs tests are passing now. I actually don't think the test vectors cover all cases (aka, 1 pre-merge, 1 capella, 1 post-capella). This is worth addressing.

Also, looking at the spec, it looks like our naming convention is inverse of what's in the spec, which we should update to be 1-1. Though, if I'm reading everything correctly, I actually think our logic is sound. "our" HistoricalSummariesProof type and HistoricalRootsProof type (which I think are the types you're referring to?) seem to be up to spec. But, honestly it's confusing with all the variable naming schemes. I don't quite consider this a blocker for this pr? I'd rather address it in a follow-up pr where I'll update all the names to be spec-compliant and add missing test vectors? But can do it here if you think that's worthwhile.

[morph-dev]

No need to do it in this PR.

[morph-dev]

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)

[njgheorghita]

Well, according to the updated test specs, it needs to be <= otherwise the test "last pre-merge" test vector ( 15537393.yaml) fails decoding. The "trigger" for the merge was difficulty-based, so I'm guessing that our use of a timestamp is somewhat arbitrary

[morph-dev]

Not strictly part of this PR, but can't we replace this structure with:

pub type HistoricalHashesAccumulatorProof = FixedVector<B256, typenum::U15>;

[morph-dev]

I applied some of my suggestions in my local work:
njgheorghita/trin@hwp-remove-none...morph-dev:pr/1672

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.

[njgheorghita]

@KolbyML Sorry if I was confusing, I just wanted to avoid bloating this pr with too many changes. And you're right we shouldn't be merging with failing ci, I was just saying so funnily

@morph-dev I moved the new types into an updated module. I incorporated most of your suggestions from your branch here. Lmk how you feel about updating the naming/test vectors in this pr or a follow up. I've copied the updated test vectors from the pending pr here so merging is no longer blocked by a failing ci.

[morph-dev]

Looks good. Few suggestions, but I'm not going to insist on them:

• I would move new content_value.rs file into ethportal-api/src/types/content_value/history_pr1666.rs
  • I'm not the biggest fan of _pr1666 suffix, and either _new or _updated is fine as well. All of this is only temporarily.
• We can also move new header_with_proof.rs one level up and name it: header_with_proof_pr1666.rs
• If I'm not mistaken, the test files (in validation/src/asssets/fluffy) are currently only used with this new header_with_proof.rs, so I would move them somewhere in ethportal-api crate.

[morph-dev]

Even tho it might be tested in some other places indirectly, I think we should have tests here that test that encoding and decoding works as expected.

This doesn't have to be done in this PR (we should have test vectors for this).

[njgheorghita]

I removed the existing tests since they were duplicates. But I guess the tests you're referencing are for encoding/decoding the HistoryContentValue and not necessarily the inner type. I'll circle back to this pr and add these later

[njgheorghita]

If I'm not mistaken, the test files (in validation/src/asssets/fluffy) are currently only used with this new header_with_proof.rs, so I would move them somewhere in ethportal-api crate.

I left them where they were because I'd like to remove that entire test asset folder after the "upgrade" since they're all duplicates from the portal-spec-tests repo (and once the updated vectors get merged, we won't need these files anymore). (one exception might be epoch_acc.bin but I'll find a new home for that file if necessary).

I updated the modules to the new naming convention (*_new.rs).

We can also move new header_with_proof.rs one level up and name it:

I ended up moving it into src/types/execution/ so that it sits at the same level as it's "older" version