Deprecate Step Parameter from our Block By Range Requests (#10914)

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

beacon-chain/sync/initial-sync/blocks_fetcher_utils.go

@@ -102,14 +102,17 @@ func (f *blocksFetcher) nonSkippedSlotAfterWithPeersTarget(
 
 	// Quickly find the close enough epoch where a non-empty slot definitely exists.
 	// Only single random slot per epoch is checked - allowing to move forward relatively quickly.
+	// This method has been changed to account for our spec change where step can only be 1 in a
+	// block by range request. https://github.com/ethereum/consensus-specs/pull/2856
+	// The downside is that this method will be less effective during periods without
+	// finality.
 	slot += nonSkippedSlotsFullSearchEpochs * slotsPerEpoch
 	upperBoundSlot, err := slots.EpochStart(targetEpoch + 1)
 	if err != nil {
 		return 0, err
 	}
-	for ind := slot + 1; ind < upperBoundSlot; ind += (slotsPerEpoch * slotsPerEpoch) / 2 {
-		start := ind.Add(uint64(f.rand.Intn(int(slotsPerEpoch)))) // lint:ignore uintcast -- Slots per epoch will never exceed int64.
-		nextSlot, err := fetch(peers[pidInd%len(peers)], start, uint64(slotsPerEpoch/2), uint64(slotsPerEpoch))
+	for ind := slot + 1; ind < upperBoundSlot; ind += slotsPerEpoch {
+		nextSlot, err := fetch(peers[pidInd%len(peers)], ind, uint64(slotsPerEpoch), 1)
 		if err != nil {
 			return 0, err
 		}

beacon-chain/sync/rpc_beacon_blocks_by_range.go

@@ -40,7 +40,10 @@ func (s *Service) beaconBlocksByRangeRPCHandler(ctx context.Context, msg interfa
 		tracing.AnnotateError(span, err)
 		return err
 	}
-
+	// Only have range requests with a step of 1 being processed.
+	if m.Step > 1 {
+		m.Step = 1
+	}
 	// The initial count for the first batch to be returned back.
 	count := m.Count
 	allowedBlocksPerSecond := uint64(flags.Get().BlockBatchLimit)
@@ -240,7 +243,7 @@ func (s *Service) filterBlocks(ctx context.Context, blks []interfaces.SignedBeac
 			// Set the previous root as the
 			// newly added block's root
 			currRoot := roots[i]
-			prevRoot = &currRoot
+			*prevRoot = currRoot
 		}
 	}
 	return newBlks, nil

beacon-chain/sync/rpc_beacon_blocks_by_range_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/prysmaticlabs/prysm/cmd/beacon-chain/flags"
 	fieldparams "github.com/prysmaticlabs/prysm/config/fieldparams"
 	"github.com/prysmaticlabs/prysm/config/params"
+	"github.com/prysmaticlabs/prysm/consensus-types/interfaces"
 	types "github.com/prysmaticlabs/prysm/consensus-types/primitives"
 	"github.com/prysmaticlabs/prysm/consensus-types/wrapper"
 	"github.com/prysmaticlabs/prysm/encoding/bytesutil"
@@ -45,9 +46,14 @@ func TestRPCBeaconBlocksByRange_RPCHandlerReturnsBlocks(t *testing.T) {
 	}
 
 	// Populate the database with blocks that would match the request.
-	for i := req.StartSlot; i < req.StartSlot.Add(req.Step*req.Count); i += types.Slot(req.Step) {
+	prevRoot := [32]byte{}
+	var err error
+	for i := req.StartSlot; i < req.StartSlot.Add(req.Count); i += types.Slot(1) {
 		blk := util.NewBeaconBlock()
 		blk.Block.Slot = i
+		copy(blk.Block.ParentRoot, prevRoot[:])
+		prevRoot, err = blk.Block.HashTreeRoot()
+		require.NoError(t, err)
 		wsb, err := wrapper.WrappedSignedBeaconBlock(blk)
 		require.NoError(t, err)
 		require.NoError(t, d.SaveBlock(context.Background(), wsb))
@@ -62,11 +68,11 @@ func TestRPCBeaconBlocksByRange_RPCHandlerReturnsBlocks(t *testing.T) {
 	wg.Add(1)
 	p2.BHost.SetStreamHandler(pcl, func(stream network.Stream) {
 		defer wg.Done()
-		for i := req.StartSlot; i < req.StartSlot.Add(req.Count*req.Step); i += types.Slot(req.Step) {
+		for i := req.StartSlot; i < req.StartSlot.Add(req.Count); i += types.Slot(1) {
 			expectSuccess(t, stream)
 			res := util.NewBeaconBlock()
 			assert.NoError(t, r.cfg.p2p.Encoding().DecodeWithMaxLength(stream, res))
-			if res.Block.Slot.SubSlot(req.StartSlot).Mod(req.Step) != 0 {
+			if res.Block.Slot.SubSlot(req.StartSlot).Mod(1) != 0 {
 				t.Errorf("Received unexpected block slot %d", res.Block.Slot)
 			}
 		}
@@ -168,19 +174,22 @@ func TestRPCBeaconBlocksByRange_RPCHandlerReturnsSortedBlocks(t *testing.T) {
 		Count:     33,
 	}
 
-	endSlot := req.StartSlot.Add(req.Step * (req.Count - 1))
+	endSlot := req.StartSlot.Add(req.Count - 1)
 	expectedRoots := make([][32]byte, req.Count)
 	// Populate the database with blocks that would match the request.
-	for i, j := endSlot, req.Count-1; i >= req.StartSlot; i -= types.Slot(req.Step) {
+	prevRoot := [32]byte{}
+	for i, j := req.StartSlot, 0; i <= endSlot; i++ {
 		blk := util.NewBeaconBlock()
 		blk.Block.Slot = i
+		copy(blk.Block.ParentRoot, prevRoot[:])
 		rt, err := blk.Block.HashTreeRoot()
 		require.NoError(t, err)
 		expectedRoots[j] = rt
+		prevRoot = rt
 		wsb, err := wrapper.WrappedSignedBeaconBlock(blk)
 		require.NoError(t, err)
 		require.NoError(t, d.SaveBlock(context.Background(), wsb))
-		j--
+		j++
 	}
 
 	// Start service with 160 as allowed blocks capacity (and almost zero capacity recovery).
@@ -195,7 +204,7 @@ func TestRPCBeaconBlocksByRange_RPCHandlerReturnsSortedBlocks(t *testing.T) {
 		defer wg.Done()
 		prevSlot := types.Slot(0)
 		require.Equal(t, uint64(len(expectedRoots)), req.Count, "Number of roots not expected")
-		for i, j := req.StartSlot, 0; i < req.StartSlot.Add(req.Count*req.Step); i += types.Slot(req.Step) {
+		for i, j := req.StartSlot, 0; i < req.StartSlot.Add(req.Count); i += types.Slot(1) {
 			expectSuccess(t, stream)
 			res := &ethpb.SignedBeaconBlock{}
 			assert.NoError(t, r.cfg.p2p.Encoding().DecodeWithMaxLength(stream, res))
@@ -286,6 +295,8 @@ func TestRPCBeaconBlocksByRange_RPCHandlerRateLimitOverflow(t *testing.T) {
 	saveBlocks := func(req *ethpb.BeaconBlocksByRangeRequest) {
 		// Populate the database with blocks that would match the request.
 		parentRoot := [32]byte{}
+		// Default to 1 to be inline with the spec.
+		req.Step = 1
 		for i := req.StartSlot; i < req.StartSlot.Add(req.Step*req.Count); i += types.Slot(req.Step) {
 			block := util.NewBeaconBlock()
 			block.Block.Slot = i
@@ -310,6 +321,8 @@ func TestRPCBeaconBlocksByRange_RPCHandlerRateLimitOverflow(t *testing.T) {
 			if !validateBlocks {
 				return
 			}
+			// Use a step of 1 to be inline with our specs.
+			req.Step = 1
 			for i := req.StartSlot; i < req.StartSlot.Add(req.Count*req.Step); i += types.Slot(req.Step) {
 				if !success {
 					continue
@@ -945,3 +958,48 @@ func TestRPCBeaconBlocksByRange_FilterBlocks(t *testing.T) {
 		require.LogsDoNotContain(t, hook, "Disconnecting bad peer")
 	})
 }
+
+func TestRPCBeaconBlocksByRange_FilterBlocks_PreviousRoot(t *testing.T) {
+	p1 := p2ptest.NewTestP2P(t)
+	p2 := p2ptest.NewTestP2P(t)
+	p1.Connect(p2)
+	assert.Equal(t, 1, len(p1.BHost.Network().Peers()), "Expected peers to be connected")
+	d := db.SetupDB(t)
+
+	req := &ethpb.BeaconBlocksByRangeRequest{
+		StartSlot: 100,
+		Step:      1,
+		Count:     uint64(flags.Get().BlockBatchLimit) * 2,
+	}
+
+	// Populate the database with blocks that would match the request.
+	prevRoot := [32]byte{}
+	var err error
+	blks := []interfaces.SignedBeaconBlock{}
+	roots := [][32]byte{}
+	for i := req.StartSlot; i < req.StartSlot.Add(req.Count); i += types.Slot(1) {
+		blk := util.NewBeaconBlock()
+		blk.Block.Slot = i
+		copy(blk.Block.ParentRoot, prevRoot[:])
+		prevRoot, err = blk.Block.HashTreeRoot()
+		require.NoError(t, err)
+		wsb, err := wrapper.WrappedSignedBeaconBlock(blk)
+		require.NoError(t, err)
+		blks = append(blks, wsb)
+		copiedRt := prevRoot
+		roots = append(roots, copiedRt)
+	}
+
+	// Start service with 160 as allowed blocks capacity (and almost zero capacity recovery).
+	r := &Service{cfg: &config{p2p: p1, beaconDB: d, chain: &chainMock.ChainService{}}, rateLimiter: newRateLimiter(p1)}
+
+	initialRoot := [32]byte{}
+	ptrRt := &initialRoot
+	newBlks, err := r.filterBlocks(context.Background(), blks, roots, ptrRt, req.Step, req.StartSlot)
+	require.NoError(t, err)
+	require.Equal(t, len(blks), len(newBlks))
+
+	// pointer should reference a new root.
+	require.NotEqual(t, *ptrRt, [32]byte{})
+
+}