Make Peer Scorer Permanent Default (#12138)

* add changes

* gaz

* fix

beacon-chain/p2p/peers/BUILD.bazel

@@ -14,7 +14,6 @@ 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",
@@ -45,7 +44,6 @@ 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,19 +5,13 @@ import (
 	"testing"
 
 	"github.com/prysmaticlabs/prysm/v3/cmd/beacon-chain/flags"
-	"github.com/prysmaticlabs/prysm/v3/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,7 +15,6 @@ 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",
@@ -40,7 +39,6 @@ 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,7 +9,6 @@ import (
 	"github.com/libp2p/go-libp2p/core/peer"
 	"github.com/prysmaticlabs/prysm/v3/beacon-chain/p2p/peers/peerdata"
 	"github.com/prysmaticlabs/prysm/v3/cmd/beacon-chain/flags"
-	"github.com/prysmaticlabs/prysm/v3/config/features"
 	"github.com/prysmaticlabs/prysm/v3/crypto/rand"
 	prysmTime "github.com/prysmaticlabs/prysm/v3/time"
 )
@@ -291,9 +290,6 @@ 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,7 +11,6 @@ import (
 	"github.com/prysmaticlabs/prysm/v3/beacon-chain/p2p/peers"
 	"github.com/prysmaticlabs/prysm/v3/beacon-chain/p2p/peers/scorers"
 	"github.com/prysmaticlabs/prysm/v3/cmd/beacon-chain/flags"
-	"github.com/prysmaticlabs/prysm/v3/config/features"
 	"github.com/prysmaticlabs/prysm/v3/crypto/rand"
 	"github.com/prysmaticlabs/prysm/v3/testing/assert"
 	"github.com/prysmaticlabs/prysm/v3/time"
@@ -460,16 +459,6 @@ 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,19 +7,13 @@ import (
 
 	"github.com/prysmaticlabs/prysm/v3/beacon-chain/p2p/peers/scorers"
 	"github.com/prysmaticlabs/prysm/v3/cmd/beacon-chain/flags"
-	"github.com/prysmaticlabs/prysm/v3/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,7 +7,6 @@ import (
 
 	"github.com/libp2p/go-libp2p/core/peer"
 	"github.com/prysmaticlabs/prysm/v3/beacon-chain/p2p/peers/peerdata"
-	"github.com/prysmaticlabs/prysm/v3/config/features"
 )
 
 var _ Scorer = (*Service)(nil)
@@ -138,10 +137,8 @@ func (s *Service) IsBadPeerNoLock(pid peer.ID) bool {
 	if s.scorers.peerStatusScorer.isBadPeer(pid) {
 		return true
 	}
-	if features.Get().EnablePeerScorer {
-		if s.scorers.gossipScorer.isBadPeer(pid) {
-			return true
-		}
+	if s.scorers.gossipScorer.isBadPeer(pid) {
+		return true
 	}
 	return false
 }

beacon-chain/p2p/peers/status.go

@@ -36,7 +36,6 @@ import (
 	"github.com/prysmaticlabs/go-bitfield"
 	"github.com/prysmaticlabs/prysm/v3/beacon-chain/p2p/peers/peerdata"
 	"github.com/prysmaticlabs/prysm/v3/beacon-chain/p2p/peers/scorers"
-	"github.com/prysmaticlabs/prysm/v3/config/features"
 	"github.com/prysmaticlabs/prysm/v3/config/params"
 	"github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
 	"github.com/prysmaticlabs/prysm/v3/crypto/rand"
@@ -544,11 +543,6 @@ 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
@@ -593,52 +587,6 @@ 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).
@@ -746,9 +694,6 @@ 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()
@@ -812,71 +757,6 @@ 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,7 +15,6 @@ import (
 	"github.com/prysmaticlabs/prysm/v3/beacon-chain/p2p/peers"
 	"github.com/prysmaticlabs/prysm/v3/beacon-chain/p2p/peers/peerdata"
 	"github.com/prysmaticlabs/prysm/v3/beacon-chain/p2p/peers/scorers"
-	"github.com/prysmaticlabs/prysm/v3/config/features"
 	"github.com/prysmaticlabs/prysm/v3/config/params"
 	"github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
 	"github.com/prysmaticlabs/prysm/v3/consensus-types/wrapper"
@@ -549,10 +548,6 @@ 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,
@@ -587,7 +582,7 @@ func TestPeerIPTracker(t *testing.T) {
 	p.Prune()
 
 	for _, pr := range badPeers {
-		assert.Equal(t, false, p.IsBad(pr), "peer with good ip is regarded as bad")
+		assert.Equal(t, true, p.IsBad(pr), "peer with good ip is regarded as bad")
 	}
 }
 
@@ -691,10 +686,6 @@ 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{
@@ -745,13 +736,11 @@ func TestPrunePeers(t *testing.T) {
 	}
 
 	// Ensure it is in the descending order.
-	currCount, err := p.Scorers().BadResponsesScorer().Count(peersToPrune[0])
-	require.NoError(t, err)
+	currScore := p.Scorers().Score(peersToPrune[0])
 	for _, pid := range peersToPrune {
-		count, err := p.Scorers().BadResponsesScorer().Count(pid)
-		require.NoError(t, err)
-		assert.Equal(t, true, currCount >= count)
-		currCount = count
+		score := p.Scorers().BadResponsesScorer().Score(pid)
+		assert.Equal(t, true, currScore >= score)
+		currScore = score
 	}
 }
 

beacon-chain/sync/initial-sync/BUILD.bazel

@@ -120,7 +120,6 @@ go_test(
         "//beacon-chain/p2p/types: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

@@ -19,7 +19,6 @@ import (
 	p2pTypes "github.com/prysmaticlabs/prysm/v3/beacon-chain/p2p/types"
 	beaconsync "github.com/prysmaticlabs/prysm/v3/beacon-chain/sync"
 	"github.com/prysmaticlabs/prysm/v3/cmd/beacon-chain/flags"
-	"github.com/prysmaticlabs/prysm/v3/config/features"
 	"github.com/prysmaticlabs/prysm/v3/config/params"
 	"github.com/prysmaticlabs/prysm/v3/consensus-types/blocks"
 	"github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
@@ -55,11 +54,6 @@ 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

@@ -43,7 +43,6 @@ 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.
@@ -176,11 +175,7 @@ func ConfigureBeaconChain(ctx *cli.Context) error {
 		logDisabled(disableGRPCConnectionLogging)
 		cfg.DisableGRPCConnectionLogs = true
 	}
-	cfg.EnablePeerScorer = true
-	if ctx.Bool(disablePeerScorer.Name) {
-		logDisabled(disablePeerScorer)
-		cfg.EnablePeerScorer = false
-	}
+
 	cfg.DisableReorgLateBlocks = true
 	if ctx.Bool(enableReorgLateBlocks.Name) {
 		logEnabled(enableReorgLateBlocks)