enforce schedule alignment when next_fork_epoch matches (#15604)

* enforce schedule alignment when next_fork_epoch matches

* lint & typo

* James feedback

---------

Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com>

beacon-chain/p2p/BUILD.bazel

@@ -195,7 +195,6 @@ go_test(
         "@com_github_multiformats_go_multiaddr//:go_default_library",
         "@com_github_pkg_errors//:go_default_library",
         "@com_github_prysmaticlabs_go_bitfield//:go_default_library",
-        "@com_github_sirupsen_logrus//:go_default_library",
         "@com_github_sirupsen_logrus//hooks/test:go_default_library",
         "@org_golang_google_protobuf//proto:go_default_library",
     ],

beacon-chain/p2p/discovery_test.go

@@ -89,7 +89,7 @@ func createTestNodeWithID(t *testing.T, id string) *enode.LocalNode {
 	localNode.SetStaticIP(net.ParseIP("127.0.0.1"))
 	localNode.Set(enr.TCP(3000))
 	localNode.Set(enr.UDP(3000))
-	localNode.Set(enr.WithEntry(eth2ENRKey, make([]byte, 16)))
+	localNode.Set(enr.WithEntry(eth2EnrKey, make([]byte, 16)))
 
 	return localNode
 }
@@ -108,7 +108,7 @@ func createTestNodeRandom(t *testing.T) *enode.LocalNode {
 	localNode.SetStaticIP(net.ParseIP("127.0.0.1"))
 	localNode.Set(enr.TCP(3000))
 	localNode.Set(enr.UDP(3000))
-	localNode.Set(enr.WithEntry(eth2ENRKey, make([]byte, 16)))
+	localNode.Set(enr.WithEntry(eth2EnrKey, make([]byte, 16)))
 
 	return localNode
 }
@@ -318,7 +318,7 @@ func TestCreateLocalNode(t *testing.T) {
 
 			// Check fork is set.
 			fork := new([]byte)
-			require.NoError(t, localNode.Node().Record().Load(enr.WithEntry(eth2ENRKey, fork)))
+			require.NoError(t, localNode.Node().Record().Load(enr.WithEntry(eth2EnrKey, fork)))
 			require.NotEmpty(t, *fork)
 
 			// Check att subnets.

beacon-chain/p2p/fork.go

@@ -13,10 +13,17 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-var errEth2ENRDigestMismatch = errors.New("fork digest of peer does not match local value")
+var (
+	errForkScheduleMismatch  = errors.New("peer fork schedule incompatible")
+	errCurrentDigestMismatch = errors.Wrap(errForkScheduleMismatch, "current_fork_digest mismatch")
+	errNextVersionMismatch   = errors.Wrap(errForkScheduleMismatch, "next_fork_version mismatch")
+	errNextDigestMismatch    = errors.Wrap(errForkScheduleMismatch, "nfd (next fork digest) mismatch")
+)
 
-// ENR key used for Ethereum consensus-related fork data.
-var eth2ENRKey = params.BeaconNetworkConfig().ETH2Key
+const (
+	eth2EnrKey = "eth2" // The `eth2` ENR entry advertizes the node's view of the fork schedule with an ssz-encoded ENRForkID value.
+	nfdEnrKey  = "nfd"  // The `nfd` ENR entry separately advertizes the "next fork digest" aspect of the fork schedule.
+)
 
 // ForkDigest returns the current fork digest of
 // the node according to the local clock.
@@ -33,44 +40,86 @@ func (s *Service) currentForkDigest() ([4]byte, error) {
 // Compares fork ENRs between an incoming peer's record and our node's
 // local record values for current and next fork version/epoch.
 func compareForkENR(self, peer *enr.Record) error {
-	peerForkENR, err := forkEntry(peer)
+	peerEntry, err := forkEntry(peer)
 	if err != nil {
 		return err
 	}
-	currentForkENR, err := forkEntry(self)
+	selfEntry, err := forkEntry(self)
 	if err != nil {
 		return err
 	}
-	enrString, err := SerializeENR(peer)
+	peerString, err := SerializeENR(peer)
 	if err != nil {
 		return err
 	}
 	// Clients SHOULD connect to peers with current_fork_digest, next_fork_version,
 	// and next_fork_epoch that match local values.
-	if !bytes.Equal(peerForkENR.CurrentForkDigest, currentForkENR.CurrentForkDigest) {
-		return errors.Wrapf(errEth2ENRDigestMismatch,
+	if !bytes.Equal(peerEntry.CurrentForkDigest, selfEntry.CurrentForkDigest) {
+		return errors.Wrapf(errCurrentDigestMismatch,
 			"fork digest of peer with ENR %s: %v, does not match local value: %v",
-			enrString,
-			peerForkENR.CurrentForkDigest,
-			currentForkENR.CurrentForkDigest,
+			peerString,
+			peerEntry.CurrentForkDigest,
+			selfEntry.CurrentForkDigest,
 		)
 	}
+
 	// Clients MAY connect to peers with the same current_fork_version but a
 	// different next_fork_version/next_fork_epoch. Unless ENRForkID is manually
 	// updated to matching prior to the earlier next_fork_epoch of the two clients,
 	// these type of connecting clients will be unable to successfully interact
 	// starting at the earlier next_fork_epoch.
-	if peerForkENR.NextForkEpoch != currentForkENR.NextForkEpoch {
+	if peerEntry.NextForkEpoch != selfEntry.NextForkEpoch {
 		log.WithFields(logrus.Fields{
-			"peerNextForkEpoch": peerForkENR.NextForkEpoch,
-			"peerENR":           enrString,
+			"peerNextForkEpoch":   peerEntry.NextForkEpoch,
+			"peerNextForkVersion": peerEntry.NextForkVersion,
+			"peerENR":             peerString,
 		}).Trace("Peer matches fork digest but has different next fork epoch")
+		// We allow the connection because we have a different view of the next fork epoch. This
+		// could be due to peers that have no upgraded ahead of a fork or BPO schedule change, so
+		// we allow the connection to continue until the fork boundary.
+		return nil
 	}
-	if !bytes.Equal(peerForkENR.NextForkVersion, currentForkENR.NextForkVersion) {
-		log.WithFields(logrus.Fields{
-			"peerNextForkVersion": peerForkENR.NextForkVersion,
-			"peerENR":             enrString,
-		}).Trace("Peer matches fork digest but has different next fork version")
+
+	// Since we agree on the next fork epoch, we require next fork version to also be in agreement.
+	if !bytes.Equal(peerEntry.NextForkVersion, selfEntry.NextForkVersion) {
+		return errors.Wrapf(errNextVersionMismatch,
+			"next fork version of peer with ENR %s: %#x, does not match local value: %#x",
+			peerString, peerEntry.NextForkVersion, selfEntry.NextForkVersion)
+	}
+
+	// Fulu adds the following to the spec:
+	// ---
+	// A new entry is added to the ENR under the key nfd, short for next fork digest. This entry
+	// communicates the digest of the next scheduled fork, regardless of whether it is a regular
+	// or a Blob-Parameters-Only fork. This new entry MUST be added once FULU_FORK_EPOCH is assigned
+	// any value other than FAR_FUTURE_EPOCH. Adding this entry prior to the Fulu fork will not
+	// impact peering as nodes will ignore unknown ENR entries and nfd mismatches do not cause
+	// disconnects.
+	// When discovering and interfacing with peers, nodes MUST evaluate nfd alongside their existing
+	// consideration of the ENRForkID::next_* fields under the eth2 key, to form a more accurate
+	// view of the peer's intended next fork for the purposes of sustained peering. If there is a
+	// mismatch, the node MUST NOT disconnect before the fork boundary, but it MAY disconnect
+	// at/after the fork boundary.
+
+	// Nodes unprepared to follow the Fulu fork will be unaware of nfd entries. However, their
+	// existing comparison of eth2 entries (concretely next_fork_epoch) is sufficient to detect
+	// upcoming divergence.
+	// ---
+
+	// Because this is a new in-bound connection, we lean into the pre-fulu point that clients
+	// MAY connect to peers with the same current_fork_version but a different
+	// next_fork_version/next_fork_epoch, which implies we can chose to not connect to them when these
+	// don't match.
+	//
+	// Given that the next_fork_epoch matches, we will require the next_fork_digest to match.
+	if !params.FuluEnabled() {
+		return nil
+	}
+	peerNFD, selfNFD := nfd(peer), nfd(self)
+	if peerNFD != selfNFD {
+		return errors.Wrapf(errNextDigestMismatch,
+			"next fork digest of peer with ENR %s: %v, does not match local value: %v",
+			peerString, peerNFD, selfNFD)
 	}
 	return nil
 }
@@ -102,7 +151,7 @@ func updateENR(node *enode.LocalNode, entry, next params.NetworkScheduleEntry) e
 	if err != nil {
 		return err
 	}
-	forkEntry := enr.WithEntry(eth2ENRKey, enc)
+	forkEntry := enr.WithEntry(eth2EnrKey, enc)
 	node.Set(forkEntry)
 	return nil
 }
@@ -111,7 +160,7 @@ func updateENR(node *enode.LocalNode, entry, next params.NetworkScheduleEntry) e
 // under the Ethereum consensus EnrKey
 func forkEntry(record *enr.Record) (*pb.ENRForkID, error) {
 	sszEncodedForkEntry := make([]byte, 16)
-	entry := enr.WithEntry(eth2ENRKey, &sszEncodedForkEntry)
+	entry := enr.WithEntry(eth2EnrKey, &sszEncodedForkEntry)
 	err := record.Load(entry)
 	if err != nil {
 		return nil, err
@@ -122,3 +171,15 @@ func forkEntry(record *enr.Record) (*pb.ENRForkID, error) {
 	}
 	return forkEntry, nil
 }
+
+// nfd retrieves the value of the `nfd` ("next fork digest") key from an ENR record.
+func nfd(record *enr.Record) [4]byte {
+	digest := [4]byte{}
+	entry := enr.WithEntry(nfdEnrKey, &digest)
+	if err := record.Load(entry); err != nil {
+		// Treat a missing nfd entry as an empty digest.
+		// We do this to avoid errors when checking peers that have not upgraded for fulu.
+		return [4]byte{}
+	}
+	return digest
+}

beacon-chain/p2p/fork_test.go

@@ -16,14 +16,12 @@ import (
 	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/p2p/enode"
 	"github.com/ethereum/go-ethereum/p2p/enr"
-	"github.com/sirupsen/logrus"
-	logTest "github.com/sirupsen/logrus/hooks/test"
 )
 
 func TestCompareForkENR(t *testing.T) {
 	params.SetupTestConfigCleanup(t)
+	params.BeaconConfig().FuluForkEpoch = params.BeaconConfig().ElectraForkEpoch + 4096
 	params.BeaconConfig().InitializeForkSchedule()
-	logrus.SetLevel(logrus.TraceLevel)
 
 	db, err := enode.OpenDB("")
 	assert.NoError(t, err)
@@ -61,10 +59,10 @@ func TestCompareForkENR(t *testing.T) {
 				require.NoError(t, updateENR(peer, currentCopy, next))
 				return peer.Node()
 			},
-			expectErr: errEth2ENRDigestMismatch,
+			expectErr: errCurrentDigestMismatch,
 		},
 		{
-			name: "next fork version mismatch",
+			name: "next_fork_epoch match, next_fork_version mismatch",
 			node: func(t *testing.T) *enode.Node {
 				// Create a peer with the same current fork digest and next fork version/epoch.
 				peer := enode.NewLocalNode(db, k)
@@ -75,25 +73,44 @@ func TestCompareForkENR(t *testing.T) {
 				require.NoError(t, updateENR(peer, current, nextCopy))
 				return peer.Node()
 			},
-			expectLog: "Peer matches fork digest but has different next fork version",
+			expectErr: errNextVersionMismatch,
 		},
 		{
-			name: "next fork epoch mismatch",
+			name: "next fork epoch mismatch, next fork digest mismatch",
 			node: func(t *testing.T) *enode.Node {
 				// Create a peer with the same current fork digest and next fork version/epoch.
 				peer := enode.NewLocalNode(db, k)
 				nextCopy := next
+				// next epoch does not match, and neither does the next fork digest.
 				nextCopy.Epoch = nextCopy.Epoch + 1
+				nfd := [4]byte{0xFF, 0xFF, 0xFF, 0xFF}
+				require.NotEqual(t, next.ForkDigest, nfd)
+				//peer.Set(enr.WithEntry(nfdEnrKey, nfd[:]))
+				nextCopy.ForkDigest = nfd
 				require.NoError(t, updateENR(peer, current, nextCopy))
 				return peer.Node()
 			},
-			expectLog: "Peer matches fork digest but has different next fork epoch",
+			// no error because we allow a different next fork version / digest if the next fork epoch does not match
+		},
+		{
+			name: "next fork epoch -match-, next fork digest mismatch",
+			node: func(t *testing.T) *enode.Node {
+				peer := enode.NewLocalNode(db, k)
+				nextCopy := next
+				nfd := [4]byte{0xFF, 0xFF, 0xFF, 0xFF}
+				// next epoch *does match*, but the next fork digest doesn't - so we should get an error.
+				require.NotEqual(t, next.ForkDigest, nfd)
+				nextCopy.ForkDigest = nfd
+				//peer.Set(enr.WithEntry(nfdEnrKey, nfd[:]))
+				require.NoError(t, updateENR(peer, current, nextCopy))
+				return peer.Node()
+			},
+			expectErr: errNextDigestMismatch,
 		},
 	}
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			hook := logTest.NewGlobal()
 			peer := c.node(t)
 			err := compareForkENR(self.Node().Record(), peer.Record())
 			if c.expectErr != nil {
@@ -101,13 +118,27 @@ func TestCompareForkENR(t *testing.T) {
 			} else {
 				require.NoError(t, err, "Expected no error comparing fork ENRs")
 			}
-			if c.expectLog != "" {
-				require.LogsContain(t, hook, c.expectLog, "Expected log message not found")
-			}
 		})
 	}
 }
 
+func TestNfdSetAndLoad(t *testing.T) {
+	params.SetupTestConfigCleanup(t)
+	params.BeaconConfig().FuluForkEpoch = params.BeaconConfig().ElectraForkEpoch + 4096
+	params.BeaconConfig().InitializeForkSchedule()
+	db, err := enode.OpenDB("")
+	assert.NoError(t, err)
+	_, k := createAddrAndPrivKey(t)
+	clock := startup.NewClock(time.Now(), params.BeaconConfig().GenesisValidatorsRoot)
+	current := params.GetNetworkScheduleEntry(clock.CurrentEpoch())
+	next := params.NextNetworkScheduleEntry(clock.CurrentEpoch())
+	next.ForkDigest = [4]byte{0xFF, 0xFF, 0xFF, 0xFF} // Ensure a unique digest for testing.
+	self := enode.NewLocalNode(db, k)
+	require.NoError(t, updateENR(self, current, next))
+	n := nfd(self.Node().Record())
+	assert.Equal(t, next.ForkDigest, n, "Expected nfd to match next fork digest")
+}
+
 func TestDiscv5_AddRetrieveForkEntryENR(t *testing.T) {
 	params.SetupTestConfigCleanup(t)
 	params.BeaconConfig().InitializeForkSchedule()
@@ -122,7 +153,7 @@ func TestDiscv5_AddRetrieveForkEntryENR(t *testing.T) {
 	}
 	enc, err := enrForkID.MarshalSSZ()
 	require.NoError(t, err)
-	entry := enr.WithEntry(eth2ENRKey, enc)
+	entry := enr.WithEntry(eth2EnrKey, enc)
 	temp := t.TempDir()
 	randNum := rand.Int()
 	tempPath := path.Join(temp, strconv.Itoa(randNum))

beacon-chain/p2p/subnets.go

@@ -27,8 +27,6 @@ import (
 	"github.com/prysmaticlabs/go-bitfield"
 )
 
-const nfdEnrKey = "nfd" // The ENR record key for "nfd" (Next Fork Digest).
-
 var (
 	attestationSubnetCount = params.BeaconConfig().AttestationSubnetCount
 	syncCommsSubnetCount   = params.BeaconConfig().SyncCommitteeSubnetCount

changelog/kasey_reject-mismatched-schedules.md

@@ -0,0 +1,2 @@
+### Changed
+- Reject incoming connections when the fork schedule of the connecting peer (parsed from their ENR) has a matching next_fork_epoch, but mismatched next_fork_version or nfd (next fork digest).

config/params/mainnet_config.go

@@ -34,7 +34,6 @@ const (
 )
 
 var mainnetNetworkConfig = &NetworkConfig{
-	ETH2Key:                    "eth2",
 	AttSubnetKey:               "attnets",
 	SyncCommsSubnetKey:         "syncnets",
 	CustodyGroupCountKey:       "cgc",

config/params/network_config.go

@@ -8,7 +8,6 @@ import (
 // NetworkConfig defines the spec based network parameters.
 type NetworkConfig struct {
 	// DiscoveryV5 Config
-	ETH2Key                    string // ETH2Key is the ENR key of the Ethereum consensus object.
 	AttSubnetKey               string // AttSubnetKey is the ENR key of the subnet bitfield.
 	SyncCommsSubnetKey         string // SyncCommsSubnetKey is the ENR key of the sync committee subnet bitfield.
 	CustodyGroupCountKey       string // CustodyGroupsCountKey is the ENR key of the custody group count.