kasey's review

beacon-chain/state/fieldtrie/field_trie_helpers.go

@@ -9,6 +9,7 @@ import (
 	"github.com/prysmaticlabs/prysm/beacon-chain/state/stateutil"
 	"github.com/prysmaticlabs/prysm/beacon-chain/state/types"
 	"github.com/prysmaticlabs/prysm/crypto/hash"
+	"github.com/prysmaticlabs/prysm/encoding/bytesutil"
 	"github.com/prysmaticlabs/prysm/encoding/ssz"
 	ethpb "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1"
 	"github.com/prysmaticlabs/prysm/runtime/version"
@@ -40,21 +41,21 @@ func fieldConverters(field types.FieldIndex, indices []uint64, elements interfac
 			return nil, errors.Errorf("Wanted type of %v but got %v",
 				reflect.TypeOf([][]byte{}).Name(), reflect.TypeOf(elements).Name())
 		}
-		return stateutil.HandleByteArrays(val, indices, convertAll)
+		return handleByteArrays(val, indices, convertAll)
 	case types.Eth1DataVotes:
 		val, ok := elements.([]*ethpb.Eth1Data)
 		if !ok {
 			return nil, errors.Errorf("Wanted type of %v but got %v",
 				reflect.TypeOf([]*ethpb.Eth1Data{}).Name(), reflect.TypeOf(elements).Name())
 		}
-		return HandleEth1DataSlice(val, indices, convertAll)
+		return handleEth1DataSlice(val, indices, convertAll)
 	case types.Validators:
 		val, ok := elements.([]*ethpb.Validator)
 		if !ok {
 			return nil, errors.Errorf("Wanted type of %v but got %v",
 				reflect.TypeOf([]*ethpb.Validator{}).Name(), reflect.TypeOf(elements).Name())
 		}
-		return stateutil.HandleValidatorSlice(val, indices, convertAll)
+		return handleValidatorSlice(val, indices, convertAll)
 	case types.PreviousEpochAttestations, types.CurrentEpochAttestations:
 		val, ok := elements.([]*ethpb.PendingAttestation)
 		if !ok {
@@ -74,8 +75,75 @@ func fieldConverters(field types.FieldIndex, indices []uint64, elements interfac
 	}
 }
 
-// HandleEth1DataSlice processes a list of eth1data and indices into the appropriate roots.
-func HandleEth1DataSlice(val []*ethpb.Eth1Data, indices []uint64, convertAll bool) ([][32]byte, error) {
+// handleByteArrays computes and returns byte arrays in a slice of root format.
+func handleByteArrays(val [][]byte, indices []uint64, convertAll bool) ([][32]byte, error) {
+	length := len(indices)
+	if convertAll {
+		length = len(val)
+	}
+	roots := make([][32]byte, 0, length)
+	rootCreator := func(input []byte) {
+		newRoot := bytesutil.ToBytes32(input)
+		roots = append(roots, newRoot)
+	}
+	if convertAll {
+		for i := range val {
+			rootCreator(val[i])
+		}
+		return roots, nil
+	}
+	if len(val) > 0 {
+		for _, idx := range indices {
+			if idx > uint64(len(val))-1 {
+				return nil, fmt.Errorf("index %d greater than number of byte arrays %d", idx, len(val))
+			}
+			rootCreator(val[idx])
+		}
+	}
+	return roots, nil
+}
+
+// handleValidatorSlice returns the validator indices in a slice of root format.
+func handleValidatorSlice(val []*ethpb.Validator, indices []uint64, convertAll bool) ([][32]byte, error) {
+	length := len(indices)
+	if convertAll {
+		length = len(val)
+	}
+	roots := make([][32]byte, 0, length)
+	hasher := hash.CustomSHA256Hasher()
+	rootCreator := func(input *ethpb.Validator) error {
+		newRoot, err := stateutil.ValidatorRootWithHasher(hasher, input)
+		if err != nil {
+			return err
+		}
+		roots = append(roots, newRoot)
+		return nil
+	}
+	if convertAll {
+		for i := range val {
+			err := rootCreator(val[i])
+			if err != nil {
+				return nil, err
+			}
+		}
+		return roots, nil
+	}
+	if len(val) > 0 {
+		for _, idx := range indices {
+			if idx > uint64(len(val))-1 {
+				return nil, fmt.Errorf("index %d greater than number of validators %d", idx, len(val))
+			}
+			err := rootCreator(val[idx])
+			if err != nil {
+				return nil, err
+			}
+		}
+	}
+	return roots, nil
+}
+
+// handleEth1DataSlice processes a list of eth1data and indices into the appropriate roots.
+func handleEth1DataSlice(val []*ethpb.Eth1Data, indices []uint64, convertAll bool) ([][32]byte, error) {
 	length := len(indices)
 	if convertAll {
 		length = len(val)
@@ -154,9 +222,9 @@ func handlePendingAttestation(val []*ethpb.PendingAttestation, indices []uint64,
 func handleBalanceSlice(val []uint64, indices []uint64, convertAll bool) ([][32]byte, error) {
 	if convertAll {
 		balancesMarshaling := make([][]byte, 0)
-		for i := 0; i < len(val); i++ {
+		for _, b := range val {
 			balanceBuf := make([]byte, 8)
-			binary.LittleEndian.PutUint64(balanceBuf, val[i])
+			binary.LittleEndian.PutUint64(balanceBuf, b)
 			balancesMarshaling = append(balancesMarshaling, balanceBuf)
 		}
 		balancesChunks, err := ssz.PackByChunk(balancesMarshaling)

beacon-chain/state/fieldtrie/helpers_test.go

@@ -18,11 +18,18 @@ func Test_handlePendingAttestation_OutOfRange(t *testing.T) {
 func Test_handleEth1DataSlice_OutOfRange(t *testing.T) {
 	items := make([]*ethpb.Eth1Data, 1)
 	indices := []uint64{3}
-	_, err := HandleEth1DataSlice(items, indices, false)
+	_, err := handleEth1DataSlice(items, indices, false)
 	assert.ErrorContains(t, "index 3 greater than number of items in eth1 data slice 1", err)
 
 }
 
+func Test_handleValidatorSlice_OutOfRange(t *testing.T) {
+	vals := make([]*ethpb.Validator, 1)
+	indices := []uint64{3}
+	_, err := handleValidatorSlice(vals, indices, false)
+	assert.ErrorContains(t, "index 3 greater than number of validators 1", err)
+}
+
 func TestBalancesSlice_CorrectRoots_All(t *testing.T) {
 	balances := []uint64{5, 2929, 34, 1291, 354305}
 	roots, err := handleBalanceSlice(balances, []uint64{}, true)

beacon-chain/state/stateutil/validator_root.go

@@ -2,7 +2,6 @@ package stateutil
 
 import (
 	"encoding/binary"
-	"fmt"
 
 	"github.com/pkg/errors"
 	"github.com/prysmaticlabs/prysm/config/params"
@@ -127,42 +126,3 @@ func ValidatorEncKey(validator *ethpb.Validator) []byte {
 
 	return enc
 }
-
-// HandleValidatorSlice returns the validator indices in a slice of root format.
-func HandleValidatorSlice(val []*ethpb.Validator, indices []uint64, convertAll bool) ([][32]byte, error) {
-	length := len(indices)
-	if convertAll {
-		length = len(val)
-	}
-	roots := make([][32]byte, 0, length)
-	hasher := hash.CustomSHA256Hasher()
-	rootCreator := func(input *ethpb.Validator) error {
-		newRoot, err := ValidatorRootWithHasher(hasher, input)
-		if err != nil {
-			return err
-		}
-		roots = append(roots, newRoot)
-		return nil
-	}
-	if convertAll {
-		for i := range val {
-			err := rootCreator(val[i])
-			if err != nil {
-				return nil, err
-			}
-		}
-		return roots, nil
-	}
-	if len(val) > 0 {
-		for _, idx := range indices {
-			if idx > uint64(len(val))-1 {
-				return nil, fmt.Errorf("index %d greater than number of validators %d", idx, len(val))
-			}
-			err := rootCreator(val[idx])
-			if err != nil {
-				return nil, err
-			}
-		}
-	}
-	return roots, nil
-}

encoding/ssz/helpers.go

@@ -116,16 +116,9 @@ func Pack(serializedItems [][]byte) ([][]byte, error) {
 
 // PackByChunk a given byte array's final chunk with zeroes if needed.
 func PackByChunk(serializedItems [][]byte) ([][bytesPerChunk]byte, error) {
-	areAllEmpty := true
-	for _, item := range serializedItems {
-		if !bytes.Equal(item, []byte{}) {
-			areAllEmpty = false
-			break
-		}
-	}
+	emptyChunk := [bytesPerChunk]byte{}
 	// If there are no items, we return an empty chunk.
-	if len(serializedItems) == 0 || areAllEmpty {
-		emptyChunk := [bytesPerChunk]byte{}
+	if len(serializedItems) == 0 {
 		return [][bytesPerChunk]byte{emptyChunk}, nil
 	} else if len(serializedItems[0]) == bytesPerChunk {
 		// If each item has exactly BYTES_PER_CHUNK length, we return the list of serialized items.
@@ -140,6 +133,11 @@ func PackByChunk(serializedItems [][]byte) ([][bytesPerChunk]byte, error) {
 	for _, item := range serializedItems {
 		orderedItems = append(orderedItems, item...)
 	}
+	// If all our serialized item slices are length zero, we
+	// exit early.
+	if len(orderedItems) == 0 {
+		return [][bytesPerChunk]byte{emptyChunk}, nil
+	}
 	numItems := len(orderedItems)
 	var chunks [][bytesPerChunk]byte
 	for i := 0; i < numItems; i += bytesPerChunk {
@@ -153,6 +151,10 @@ func PackByChunk(serializedItems [][]byte) ([][bytesPerChunk]byte, error) {
 		// indices determined above.
 		// Right-pad the last chunk with zero bytes if it does not
 		// have length bytesPerChunk from the helper.
+		// The ToBytes32 helper allocates a 32-byte array, before
+		// copying the ordered items in. This ensures that even if
+		// the last chunk is != 32 in length, we will right-pad it with
+		// zero bytes.
 		chunks = append(chunks, bytesutil.ToBytes32(orderedItems[i:j]))
 	}
 

encoding/ssz/helpers_test.go

@@ -94,6 +94,22 @@ func TestPack(t *testing.T) {
 	}
 }
 
+func TestPackByChunk(t *testing.T) {
+	byteSlice2D := [][]byte{
+		{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 2, 5, 2, 6, 2, 7},
+		{1, 1, 2, 3, 5, 8, 13, 21, 34},
+	}
+	expected := [][32]byte{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 2, 5, 2, 6, 2, 7, 1, 1},
+		{2, 3, 5, 8, 13, 21, 34, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}}
+
+	result, err := ssz.PackByChunk(byteSlice2D)
+	require.NoError(t, err)
+	assert.Equal(t, len(expected), len(result))
+	for i, v := range expected {
+		assert.DeepEqual(t, v, result[i])
+	}
+}
+
 func TestMixInLength(t *testing.T) {
 	byteSlice := [32]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32}
 	length := []byte{1, 2, 3}