Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(l2-withdrawals): consensus rules #14308

Merged
merged 30 commits into from
Feb 15, 2025
Merged

feat(l2-withdrawals): consensus rules #14308

merged 30 commits into from
Feb 15, 2025
+289 −20

Conversation

emhane
Copy link
Member

@emhane emhane commented Feb 7, 2025

Checks out consensus rules from #14124

@emhane emhane added A-consensus Related to the consensus engine A-op-reth Related to Optimism and op-reth labels Feb 7, 2025
emhane
emhane commented Feb 7, 2025
View reviewed changes
crates/consensus/consensus/src/lib.rs Outdated
Comment on lines 453 to 455
/// Other, likely an L2 error.
#[display("other, search logs for l2 error")]
Other,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this variant is temp, is possible to remove since #13655 lays way for this, but grows scope of this pr way too large since is hardcoded Error = ConsensusError for use Consensus trait in many places in codebase

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me see if we can relax the clone here then we can use Other(Box here

@emhane emhane mentioned this pull request Feb 7, 2025
Closed
@emhane emhane requested a review from shekhirin as a code owner February 7, 2025 18:00
@emhane
Copy link
Member Author

emhane commented Feb 7, 2025

removed the dep on reth-trie since requested change to move HashedStorage to a no_std compatible crate wasn't desired #14208

@emhane
Copy link
Member Author

emhane commented Feb 8, 2025

windows failing ci #14337

@emhane emhane enabled auto-merge February 9, 2025 15:11
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Feb 9, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #14308 will not alter performance

Comparing emhane/l2-withdrawals-consensus-rules (e47bd7d) with main (113a87b)

Summary

✅ 77 untouched benchmarks

@emhane emhane requested a review from klkvr February 12, 2025 07:53
mattsse
mattsse requested changes Feb 12, 2025
View reviewed changes
crates/consensus/consensus/src/lib.rs Outdated
Comment on lines 453 to 455
/// Other, likely an L2 error.
#[display("other, search logs for l2 error")]
Other,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me see if we can relax the clone here then we can use Other(Box here

@emhane
Copy link
Member Author

emhane commented Feb 12, 2025

let me see if we can relax the clone here then we can use Other(Box here

I tried not too long ago, and it was painful @mattsse

@emhane emhane requested a review from mattsse February 12, 2025 18:03
@emhane emhane mentioned this pull request Feb 13, 2025
Open
mattsse
mattsse requested changes Feb 13, 2025
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few more suggestions, lg over all

@emhane emhane force-pushed the emhane/l2-withdrawals-consensus-rules branch from c9d6f8e to a4c2bb2 Compare February 13, 2025 18:39
@emhane emhane requested a review from DaniPopes February 13, 2025 18:58
mattsse
mattsse requested changes Feb 14, 2025
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last pedantic error nit

@emhane emhane added the S-blocked This cannot more forward until something else changes label Feb 14, 2025
@emhane
Copy link
Member Author

emhane commented Feb 14, 2025

blocked by #14499

@emhane emhane removed the S-blocked This cannot more forward until something else changes label Feb 14, 2025
@emhane emhane requested a review from mattsse February 14, 2025 21:45
@emhane emhane added this pull request to the merge queue Feb 15, 2025
mattsse
mattsse approved these changes Feb 15, 2025
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm,

one step closer

Merged via the queue into main with commit 9c1988b Feb 15, 2025
44 checks passed
@emhane emhane deleted the emhane/l2-withdrawals-consensus-rules branch February 15, 2025 08:27
@emhane emhane mentioned this pull request Feb 19, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

@rkrasiuk rkrasiuk rkrasiuk approved these changes

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

@fgimenez fgimenez Awaiting requested review from fgimenez fgimenez is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@shekhirin shekhirin Awaiting requested review from shekhirin

@klkvr klkvr Awaiting requested review from klkvr

@DaniPopes DaniPopes Awaiting requested review from DaniPopes

Assignees
No one assigned
Labels
A-consensus Related to the consensus engine A-op-reth Related to Optimism and op-reth
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emhane @mattsse @rkrasiuk