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

Add operation for withdrawal_credentials change operation #2213

Open
zilm13 opened this issue Feb 24, 2021 · 6 comments
Open

Add operation for withdrawal_credentials change operation #2213

zilm13 opened this issue Feb 24, 2021 · 6 comments

Comments

@zilm13
Copy link
Contributor

zilm13 commented Feb 24, 2021

What?

New operation and signed message is proposed for Eth2, BLSSetWithdrawal, which could permanently change withdrawal_credentials:

BLSSetWithdrawal

class BLSSetWithdrawal(Container):
    bls_withdrawal_pubkey: Bytes32   # Reveal of withdrawal public key
    withdrawal_credentials: Bytes32  # New withdrawal credentials, shouldn't start with BLS_WITHDRAWAL_PREFIX (0x00)
    validator_index: ValidatorIndex

And SignedBLSSetWithdrawal message:

SignedBLSSetWithdrawal

class SignedBLSSetWithdrawal(Container):
    message: BLSSetWithdrawal
    signature: BLSSignature  # Signed by withdrawal BLS key

Why?

It's already confirmed that at least one extra withdrawal target option will be supported with merge of PR #2149. While new validators could submit deposits using this option, there were already tons of deposits before this PR was merged and some of their owners could be interested in Eth1 withdrawal. Moreover, for some parties it could be a best flow of withdrawal control during validator lifecycle to start with BLS key 0x00 withdrawal credentials and change it's target to Eth1 address later with 0x01 or another options introduced in future.

How?

  • we verify that current validator’s withdrawal_credentials is started with BLS_WITHDRAWAL_PREFIX (0x00)
  • we verify that new credentials are not started with BLS_WITHDRAWAL_PREFIX (0x00)

There are several assessments when choosing such behavior of this operation:

  • Why don’t allow change to another BLS public key? There are 2 most obvious usages for 0x00 to another 0x00 swaps:
    • Validator's withdrawal key was compromised. For some validators it would mean the validator signing key could be under attack too and the safest way is to withdraw now. Changing credentials to explicit target works in the safest way in this case
    • Validators could be traded on some market by withdrawal pubkey change. Currently there is no mechanism to change validator's signing key and it's not known whether it will be introduced in future. Also it provides additional load on beacon chain network and block size making this type of message to fight for its place under the sun with Deposit, Attestation and other more important network message types.
  • Whether to check that withdrawal_credentials is in the currently supported range. The answer should be “no”, as the deposit contract already doesn’t check it. It’s a kind of risk for the minutest number of user errors, but by changing this we could open a way to a number of bugs that could affect all users.

Draft implementation:

def process_bls_set_withdrawal(state: BeaconState, signed_bls_set_withdrawal: SignedBLSSetWithdrawal) -> None:
    set_withdrawal = signed_bls_set_withdrawal.message
    validator = state.validators[set_withdrawal.validator_index]
    # Verify the validator has BLS pubkey commitment
    assert validator.withdrawal_credentials[:1] == BLS_WITHDRAWAL_PREFIX
    # Verify the validator is active
    assert is_active_validator(validator, get_current_epoch(state))
    # Verify exit has not been initiated
    assert validator.exit_epoch == FAR_FUTURE_EPOCH
    # Verify new withdrawal credentials doesn't start with BLS_WITHDRAWAL_PREFIX
    assert set_withdrawal.withdrawal_credentials[:1] != BLS_WITHDRAWAL_PREFIX
    # Verify BLS pubkey reveal is correct
    assert hash(set_withdrawal.withdrawal_pubkey)[1:] == validator.withdrawal_credentials[1:]
    # Verify signature
    domain = get_domain(state, DOMAIN_BLS_SET_WITHDRAWAL, get_current_epoch(state))
    signing_root = compute_signing_root(set_withdrawal, domain)
    assert bls.Verify(set_withdrawal.withdrawal_pubkey, signing_root, signed_bls_set_withdrawal.signature)
    # Update withdrawal credentials
    validator.withdrawal_credentials = set_withdrawal.withdrawal_credentials

Some concerns

We have two options here, and I'm not sure, which way should we go:

  • allow change of credentials only for validators that doesn't initiate the exit
  • allow change of credentials for exited validators

Second option requires extra processing of withdrawals after withdrawal_credentials are changed for already exited validators. It’s not straightforward and requires thoughtful implementation to avoid bugs. For example, when we have 2 active withdrawal options, we should verify that one was not processed before withdrawal_credentials change, though Eth1 withdrawal is planned to be automatically on validator exit without extra messages on Beacon Chain, while BLS key withdrawal will require new message with processing.

Some important notes

  • this approach means once withdrawal_credentials are changed from BLS_WITHDRAWAL_PREFIX (0x00) to any other, withdrawal target could not be changed anymore. But this is already concreted by PR Eth1 withdrawal credentials (0x01) #2149
  • BLSSetWithdrawal could be added already, as nothing prevents to submit withdrawal_credentials to Deposit Contract today with a prefix other than BLS_WITHDRAWAL_PREFIX (0x00)

Links

Simple eth1 withdrawals (beacon-chain centric) with BLSSetWithdrawalAddress proposal by @djrtwo inspiring this issue

Eth1 withdrawal credentials (0x01) #2149 by @djrtwo - PR merged in specs

Withdrawal credential rotation from BLS to Eth1 - some alternative approaches of withdrawal credentials change

Allow withdrawal_credentials to point to an eth1 address #2040 by @dapplion - Issue to add eth1 withdrawal prefix

@mcdee
Copy link
Contributor

mcdee commented Feb 24, 2021

Regarding your points about not allowing a different set of withdrawal credentials with BLS:

Validator's withdrawal key was compromised. For some validators it would mean the validator signing key could be under attack too and the safest way is to withdraw now. Changing credentials to explicit target works in the safest way in this case

If the withdrawal key is compromised I think we have to accept that the funds protected by that withdrawal key are likely to be lost. This is especially the case given that in the vast majority of cases compromise of the withdrawal key implies compromise of the validator key as well.

Validators could be traded on some market by withdrawal pubkey change. Currently there is no mechanism to change validator's signing key and it's not known whether it will be introduced in future.

As soon as we have the ability to assign an Ethereum 1 address to a validator it creates a market, as it is trivial to target a smart contract that can disburse funds as it sees fit.

Also it provides additional load on beacon chain network and block size making this type of message to fight for its place

I think that this is the critical item, and the reason why changing withdrawal credentials should be a one-time operation.

Regarding the state of the validator to allow this operation, I think that it should only apply while the validator is active. Otherwise we move in to areas of race conditions / potential impact of censorship of the operation and the like. And, as you mention, it potentially restricts the exit process with this additional complexity.

@zilm13
Copy link
Contributor Author

zilm13 commented Feb 24, 2021

Thanks for the comment

If the withdrawal key is compromised I think we have to accept that the funds protected by that withdrawal key are likely to be lost. This is especially the case given that in the vast majority of cases compromise of the withdrawal key implies compromise of the validator key as well.

I mean the case when it was probably compromised, or some hackable keystore gone, and your validator is still under control, but it's under the risk in the near future.

@lsankar4033
Copy link
Contributor

+1 on designing withdrawal-credential changes as a one-time operation scoped only to those with 0x00 creds. There aren't obvious use cases for this beyond allowing staking pools that launched prior to 0x01 to switch over so the generality doesn't seem worth the congestion/avenue for DoS vectors.

@vshvsh
Copy link

vshvsh commented Feb 25, 2021

That's a great way to introduce key rotation for staking pools that want to reduce their reliance on custodial or semi-custodial solutions.

I think that withdrawal credential compromise scenario shouldn't be a factor when evaluating the proposed solution. If it helps, it's a good side effect, but the setting where it matters is pretty convoluted IMO.

Re: allow change of credentials for exited validators, I think that if it's not too much work it should be implemented (mainly to the benefit of slashed validators owners), but it's a low-value feature. Only working with active validators is good enough.

@mcdee
Copy link
Contributor

mcdee commented Feb 25, 2021

That's a great way to introduce key rotation for staking pools that want to reduce their reliance on custodial or semi-custodial solutions.

Note that this proposal does not allow a change of validator key, so wouldn't help much in this situation.

@vshvsh
Copy link

vshvsh commented Feb 26, 2021

Note that this proposal does not allow a change of validator key, so wouldn't help much in this situation.

Validators are not custodians though.

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

No branches or pull requests

5 participants
@lsankar4033 @mcdee @vshvsh @zilm13 and others