From cfa64ae0139aaa0304dd80cee713b90b6a4ff59c Mon Sep 17 00:00:00 2001 From: Preston Van Loon Date: Tue, 23 May 2023 08:53:02 -0500 Subject: [PATCH] Restore disable-peer-scorer flag (#12386) * Revert "Make Peer Scorer Permanent Default (#12138)" This reverts commit 4d28d69fd9ce6715235aeb4029ed4e2ee3c34b57. * make peer scoring flag warning scary --- beacon-chain/p2p/peers/BUILD.bazel | 2 + beacon-chain/p2p/peers/peers_test.go | 6 + beacon-chain/p2p/peers/scorers/BUILD.bazel | 2 + .../p2p/peers/scorers/block_providers.go | 4 + .../p2p/peers/scorers/block_providers_test.go | 11 ++ .../p2p/peers/scorers/scorers_test.go | 6 + beacon-chain/p2p/peers/scorers/service.go | 7 +- beacon-chain/p2p/peers/status.go | 120 ++++++++++++++++++ beacon-chain/p2p/peers/status_test.go | 21 ++- beacon-chain/sync/initial-sync/BUILD.bazel | 1 + .../sync/initial-sync/initial_sync_test.go | 6 + config/features/config.go | 6 + config/features/deprecated_flags.go | 6 - config/features/flags.go | 5 + 14 files changed, 190 insertions(+), 13 deletions(-) diff --git a/beacon-chain/p2p/peers/BUILD.bazel b/beacon-chain/p2p/peers/BUILD.bazel index c2d1fcde34..ce51eb0422 100644 --- a/beacon-chain/p2p/peers/BUILD.bazel +++ b/beacon-chain/p2p/peers/BUILD.bazel @@ -14,6 +14,7 @@ go_library( deps = [ "//beacon-chain/p2p/peers/peerdata:go_default_library", "//beacon-chain/p2p/peers/scorers:go_default_library", + "//config/features:go_default_library", "//config/params:go_default_library", "//consensus-types/primitives:go_default_library", "//crypto/rand:go_default_library", @@ -44,6 +45,7 @@ go_test( "//beacon-chain/p2p/peers/peerdata:go_default_library", "//beacon-chain/p2p/peers/scorers:go_default_library", "//cmd/beacon-chain/flags:go_default_library", + "//config/features:go_default_library", "//config/params:go_default_library", "//consensus-types/primitives:go_default_library", "//consensus-types/wrapper:go_default_library", diff --git a/beacon-chain/p2p/peers/peers_test.go b/beacon-chain/p2p/peers/peers_test.go index af27648818..661e4896b9 100644 --- a/beacon-chain/p2p/peers/peers_test.go +++ b/beacon-chain/p2p/peers/peers_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/flags" + "github.com/prysmaticlabs/prysm/v4/config/features" "github.com/sirupsen/logrus" ) @@ -12,6 +13,11 @@ func TestMain(m *testing.M) { logrus.SetLevel(logrus.DebugLevel) logrus.SetOutput(io.Discard) + resetCfg := features.InitWithReset(&features.Flags{ + EnablePeerScorer: true, + }) + defer resetCfg() + resetFlags := flags.Get() flags.Init(&flags.GlobalFlags{ BlockBatchLimit: 64, diff --git a/beacon-chain/p2p/peers/scorers/BUILD.bazel b/beacon-chain/p2p/peers/scorers/BUILD.bazel index 4ca89a91a3..fd3088af33 100644 --- a/beacon-chain/p2p/peers/scorers/BUILD.bazel +++ b/beacon-chain/p2p/peers/scorers/BUILD.bazel @@ -15,6 +15,7 @@ go_library( "//beacon-chain/p2p/peers/peerdata:go_default_library", "//beacon-chain/p2p/types:go_default_library", "//cmd/beacon-chain/flags:go_default_library", + "//config/features:go_default_library", "//consensus-types/primitives:go_default_library", "//crypto/rand:go_default_library", "//proto/prysm/v1alpha1:go_default_library", @@ -39,6 +40,7 @@ go_test( "//beacon-chain/p2p/peers/peerdata:go_default_library", "//beacon-chain/p2p/types:go_default_library", "//cmd/beacon-chain/flags:go_default_library", + "//config/features:go_default_library", "//consensus-types/primitives:go_default_library", "//crypto/rand:go_default_library", "//proto/prysm/v1alpha1:go_default_library", diff --git a/beacon-chain/p2p/peers/scorers/block_providers.go b/beacon-chain/p2p/peers/scorers/block_providers.go index 4c93c2348f..2c1bf71ac5 100644 --- a/beacon-chain/p2p/peers/scorers/block_providers.go +++ b/beacon-chain/p2p/peers/scorers/block_providers.go @@ -9,6 +9,7 @@ import ( "github.com/libp2p/go-libp2p/core/peer" "github.com/prysmaticlabs/prysm/v4/beacon-chain/p2p/peers/peerdata" "github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/flags" + "github.com/prysmaticlabs/prysm/v4/config/features" "github.com/prysmaticlabs/prysm/v4/crypto/rand" prysmTime "github.com/prysmaticlabs/prysm/v4/time" ) @@ -290,6 +291,9 @@ func (s *BlockProviderScorer) mapScoresAndPeers( func (s *BlockProviderScorer) FormatScorePretty(pid peer.ID) string { s.store.RLock() defer s.store.RUnlock() + if !features.Get().EnablePeerScorer { + return "disabled" + } score := s.score(pid) return fmt.Sprintf("[%0.1f%%, raw: %0.2f, blocks: %d/%d]", (score/s.MaxScore())*100, score, s.processedBlocks(pid), s.config.ProcessedBlocksCap) diff --git a/beacon-chain/p2p/peers/scorers/block_providers_test.go b/beacon-chain/p2p/peers/scorers/block_providers_test.go index c993fbd04f..5343856805 100644 --- a/beacon-chain/p2p/peers/scorers/block_providers_test.go +++ b/beacon-chain/p2p/peers/scorers/block_providers_test.go @@ -11,6 +11,7 @@ import ( "github.com/prysmaticlabs/prysm/v4/beacon-chain/p2p/peers" "github.com/prysmaticlabs/prysm/v4/beacon-chain/p2p/peers/scorers" "github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/flags" + "github.com/prysmaticlabs/prysm/v4/config/features" "github.com/prysmaticlabs/prysm/v4/crypto/rand" "github.com/prysmaticlabs/prysm/v4/testing/assert" "github.com/prysmaticlabs/prysm/v4/time" @@ -459,6 +460,16 @@ func TestScorers_BlockProvider_FormatScorePretty(t *testing.T) { tt.check(scorer) }) } + + t.Run("peer scorer disabled", func(t *testing.T) { + resetCfg := features.InitWithReset(&features.Flags{ + EnablePeerScorer: false, + }) + defer resetCfg() + peerStatuses := peerStatusGen() + scorer := peerStatuses.Scorers().BlockProviderScorer() + assert.Equal(t, "disabled", scorer.FormatScorePretty("peer1")) + }) } func TestScorers_BlockProvider_BadPeerMarking(t *testing.T) { diff --git a/beacon-chain/p2p/peers/scorers/scorers_test.go b/beacon-chain/p2p/peers/scorers/scorers_test.go index f2a74650c4..300fc06a86 100644 --- a/beacon-chain/p2p/peers/scorers/scorers_test.go +++ b/beacon-chain/p2p/peers/scorers/scorers_test.go @@ -7,6 +7,7 @@ import ( "github.com/prysmaticlabs/prysm/v4/beacon-chain/p2p/peers/scorers" "github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/flags" + "github.com/prysmaticlabs/prysm/v4/config/features" "github.com/sirupsen/logrus" ) @@ -14,6 +15,11 @@ func TestMain(m *testing.M) { logrus.SetLevel(logrus.DebugLevel) logrus.SetOutput(io.Discard) + resetCfg := features.InitWithReset(&features.Flags{ + EnablePeerScorer: true, + }) + defer resetCfg() + resetFlags := flags.Get() flags.Init(&flags.GlobalFlags{ BlockBatchLimit: 64, diff --git a/beacon-chain/p2p/peers/scorers/service.go b/beacon-chain/p2p/peers/scorers/service.go index d826803051..be6b92ace8 100644 --- a/beacon-chain/p2p/peers/scorers/service.go +++ b/beacon-chain/p2p/peers/scorers/service.go @@ -7,6 +7,7 @@ import ( "github.com/libp2p/go-libp2p/core/peer" "github.com/prysmaticlabs/prysm/v4/beacon-chain/p2p/peers/peerdata" + "github.com/prysmaticlabs/prysm/v4/config/features" ) var _ Scorer = (*Service)(nil) @@ -137,8 +138,10 @@ func (s *Service) IsBadPeerNoLock(pid peer.ID) bool { if s.scorers.peerStatusScorer.isBadPeer(pid) { return true } - if s.scorers.gossipScorer.isBadPeer(pid) { - return true + if features.Get().EnablePeerScorer { + if s.scorers.gossipScorer.isBadPeer(pid) { + return true + } } return false } diff --git a/beacon-chain/p2p/peers/status.go b/beacon-chain/p2p/peers/status.go index de58f401f3..e989e94ac5 100644 --- a/beacon-chain/p2p/peers/status.go +++ b/beacon-chain/p2p/peers/status.go @@ -36,6 +36,7 @@ import ( "github.com/prysmaticlabs/go-bitfield" "github.com/prysmaticlabs/prysm/v4/beacon-chain/p2p/peers/peerdata" "github.com/prysmaticlabs/prysm/v4/beacon-chain/p2p/peers/scorers" + "github.com/prysmaticlabs/prysm/v4/config/features" "github.com/prysmaticlabs/prysm/v4/config/params" "github.com/prysmaticlabs/prysm/v4/consensus-types/primitives" "github.com/prysmaticlabs/prysm/v4/crypto/rand" @@ -543,6 +544,11 @@ func (p *Status) Prune() { p.store.Lock() defer p.store.Unlock() + // Default to old method if flag isnt enabled. + if !features.Get().EnablePeerScorer { + p.deprecatedPrune() + return + } // Exit early if there is nothing to prune. if len(p.store.Peers()) <= p.store.Config().MaxPeers { return @@ -587,6 +593,52 @@ func (p *Status) Prune() { p.tallyIPTracker() } +// Deprecated: This is the old peer pruning method based on +// bad response counts. +func (p *Status) deprecatedPrune() { + // Exit early if there is nothing to prune. + if len(p.store.Peers()) <= p.store.Config().MaxPeers { + return + } + + notBadPeer := func(peerData *peerdata.PeerData) bool { + return peerData.BadResponses < p.scorers.BadResponsesScorer().Params().Threshold + } + type peerResp struct { + pid peer.ID + badResp int + } + peersToPrune := make([]*peerResp, 0) + // Select disconnected peers with a smaller bad response count. + for pid, peerData := range p.store.Peers() { + if peerData.ConnState == PeerDisconnected && notBadPeer(peerData) { + peersToPrune = append(peersToPrune, &peerResp{ + pid: pid, + badResp: peerData.BadResponses, + }) + } + } + + // Sort peers in ascending order, so the peers with the + // least amount of bad responses are pruned first. This + // is to protect the node from malicious/lousy peers so + // that their memory is still kept. + sort.Slice(peersToPrune, func(i, j int) bool { + return peersToPrune[i].badResp < peersToPrune[j].badResp + }) + + limitDiff := len(p.store.Peers()) - p.store.Config().MaxPeers + if limitDiff > len(peersToPrune) { + limitDiff = len(peersToPrune) + } + peersToPrune = peersToPrune[:limitDiff] + // Delete peers from map. + for _, peerData := range peersToPrune { + p.store.DeletePeerData(peerData.pid) + } + p.tallyIPTracker() +} + // BestFinalized returns the highest finalized epoch equal to or higher than ours that is agreed // upon by the majority of peers. This method may not return the absolute highest finalized, but // the finalized epoch in which most peers can serve blocks (plurality voting). @@ -694,6 +746,9 @@ func (p *Status) BestNonFinalized(minPeers int, ourHeadEpoch primitives.Epoch) ( // bad response count. In the future scoring will be used // to determine the most suitable peers to take out. func (p *Status) PeersToPrune() []peer.ID { + if !features.Get().EnablePeerScorer { + return p.deprecatedPeersToPrune() + } connLimit := p.ConnectedPeerLimit() inBoundLimit := uint64(p.InboundLimit()) activePeers := p.Active() @@ -757,6 +812,71 @@ func (p *Status) PeersToPrune() []peer.ID { return ids } +// Deprecated: Is used to represent the older method +// of pruning which utilized bad response counts. +func (p *Status) deprecatedPeersToPrune() []peer.ID { + connLimit := p.ConnectedPeerLimit() + inBoundLimit := p.InboundLimit() + activePeers := p.Active() + numInboundPeers := len(p.InboundConnected()) + // Exit early if we are still below our max + // limit. + if uint64(len(activePeers)) <= connLimit { + return []peer.ID{} + } + p.store.Lock() + defer p.store.Unlock() + + type peerResp struct { + pid peer.ID + badResp int + } + peersToPrune := make([]*peerResp, 0) + // Select connected and inbound peers to prune. + for pid, peerData := range p.store.Peers() { + if peerData.ConnState == PeerConnected && + peerData.Direction == network.DirInbound { + peersToPrune = append(peersToPrune, &peerResp{ + pid: pid, + badResp: peerData.BadResponses, + }) + } + } + + // Sort in descending order to favour pruning peers with a + // higher bad response count. + sort.Slice(peersToPrune, func(i, j int) bool { + return peersToPrune[i].badResp > peersToPrune[j].badResp + }) + + // Determine amount of peers to prune using our + // max connection limit. + amountToPrune, err := pmath.Sub64(uint64(len(activePeers)), connLimit) + if err != nil { + // This should never happen + log.WithError(err).Error("Failed to determine amount of peers to prune") + return []peer.ID{} + } + // Also check for inbound peers above our limit. + excessInbound := uint64(0) + if numInboundPeers > inBoundLimit { + excessInbound = uint64(numInboundPeers - inBoundLimit) + } + // Prune the largest amount between excess peers and + // excess inbound peers. + if excessInbound > amountToPrune { + amountToPrune = excessInbound + } + if amountToPrune < uint64(len(peersToPrune)) { + peersToPrune = peersToPrune[:amountToPrune] + } + ids := make([]peer.ID, 0, len(peersToPrune)) + for _, pr := range peersToPrune { + ids = append(ids, pr.pid) + } + return ids +} + // HighestEpoch returns the highest epoch reported epoch amongst peers. func (p *Status) HighestEpoch() primitives.Epoch { p.store.RLock() diff --git a/beacon-chain/p2p/peers/status_test.go b/beacon-chain/p2p/peers/status_test.go index ce69ebbf05..72a710356c 100644 --- a/beacon-chain/p2p/peers/status_test.go +++ b/beacon-chain/p2p/peers/status_test.go @@ -15,6 +15,7 @@ import ( "github.com/prysmaticlabs/prysm/v4/beacon-chain/p2p/peers" "github.com/prysmaticlabs/prysm/v4/beacon-chain/p2p/peers/peerdata" "github.com/prysmaticlabs/prysm/v4/beacon-chain/p2p/peers/scorers" + "github.com/prysmaticlabs/prysm/v4/config/features" "github.com/prysmaticlabs/prysm/v4/config/params" "github.com/prysmaticlabs/prysm/v4/consensus-types/primitives" "github.com/prysmaticlabs/prysm/v4/consensus-types/wrapper" @@ -548,6 +549,10 @@ func TestPrune(t *testing.T) { } func TestPeerIPTracker(t *testing.T) { + resetCfg := features.InitWithReset(&features.Flags{ + EnablePeerScorer: false, + }) + defer resetCfg() maxBadResponses := 2 p := peers.NewStatus(context.Background(), &peers.StatusConfig{ PeerLimit: 30, @@ -582,7 +587,7 @@ func TestPeerIPTracker(t *testing.T) { p.Prune() for _, pr := range badPeers { - assert.Equal(t, true, p.IsBad(pr), "peer with good ip is regarded as bad") + assert.Equal(t, false, p.IsBad(pr), "peer with good ip is regarded as bad") } } @@ -686,6 +691,10 @@ func TestAtInboundPeerLimit(t *testing.T) { } func TestPrunePeers(t *testing.T) { + resetCfg := features.InitWithReset(&features.Flags{ + EnablePeerScorer: false, + }) + defer resetCfg() p := peers.NewStatus(context.Background(), &peers.StatusConfig{ PeerLimit: 30, ScorerParams: &scorers.Config{ @@ -736,11 +745,13 @@ func TestPrunePeers(t *testing.T) { } // Ensure it is in the descending order. - currScore := p.Scorers().Score(peersToPrune[0]) + currCount, err := p.Scorers().BadResponsesScorer().Count(peersToPrune[0]) + require.NoError(t, err) for _, pid := range peersToPrune { - score := p.Scorers().BadResponsesScorer().Score(pid) - assert.Equal(t, true, currScore >= score) - currScore = score + count, err := p.Scorers().BadResponsesScorer().Count(pid) + require.NoError(t, err) + assert.Equal(t, true, currCount >= count) + currCount = count } } diff --git a/beacon-chain/sync/initial-sync/BUILD.bazel b/beacon-chain/sync/initial-sync/BUILD.bazel index 6527b5ae62..bbb7929e09 100644 --- a/beacon-chain/sync/initial-sync/BUILD.bazel +++ b/beacon-chain/sync/initial-sync/BUILD.bazel @@ -118,6 +118,7 @@ go_test( "//beacon-chain/startup:go_default_library", "//beacon-chain/sync:go_default_library", "//cmd/beacon-chain/flags:go_default_library", + "//config/features:go_default_library", "//config/params:go_default_library", "//consensus-types/blocks:go_default_library", "//consensus-types/interfaces:go_default_library", diff --git a/beacon-chain/sync/initial-sync/initial_sync_test.go b/beacon-chain/sync/initial-sync/initial_sync_test.go index 85ac72eb73..d50c29e726 100644 --- a/beacon-chain/sync/initial-sync/initial_sync_test.go +++ b/beacon-chain/sync/initial-sync/initial_sync_test.go @@ -20,6 +20,7 @@ import ( "github.com/prysmaticlabs/prysm/v4/beacon-chain/startup" beaconsync "github.com/prysmaticlabs/prysm/v4/beacon-chain/sync" "github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/flags" + "github.com/prysmaticlabs/prysm/v4/config/features" "github.com/prysmaticlabs/prysm/v4/config/params" "github.com/prysmaticlabs/prysm/v4/consensus-types/blocks" "github.com/prysmaticlabs/prysm/v4/consensus-types/primitives" @@ -55,6 +56,11 @@ func TestMain(m *testing.M) { logrus.SetLevel(logrus.DebugLevel) logrus.SetOutput(io.Discard) + resetCfg := features.InitWithReset(&features.Flags{ + EnablePeerScorer: true, + }) + defer resetCfg() + resetFlags := flags.Get() flags.Init(&flags.GlobalFlags{ BlockBatchLimit: 64, diff --git a/config/features/config.go b/config/features/config.go index ee84890478..d4c5bd8483 100644 --- a/config/features/config.go +++ b/config/features/config.go @@ -39,6 +39,7 @@ type Flags struct { // Feature related flags. RemoteSlasherProtection bool // RemoteSlasherProtection utilizes a beacon node with --slasher mode for validator slashing protection. WriteSSZStateTransitions bool // WriteSSZStateTransitions to tmp directory. + EnablePeerScorer bool // EnablePeerScorer enables experimental peer scoring in p2p. DisableReorgLateBlocks bool // DisableReorgLateBlocks disables reorgs of late blocks. WriteWalletPasswordOnWebOnboarding bool // WriteWalletPasswordOnWebOnboarding writes the password to disk after Prysm web signup. EnableDoppelGanger bool // EnableDoppelGanger enables doppelganger protection on startup for the validator. @@ -178,6 +179,11 @@ func ConfigureBeaconChain(ctx *cli.Context) error { logEnabled(disableReorgLateBlocks) cfg.DisableReorgLateBlocks = true } + cfg.EnablePeerScorer = true + if ctx.Bool(disablePeerScorer.Name) { + logDisabled(disablePeerScorer) + cfg.EnablePeerScorer = false + } if ctx.Bool(disableBroadcastSlashingFlag.Name) { logDisabled(disableBroadcastSlashingFlag) cfg.DisableBroadcastSlashings = true diff --git a/config/features/deprecated_flags.go b/config/features/deprecated_flags.go index 0798e80fb9..cc8781d898 100644 --- a/config/features/deprecated_flags.go +++ b/config/features/deprecated_flags.go @@ -12,11 +12,6 @@ var ( Usage: deprecatedUsage, Hidden: true, } - deprecatedDisablePeerScorer = &cli.BoolFlag{ - Name: "disable-peer-scorer", - Usage: deprecatedUsage, - Hidden: true, - } deprecatedDisableVecHTR = &cli.BoolFlag{ Name: "disable-vectorized-htr", Usage: deprecatedUsage, @@ -42,7 +37,6 @@ var ( // Deprecated flags for both the beacon node and validator client. var deprecatedFlags = []cli.Flag{ exampleDeprecatedFeatureFlag, - deprecatedDisablePeerScorer, deprecatedDisableVecHTR, deprecatedEnableReorgLateBlocks, deprecatedDisableGossipBatchAggregation, diff --git a/config/features/flags.go b/config/features/flags.go index b7f09b387f..68c32cd8ed 100644 --- a/config/features/flags.go +++ b/config/features/flags.go @@ -45,6 +45,10 @@ var ( Name: "disable-reorg-late-blocks", Usage: "Disables reorgs of late blocks", } + disablePeerScorer = &cli.BoolFlag{ + Name: "disable-peer-scorer", + Usage: "(Danger): Disables P2P peer scorer. Do NOT use this in production!", + } writeWalletPasswordOnWebOnboarding = &cli.BoolFlag{ Name: "write-wallet-password-on-web-onboarding", Usage: "(Danger): Writes the wallet password to the wallet directory on completing Prysm web onboarding. " + @@ -179,6 +183,7 @@ var BeaconChainFlags = append(deprecatedBeaconFlags, append(deprecatedFlags, []c PraterTestnet, SepoliaTestnet, Mainnet, + disablePeerScorer, disableBroadcastSlashingFlag, enableSlasherFlag, enableHistoricalSpaceRepresentation,