Skip to content

eip7251: Do not change creds type on consolidation#4020

Merged
jtraglia merged 5 commits intoethereum:devfrom
mkalinin:no-creds-change-on-consolidation
Dec 3, 2024
Merged

eip7251: Do not change creds type on consolidation#4020
jtraglia merged 5 commits intoethereum:devfrom
mkalinin:no-creds-change-on-consolidation

Conversation

@mkalinin
Copy link
Copy Markdown
Contributor

@mkalinin mkalinin commented Nov 25, 2024 •
edited
Loading

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 mentioned this pull request Nov 25, 2024
Closed
@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 mentioned this pull request Nov 25, 2024
Closed
@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
Copy Markdown
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
Copy Markdown
Member

ralexstokes commented Nov 25, 2024

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
Copy Markdown
Contributor

lucassaldanha commented Nov 26, 2024

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
Copy Markdown
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
Copy Markdown
Contributor

rolfyone commented Nov 26, 2024

LGTM

haxxpop
haxxpop approved these changes Nov 28, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@haxxpop haxxpop 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.

g11tech
g11tech approved these changes Nov 28, 2024
View reviewed changes
Copy link
Copy Markdown
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

@hwwhww hwwhww mentioned this pull request Nov 28, 2024
Closed
@terencechain terencechain mentioned this pull request Nov 30, 2024
Closed
@mkalinin mkalinin mentioned this pull request Dec 2, 2024
Merged
1 task
@mkalinin
Copy link
Copy Markdown
Contributor Author

mkalinin commented Dec 2, 2024

The PR is ready to be merged

@lucassaldanha lucassaldanha mentioned this pull request Dec 3, 2024
Closed
mkalinin and others added 2 commits December 3, 2024 13:07
@mkalinin @jtraglia
Apply suggestions from @jtraglia
cd42481 
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
@mkalinin
Bring back comp consolidation test
35bbe4f
jtraglia
jtraglia approved these changes Dec 3, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@jtraglia jtraglia mentioned this pull request Dec 3, 2024
Merged
13 tasks
@ensi321 ensi321 mentioned this pull request Dec 9, 2024
Merged
@mkalinin mkalinin mentioned this pull request Dec 12, 2024
Merged
@terencechain terencechain mentioned this pull request Dec 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jtraglia jtraglia jtraglia approved these changes

+4 more reviewers

@haxxpop haxxpop haxxpop approved these changes

@ralexstokes ralexstokes ralexstokes approved these changes

@terencechain terencechain terencechain approved these changes

@g11tech g11tech g11tech approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@mkalinin @fradamt @ralexstokes @lucassaldanha @rolfyone @jtraglia @haxxpop @terencechain @g11tech