Restore disable-peer-scorer flag (#12386)

* Revert "Make Peer Scorer Permanent Default (#12138)"

This reverts commit 4d28d69.

* make peer scoring flag warning scary

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",

beacon-chain/p2p/peers/peers_test.go

@@ -5,13 +5,19 @@ import (
 	"testing"
 
 	"github.com/prysmaticlabs/prysm/v4/cmd/beacon-chain/flags"
+	"github.com/prysmaticlabs/prysm/v4/config/features"
 	"github.com/sirupsen/logrus"
 )
 
 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,

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",

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)

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) {

beacon-chain/p2p/peers/scorers/scorers_test.go

@@ -7,13 +7,19 @@ 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"
 )
 
 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,

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
 }

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()

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
 	}
 }
 

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",

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,

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