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.

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

Handle top-ups to exiting/exited validators #3776

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

fradamt
Copy link
Contributor

@fradamt fradamt commented May 22, 2024

This PR does the same thing as #3650, which was based on the EIP-7251 spec. Meanwhile, the rationale for it has evolved somewhat, with the realization that it is actually more security-critical than previously realized understood. An attacker can do the following:

  1. Initiate a validator’s exit
  2. Make a top-up while the validator is still exiting
  3. The top-up happens before the exit
  4. Once the validator exits, the topped-up balance leaves the validator set without going through the churn

This can lead to churn limit invariants being violated, even though top-ups go through the deposit churn. The reason for this is subtle: the top-ups can go through the deposit churn slowly, over a long period of time, but the same balance can then exit much faster. For example, say the exit queue is one week long, while the activation queue is completely empty, and an attacker executes the strategy above, exiting validators with minimum balance and topping them up to max balance. Because of the exit queue being full, the large top-ups have one whole week to go through the deposit churn and be processed. Moreover, all exits happen very fast once the week is over, because the validators had minimum balance when the exits were initiated, and so the exits are scheduled very close to each other. The end result is that a bunch of very large exits all happen in a short period of time.

More details in this section of the WIP annotated spec

@fradamt fradamt changed the title handle top-ups to exiting/exited validators Handle top-ups to exiting/exited validators May 22, 2024
@ralexstokes ralexstokes added scope:security General protocol security-related items Electra labels May 22, 2024
Copy link
Contributor

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Makes sense to me

@hwwhww hwwhww mentioned this pull request Jun 4, 2024
10 tasks
@hwwhww hwwhww merged commit 5efd6e1 into ethereum:dev Jun 5, 2024
26 checks passed
@michaelsproul
Copy link
Contributor

michaelsproul commented Jun 24, 2024

It seems this change is necessary for security, but I just wanted to note that it probably has a detrimental impact on the performance of Lighthouse's single-pass epoch processing.

Previously, process_pending_balance_deposits did not depend on state.validators, which allowed us to use the following algorithm:

  1. Loop through pending_balance_deposits and collect applicable deposits into a hashmap
  2. Loop (once) through state.validators and for each index apply all deposits (from the hashmap) for that index

Now we are forced to add random-access in step 1 to load validators and check their exit_epoch and withdrawable_epoch. This is probably acceptable because the length of pending_balance_deposits is not enormous, but I thought I'd just note the sorts of implementation challenges that spec changes like this present.

@mkalinin
Copy link
Collaborator

@michaelsproul thanks for raising this performance point, I am currently working on the set of changes to the eip6110 that was agreed during Nyota (#3689 (comment)).

Those changes leverage on and extend pending_balance_deposits to keep full deposits and do full deposit processing including sig verification and new validator creation, so it involves reads and modifications to the state.validators. One of the points that we have agreed is to limit a number of deposits that can be processed during one pass. Although, it is already limited by the churn.

@michaelsproul
Copy link
Contributor

Thanks for the update @mkalinin, I had not been following!

A short queue of deposits that we can process with random access sounds fine

prestonvanloon added a commit to prysmaticlabs/prysm that referenced this pull request Jul 3, 2024
prestonvanloon added a commit to prysmaticlabs/prysm that referenced this pull request Jul 3, 2024
prestonvanloon added a commit to prysmaticlabs/prysm that referenced this pull request Jul 3, 2024
prestonvanloon added a commit to prysmaticlabs/prysm that referenced this pull request Jul 3, 2024
github-merge-queue bot pushed a commit to prysmaticlabs/prysm that referenced this pull request Jul 8, 2024
* EIP-7251: Implement changes from ethereum/consensus-specs#3776

* Unskip epoch processing spectest

* EIP-7251: unit tests for logic in ethereum/consensus-specs#3776

* Radek feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Electra scope:security General protocol security-related items
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants