#37148 [BC-Insight] `wantedPeerDials()` branch will never be executed

Submitted on Nov 26th 2024 at 19:59:03 UTC by @br0nz3p1ck4x3 for Attackathon | Ethereum Protocol

• Report ID: #37148

• Report Type: Blockchain/DLT

• Report severity: Insight

• Target: https://github.com/prysmaticlabs/prysm

• Impacts:

  • (Specifications) A bug in specifications with no direct impact on client implementations

Description

Hey Triagers, Ethereum Foundation, Prysm!

We encountered the following during our research and we think it can add value to the Prysm team.

Description

Inside prysm/beacon-chain/p2p/discovery.go::listenForNodes() we can find the following:

func (s *Service) listenForNewNodes() {
//..
			if s.isPeerAtLimit(false /* inbound */) {
				// Pause the main loop for a period to stop looking
				// for new peers.
				log.Trace("Not looking for peers, at peer limit")
				time.Sleep(pollingPeriod)
				continue
			}
			wantedCount := s.wantedPeerDials()
			if wantedCount == 0 {
				log.Trace("Not looking for peers, at peer limit")
				time.Sleep(pollingPeriod)
				continue
			}
//..
}

There is a problem here though in the logic.

To showcase this, we will first explain it with arbitrary values. After reading this, please run the Proof of Concept attached to the report.

Alright.

If we look at isPeerAtLimit(), we see the following:

func (s *Service) isPeerAtLimit(inbound bool) bool {
	numOfConns := len(s.host.Network().Peers())
	maxPeers := int(s.cfg.MaxPeers)
	// If we are measuring the limit for inbound peers
	// we apply the high watermark buffer.
	if inbound {
		maxPeers += highWatermarkBuffer
		maxInbound := s.peers.InboundLimit() + highWatermarkBuffer
		currInbound := len(s.peers.InboundConnected())
		// Exit early if we are at the inbound limit.
		if currInbound >= maxInbound {
			return true
		}
	}
	activePeers := len(s.Peers().Active())
	return activePeers >= maxPeers || numOfConns >= maxPeers
}

Let's take the following arbitrary values that maximize the numbers such that the return statement will return true:

• numOfConns = 9

• maxPeers = 10

• activePeers = 9

These values will evaluate to true in this return statement:

• activePeers >= maxPeers || numOfConns >= maxPeers

Now, we go back to listenForNewNodes():

func (s *Service) listenForNewNodes() {
//..
			if s.isPeerAtLimit(false /* inbound */) {
				// Pause the main loop for a period to stop looking
				// for new peers.
				log.Trace("Not looking for peers, at peer limit")
				time.Sleep(pollingPeriod)
				continue
			}
			wantedCount := s.wantedPeerDials()
			if wantedCount == 0 {
				log.Trace("Not looking for peers, at peer limit")
				time.Sleep(pollingPeriod)
				continue
			}
			//..
}

Now, wantedPeerDials() is called. If we look at the implementation:

func (s *Service) wantedPeerDials() int {
	maxPeers := int(s.cfg.MaxPeers)

	activePeers := len(s.Peers().Active())
	wantedCount := 0
	if maxPeers > activePeers {
		wantedCount = maxPeers - activePeers
	}
	return wantedCount
}

The problem here is the following if-statement:

• if maxPeers > activePeers {

As we saw in isPeerAtLimit(), which was called before wantedPeerDials(), maxPeers will always be bigger than activePeers. If it was not bigger than activePeers, it would enter the if-statement and continue:

func (s *Service) listenForNewNodes() {
//..
			if s.isPeerAtLimit(false /* inbound */) {
				// Pause the main loop for a period to stop looking
				// for new peers.
->				log.Trace("Not looking for peers, at peer limit")
->				time.Sleep(pollingPeriod)
->				continue
			}
			wantedCount := s.wantedPeerDials()
			if wantedCount == 0 {
				log.Trace("Not looking for peers, at peer limit")
				time.Sleep(pollingPeriod)
				continue
			}
			//..
}

Thus, wantedPeerDials() will always return a non-nil value and thus, this if-statement inside listenForNewNodes() will never be executed:

func (s *Service) listenForNewNodes() {
//..
			if s.isPeerAtLimit(false /* inbound */) {
				// Pause the main loop for a period to stop looking
				// for new peers.
				log.Trace("Not looking for peers, at peer limit")
				time.Sleep(pollingPeriod)
				continue
			}
			wantedCount := s.wantedPeerDials()
->			if wantedCount == 0 {
				log.Trace("Not looking for peers, at peer limit")
				time.Sleep(pollingPeriod)
				continue
			}
			//..
}

Proof of Concept

Proof of Concept

• cd prysm/beacon-chain/p2p

• vim discovery_test.go Paste the following test in this file:

func TestPoc(t *testing.T) {
	// - `numOfConns = 9`
	// - `maxPeers = 10`
	// - `activePeers = 9`
	fakePeer := testp2p.NewTestP2P(t)
	s := &Service{
		cfg:       &Config{MaxPeers: 10},
		ipLimiter: leakybucket.NewCollector(ipLimit, ipBurst, 1*time.Second, false),
		peers: peers.NewStatus(context.Background(), &peers.StatusConfig{
			PeerLimit:    10,
			ScorerParams: &scorers.Config{},
		}),
		host: fakePeer.BHost,
	}

	for i := 0; i < 9; i++ {
		_ = addPeer(t, s.peers, peerdata.PeerConnectionState(ethpb.ConnectionState_CONNECTED), false)
	}

	// We will make two cases, one case where there are 9 peers with a limit of 10 maxPeers
	fmt.Print("Current setup: 9 peers with a limit of 10 peers\n")
	isAtLimit := s.isPeerAtLimit(false)
	fmt.Print("Has the limit been hit? ", isAtLimit, "\n")

	wantedPeers := s.wantedPeerDials()
	fmt.Print("Wanted Peers:", wantedPeers, "\n")

	// Add one more peer so we hit the limit
	_ = addPeer(t, s.peers, peerdata.PeerConnectionState(ethpb.ConnectionState_CONNECTED), false)
	fmt.Print("Current setup: 10 peers with a limit of 10 peers\n")
	isAtLimit = s.isPeerAtLimit(false)
	fmt.Print("Has the limit been hit? ", isAtLimit, "\n")

	fmt.Print("This will never execute because the previous `isPeerAtLimit()` call will lead to `continue` and replaying the loop\n")
	wantedPeers = s.wantedPeerDials()
	fmt.Print("Wanted Peers:", wantedPeers, "\n")
}

• Run using:

  • go test -timeout 30s -run ^TestPoc

The output will be:

Current setup: 9 peers with a limit of 10 peers
Has the limit been hit? false
Wanted Peers:1
Current setup: 10 peers with a limit of 10 peers
Has the limit been hit? true
This will never execute because the previous `isPeerAtLimit()` call will lead to `continue` and replaying the loop
Wanted Peers:0
PASS
ok  	github.com/prysmaticlabs/prysm/v5/beacon-chain/p2p	0.627s

As we can see, the only way to get 0wanted peers is for activePeers == maxPeers, but as shown, this can never happen because the for-loop inside listenForNewNodes() will return early because of:

			if s.isPeerAtLimit(false /* inbound */) {
				// Pause the main loop for a period to stop looking
				// for new peers.
				log.Trace("Not looking for peers, at peer limit")
				time.Sleep(pollingPeriod)
				continue
			}

Recommended Patch

Consider removing the wantedPeerDials() usage since it can not be invoked right now.