Merge branch 'develop' into fix-optimistic-check

* Broadcasting BLS to execution changes should not use the request context in a go routine

* Changelog fragment

beacon-chain/db/kv/blocks.go

@@ -30,22 +30,32 @@ var errInvalidSlotRange = errors.New("invalid end slot and start slot provided")
 func (s *Store) Block(ctx context.Context, blockRoot [32]byte) (interfaces.ReadOnlySignedBeaconBlock, error) {
 	ctx, span := trace.StartSpan(ctx, "BeaconDB.Block")
 	defer span.End()
-	// Return block from cache if it exists.
+	blk, err := s.getBlock(ctx, blockRoot, nil)
+	if errors.Is(err, ErrNotFound) {
+		return nil, nil
+	}
+	return blk, err
+}
+
+func (s *Store) getBlock(ctx context.Context, blockRoot [32]byte, tx *bolt.Tx) (interfaces.ReadOnlySignedBeaconBlock, error) {
 	if v, ok := s.blockCache.Get(string(blockRoot[:])); v != nil && ok {
 		return v.(interfaces.ReadOnlySignedBeaconBlock), nil
 	}
-	var blk interfaces.ReadOnlySignedBeaconBlock
-	err := s.db.View(func(tx *bolt.Tx) error {
-		bkt := tx.Bucket(blocksBucket)
-		enc := bkt.Get(blockRoot[:])
-		if enc == nil {
-			return nil
-		}
+	// This method allows the caller to pass in its tx if one is already open.
+	// Or if a nil value is used, a transaction will be managed intenally.
+	if tx == nil {
 		var err error
-		blk, err = unmarshalBlock(ctx, enc)
-		return err
-	})
-	return blk, err
+		tx, err = s.db.Begin(false)
+		if err != nil {
+			return nil, err
+		}
+		defer func() {
+			if err := tx.Rollback(); err != nil {
+				log.WithError(err).Error("could not rollback read-only getBlock transaction")
+			}
+		}()
+	}
+	return unmarshalBlock(ctx, tx.Bucket(blocksBucket).Get(blockRoot[:]))
 }
 
 // OriginCheckpointBlockRoot returns the value written to the db in SaveOriginCheckpointBlockRoot
@@ -227,6 +237,21 @@ func (s *Store) DeleteBlock(ctx context.Context, root [32]byte) error {
 			return ErrDeleteJustifiedAndFinalized
 		}
 
+		// Look up the block to find its slot; needed to remove the slot index entry.
+		blk, err := s.getBlock(ctx, root, tx)
+		if err != nil {
+			// getBlock can return ErrNotFound, in which case we won't even try to delete it.
+			if errors.Is(err, ErrNotFound) {
+				return nil
+			}
+			return err
+		}
+		if err := s.deleteSlotIndexEntry(tx, blk.Block().Slot(), root); err != nil {
+			return err
+		}
+		if err := s.deleteMatchingParentIndex(tx, blk.Block().ParentRoot(), root); err != nil {
+			return err
+		}
 		if err := s.deleteBlock(tx, root[:]); err != nil {
 			return err
 		}
@@ -899,6 +924,9 @@ func createBlockIndicesFromFilters(ctx context.Context, f *filters.QueryFilter)
 
 // unmarshal block from marshaled proto beacon block bytes to versioned beacon block struct type.
 func unmarshalBlock(_ context.Context, enc []byte) (interfaces.ReadOnlySignedBeaconBlock, error) {
+	if len(enc) == 0 {
+		return nil, errors.Wrap(ErrNotFound, "empty block bytes in db")
+	}
 	var err error
 	enc, err = snappy.Decode(nil, enc)
 	if err != nil {
@@ -1050,6 +1078,47 @@ func (s *Store) deleteBlock(tx *bolt.Tx, root []byte) error {
 	return nil
 }
 
+func (s *Store) deleteMatchingParentIndex(tx *bolt.Tx, parent, child [32]byte) error {
+	bkt := tx.Bucket(blockParentRootIndicesBucket)
+	if err := deleteRootIndexEntry(bkt, parent[:], child); err != nil {
+		return errors.Wrap(err, "could not delete parent root index entry")
+	}
+	return nil
+}
+
+func (s *Store) deleteSlotIndexEntry(tx *bolt.Tx, slot primitives.Slot, root [32]byte) error {
+	key := bytesutil.SlotToBytesBigEndian(slot)
+	bkt := tx.Bucket(blockSlotIndicesBucket)
+	if err := deleteRootIndexEntry(bkt, key, root); err != nil {
+		return errors.Wrap(err, "could not delete slot index entry")
+	}
+	return nil
+}
+
+func deleteRootIndexEntry(bkt *bolt.Bucket, key []byte, root [32]byte) error {
+	packed := bkt.Get(key)
+	if len(packed) == 0 {
+		return nil
+	}
+	updated, err := removeRoot(packed, root)
+	if err != nil {
+		return err
+	}
+	// Don't update the value if the root was not found.
+	if bytes.Equal(updated, packed) {
+		return nil
+	}
+	// If there are no other roots in the key, just delete it.
+	if len(updated) == 0 {
+		if err := bkt.Delete(key); err != nil {
+			return err
+		}
+		return nil
+	}
+	// Update the key with the root removed.
+	return bkt.Put(key, updated)
+}
+
 func (s *Store) deleteValidatorHashes(tx *bolt.Tx, root []byte) error {
 	ok, err := s.isStateValidatorMigrationOver()
 	if err != nil {

beacon-chain/db/kv/blocks_test.go

@@ -196,9 +196,13 @@ func TestStore_BlocksCRUD(t *testing.T) {
 			blockRoot, err := blk.Block().HashTreeRoot()
 			require.NoError(t, err)
 
+			_, err = db.getBlock(ctx, blockRoot, nil)
+			require.ErrorIs(t, err, ErrNotFound)
 			retrievedBlock, err := db.Block(ctx, blockRoot)
 			require.NoError(t, err)
 			assert.DeepEqual(t, nil, retrievedBlock, "Expected nil block")
+			_, err = db.getBlock(ctx, blockRoot, nil)
+			require.ErrorIs(t, err, ErrNotFound)
 
 			require.NoError(t, db.SaveBlock(ctx, blk))
 			assert.Equal(t, true, db.HasBlock(ctx, blockRoot), "Expected block to exist in the db")
@@ -214,10 +218,34 @@ func TestStore_BlocksCRUD(t *testing.T) {
 			retrievedPb, err := retrievedBlock.Proto()
 			require.NoError(t, err)
 			assert.Equal(t, true, proto.Equal(wantedPb, retrievedPb), "Wanted: %v, received: %v", wanted, retrievedBlock)
+			// Check that the block is in the slot->block index
+			found, roots, err := db.BlockRootsBySlot(ctx, blk.Block().Slot())
+			require.NoError(t, err)
+			require.Equal(t, true, found)
+			require.Equal(t, 1, len(roots))
+			require.Equal(t, blockRoot, roots[0])
+			// Delete the block, then check that it is no longer in the index.
+
+			parent := blk.Block().ParentRoot()
+			testCheckParentIndices(t, db.db, parent, true)
+			require.NoError(t, db.DeleteBlock(ctx, blockRoot))
+			require.NoError(t, err)
+			testCheckParentIndices(t, db.db, parent, false)
+			found, roots, err = db.BlockRootsBySlot(ctx, blk.Block().Slot())
+			require.NoError(t, err)
+			require.Equal(t, false, found)
+			require.Equal(t, 0, len(roots))
 		})
 	}
 }
 
+func testCheckParentIndices(t *testing.T, db *bolt.DB, parent [32]byte, expected bool) {
+	require.NoError(t, db.View(func(tx *bolt.Tx) error {
+		require.Equal(t, expected, tx.Bucket(blockParentRootIndicesBucket).Get(parent[:]) != nil)
+		return nil
+	}))
+}
+
 func TestStore_BlocksHandleZeroCase(t *testing.T) {
 	for _, tt := range blockTests {
 		t.Run(tt.name, func(t *testing.T) {

beacon-chain/db/kv/utils.go

@@ -114,3 +114,27 @@ func splitRoots(b []byte) ([][32]byte, error) {
 	}
 	return rl, nil
 }
+
+func removeRoot(roots []byte, root [32]byte) ([]byte, error) {
+	if len(roots) == 0 {
+		return []byte{}, nil
+	}
+	if len(roots) == 32 && bytes.Equal(roots, root[:]) {
+		return []byte{}, nil
+	}
+	if len(roots)%32 != 0 {
+		return nil, errors.Wrapf(errMisalignedRootList, "root list len=%d", len(roots))
+	}
+
+	search := root[:]
+	for i := 0; i <= len(roots)-32; i += 32 {
+		if bytes.Equal(roots[i:i+32], search) {
+			result := make([]byte, len(roots)-32)
+			copy(result, roots[:i])
+			copy(result[i:], roots[i+32:])
+			return result, nil
+		}
+	}
+
+	return roots, nil
+}

beacon-chain/db/kv/utils_test.go

@@ -1,6 +1,7 @@
 package kv
 
 import (
+	"bytes"
 	"context"
 	"crypto/rand"
 	"testing"
@@ -195,3 +196,85 @@ func TestSplitRoots(t *testing.T) {
 		})
 	}
 }
+
+func tPad(p ...[]byte) []byte {
+	r := make([]byte, 32*len(p))
+	for i, b := range p {
+		copy(r[i*32:], b)
+	}
+	return r
+}
+
+func TestRemoveRoot(t *testing.T) {
+	cases := []struct {
+		name   string
+		roots  []byte
+		root   [32]byte
+		expect []byte
+		err    error
+	}{
+		{
+			name:   "empty",
+			roots:  []byte{},
+			root:   [32]byte{0xde, 0xad, 0xbe, 0xef},
+			expect: []byte{},
+		},
+		{
+			name:   "single",
+			roots:  tPad([]byte{0xde, 0xad, 0xbe, 0xef}),
+			root:   [32]byte{0xde, 0xad, 0xbe, 0xef},
+			expect: []byte{},
+		},
+		{
+			name:   "single, different",
+			roots:  tPad([]byte{0xde, 0xad, 0xbe, 0xef}),
+			root:   [32]byte{0xde, 0xad, 0xbe, 0xee},
+			expect: tPad([]byte{0xde, 0xad, 0xbe, 0xef}),
+		},
+		{
+			name:   "multi",
+			roots:  tPad([]byte{0xde, 0xad, 0xbe, 0xef}, []byte{0xac, 0x1d, 0xfa, 0xce}),
+			root:   [32]byte{0xac, 0x1d, 0xfa, 0xce},
+			expect: tPad([]byte{0xde, 0xad, 0xbe, 0xef}),
+		},
+		{
+			name:   "multi, reordered",
+			roots:  tPad([]byte{0xac, 0x1d, 0xfa, 0xce}, []byte{0xde, 0xad, 0xbe, 0xef}),
+			root:   [32]byte{0xac, 0x1d, 0xfa, 0xce},
+			expect: tPad([]byte{0xde, 0xad, 0xbe, 0xef}),
+		},
+		{
+			name:   "multi, 3",
+			roots:  tPad([]byte{0xac, 0x1d, 0xfa, 0xce}, []byte{0xbe, 0xef, 0xca, 0xb5}, []byte{0xde, 0xad, 0xbe, 0xef}),
+			root:   [32]byte{0xac, 0x1d, 0xfa, 0xce},
+			expect: tPad([]byte{0xbe, 0xef, 0xca, 0xb5}, []byte{0xde, 0xad, 0xbe, 0xef}),
+		},
+		{
+			name:   "multi, different",
+			roots:  tPad([]byte{0xde, 0xad, 0xbe, 0xef}, []byte{0xac, 0x1d, 0xfa, 0xce}),
+			root:   [32]byte{0xac, 0x1d, 0xbe, 0xa7},
+			expect: tPad([]byte{0xde, 0xad, 0xbe, 0xef}, []byte{0xac, 0x1d, 0xfa, 0xce}),
+		},
+		{
+			name:  "misaligned",
+			roots: make([]byte, 61),
+			root:  [32]byte{0xac, 0x1d, 0xbe, 0xa7},
+			err:   errMisalignedRootList,
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			before := make([]byte, len(c.roots))
+			copy(before, c.roots)
+			r, err := removeRoot(c.roots, c.root)
+			if c.err != nil {
+				require.ErrorIs(t, err, c.err)
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, len(c.expect), len(r))
+			require.Equal(t, true, bytes.Equal(c.expect, r))
+			require.Equal(t, true, bytes.Equal(before, c.roots))
+		})
+	}
+}

beacon-chain/rpc/eth/beacon/handlers.go

@@ -1158,11 +1158,10 @@ func (s *Server) GetBlockRoot(w http.ResponseWriter, r *http.Request) {
 			}
 		}
 	}
-
 	b32Root := bytesutil.ToBytes32(root)
-	isOptimistic, err := s.OptimisticModeFetcher.IsOptimisticForRoot(ctx, b32Root)
+	isOptimistic, err := s.OptimisticModeFetcher.IsOptimistic(ctx)
 	if err != nil {
-		httputil.HandleError(w, "Could not check if block is optimistic: "+err.Error(), http.StatusInternalServerError)
+		httputil.HandleError(w, "Could not retrieve optimistic status: "+err.Error(), http.StatusInternalServerError)
 		return
 	}
 	response := &structs.BlockRootResponse{

beacon-chain/rpc/eth/beacon/handlers_pool.go

@@ -633,7 +633,7 @@ func (s *Server) SubmitBLSToExecutionChanges(w http.ResponseWriter, r *http.Requ
 			toBroadcast = append(toBroadcast, sbls)
 		}
 	}
-	go s.broadcastBLSChanges(ctx, toBroadcast)
+	go s.broadcastBLSChanges(context.Background(), toBroadcast)
 	if len(failures) > 0 {
 		failuresErr := &server.IndexedVerificationFailureError{
 			Code:     http.StatusBadRequest,

beacon-chain/rpc/eth/config/handlers_test.go

@@ -160,6 +160,8 @@ func TestGetSpec(t *testing.T) {
 	config.MaxTransactionsPerPayload = 99
 	config.FieldElementsPerBlob = 100
 	config.KzgCommitmentInclusionProofDepth = 101
+	config.BlobsidecarSubnetCount = 102
+	config.BlobsidecarSubnetCountElectra = 103
 
 	var dbp [4]byte
 	copy(dbp[:], []byte{'0', '0', '0', '1'})
@@ -198,7 +200,7 @@ func TestGetSpec(t *testing.T) {
 	data, ok := resp.Data.(map[string]interface{})
 	require.Equal(t, true, ok)
 
-	assert.Equal(t, 168, len(data))
+	assert.Equal(t, 170, len(data))
 	for k, v := range data {
 		t.Run(k, func(t *testing.T) {
 			switch k {
@@ -559,6 +561,10 @@ func TestGetSpec(t *testing.T) {
 				assert.Equal(t, "100", v)
 			case "KZG_COMMITMENT_INCLUSION_PROOF_DEPTH":
 				assert.Equal(t, "101", v)
+			case "BLOB_SIDECAR_SUBNET_COUNT":
+				assert.Equal(t, "102", v)
+			case "BLOB_SIDECAR_SUBNET_COUNT_ELECTRA":
+				assert.Equal(t, "103", v)
 			default:
 				t.Errorf("Incorrect key: %s", k)
 			}

beacon-chain/rpc/eth/validator/handlers.go

@@ -636,6 +636,15 @@ func (s *Server) ProduceSyncCommitteeContribution(w http.ResponseWriter, r *http
 	ctx, span := trace.StartSpan(r.Context(), "validator.ProduceSyncCommitteeContribution")
 	defer span.End()
 
+	isOptimistic, err := s.OptimisticModeFetcher.IsOptimistic(ctx)
+	if err != nil {
+		httputil.HandleError(w, err.Error(), http.StatusInternalServerError)
+		return
+	}
+	if isOptimistic {
+		httputil.HandleError(w, "Beacon node is currently syncing and not serving request on that endpoint", http.StatusServiceUnavailable)
+		return
+	}
 	_, index, ok := shared.UintFromQuery(w, r, "subcommittee_index", true)
 	if !ok {
 		return

beacon-chain/rpc/eth/validator/handlers_test.go

@@ -1584,7 +1584,8 @@ func TestProduceSyncCommitteeContribution(t *testing.T) {
 				SyncCommitteeIndices: []primitives.CommitteeIndex{0},
 			},
 		},
-		SyncCommitteePool: syncCommitteePool,
+		SyncCommitteePool:     syncCommitteePool,
+		OptimisticModeFetcher: &mockChain.ChainService{},
 	}
 	t.Run("ok", func(t *testing.T) {
 		url := "http://example.com?slot=1&subcommittee_index=1&beacon_block_root=0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2"
@@ -1672,7 +1673,8 @@ func TestProduceSyncCommitteeContribution(t *testing.T) {
 					SyncCommitteeIndices: []primitives.CommitteeIndex{0},
 				},
 			},
-			SyncCommitteePool: syncCommitteePool,
+			SyncCommitteePool:     syncCommitteePool,
+			OptimisticModeFetcher: &mockChain.ChainService{},
 		}
 		server.ProduceSyncCommitteeContribution(writer, request)
 		assert.Equal(t, http.StatusNotFound, writer.Code)

changelog/james-prysm_fix-blobl-subnet-config.md

@@ -0,0 +1,3 @@
+### Fixed
+
+- fixed /eth/v1/config/spec displays BLOB_SIDECAR_SUBNET_COUNT,BLOB_SIDECAR_SUBNET_COUNT_ELECTRA

changelog/james-prysm_fix-optimistic-check.md

@@ -0,0 +1,3 @@
+### Fixed
+
+- fixes e2e introduced by PR#14997 by changing the optimistic check in getblockroot for rest APIs.

changelog/kasey_delete-block-idx.md

@@ -0,0 +1,2 @@
+### Fixed
+- Ensure that deleting a block from the database clears its entry in the slot->root db index.

changelog/pvl_bls-ctx.md

@@ -0,0 +1,3 @@
+### Fixed
+
+- Broadcasting BLS to execution changes should not use the request context in a go routine. Use context.Background() for the broadcasting go routine.

config/params/config.go

@@ -235,8 +235,8 @@ type BeaconChainConfig struct {
 	ExecutionEngineTimeoutValue uint64 // ExecutionEngineTimeoutValue defines the seconds to wait before timing out engine endpoints with execution payload execution semantics (newPayload, forkchoiceUpdated).
 
 	// Subnet value
-	BlobsidecarSubnetCount        uint64 `yaml:"BLOB_SIDECAR_SUBNET_COUNT"`         // BlobsidecarSubnetCount is the number of blobsidecar subnets used in the gossipsub protocol.
-	BlobsidecarSubnetCountElectra uint64 `yaml:"BLOB_SIDECAR_SUBNET_COUNT_ELECTRA"` // BlobsidecarSubnetCountElectra is the number of blobsidecar subnets used in the gossipsub protocol post Electra hard fork.
+	BlobsidecarSubnetCount        uint64 `yaml:"BLOB_SIDECAR_SUBNET_COUNT" spec:"true"`         // BlobsidecarSubnetCount is the number of blobsidecar subnets used in the gossipsub protocol.
+	BlobsidecarSubnetCountElectra uint64 `yaml:"BLOB_SIDECAR_SUBNET_COUNT_ELECTRA" spec:"true"` // BlobsidecarSubnetCountElectra is the number of blobsidecar subnets used in the gossipsub protocol post Electra hard fork.
 
 	// Values introduced in Deneb hard fork
 	MaxPerEpochActivationChurnLimit  uint64           `yaml:"MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT" spec:"true"`  // MaxPerEpochActivationChurnLimit is the maximum amount of churn allotted for validator activation.