diff --git a/blocksync/pool.go b/blocksync/pool.go index 9747f449d5f..4c4d3ada096 100644 --- a/blocksync/pool.go +++ b/blocksync/pool.go @@ -254,6 +254,9 @@ func (pool *BlockPool) PopRequest() { delete(pool.requesters, pool.height) pool.height++ + // Re-evaluate maxPeerHeight: peers whose pruned base was just beyond the previous pool.height may now be able to contribute + pool.updateMaxPeerHeight() + // Notify the next minBlocksForSingleRequest requesters about new height, so // they can potentially request a block from the second peer. for i := int64(0); i < minBlocksForSingleRequest && i < int64(len(pool.requesters)); i++ { @@ -359,6 +362,16 @@ func (pool *BlockPool) SetPeerRange(peerID p2p.ID, base int64, height int64) { pool.mtx.Lock() defer pool.mtx.Unlock() + // A peer whose own reported base exceeds its own height is structurally impossible and treated as malicious. + if base > height { + pool.Logger.Info("Peer reporting base greater than height", "peer", peerID, "base", base, "height", height) + if _, exists := pool.peers[peerID]; exists { + pool.removePeer(peerID) + } + pool.banPeer(peerID) + return + } + peer := pool.peers[peerID] if peer != nil { if base < peer.base || height < peer.height { @@ -386,9 +399,7 @@ func (pool *BlockPool) SetPeerRange(peerID p2p.ID, base int64, height int64) { pool.sortedPeers = append([]*bpPeer{peer}, pool.sortedPeers...) } - if height > pool.maxPeerHeight { - pool.maxPeerHeight = height - } + pool.updateMaxPeerHeight() } // RemovePeer removes the peer with peerID from the pool. If there's no peer @@ -429,10 +440,16 @@ func (pool *BlockPool) removePeer(peerID p2p.ID) { } } -// If no peers are left, maxPeerHeight is set to 0. +// updateMaxPeerHeight sets maxPeerHeight to the highest height among peers. func (pool *BlockPool) updateMaxPeerHeight() { var max int64 for _, peer := range pool.peers { + if pool.height > 0 && peer.base > pool.height { + // Blocks a malicious peer from poisoning maxPeerHeight with an + // inflated base/height pair no peer can actually serve, which + // would stall IsCaughtUp forever. + continue + } if peer.height > max { max = peer.height } diff --git a/blocksync/pool_test.go b/blocksync/pool_test.go index 3be8b7b1153..1ad962b52b9 100644 --- a/blocksync/pool_test.go +++ b/blocksync/pool_test.go @@ -187,7 +187,8 @@ func TestBlockPoolTimeout(t *testing.T) { // Introduce each peer. go func() { for _, peer := range peers { - pool.SetPeerRange(peer.id, peer.base, peer.height) + // Force uniform base so every peer contributes to maxPeerHeight at pool startup. + pool.SetPeerRange(peer.id, start, peer.height) } }() @@ -497,3 +498,53 @@ func TestBlockPoolMaliciousNodeMaxInt64(t *testing.T) { } } } + +// TestBlockPoolBansPeerWithBaseGreaterThanHeight verifies that a peer whose self-reported base +// exceeds its own height (a structurally impossible state) is banned. +func TestBlockPoolBansPeerWithBaseGreaterThanHeight(t *testing.T) { + requestsCh := make(chan BlockRequest, 10) + errorsCh := make(chan peerError, 10) + + pool := NewBlockPool(1, requestsCh, errorsCh) + pool.SetLogger(log.TestingLogger()) + + badID := p2p.ID("bad") + pool.SetPeerRange(badID, 500, 100) + + require.True(t, pool.IsPeerBanned(badID), "peer reporting base > height must be banned") + require.EqualValues(t, 0, pool.MaxPeerHeight(), "banned peer must not raise maxPeerHeight") +} + +// TestBlockPoolMaxPeerHeightRefreshesOnPopRequest covers: +// 1. A peer whose base is ahead of pool.height must not contribute to maxPeerHeight +// 2. When pool.height advances past a pruned peer's base, maxPeerHeight is re-evaluated. +func TestBlockPoolMaxPeerHeightRefreshesOnPopRequest(t *testing.T) { + requestsCh := make(chan BlockRequest, 10) + errorsCh := make(chan peerError, 10) + + pool := NewBlockPool(10, requestsCh, errorsCh) + pool.SetLogger(log.TestingLogger()) + + // Peer A's range covers pool.height, so it contributes to maxPeerHeight. + pool.SetPeerRange(p2p.ID("A"), 1, 20) + // Peer B is pruned ahead of pool.height and must be excluded until the + // pool advances past its base. + pool.SetPeerRange(p2p.ID("B"), 15, 100) + require.EqualValues(t, 20, pool.MaxPeerHeight(), + "peer B is pruned ahead of pool.height and must not contribute yet") + + // Advance pool.height from 10 to 15 via PopRequest. Install a dummy + // requester at each height so PopRequest has something to pop; the + // requester is never started, so Stop() is a logged no-op. + for h := int64(10); h < 15; h++ { + pool.mtx.Lock() + pool.requesters[h] = newBPRequester(pool, h) + pool.mtx.Unlock() + pool.PopRequest() + } + + // pool.height is now 15, so B (base=15) becomes eligible and must lift + // maxPeerHeight to its advertised height without B re-sending status. + require.EqualValues(t, 100, pool.MaxPeerHeight(), + "peer B must contribute to maxPeerHeight once pool.height reaches its base") +}