From 22b6d1751da75afd0bfa675edb6e7d1b864cef6b Mon Sep 17 00:00:00 2001
From: Nishant Das <nishdas93@gmail.com>
Date: Fri, 26 Jan 2024 12:37:41 +0800
Subject: [PATCH] Enable Backfill in E2E (#13524)

* enable backfill for devmode

* enable backfill

* gaz

* move to its own package

* fix panic

* fix bug

* gaz

* kasey's review
---
 beacon-chain/sync/backfill/BUILD.bazel        |  1 +
 beacon-chain/sync/backfill/service.go         | 10 ++++-
 beacon-chain/sync/backfill/service_test.go    | 16 ++++++++
 beacon-chain/sync/backfill/status.go          |  6 +++
 beacon-chain/sync/backfill/worker.go          |  2 +
 cmd/beacon-chain/BUILD.bazel                  |  1 +
 cmd/beacon-chain/main.go                      |  9 +++--
 cmd/beacon-chain/sync/backfill/BUILD.bazel    |  1 +
 .../sync/backfill/flags/BUILD.bazel           |  9 +++++
 cmd/beacon-chain/sync/backfill/flags/flags.go | 38 ++++++++++++++++++
 cmd/beacon-chain/sync/backfill/options.go     | 39 ++-----------------
 cmd/beacon-chain/usage.go                     |  2 +-
 config/features/BUILD.bazel                   |  1 +
 config/features/flags.go                      |  2 +
 testing/endtoend/components/beacon_node.go    |  2 +-
 15 files changed, 96 insertions(+), 43 deletions(-)
 create mode 100644 cmd/beacon-chain/sync/backfill/flags/BUILD.bazel
 create mode 100644 cmd/beacon-chain/sync/backfill/flags/flags.go

diff --git a/beacon-chain/sync/backfill/BUILD.bazel b/beacon-chain/sync/backfill/BUILD.bazel
index 1a0662d75084..6abde4fa563c 100644
--- a/beacon-chain/sync/backfill/BUILD.bazel
+++ b/beacon-chain/sync/backfill/BUILD.bazel
@@ -54,6 +54,7 @@ go_test(
     ],
     embed = [":go_default_library"],
     deps = [
+        "//beacon-chain/core/helpers:go_default_library",
         "//beacon-chain/core/signing:go_default_library",
         "//beacon-chain/db:go_default_library",
         "//beacon-chain/p2p/testing:go_default_library",
diff --git a/beacon-chain/sync/backfill/service.go b/beacon-chain/sync/backfill/service.go
index 33b7ddd50023..41ed8a1a878f 100644
--- a/beacon-chain/sync/backfill/service.go
+++ b/beacon-chain/sync/backfill/service.go
@@ -227,6 +227,10 @@ func (s *Service) Start() {
 	}
 	s.ms.setClock(clock)
 
+	if s.store.isGenesisSync() {
+		log.Info("Exiting backfill service as the node has been initialized with a genesis state or the backfill status is missing")
+		return
+	}
 	status := s.store.status()
 	// Exit early if there aren't going to be any batches to backfill.
 	if primitives.Slot(status.LowSlot) <= s.ms.minimumSlot() {
@@ -293,8 +297,10 @@ func minimumBackfillSlot(current primitives.Slot) primitives.Slot {
 		oe = slots.MaxSafeEpoch()
 	}
 	offset := slots.UnsafeEpochStart(oe)
-	if offset > current {
-		return 0
+	if offset >= current {
+		// Slot 0 is the genesis block, therefore the signature in it is invalid.
+		// To prevent us from rejecting a batch, we restrict the minimum backfill batch till only slot 1
+		return 1
 	}
 	return current - offset
 }
diff --git a/beacon-chain/sync/backfill/service_test.go b/beacon-chain/sync/backfill/service_test.go
index c4dd121afc9e..c723f28e1313 100644
--- a/beacon-chain/sync/backfill/service_test.go
+++ b/beacon-chain/sync/backfill/service_test.go
@@ -5,9 +5,11 @@ import (
 	"testing"
 	"time"
 
+	"github.com/prysmaticlabs/prysm/v4/beacon-chain/core/helpers"
 	p2ptest "github.com/prysmaticlabs/prysm/v4/beacon-chain/p2p/testing"
 	"github.com/prysmaticlabs/prysm/v4/beacon-chain/startup"
 	"github.com/prysmaticlabs/prysm/v4/beacon-chain/state"
+	"github.com/prysmaticlabs/prysm/v4/config/params"
 	"github.com/prysmaticlabs/prysm/v4/consensus-types/primitives"
 	"github.com/prysmaticlabs/prysm/v4/proto/dbval"
 	"github.com/prysmaticlabs/prysm/v4/testing/require"
@@ -75,6 +77,20 @@ func TestServiceInit(t *testing.T) {
 	}
 }
 
+func TestMinimumBackfillSlot(t *testing.T) {
+	oe := helpers.MinEpochsForBlockRequests()
+
+	currSlot := (oe + 100).Mul(uint64(params.BeaconConfig().SlotsPerEpoch))
+	minSlot := minimumBackfillSlot(primitives.Slot(currSlot))
+	require.Equal(t, 100*params.BeaconConfig().SlotsPerEpoch, minSlot)
+
+	oe = helpers.MinEpochsForBlockRequests()
+
+	currSlot = oe.Mul(uint64(params.BeaconConfig().SlotsPerEpoch))
+	minSlot = minimumBackfillSlot(primitives.Slot(currSlot))
+	require.Equal(t, primitives.Slot(1), minSlot)
+}
+
 func testReadN(t *testing.T, ctx context.Context, c chan batch, n int, into []batch) []batch {
 	for i := 0; i < n; i++ {
 		select {
diff --git a/beacon-chain/sync/backfill/status.go b/beacon-chain/sync/backfill/status.go
index 79c68e38e8dd..2ec809caae58 100644
--- a/beacon-chain/sync/backfill/status.go
+++ b/beacon-chain/sync/backfill/status.go
@@ -149,6 +149,12 @@ func (s *Store) swapStatus(bs *dbval.BackfillStatus) {
 	s.bs = bs
 }
 
+func (s *Store) isGenesisSync() bool {
+	s.RLock()
+	defer s.RUnlock()
+	return s.genesisSync
+}
+
 // originState looks up the state for the checkpoint sync origin. This is a hack, because StatusUpdater is the only
 // thing that needs db access and it has the origin root handy, so it's convenient to look it up here. The state is
 // needed by the verifier.
diff --git a/beacon-chain/sync/backfill/worker.go b/beacon-chain/sync/backfill/worker.go
index bc6e0b31a11b..954575c37895 100644
--- a/beacon-chain/sync/backfill/worker.go
+++ b/beacon-chain/sync/backfill/worker.go
@@ -40,11 +40,13 @@ func (w *p2pWorker) handle(ctx context.Context, b batch) batch {
 	dlt := time.Now()
 	backfillBatchTimeDownloading.Observe(float64(dlt.Sub(start).Milliseconds()))
 	if err != nil {
+		log.WithError(err).WithFields(b.logFields()).Debug("Batch requesting failed")
 		return b.withRetryableError(err)
 	}
 	vb, err := w.v.verify(results)
 	backfillBatchTimeVerifying.Observe(float64(time.Since(dlt).Milliseconds()))
 	if err != nil {
+		log.WithError(err).WithFields(b.logFields()).Debug("Batch validation failed")
 		return b.withRetryableError(err)
 	}
 	// This is a hack to get the rough size of the batch. This helps us approximate the amount of memory needed
diff --git a/cmd/beacon-chain/BUILD.bazel b/cmd/beacon-chain/BUILD.bazel
index 6a788de4734e..c344c1359872 100644
--- a/cmd/beacon-chain/BUILD.bazel
+++ b/cmd/beacon-chain/BUILD.bazel
@@ -22,6 +22,7 @@ go_library(
         "//cmd/beacon-chain/jwt:go_default_library",
         "//cmd/beacon-chain/storage:go_default_library",
         "//cmd/beacon-chain/sync/backfill:go_default_library",
+        "//cmd/beacon-chain/sync/backfill/flags:go_default_library",
         "//cmd/beacon-chain/sync/checkpoint:go_default_library",
         "//cmd/beacon-chain/sync/genesis:go_default_library",
         "//config/features:go_default_library",
diff --git a/cmd/beacon-chain/main.go b/cmd/beacon-chain/main.go
index 9871a91a7ad3..046ab4239d7a 100644
--- a/cmd/beacon-chain/main.go
+++ b/cmd/beacon-chain/main.go
@@ -20,7 +20,8 @@ import (
 	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/flags"
 	jwtcommands "github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/jwt"
 	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/storage"
-	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/backfill"
+	backfill "github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/backfill"
+	bflags "github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/backfill/flags"
 	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/checkpoint"
 	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/genesis"
 	"github.com/prysmaticlabs/prysm/v4/config/features"
@@ -139,9 +140,9 @@ var appFlags = []cli.Flag{
 	flags.JwtId,
 	storage.BlobStoragePathFlag,
 	storage.BlobRetentionEpochFlag,
-	backfill.EnableExperimentalBackfill,
-	backfill.BackfillBatchSize,
-	backfill.BackfillWorkerCount,
+	bflags.EnableExperimentalBackfill,
+	bflags.BackfillBatchSize,
+	bflags.BackfillWorkerCount,
 }
 
 func init() {
diff --git a/cmd/beacon-chain/sync/backfill/BUILD.bazel b/cmd/beacon-chain/sync/backfill/BUILD.bazel
index 8ca14bbf81f9..296e64fb9495 100644
--- a/cmd/beacon-chain/sync/backfill/BUILD.bazel
+++ b/cmd/beacon-chain/sync/backfill/BUILD.bazel
@@ -8,6 +8,7 @@ go_library(
     deps = [
         "//beacon-chain/node:go_default_library",
         "//beacon-chain/sync/backfill:go_default_library",
+        "//cmd/beacon-chain/sync/backfill/flags:go_default_library",
         "@com_github_urfave_cli_v2//:go_default_library",
     ],
 )
diff --git a/cmd/beacon-chain/sync/backfill/flags/BUILD.bazel b/cmd/beacon-chain/sync/backfill/flags/BUILD.bazel
new file mode 100644
index 000000000000..170747dddc82
--- /dev/null
+++ b/cmd/beacon-chain/sync/backfill/flags/BUILD.bazel
@@ -0,0 +1,9 @@
+load("@prysm//tools/go:def.bzl", "go_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["flags.go"],
+    importpath = "github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/backfill/flags",
+    visibility = ["//visibility:public"],
+    deps = ["@com_github_urfave_cli_v2//:go_default_library"],
+)
diff --git a/cmd/beacon-chain/sync/backfill/flags/flags.go b/cmd/beacon-chain/sync/backfill/flags/flags.go
new file mode 100644
index 000000000000..0d7b9d291dc2
--- /dev/null
+++ b/cmd/beacon-chain/sync/backfill/flags/flags.go
@@ -0,0 +1,38 @@
+package flags
+
+import (
+	"github.com/urfave/cli/v2"
+)
+
+var (
+	backfillBatchSizeName   = "backfill-batch-size"
+	backfillWorkerCountName = "backfill-worker-count"
+
+	// EnableExperimentalBackfill enables backfill for checkpoint synced nodes.
+	// This flag will be removed onced backfill is enabled by default.
+	EnableExperimentalBackfill = &cli.BoolFlag{
+		Name: "enable-experimental-backfill",
+		Usage: "Backfill is still experimental at this time." +
+			"It will only be enabled if this flag is specified and the node was started using checkpoint sync.",
+	}
+	// BackfillBatchSize allows users to tune block backfill request sizes to maximize network utilization
+	// at the cost of higher memory.
+	BackfillBatchSize = &cli.Uint64Flag{
+		Name: backfillBatchSizeName,
+		Usage: "Number of blocks per backfill batch. " +
+			"A larger number will request more blocks at once from peers, but also consume more system memory to " +
+			"hold batches in memory during processing. This has a multiplicative effect with " + backfillWorkerCountName,
+		Value: 64,
+	}
+	// BackfillWorkerCount allows users to tune the number of concurrent backfill batches to download, to maximize
+	// network utilization at the cost of higher memory.
+	BackfillWorkerCount = &cli.IntFlag{
+		Name: backfillWorkerCountName,
+		Usage: "Number of concurrent backfill batch requests. " +
+			"A larger number will better utilize network resources, up to a system-dependent limit, but will also " +
+			"consume more system memory to hold batches in memory during processing. Multiply by backfill-batch-size and " +
+			"average block size (~2MB before deneb) to find the right number for your system. " +
+			"This has a multiplicatice effect with " + backfillBatchSizeName,
+		Value: 2,
+	}
+)
diff --git a/cmd/beacon-chain/sync/backfill/options.go b/cmd/beacon-chain/sync/backfill/options.go
index bd39d65e6acc..1d15ff58a19d 100644
--- a/cmd/beacon-chain/sync/backfill/options.go
+++ b/cmd/beacon-chain/sync/backfill/options.go
@@ -3,49 +3,18 @@ package backfill
 import (
 	"github.com/prysmaticlabs/prysm/v4/beacon-chain/node"
 	"github.com/prysmaticlabs/prysm/v4/beacon-chain/sync/backfill"
+	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/backfill/flags"
 	"github.com/urfave/cli/v2"
 )
 
-var (
-	backfillBatchSizeName   = "backfill-batch-size"
-	backfillWorkerCountName = "backfill-worker-count"
-	// EnableExperimentalBackfill enables backfill for checkpoint synced nodes.
-	// This flag will be removed onced backfill is enabled by default.
-	EnableExperimentalBackfill = &cli.BoolFlag{
-		Name: "enable-experimental-backfill",
-		Usage: "Backfill is still experimental at this time." +
-			"It will only be enabled if this flag is specified and the node was started using checkpoint sync.",
-	}
-	// BackfillBatchSize allows users to tune block backfill request sizes to maximize network utilization
-	// at the cost of higher memory.
-	BackfillBatchSize = &cli.Uint64Flag{
-		Name: backfillBatchSizeName,
-		Usage: "Number of blocks per backfill batch. " +
-			"A larger number will request more blocks at once from peers, but also consume more system memory to " +
-			"hold batches in memory during processing. This has a multiplicative effect with " + backfillWorkerCountName,
-		Value: 64,
-	}
-	// BackfillWorkerCount allows users to tune the number of concurrent backfill batches to download, to maximize
-	// network utilization at the cost of higher memory.
-	BackfillWorkerCount = &cli.IntFlag{
-		Name: backfillWorkerCountName,
-		Usage: "Number of concurrent backfill batch requests. " +
-			"A larger number will better utilize network resources, up to a system-dependent limit, but will also " +
-			"consume more system memory to hold batches in memory during processing. Multiply by backfill-batch-size and " +
-			"average block size (~2MB before deneb) to find the right number for your system. " +
-			"This has a multiplicatice effect with " + backfillBatchSizeName,
-		Value: 2,
-	}
-)
-
 // BeaconNodeOptions sets the appropriate functional opts on the *node.BeaconNode value, to decouple options
 // from flag parsing.
 func BeaconNodeOptions(c *cli.Context) ([]node.Option, error) {
 	opt := func(node *node.BeaconNode) (err error) {
 		node.BackfillOpts = []backfill.ServiceOption{
-			backfill.WithBatchSize(c.Uint64(BackfillBatchSize.Name)),
-			backfill.WithWorkerCount(c.Int(BackfillWorkerCount.Name)),
-			backfill.WithEnableBackfill(c.Bool(EnableExperimentalBackfill.Name)),
+			backfill.WithBatchSize(c.Uint64(flags.BackfillBatchSize.Name)),
+			backfill.WithWorkerCount(c.Int(flags.BackfillWorkerCount.Name)),
+			backfill.WithEnableBackfill(c.Bool(flags.EnableExperimentalBackfill.Name)),
 		}
 		return nil
 	}
diff --git a/cmd/beacon-chain/usage.go b/cmd/beacon-chain/usage.go
index 186caf14c9da..4248762d19df 100644
--- a/cmd/beacon-chain/usage.go
+++ b/cmd/beacon-chain/usage.go
@@ -8,7 +8,7 @@ import (
 	"github.com/prysmaticlabs/prysm/v4/cmd"
 	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/flags"
 	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/storage"
-	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/backfill"
+	backfill "github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/backfill/flags"
 	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/checkpoint"
 	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/genesis"
 	"github.com/prysmaticlabs/prysm/v4/config/features"
diff --git a/config/features/BUILD.bazel b/config/features/BUILD.bazel
index dac0d80a4002..78890487d0cf 100644
--- a/config/features/BUILD.bazel
+++ b/config/features/BUILD.bazel
@@ -12,6 +12,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//cmd:go_default_library",
+        "//cmd/beacon-chain/sync/backfill/flags:go_default_library",
         "//config/params:go_default_library",
         "@com_github_sirupsen_logrus//:go_default_library",
         "@com_github_urfave_cli_v2//:go_default_library",
diff --git a/config/features/flags.go b/config/features/flags.go
index 0327a973cddb..bb53d5b978c0 100644
--- a/config/features/flags.go
+++ b/config/features/flags.go
@@ -3,6 +3,7 @@ package features
 import (
 	"time"
 
+	backfill "github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/sync/backfill/flags"
 	"github.com/urfave/cli/v2"
 )
 
@@ -161,6 +162,7 @@ var devModeFlags = []cli.Flag{
 	enableVerboseSigVerification,
 	EnableEIP4881,
 	enableExperimentalState,
+	backfill.EnableExperimentalBackfill,
 }
 
 // ValidatorFlags contains a list of all the feature flags that apply to the validator client.
diff --git a/testing/endtoend/components/beacon_node.go b/testing/endtoend/components/beacon_node.go
index a895e4e73b04..549618793215 100644
--- a/testing/endtoend/components/beacon_node.go
+++ b/testing/endtoend/components/beacon_node.go
@@ -283,7 +283,7 @@ func (node *BeaconNode) Start(ctx context.Context) error {
 	// on our features or the beacon index is a multiplier of 2 (idea is to split nodes
 	// equally down the line with one group having feature flags and the other without
 	// feature flags; this is to allow A-B testing on new features)
-	if !config.TestFeature || index%2 == 0 {
+	if !config.TestFeature || index != 1 {
 		args = append(args, features.E2EBeaconChainFlags...)
 	}
 	if config.UseBuilder {