primitives: use alloy `Header` struct #10691

tcoratger · 2024-09-04T14:41:18Z

Should close #10687

Blocked by:

tcoratger · 2024-09-04T14:47:59Z

I think that we should revise the types in alloy to transform all u128 to u64 to limit the conversions and be fully aligned between alloy and reth. Maybe we could do that in a follow up PR in alloy if there are no limitations about this on alloy side?

mattsse

the casts are indeed excessive and we should change them in alloy, because only u64 are useful here

wdyt @klkvr

mattsse · 2024-09-07T13:52:59Z

crates/net/downloaders/src/file_client.rs

+        self.headers.get(&self.max_block()?).map(|h| {
+            let sealed = h.clone().seal_slow();
+            SealedHeader::new(sealed.inner().clone(), sealed.seal())
+        })


why does this have to be changed?

Because in alloy seal_slow returns a generic Sealed<T> type which is different from the SealedHeader that we have in reth. So that to be compatible with reth at the moment we have to reconstruct a reth SealedHeader from the alloy Sealed<Header> type.

But I'm thinking about a follow up PR to replace our reth SealedHeader by the alloy Sealed<Header> primitive wdyt?

this now has an additional clone, this is not great and looks just weird.
we don't need the seal function here if we only need the hash

this can also be solved with a SealedHeader:: seal(Header) function

Maybe to reduce overhead, what would you say to replace also in this PR SealedHeader by alloy Sealed type?

Suggested change

self.headers.get(&self.max_block()?).map(|h| {

let sealed = h.clone().seal_slow();

SealedHeader::new(sealed.inner().clone(), sealed.seal())

})

self.headers.get(&self.max_block()?).map(|h| {

let sealed = h.clone().seal_slow();

let (header, seal) = sealed.into_parts();

SealedHeader::new(header, seal);

})

I think we can just do this?

Just fixed this but I think that in a follow up we can simply rm SealedHeader from reth and use Sealed instead, it will simplify everything no?

crates/net/downloaders/src/headers/reverse_headers.rs

tcoratger · 2024-09-07T13:57:52Z

the casts are indeed excessive and we should change them in alloy, because only u64 are useful here

wdyt @klkvr

@mattsse Yeah related alloy-rs/alloy#1241 cc @greged93

klkvr · 2024-09-07T17:53:06Z

the casts are indeed excessive and we should change them in alloy, because only u64 are useful here

agreed, we should definitely unify all of this, there's no point in u128 as revm uses u64 anyway, wondering if there's a spec for gas value type?

tcoratger · 2024-09-07T18:05:22Z

the casts are indeed excessive and we should change them in alloy, because only u64 are useful here

agreed, we should definitely unify all of this, there's no point in u128 as revm uses u64 anyway, wondering if there's a spec for gas value type?

@klkvr If you validate alloy-rs/alloy#1241 (please can you comment there to be sure this is accepted), me or @greged93 can start working on this :)

tcoratger · 2024-09-10T16:17:13Z

@klkvr @mattsse In my opinion:

We have way too many conversions here (it's not avoidable in the current state). In my opinion we should first merge this PR to get through this big part and then directly after follow up on [Feature] Make some gas fields u64 for Header alloy-rs/alloy#1241 to have a clear consensus on the types u64 vs u128.
Then we should also do a follow up to remove SealedHeader from the repo and use the alloy type Sealed<Header> in order to remove the unnecessary lines of code to convert a Sealed<Header> to SealedHeader each time (but if we do it in this PR it will be way too long).

emhane · 2024-09-20T02:12:25Z

need help fixing merge conflicts @tcoratger ? all for removing redundancy, think it will be brilliant

tcoratger · 2024-09-20T04:43:20Z

need help fixing merge conflicts @tcoratger ? all for removing redundancy, think it will be brilliant

@emhane No worries, I think I can handle it. Can you provide just an overview review to confirm that everything (the approach, the conversion handling...) is good so that directly after solving conflicts we can merge and avoid further conflicts?

emhane

lgtm

tcoratger · 2024-09-22T22:07:02Z

@emhane @mattsse Should be good now, can we merge asap? so that we don't have to handle additional conflicts because a lot of files are involved so that this is very easy to generate new conflicts :)

emhane · 2024-09-23T12:53:31Z

crates/consensus/beacon/src/engine/sync.rs

+            let sealed = header.seal_slow();
+            let (header, seal) = sealed.into_parts();
+            sealed_header = SealedHeader::new(header, seal);


pls open issue if one doesn't exist to remove SealedHeader in favour of alloy Sealed as you suggested, to avoid these conversions

Done #11123

primitives: use alloy Header struct

9a93fba

tcoratger mentioned this pull request Sep 5, 2024

[Feature] Make some gas fields u64 for Header alloy-rs/alloy#1241

Closed

Merge branch 'main' into alloy-header

4aa170c

mattsse reviewed Sep 7, 2024

View reviewed changes

fix comments

a9ef8d6

tcoratger mentioned this pull request Sep 9, 2024

consensus: implement AsRef for Header alloy-rs/alloy#1260

Merged

tcoratger and others added 5 commits September 9, 2024 18:55

bump alloy to 0.3.2

895629d

Merge branch 'main' into alloy-header

a68a693

update

c2bfd42

update

fcd99a0

clean up

9345aee

tcoratger marked this pull request as ready for review September 10, 2024 16:13

tcoratger requested review from joshieDo, rkrasiuk, rakita, shekhirin, onbjerg, Rjected, emhane, fgimenez and gakonst as code owners September 10, 2024 16:13

tcoratger mentioned this pull request Sep 10, 2024

consensus: implement seal helper for Header alloy-rs/alloy#1269

Merged

tcoratger added 3 commits September 10, 2024 18:24

fix test

c808216

fix test

a122246

rm useless test

c868fe7

emhane approved these changes Sep 22, 2024

View reviewed changes

tcoratger added 3 commits September 22, 2024 23:40

fix conflicts

601c7a6

fix conflicts

7aee92f

some fixes after rebase

e54355a

tcoratger added 2 commits September 23, 2024 00:28

fix clippy

152fb63

fix clippy

05c3eb7

emhane reviewed Sep 23, 2024

View reviewed changes

emhane added this pull request to the merge queue Sep 23, 2024

tcoratger mentioned this pull request Sep 23, 2024

primitives: use alloy Sealed<Header> instead of SealedHeader #11123

Open

Merged via the queue into paradigmxyz:main with commit ed1de89 Sep 23, 2024
36 checks passed

This was referenced Sep 23, 2024

consensus: add BlockHeader getter trait alloy-rs/alloy#1302

Merged

primitives: make block header gas limit field compatible with u128 #11130

Closed

0xForerunner pushed a commit to 0xForerunner/reth that referenced this pull request Sep 25, 2024

primitives: use alloy Header struct (paradigmxyz#10691)

05b3aa3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

primitives: use alloy `Header` struct #10691

primitives: use alloy `Header` struct #10691

tcoratger commented Sep 4, 2024

tcoratger commented Sep 4, 2024

mattsse left a comment

mattsse Sep 7, 2024

tcoratger Sep 7, 2024

mattsse Sep 7, 2024 •

edited

Loading

mattsse Sep 7, 2024

tcoratger Sep 7, 2024

klkvr Sep 7, 2024

tcoratger Sep 7, 2024

tcoratger commented Sep 7, 2024

klkvr commented Sep 7, 2024

tcoratger commented Sep 7, 2024

tcoratger commented Sep 10, 2024

emhane commented Sep 20, 2024

tcoratger commented Sep 20, 2024

emhane left a comment

tcoratger commented Sep 22, 2024

emhane Sep 23, 2024

tcoratger Sep 23, 2024

primitives: use alloy Header struct #10691

primitives: use alloy Header struct #10691

Conversation

tcoratger commented Sep 4, 2024

tcoratger commented Sep 4, 2024

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse Sep 7, 2024 • 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

tcoratger commented Sep 7, 2024

klkvr commented Sep 7, 2024

tcoratger commented Sep 7, 2024

tcoratger commented Sep 10, 2024

emhane commented Sep 20, 2024

tcoratger commented Sep 20, 2024

emhane left a comment

Choose a reason for hiding this comment

tcoratger commented Sep 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

primitives: use alloy `Header` struct #10691

primitives: use alloy `Header` struct #10691

mattsse Sep 7, 2024 •

edited

Loading