Skip to content

feat: electra withdrawals #1431

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.

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

Merged
merged 114 commits into from
Apr 25, 2025
Merged

feat: electra withdrawals #1431

merged 114 commits into from
Apr 25, 2025

Conversation

LeanSerra
Copy link
Contributor

@LeanSerra LeanSerra commented Apr 15, 2025

Motivation

Implement changes in Electra to withdrawals + fix all spec-tests related to it.

Description

  • Modified get_expected_withdrawals link
  • Modified process_withdrawals link

Fixed Spec Tests

test/generated/mainnet/electra/operations.exs:
previous: 333 tests, 19 failures
now: 333 tests, 0 failures

test/generated/mainnet/electra/sanity.exs:
previous: 97 tests, 8 failures, 1 skipped
now: 97 tests, 2 failures, 1 skipped

test/generated/minimal/electra/operations.exs:
previous: 366 tests, 38 failures
now: 366 tests, 0 failures

test/generated/minimal/electra/sanity.exs:
previous: 104 tests, 8 failures, 1 skipped
now: 104 tests, 2 failures, 1 skipped

tests fixed: 69

Spec Test Progress

11370 tests, 4 failures, 784 skipped

LeanSerra added 30 commits April 3, 2025 11:21

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
in fails
@LeanSerra LeanSerra force-pushed the electra_withdrawals branch from 2a73484 to 1bb0e6c Compare April 16, 2025 17:16
LeanSerra and others added 16 commits April 16, 2025 15:06
…g in with at validate_attestation
…g in with at validate_attestation
Base automatically changed from electra_consolidations to electra-support April 24, 2025 20:54
@LeanSerra LeanSerra marked this pull request as ready for review April 24, 2025 21:13
@LeanSerra LeanSerra requested a review from a team as a code owner April 24, 2025 21:13
LeanSerra and others added 2 commits April 25, 2025 11:45
Copy link
Collaborator

@rodrigo-o rodrigo-o left a comment

Choose a reason for hiding this comment

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

LGTM, just some comments for the future and one question

Comment on lines +365 to 367
@spec get_expected_withdrawals(BeaconState.t()) ::
{list(Withdrawal.t()), non_neg_integer()}
def get_expected_withdrawals(%BeaconState{} = state) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most of this function was preserved, but probably worth to take a look at it in the future regarding to clarity (and even performance after measuring it)

{complete_withdrawals, processed_partial_withdrawals_count}
end

defp process_partial_withdrawal(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can improve both of this functions (process_partial_withdrawal and do_process_partial_withdrawal), but i guess it's better to have them merged now to the electra-support branch and pass the spec-test. I'm going to add these 2 to the list of functions we need to revisit for legibility.

Comment on lines +452 to +454
if withdrawal.withdrawable_epoch > epoch ||
processed_partial_withdrawals_count == max_pending_partials_per_withdrawals_sweep do
{:halt, {processed_partial_withdrawals_count, withdrawal_index, withdrawals}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is mostly a question I have, they are ordered right? specially for the withdrawal.withdrawable_epoch > epoch because that just make sense to halt if you have them ordered by epoch, or am I missing something?

Btw I checked that this was the correct condition to break the loop in the spec but i was intrigued about the order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question, I don't explicitly order them but I've yet to see a case in which they aren't not in the spec tests and not while syncing Sepolia. I can add that or maybe leave a comment so that if we run into this issue later in testing we can fix it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe there is no way to get them unordered but yes, I think we should left at least a comment for the future (but probably where we actually add the pending widthrawal instead of here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left comment both there and when we are adding the partial withdrawals to the state here

@@ -1030,7 +1134,7 @@ defmodule LambdaEthereumConsensus.StateTransition.Operations do

is_correct_source_address =
case validator.withdrawal_credentials do
<<_::binary-size(12), rest>> -> rest == address
<<_::binary-size(12), validator_address::binary>> -> validator_address == address
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

…if they are not ordered
@LeanSerra LeanSerra merged commit 911af45 into electra-support Apr 25, 2025
11 of 13 checks passed
@LeanSerra LeanSerra deleted the electra_withdrawals branch April 25, 2025 17:39
rodrigo-o pushed a commit that referenced this pull request Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electra elixir Pull requests that update Elixir code
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants