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

EIP-7251: Add check for target withdrawal credentials against source address #4006

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

aimxhaisse
Copy link

@aimxhaisse aimxhaisse commented Nov 8, 2024

This is a tentative PR to open discussions on the current semantics around consolidation and source/target withdrawal credentials under EIP-7251.

In the current specification, it is possible for an entity controlling a source stake to convert any target stake with eth1 credentials from 0x01 to 0x02, regardless of whether or not they own the target stake. The assumption is that it would likely be a stupid move as the source of the consolidation would effectively lose the amount staked on the source validator.

In some contexts/countries, owning an auto-compounding stake can have different legal implications, such that some actors will be willing to stay in 0x01. However, the possibility that arbitrarily external entities can convert their stakes to 0x02, without them having a say in it, can be a blocker.

This PR ensures that both the source withdrawal credentials and the target withdrawal credentials point to the source address of the consolidation request.

On the downside, such a constraint would prevent some stakers from changing their withdrawal credentials (i.e: by first staking something on a new validator with a new withdrawal credentials, then consolidating against it), this could however be addressed/make more sense via a future separate request type.

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.

I believe this change would be quite limiting. It would prevent consolidations for:

  • An entity wants to sell their validator to another staker.
  • Two entities merge and want to consolidate their validators.
  • An entity with multiple validators that used multiple withdrawal addresses.

I would argue that an entity that controls the withdrawal address for a validator which has an eth1 withdrawal credential owns that validator & should be allowed to do whatever they want. Is there an example where this isn't true? What's preventing such a entity from exiting the validator and transferring the proceeds to another validator via a deposit request?

specs/electra/beacon-chain.md Show resolved Hide resolved
@aimxhaisse
Copy link
Author

aimxhaisse commented Nov 8, 2024

I believe this change would be quite limiting. It would prevent consolidations for:

  • An entity wants to sell their validator to another staker.
  • Two entities merge and want to consolidate their validators.
  • An entity with multiple validators that used multiple withdrawal addresses.

Totally agree, those flows wouldn't be possible anymore, I think it'd make more sense to have a specific flow to change withdrawal credentials instead of a convoluted consolidation.

I would argue that an entity that controls the withdrawal address for a validator which has an eth1 withdrawal credential owns that validator & should be allowed to do whatever they want. Is there an example where this isn't true?

Yes, that's also the point of this change: currently, if you own a 0x01 validator, someone else, which has 0 control on your withdrawal credential, can convert your stake to 0x02, with potential implications for you.

What's preventing such a entity from exiting the validator and transferring the proceeds to another validator via a deposit request?

This is different than depositing because depositing won't change the behaviour / nature of your stake.

@jtraglia
Copy link
Member

jtraglia commented Nov 8, 2024

Hmm yes I see the problem now. I'm not sure that it's worth changing though. For one, there's an almost 0% chance that someone would consolidate their validator into yours just to change your withdrawal credential. If I were the recipient and didn't want a compounding validator, I would then exit the validator and create two new validators.

@aimxhaisse
Copy link
Author

Hmm yes I see the problem now. I'm not sure that it's worth changing though. For one, there's an almost 0% chance that someone would consolidate their validator into yours just to change your withdrawal credential. If I were the recipient and didn't want a compounding validator, I would then exit the validator and create two new validators.

Yup, this is fine as an individual, if you are a pool it's different though as the pool is now tainted with a 0x02 validator / potential legal implications.

@rolfyone
Copy link
Contributor

I think we specifically don't want to restrict the target, because it'll reduce the usability a lot.

Basically this would reduce the main use-case for the entire feature... I do get the concern that someone may get the target incorrect, it's been mentioned several times.

The main result i think would be partially withdrawing the funds out, and the person that did the incorrect consolidation i guess would have to wear the tax implications from their incorrect consolidation? I'm not a finance person but it does seem like a very expensive day for someone if they get the target incorrect...

I really don't think that we can have this change unless we ask a lot of stakeholders if it remains useful. Do we need to start reaching out to them?

@mkalinin
Copy link
Collaborator

Alternative way to disallow unauthorized creds type switch:

@lucassaldanha
Copy link
Contributor

I think we specifically don't want to restrict the target, because it'll reduce the usability a lot.

I agree! I think the alternative of requiring the validator to change to compound creds before consolidating a better trade-off (#4020)

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.

5 participants