update batch deposit message verification for better readability (#14526

)

* reversing boolean return for better readability

* skips more reversals

* changelog

CHANGELOG.md

@@ -55,6 +55,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve
 - Updated Sepolia bootnodes.
 - Make committee aware packing the default by deprecating `--enable-committee-aware-packing`.
 - Moved `ConvertKzgCommitmentToVersionedHash` to the `primitives` package.
+- reversed the boolean return on `BatchVerifyDepositsSignatures`, from need verification, to all keys successfully verified
 
 ### Deprecated
 - `--disable-grpc-gateway` flag is deprecated due to grpc gateway removal.

beacon-chain/core/altair/deposit.go

@@ -37,7 +37,7 @@ func ProcessDeposits(
 	beaconState state.BeaconState,
 	deposits []*ethpb.Deposit,
 ) (state.BeaconState, error) {
-	batchVerified, err := blocks.BatchVerifyDepositsSignatures(ctx, deposits)
+	allSignaturesVerified, err := blocks.BatchVerifyDepositsSignatures(ctx, deposits)
 	if err != nil {
 		return nil, err
 	}
@@ -46,7 +46,7 @@ func ProcessDeposits(
 		if deposit == nil || deposit.Data == nil {
 			return nil, errors.New("got a nil deposit in block")
 		}
-		beaconState, err = ProcessDeposit(beaconState, deposit, batchVerified)
+		beaconState, err = ProcessDeposit(beaconState, deposit, allSignaturesVerified)
 		if err != nil {
 			return nil, errors.Wrapf(err, "could not process deposit from %#x", bytesutil.Trunc(deposit.Data.PublicKey))
 		}
@@ -81,7 +81,7 @@ func ProcessDeposits(
 //	  amount=deposit.data.amount,
 //	  signature=deposit.data.signature,
 //	 )
-func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verifySignature bool) (state.BeaconState, error) {
+func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, allSignaturesVerified bool) (state.BeaconState, error) {
 	if err := blocks.VerifyDeposit(beaconState, deposit); err != nil {
 		if deposit == nil || deposit.Data == nil {
 			return nil, err
@@ -92,7 +92,7 @@ func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verif
 		return nil, err
 	}
 
-	return ApplyDeposit(beaconState, deposit.Data, verifySignature)
+	return ApplyDeposit(beaconState, deposit.Data, allSignaturesVerified)
 }
 
 // ApplyDeposit
@@ -115,13 +115,13 @@ func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verif
 //	    # Increase balance by deposit amount
 //	    index = ValidatorIndex(validator_pubkeys.index(pubkey))
 //	    increase_balance(state, index, amount)
-func ApplyDeposit(beaconState state.BeaconState, data *ethpb.Deposit_Data, verifySignature bool) (state.BeaconState, error) {
+func ApplyDeposit(beaconState state.BeaconState, data *ethpb.Deposit_Data, allSignaturesVerified bool) (state.BeaconState, error) {
 	pubKey := data.PublicKey
 	amount := data.Amount
 	withdrawalCredentials := data.WithdrawalCredentials
 	index, ok := beaconState.ValidatorIndexByPubkey(bytesutil.ToBytes48(pubKey))
 	if !ok {
-		if verifySignature {
+		if !allSignaturesVerified {
 			valid, err := blocks.IsValidDepositSignature(data)
 			if err != nil {
 				return nil, err

beacon-chain/core/altair/deposit_test.go

@@ -199,7 +199,7 @@ func TestProcessDeposit_SkipsInvalidDeposit(t *testing.T) {
 		},
 	})
 	require.NoError(t, err)
-	newState, err := altair.ProcessDeposit(beaconState, dep[0], true)
+	newState, err := altair.ProcessDeposit(beaconState, dep[0], false)
 	require.NoError(t, err, "Expected invalid block deposit to be ignored without error")
 
 	if newState.Eth1DepositIndex() != 1 {

beacon-chain/core/blocks/deposit.go

@@ -55,12 +55,11 @@ func BatchVerifyDepositsSignatures(ctx context.Context, deposits []*ethpb.Deposi
 		return false, err
 	}
 
-	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 false, nil
 	}
-	return verified, nil
+	return true, nil
 }
 
 // IsValidDepositSignature returns whether deposit_data is valid

beacon-chain/core/blocks/deposit_test.go

@@ -17,6 +17,41 @@ import (
 )
 
 func TestBatchVerifyDepositsSignatures_Ok(t *testing.T) {
+	sk, err := bls.RandKey()
+	require.NoError(t, err)
+	domain, err := signing.ComputeDomain(params.BeaconConfig().DomainDeposit, nil, nil)
+	require.NoError(t, err)
+	deposit := &ethpb.Deposit{
+		Data: &ethpb.Deposit_Data{
+			PublicKey:             sk.PublicKey().Marshal(),
+			WithdrawalCredentials: make([]byte, 32),
+			Amount:                3000,
+		},
+	}
+	sr, err := signing.ComputeSigningRoot(&ethpb.DepositMessage{
+		PublicKey:             deposit.Data.PublicKey,
+		WithdrawalCredentials: deposit.Data.WithdrawalCredentials,
+		Amount:                3000,
+	}, domain)
+	require.NoError(t, err)
+	sig := sk.Sign(sr[:])
+	deposit.Data.Signature = sig.Marshal()
+	leaf, err := deposit.Data.HashTreeRoot()
+	require.NoError(t, err)
+	// We then create a merkle branch for the test.
+	depositTrie, err := trie.GenerateTrieFromItems([][]byte{leaf[:]}, params.BeaconConfig().DepositContractTreeDepth)
+	require.NoError(t, err, "Could not generate trie")
+	proof, err := depositTrie.MerkleProof(0)
+	require.NoError(t, err, "Could not generate proof")
+
+	deposit.Proof = proof
+	require.NoError(t, err)
+	verified, err := blocks.BatchVerifyDepositsSignatures(context.Background(), []*ethpb.Deposit{deposit})
+	require.NoError(t, err)
+	require.Equal(t, true, verified)
+}
+
+func TestBatchVerifyDepositsSignatures_InvalidSignature(t *testing.T) {
 	deposit := &ethpb.Deposit{
 		Data: &ethpb.Deposit_Data{
 			PublicKey:             bytesutil.PadTo([]byte{1, 2, 3}, 48),
@@ -34,9 +69,9 @@ func TestBatchVerifyDepositsSignatures_Ok(t *testing.T) {
 
 	deposit.Proof = proof
 	require.NoError(t, err)
-	ok, err := blocks.BatchVerifyDepositsSignatures(context.Background(), []*ethpb.Deposit{deposit})
+	verified, err := blocks.BatchVerifyDepositsSignatures(context.Background(), []*ethpb.Deposit{deposit})
 	require.NoError(t, err)
-	require.Equal(t, true, ok)
+	require.Equal(t, false, verified)
 }
 
 func TestVerifyDeposit_MerkleBranchFailsVerification(t *testing.T) {

beacon-chain/core/electra/deposits.go

@@ -38,7 +38,7 @@ func ProcessDeposits(
 	defer span.End()
 	// Attempt to verify all deposit signatures at once, if this fails then fall back to processing
 	// individual deposits with signature verification enabled.
-	batchVerified, err := blocks.BatchVerifyDepositsSignatures(ctx, deposits)
+	allSignaturesVerified, err := blocks.BatchVerifyDepositsSignatures(ctx, deposits)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not verify deposit signatures in batch")
 	}
@@ -47,7 +47,7 @@ func ProcessDeposits(
 		if d == nil || d.Data == nil {
 			return nil, errors.New("got a nil deposit in block")
 		}
-		beaconState, err = ProcessDeposit(beaconState, d, batchVerified)
+		beaconState, err = ProcessDeposit(beaconState, d, allSignaturesVerified)
 		if err != nil {
 			return nil, errors.Wrapf(err, "could not process deposit from %#x", bytesutil.Trunc(d.Data.PublicKey))
 		}
@@ -82,7 +82,7 @@ func ProcessDeposits(
 //	  amount=deposit.data.amount,
 //	  signature=deposit.data.signature,
 //	 )
-func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verifySignature bool) (state.BeaconState, error) {
+func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, allSignaturesVerified bool) (state.BeaconState, error) {
 	if err := blocks.VerifyDeposit(beaconState, deposit); err != nil {
 		if deposit == nil || deposit.Data == nil {
 			return nil, err
@@ -92,7 +92,7 @@ func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verif
 	if err := beaconState.SetEth1DepositIndex(beaconState.Eth1DepositIndex() + 1); err != nil {
 		return nil, err
 	}
-	return ApplyDeposit(beaconState, deposit.Data, verifySignature)
+	return ApplyDeposit(beaconState, deposit.Data, allSignaturesVerified)
 }
 
 // ApplyDeposit
@@ -116,13 +116,13 @@ func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verif
 //	 and is_valid_deposit_signature(pubkey, withdrawal_credentials, amount, signature)
 //	):
 //	 switch_to_compounding_validator(state, index)
-func ApplyDeposit(beaconState state.BeaconState, data *ethpb.Deposit_Data, verifySignature bool) (state.BeaconState, error) {
+func ApplyDeposit(beaconState state.BeaconState, data *ethpb.Deposit_Data, allSignaturesVerified bool) (state.BeaconState, error) {
 	pubKey := data.PublicKey
 	amount := data.Amount
 	withdrawalCredentials := data.WithdrawalCredentials
 	index, ok := beaconState.ValidatorIndexByPubkey(bytesutil.ToBytes48(pubKey))
 	if !ok {
-		if verifySignature {
+		if !allSignaturesVerified {
 			valid, err := IsValidDepositSignature(data)
 			if err != nil {
 				return nil, errors.Wrap(err, "could not verify deposit signature")
@@ -144,7 +144,7 @@ func ApplyDeposit(beaconState state.BeaconState, data *ethpb.Deposit_Data, verif
 			return nil, err
 		}
 		if helpers.IsCompoundingWithdrawalCredential(withdrawalCredentials) && helpers.HasETH1WithdrawalCredential(val) {
-			if verifySignature {
+			if !allSignaturesVerified {
 				valid, err := IsValidDepositSignature(data)
 				if err != nil {
 					return nil, errors.Wrap(err, "could not verify deposit signature")
@@ -325,12 +325,12 @@ func ProcessDepositRequests(ctx context.Context, beaconState state.BeaconState,
 			},
 		})
 	}
-	batchVerified, err := blocks.BatchVerifyDepositsSignatures(ctx, deposits)
+	allSignaturesVerified, err := blocks.BatchVerifyDepositsSignatures(ctx, deposits)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not verify deposit signatures in batch")
 	}
 	for _, receipt := range requests {
-		beaconState, err = processDepositRequest(beaconState, receipt, batchVerified)
+		beaconState, err = processDepositRequest(beaconState, receipt, allSignaturesVerified)
 		if err != nil {
 			return nil, errors.Wrap(err, "could not apply deposit request")
 		}
@@ -352,7 +352,7 @@ func ProcessDepositRequests(ctx context.Context, beaconState state.BeaconState,
 //	    amount=deposit_request.amount,
 //	    signature=deposit_request.signature,
 //	)
-func processDepositRequest(beaconState state.BeaconState, request *enginev1.DepositRequest, verifySignature bool) (state.BeaconState, error) {
+func processDepositRequest(beaconState state.BeaconState, request *enginev1.DepositRequest, allSignaturesVerified bool) (state.BeaconState, error) {
 	requestsStartIndex, err := beaconState.DepositRequestsStartIndex()
 	if err != nil {
 		return nil, errors.Wrap(err, "could not get deposit requests start index")
@@ -367,5 +367,5 @@ func processDepositRequest(beaconState state.BeaconState, request *enginev1.Depo
 		Amount:                request.Amount,
 		WithdrawalCredentials: bytesutil.SafeCopyBytes(request.WithdrawalCredentials),
 		Signature:             bytesutil.SafeCopyBytes(request.Signature),
-	}, verifySignature)
+	}, allSignaturesVerified)
 }

beacon-chain/core/electra/deposits_test.go

@@ -322,7 +322,7 @@ func TestProcessDeposit_SkipsInvalidDeposit(t *testing.T) {
 		},
 	})
 	require.NoError(t, err)
-	newState, err := electra.ProcessDeposit(beaconState, dep[0], true)
+	newState, err := electra.ProcessDeposit(beaconState, dep[0], false)
 	require.NoError(t, err, "Expected invalid block deposit to be ignored without error")
 
 	if newState.Eth1DepositIndex() != 1 {
@@ -359,7 +359,7 @@ func TestApplyDeposit_TopUps_WithBadSignature(t *testing.T) {
 	vals[0].PublicKey = sk.PublicKey().Marshal()
 	vals[0].WithdrawalCredentials[0] = params.BeaconConfig().ETH1AddressWithdrawalPrefixByte
 	require.NoError(t, st.SetValidators(vals))
-	adSt, err := electra.ApplyDeposit(st, depositData, true)
+	adSt, err := electra.ApplyDeposit(st, depositData, false)
 	require.NoError(t, err)
 	pbd, err := adSt.PendingBalanceDeposits()
 	require.NoError(t, err)
@@ -390,7 +390,7 @@ func TestApplyDeposit_Electra_SwitchToCompoundingValidator(t *testing.T) {
 	require.NoError(t, err)
 	sig := sk.Sign(sr[:])
 	depositData.Signature = sig.Marshal()
-	adSt, err := electra.ApplyDeposit(st, depositData, false)
+	adSt, err := electra.ApplyDeposit(st, depositData, true)
 	require.NoError(t, err)
 	pbd, err := adSt.PendingBalanceDeposits()
 	require.NoError(t, err)