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

eip7251: Do not change creds type on consolidation #4020

Merged
merged 5 commits into from
Dec 3, 2024

Conversation

mkalinin
Copy link
Collaborator

@mkalinin mkalinin commented Nov 25, 2024

This PR introduces the following changes to the consolidation request processing:

  1. Disallows consolidations if the target doesn’t have 0x02 creds
  2. Does not switch to compounding when consolidation happens

Which effectively changes consolidation flow in a way that stakers will have to do it in two steps:

  1. Switch the target to compounding creds via switch to compounding request (consolidation with source==target)
  2. Consolidate the source balance to the target

This change disallows an unauthorized update of withdrawal credential type and simplifies the protocol by trading off consolidation UX. To alleviate the UX downgrade this PR increases the MAX consolidation requests per block to 2 which allows to accommodate creds switch and consolidation in a single block.

Supplants #4006

ToDo

@mkalinin mkalinin changed the title eip7251: Do no creds change on consolidation eip7251: Do no creds type change on consolidation Nov 25, 2024
@mkalinin mkalinin changed the title eip7251: Do no creds type change on consolidation eip7251: Do not change creds type on consolidation Nov 25, 2024
@fradamt
Copy link
Contributor

fradamt commented Nov 25, 2024

Lgtm.

Imo the UX impact is quite limited and it makes sense to get rid of this strange behavior, even if it's not really an attack vector.

@ralexstokes
Copy link
Member

I like that this simplifies the complexity of the state transition and general validator lifecycle state graph

it also handles the unauthorized consolidation issue

will need to fix tests but I think we should 🚢 it

@lucassaldanha
Copy link
Contributor

I agree that this is not a great UX but simplify the complexity of state transition. It also prevents anyone from causing a permanent side-effect on someone else's validator (0x01 -> 0x02 unwanted creds change).

@fradamt
Copy link
Contributor

fradamt commented Nov 26, 2024

Something else to mention about the impact is that the primary use case is staking pools consolidating, in which case needing to call the contract 63 times versus 64 times doesn't really make any difference

@rolfyone
Copy link
Contributor

LGTM

Copy link
Member

@ppopth ppopth left a comment

Choose a reason for hiding this comment

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

This is a good idea. Previously, I was confused a bit by when credential switching happens.

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm

@mkalinin
Copy link
Collaborator Author

mkalinin commented Dec 2, 2024

The PR is ready to be merged

mkalinin and others added 2 commits December 3, 2024 13:07
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jtraglia
Copy link
Member

jtraglia commented Dec 3, 2024

We're missing some coverage here. I think we need to update some more tests.

image

@jtraglia
Copy link
Member

jtraglia commented Dec 3, 2024

We're missing some coverage here. I think we need to update some more tests.

Fixed with the latest commit.

@jtraglia jtraglia merged commit 7c3ba69 into ethereum:dev Dec 3, 2024
27 checks passed
@jtraglia jtraglia mentioned this pull request Dec 3, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants