Check BLS changes when requesting from pool (#11718)

* Check BLS changes when requesting from pool

* Terence's suggestions

* Radek's suggestion

Co-authored-by: terencechain <terence@prysmaticlabs.com>
Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>

beacon-chain/core/blocks/withdrawals.go

@@ -2,7 +2,6 @@ package blocks
 
 import (
 	"bytes"
-	"context"
 
 	"github.com/pkg/errors"
 	"github.com/prysmaticlabs/prysm/v3/beacon-chain/core/helpers"
@@ -153,7 +152,6 @@ func ProcessWithdrawals(st state.BeaconState, withdrawals []*enginev1.Withdrawal
 }
 
 func BLSChangesSignatureBatch(
-	ctx context.Context,
 	st state.ReadOnlyBeaconState,
 	changes []*ethpb.SignedBLSToExecutionChange,
 ) (*bls.SignatureBatch, error) {
@@ -172,9 +170,6 @@ func BLSChangesSignatureBatch(
 		return nil, err
 	}
 	for i, change := range changes {
-		if ctx.Err() != nil {
-			return nil, ctx.Err()
-		}
 		batch.Signatures[i] = change.Signature
 		publicKey, err := bls.PublicKeyFromBytes(change.Message.FromBlsPubkey)
 		if err != nil {

beacon-chain/core/blocks/withdrawals_test.go

@@ -1,7 +1,6 @@
 package blocks_test
 
 import (
-	"context"
 	"math/rand"
 	"testing"
 
@@ -746,7 +745,7 @@ func TestBLSChangesSignatureBatch(t *testing.T) {
 		}
 		signedChanges[i] = signed
 	}
-	batch, err := blocks.BLSChangesSignatureBatch(context.Background(), st, signedChanges)
+	batch, err := blocks.BLSChangesSignatureBatch(st, signedChanges)
 	require.NoError(t, err)
 	verify, err := batch.Verify()
 	require.NoError(t, err)

beacon-chain/core/transition/transition_no_verify_sig.go

@@ -205,7 +205,7 @@ func ProcessBlockNoVerifyAnySig(
 		if err != nil {
 			return nil, nil, errors.Wrap(err, "could not get BLSToExecutionChanges")
 		}
-		cSet, err := b.BLSChangesSignatureBatch(ctx, st, changes)
+		cSet, err := b.BLSChangesSignatureBatch(st, changes)
 		if err != nil {
 			return nil, nil, errors.Wrap(err, "could not get BLSToExecutionChanges signatures")
 		}

beacon-chain/operations/blstoexec/BUILD.bazel

@@ -11,10 +11,15 @@ go_library(
         "//beacon-chain:__subpackages__",
     ],
     deps = [
+        "//beacon-chain/core/blocks:go_default_library",
+        "//beacon-chain/state:go_default_library",
         "//config/params:go_default_library",
         "//consensus-types/primitives:go_default_library",
         "//container/doubly-linked-list:go_default_library",
+        "//crypto/bls/blst:go_default_library",
         "//proto/prysm/v1alpha1:go_default_library",
+        "@com_github_pkg_errors//:go_default_library",
+        "@com_github_sirupsen_logrus//:go_default_library",
     ],
 )
 
@@ -24,8 +29,15 @@ go_test(
     srcs = ["pool_test.go"],
     embed = [":go_default_library"],
     deps = [
+        "//beacon-chain/core/signing:go_default_library",
+        "//beacon-chain/core/time:go_default_library",
+        "//beacon-chain/state/state-native:go_default_library",
         "//config/params:go_default_library",
         "//consensus-types/primitives:go_default_library",
+        "//crypto/bls:go_default_library",
+        "//crypto/bls/common:go_default_library",
+        "//crypto/hash:go_default_library",
+        "//encoding/ssz:go_default_library",
         "//proto/prysm/v1alpha1:go_default_library",
         "//testing/assert:go_default_library",
         "//testing/require:go_default_library",

beacon-chain/operations/blstoexec/pool.go

@@ -4,17 +4,22 @@ import (
 	"math"
 	"sync"
 
+	"github.com/pkg/errors"
+	"github.com/prysmaticlabs/prysm/v3/beacon-chain/core/blocks"
+	"github.com/prysmaticlabs/prysm/v3/beacon-chain/state"
 	"github.com/prysmaticlabs/prysm/v3/config/params"
 	types "github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
 	doublylinkedlist "github.com/prysmaticlabs/prysm/v3/container/doubly-linked-list"
+	"github.com/prysmaticlabs/prysm/v3/crypto/bls/blst"
 	ethpb "github.com/prysmaticlabs/prysm/v3/proto/prysm/v1alpha1"
+	"github.com/sirupsen/logrus"
 )
 
 // PoolManager maintains pending and seen BLS-to-execution-change objects.
 // This pool is used by proposers to insert BLS-to-execution-change objects into new blocks.
 type PoolManager interface {
 	PendingBLSToExecChanges() ([]*ethpb.SignedBLSToExecutionChange, error)
-	BLSToExecChangesForInclusion() ([]*ethpb.SignedBLSToExecutionChange, error)
+	BLSToExecChangesForInclusion(state.BeaconState) ([]*ethpb.SignedBLSToExecutionChange, error)
 	InsertBLSToExecChange(change *ethpb.SignedBLSToExecutionChange)
 	MarkIncluded(change *ethpb.SignedBLSToExecutionChange) error
 	ValidatorExists(idx types.ValidatorIndex) bool
@@ -58,25 +63,69 @@ func (p *Pool) PendingBLSToExecChanges() ([]*ethpb.SignedBLSToExecutionChange, e
 
 // BLSToExecChangesForInclusion returns objects that are ready for inclusion at the given slot.
 // This method will not return more than the block enforced MaxBlsToExecutionChanges.
-func (p *Pool) BLSToExecChangesForInclusion() ([]*ethpb.SignedBLSToExecutionChange, error) {
+func (p *Pool) BLSToExecChangesForInclusion(st state.BeaconState) ([]*ethpb.SignedBLSToExecutionChange, error) {
 	p.lock.RLock()
-	defer p.lock.RUnlock()
-
 	length := int(math.Min(float64(params.BeaconConfig().MaxBlsToExecutionChanges), float64(p.pending.Len())))
-	result := make([]*ethpb.SignedBLSToExecutionChange, length)
+	result := make([]*ethpb.SignedBLSToExecutionChange, 0, length)
 	node := p.pending.First()
-	var err error
+	for node != nil && len(result) < length {
-	for i := 0; node != nil && i < length; i++ {
+		change, err := node.Value()
-		result[i], err = node.Value()
 		if err != nil {
+			p.lock.RUnlock()
 			return nil, err
 		}
+		_, err = blocks.ValidateBLSToExecutionChange(st, change)
+		if err != nil {
+			logrus.WithError(err).Warning("removing invalid BLSToExecutionChange from pool")
+			// MarkIncluded removes the invalid change from the pool
+			p.lock.RUnlock()
+			if err := p.MarkIncluded(change); err != nil {
+				return nil, errors.Wrap(err, "could not mark BLSToExecutionChange as included")
+			}
+			p.lock.RLock()
+		} else {
+			result = append(result, change)
+		}
 		node, err = node.Next()
 		if err != nil {
+			p.lock.RUnlock()
 			return nil, err
 		}
 	}
-	return result, nil
+	p.lock.RUnlock()
+	if len(result) == 0 {
+		return result, nil
+	}
+	// We now verify the signatures in batches
+	cSet, err := blocks.BLSChangesSignatureBatch(st, result)
+	if err != nil {
+		logrus.WithError(err).Warning("could not get BLSToExecutionChanges signatures")
+	} else {
+		ok, err := cSet.Verify()
+		if err != nil {
+			logrus.WithError(err).Warning("could not batch verify BLSToExecutionChanges signatures")
+		} else if ok {
+			return result, nil
+		}
+	}
+	// Batch signature failed, check signatures individually
+	verified := make([]*ethpb.SignedBLSToExecutionChange, 0, length)
+	for i, sig := range cSet.Signatures {
+		signature, err := blst.SignatureFromBytes(sig)
+		if err != nil {
+			logrus.WithError(err).Warning("could not get signature from bytes")
+			continue
+		}
+		if !signature.Verify(cSet.PublicKeys[i], cSet.Messages[i][:]) {
+			logrus.Warning("removing BLSToExecutionChange with invalid signature from pool")
+			if err := p.MarkIncluded(result[i]); err != nil {
+				return nil, errors.Wrap(err, "could not mark BLSToExecutionChange as included")
+			}
+		} else {
+			verified = append(verified, result[i])
+		}
+	}
+	return verified, nil
 }
 
 // InsertBLSToExecChange inserts an object into the pool.

beacon-chain/operations/blstoexec/pool_test.go

@@ -3,8 +3,15 @@ package blstoexec
 import (
 	"testing"
 
+	"github.com/prysmaticlabs/prysm/v3/beacon-chain/core/signing"
+	"github.com/prysmaticlabs/prysm/v3/beacon-chain/core/time"
+	state_native "github.com/prysmaticlabs/prysm/v3/beacon-chain/state/state-native"
 	"github.com/prysmaticlabs/prysm/v3/config/params"
 	types "github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
+	"github.com/prysmaticlabs/prysm/v3/crypto/bls"
+	"github.com/prysmaticlabs/prysm/v3/crypto/bls/common"
+	"github.com/prysmaticlabs/prysm/v3/crypto/hash"
+	"github.com/prysmaticlabs/prysm/v3/encoding/ssz"
 	eth "github.com/prysmaticlabs/prysm/v3/proto/prysm/v1alpha1"
 	"github.com/prysmaticlabs/prysm/v3/testing/assert"
 	"github.com/prysmaticlabs/prysm/v3/testing/require"
@@ -36,42 +43,89 @@ func TestPendingBLSToExecChanges(t *testing.T) {
 }
 
 func TestBLSToExecChangesForInclusion(t *testing.T) {
+	spb := &eth.BeaconStateCapella{
+		Fork: &eth.Fork{
+			CurrentVersion:  params.BeaconConfig().GenesisForkVersion,
+			PreviousVersion: params.BeaconConfig().GenesisForkVersion,
+		},
+	}
+	numValidators := 2 * params.BeaconConfig().MaxBlsToExecutionChanges
+	validators := make([]*eth.Validator, numValidators)
+	blsChanges := make([]*eth.BLSToExecutionChange, numValidators)
+	spb.Balances = make([]uint64, numValidators)
+	privKeys := make([]common.SecretKey, numValidators)
+	maxEffectiveBalance := params.BeaconConfig().MaxEffectiveBalance
+	executionAddress := []byte{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13}
+
+	for i := range validators {
+		v := &eth.Validator{}
+		v.EffectiveBalance = maxEffectiveBalance
+		v.WithdrawableEpoch = params.BeaconConfig().FarFutureEpoch
+		v.WithdrawalCredentials = make([]byte, 32)
+		priv, err := bls.RandKey()
+		require.NoError(t, err)
+		privKeys[i] = priv
+		pubkey := priv.PublicKey().Marshal()
+
+		message := &eth.BLSToExecutionChange{
+			ToExecutionAddress: executionAddress,
+			ValidatorIndex:     types.ValidatorIndex(i),
+			FromBlsPubkey:      pubkey,
+		}
+
+		hashFn := ssz.NewHasherFunc(hash.CustomSHA256Hasher())
+		digest := hashFn.Hash(pubkey)
+		digest[0] = params.BeaconConfig().BLSWithdrawalPrefixByte
+		copy(v.WithdrawalCredentials, digest[:])
+		validators[i] = v
+		blsChanges[i] = message
+	}
+	spb.Validators = validators
+	st, err := state_native.InitializeFromProtoCapella(spb)
+	require.NoError(t, err)
+
+	signedChanges := make([]*eth.SignedBLSToExecutionChange, numValidators)
+	for i, message := range blsChanges {
+		signature, err := signing.ComputeDomainAndSign(st, time.CurrentEpoch(st), message, params.BeaconConfig().DomainBLSToExecutionChange, privKeys[i])
+		require.NoError(t, err)
+
+		signed := &eth.SignedBLSToExecutionChange{
+			Message:   message,
+			Signature: signature,
+		}
+		signedChanges[i] = signed
+	}
+
 	t.Run("empty pool", func(t *testing.T) {
 		pool := NewPool()
-		for i := uint64(0); i < params.BeaconConfig().MaxBlsToExecutionChanges-1; i++ {
+		changes, err := pool.BLSToExecChangesForInclusion(st)
-			pool.InsertBLSToExecChange(&eth.SignedBLSToExecutionChange{
-				Message: &eth.BLSToExecutionChange{
-					ValidatorIndex: types.ValidatorIndex(i),
-				},
-			})
-		}
-		changes, err := pool.BLSToExecChangesForInclusion()
 		require.NoError(t, err)
-		assert.Equal(t, int(params.BeaconConfig().MaxBlsToExecutionChanges-1), len(changes))
+		assert.Equal(t, 0, len(changes))
+	})
+	t.Run("Less than MaxBlsToExecutionChanges in pool", func(t *testing.T) {
+		pool := NewPool()
+		for i := uint64(0); i < params.BeaconConfig().MaxBlsToExecutionChanges-1; i++ {
+			pool.InsertBLSToExecChange(signedChanges[i])
+		}
+		changes, err := pool.BLSToExecChangesForInclusion(st)
+		require.NoError(t, err)
+		assert.Equal(t, int(params.BeaconConfig().MaxBlsToExecutionChanges)-1, len(changes))
 	})
 	t.Run("MaxBlsToExecutionChanges in pool", func(t *testing.T) {
 		pool := NewPool()
 		for i := uint64(0); i < params.BeaconConfig().MaxBlsToExecutionChanges; i++ {
-			pool.InsertBLSToExecChange(&eth.SignedBLSToExecutionChange{
+			pool.InsertBLSToExecChange(signedChanges[i])
-				Message: &eth.BLSToExecutionChange{
-					ValidatorIndex: types.ValidatorIndex(i),
-				},
-			})
 		}
-		changes, err := pool.BLSToExecChangesForInclusion()
+		changes, err := pool.BLSToExecChangesForInclusion(st)
 		require.NoError(t, err)
 		assert.Equal(t, int(params.BeaconConfig().MaxBlsToExecutionChanges), len(changes))
 	})
 	t.Run("more than MaxBlsToExecutionChanges in pool", func(t *testing.T) {
 		pool := NewPool()
-		for i := uint64(0); i < params.BeaconConfig().MaxBlsToExecutionChanges+1; i++ {
+		for i := uint64(0); i < numValidators; i++ {
-			pool.InsertBLSToExecChange(&eth.SignedBLSToExecutionChange{
+			pool.InsertBLSToExecChange(signedChanges[i])
-				Message: &eth.BLSToExecutionChange{
-					ValidatorIndex: types.ValidatorIndex(i),
-				},
-			})
 		}
-		changes, err := pool.BLSToExecChangesForInclusion()
+		changes, err := pool.BLSToExecChangesForInclusion(st)
 		require.NoError(t, err)
 		// We want FIFO semantics, which means validator with index 16 shouldn't be returned
 		assert.Equal(t, int(params.BeaconConfig().MaxBlsToExecutionChanges), len(changes))
@@ -79,6 +133,30 @@ func TestBLSToExecChangesForInclusion(t *testing.T) {
 			assert.NotEqual(t, types.ValidatorIndex(16), ch.Message.ValidatorIndex)
 		}
 	})
+	t.Run("One Bad change", func(t *testing.T) {
+		pool := NewPool()
+		saveByte := signedChanges[1].Message.FromBlsPubkey[5]
+		signedChanges[1].Message.FromBlsPubkey[5] = 0xff
+		for i := uint64(0); i < numValidators; i++ {
+			pool.InsertBLSToExecChange(signedChanges[i])
+		}
+		changes, err := pool.BLSToExecChangesForInclusion(st)
+		require.NoError(t, err)
+		assert.Equal(t, int(params.BeaconConfig().MaxBlsToExecutionChanges), len(changes))
+		assert.Equal(t, types.ValidatorIndex(2), changes[1].Message.ValidatorIndex)
+		signedChanges[1].Message.FromBlsPubkey[5] = saveByte
+	})
+	t.Run("One Bad Signature", func(t *testing.T) {
+		pool := NewPool()
+		copy(signedChanges[1].Signature, signedChanges[2].Signature)
+		for i := uint64(0); i < numValidators; i++ {
+			pool.InsertBLSToExecChange(signedChanges[i])
+		}
+		changes, err := pool.BLSToExecChangesForInclusion(st)
+		require.NoError(t, err)
+		assert.Equal(t, int(params.BeaconConfig().MaxBlsToExecutionChanges)-1, len(changes))
+		assert.Equal(t, types.ValidatorIndex(2), changes[1].Message.ValidatorIndex)
+	})
 }
 
 func TestInsertBLSToExecChange(t *testing.T) {

beacon-chain/sync/validate_bls_to_execution_change.go

@@ -57,7 +57,7 @@ func (s *Service) validateBlsToExecutionChange(ctx context.Context, pid peer.ID,
 		return pubsub.ValidationReject, err
 	}
 	// Validate the signature of the message using our batch gossip verifier.
-	sigBatch, err := blocks.BLSChangesSignatureBatch(ctx, st, []*ethpb.SignedBLSToExecutionChange{blsChange})
+	sigBatch, err := blocks.BLSChangesSignatureBatch(st, []*ethpb.SignedBLSToExecutionChange{blsChange})
 	if err != nil {
 		return pubsub.ValidationReject, err
 	}

crypto/bls/blst/BUILD.bazel

@@ -16,6 +16,7 @@ go_library(
     ],
     importpath = "github.com/prysmaticlabs/prysm/v3/crypto/bls/blst",
     visibility = [
+        "//beacon-chain/operations/blstoexec:__pkg__",
         "//crypto/bls:__pkg__",
     ],
     deps = [

crypto/bls/common/BUILD.bazel

@@ -10,6 +10,7 @@ go_library(
     importpath = "github.com/prysmaticlabs/prysm/v3/crypto/bls/common",
     visibility = [
         "//beacon-chain/core/blocks:__subpackages__",
+        "//beacon-chain/operations/blstoexec:__pkg__",
         "//crypto/bls:__subpackages__",
         "//testing:__subpackages__",
     ],

testing/spectest/shared/capella/operations/bls_to_execution_changes.go

@@ -44,7 +44,7 @@ func RunBLSToExecutionChangeTest(t *testing.T, config string) {
 				if err != nil {
 					return nil, err
 				}
-				cSet, err := blocks.BLSChangesSignatureBatch(ctx, st, changes)
+				cSet, err := blocks.BLSChangesSignatureBatch(st, changes)
 				if err != nil {
 					return nil, err
 				}