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

Block deposit: refactor batch verify and validation activation #8698

Merged
merged 12 commits into from
Apr 5, 2021
Merged
39 changes: 28 additions & 11 deletions beacon-chain/core/blocks/deposit.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ func ProcessPreGenesisDeposits(
if err != nil {
return nil, errors.Wrap(err, "could not process deposit")
}
beaconState, err = activateValidatorWithEffectiveBalance(beaconState, deposits)
if err != nil {
return nil, err
}
return beaconState, nil
}

// This updates validator's effective balance, and if it's above MaxEffectiveBalance, validator becomes active in genesis.
func activateValidatorWithEffectiveBalance(beaconState iface.BeaconState, deposits []*ethpb.Deposit) (iface.BeaconState, error) {
for _, deposit := range deposits {
pubkey := deposit.Data.PublicKey
index, ok := beaconState.ValidatorIndexByPubkey(bytesutil.ToBytes48(pubkey))
Expand Down Expand Up @@ -74,33 +83,41 @@ func ProcessDeposits(
return nil, err
}

// Attempt to verify all deposit signatures at once, if this fails then fall back to processing
// individual deposits with signature verification enabled.
deposits := b.Block.Body.Deposits
var err error
domain, err := helpers.ComputeDomain(params.BeaconConfig().DomainDeposit, nil, nil)
batchVerified, err := batchVerifyDepositsSignatures(ctx, deposits)
if err != nil {
return nil, err
}

// Attempt to verify all deposit signatures at once, if this fails then fall back to processing
// individual deposits with signature verification enabled.
var verifySignature bool
if err := verifyDepositDataWithDomain(ctx, deposits, domain); err != nil {
log.WithError(err).Debug("Failed to verify deposit data, verifying signatures individually")
verifySignature = true
}

for _, deposit := range deposits {
if deposit == nil || deposit.Data == nil {
return nil, errors.New("got a nil deposit in block")
}
beaconState, err = ProcessDeposit(beaconState, deposit, verifySignature)
beaconState, err = ProcessDeposit(beaconState, deposit, batchVerified)
if err != nil {
return nil, errors.Wrapf(err, "could not process deposit from %#x", bytesutil.Trunc(deposit.Data.PublicKey))
}
}
return beaconState, nil
}

func batchVerifyDepositsSignatures(ctx context.Context, deposits []*ethpb.Deposit) (bool, error) {
var err error
domain, err := helpers.ComputeDomain(params.BeaconConfig().DomainDeposit, nil, nil)
if err != nil {
return false, err
}

var verified bool
Copy link
Member

Choose a reason for hiding this comment

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

This var name is pretty confusing for its actual use case, as on a first read it would look like the signature is verified when in truth it means the opposite

Copy link
Member Author

Choose a reason for hiding this comment

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

It's more standard than verifySignature which signals that the variable is actually a function.

Verified doesn't always imply true, it reads more as the act of verifying. ie. the state of being verified

In our code base, we do use verified as boolean through out

One way to improve is to make it verified := false

if err := verifyDepositDataWithDomain(ctx, deposits, domain); err != nil {
log.WithError(err).Debug("Failed to batch verify deposits signatures, will try individual verify")
verified = true
}
return verified, nil
}

// ProcessDeposit takes in a deposit object and inserts it
// into the registry as a new validator or balance change.
//
Expand Down