From c5135f699535c3a8d958e02f691a679fbd957a6a Mon Sep 17 00:00:00 2001 From: kasey <489222+kasey@users.noreply.github.com> Date: Wed, 20 Aug 2025 18:57:30 -0500 Subject: [PATCH] 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 --- beacon-chain/p2p/BUILD.bazel | 1 - beacon-chain/p2p/discovery_test.go | 6 +- beacon-chain/p2p/fork.go | 103 ++++++++++++++---- beacon-chain/p2p/fork_test.go | 57 +++++++--- beacon-chain/p2p/subnets.go | 2 - .../kasey_reject-mismatched-schedules.md | 2 + config/params/mainnet_config.go | 1 - config/params/network_config.go | 1 - 8 files changed, 131 insertions(+), 42 deletions(-) create mode 100644 changelog/kasey_reject-mismatched-schedules.md diff --git a/beacon-chain/p2p/BUILD.bazel b/beacon-chain/p2p/BUILD.bazel index 8e7c02cf87..a85b3cc647 100644 --- a/beacon-chain/p2p/BUILD.bazel +++ b/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", ], diff --git a/beacon-chain/p2p/discovery_test.go b/beacon-chain/p2p/discovery_test.go index 3d95a97e20..8ee1b43480 100644 --- a/beacon-chain/p2p/discovery_test.go +++ b/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. diff --git a/beacon-chain/p2p/fork.go b/beacon-chain/p2p/fork.go index f3a279b4ce..0d783e1207 100644 --- a/beacon-chain/p2p/fork.go +++ b/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 +} diff --git a/beacon-chain/p2p/fork_test.go b/beacon-chain/p2p/fork_test.go index 4b0f7f1b05..60a25f172d 100644 --- a/beacon-chain/p2p/fork_test.go +++ b/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)) diff --git a/beacon-chain/p2p/subnets.go b/beacon-chain/p2p/subnets.go index 0812d67085..62196d206c 100644 --- a/beacon-chain/p2p/subnets.go +++ b/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 diff --git a/changelog/kasey_reject-mismatched-schedules.md b/changelog/kasey_reject-mismatched-schedules.md new file mode 100644 index 0000000000..dd7ff36e0e --- /dev/null +++ b/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). diff --git a/config/params/mainnet_config.go b/config/params/mainnet_config.go index 15773bb818..ea41c51ecb 100644 --- a/config/params/mainnet_config.go +++ b/config/params/mainnet_config.go @@ -34,7 +34,6 @@ const ( ) var mainnetNetworkConfig = &NetworkConfig{ - ETH2Key: "eth2", AttSubnetKey: "attnets", SyncCommsSubnetKey: "syncnets", CustodyGroupCountKey: "cgc", diff --git a/config/params/network_config.go b/config/params/network_config.go index f03e558709..e852570583 100644 --- a/config/params/network_config.go +++ b/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.