Backport 5803 5820

CHANGELOG.md

@@ -6,6 +6,12 @@
 
 ### BUG FIXES
 
+- `[evidence]` Add validation for Light Client Attack evidence ByzantineValidators
+  ([\#5638](https://github.com/cometbft/cometbft/pull/5638))
+
+- `[light]`: stop witness comparison after divergence checks
+  ([\#5820](https://github.com/cometbft/cometbft/pull/5820))
+
 ### IMPROVEMENTS
 - `[statesync]` Add configurable `max-snapshot-chunks` parameter to validate max amount of chunks in a `SnapshotResponse`.
   ([\#5549](https://github.com/cometbft/cometbft/pull/5549))

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
 		}

blocksync/pool_test.go

@@ -187,7 +187,8 @@
 	// 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 @@
 		}
 	}
 }
+
+// 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")
+}

light/detector.go

@@ -207,12 +207,14 @@ func (c *Client) compareNewLightBlockWithWitness(ctx context.Context, errc chan
 
 	if !bytes.Equal(h.Hash(), lightBlock.Hash()) {
 		errc <- ErrConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex, peer: peer}
+		return
 	}
 
 	// ProposerPriorityHash is not part of the header hash, so we need to check it separately.
 	wanted, got := l.ValidatorSet.ProposerPriorityHash(), lightBlock.ValidatorSet.ProposerPriorityHash()
 	if !bytes.Equal(wanted, got) {
 		errc <- ErrProposerPrioritiesDiverge{WitnessHash: got, WitnessIndex: witnessIndex, PrimaryHash: wanted}
+		return
 	}
 
 	c.logger.Debug("Matching header received by witness", "height", h.Height, "witness", witnessIndex)
@@ -308,7 +310,6 @@ func (c *Client) examineConflictingHeaderAgainstTrace(
 	targetBlock *types.LightBlock,
 	source provider.Provider, now time.Time,
 ) ([]*types.LightBlock, *types.LightBlock, error) {
-
 	var (
 		previouslyVerifiedBlock, sourceBlock *types.LightBlock
 		sourceTrace                          []*types.LightBlock
@@ -385,7 +386,6 @@ func (c *Client) examineConflictingHeaderAgainstTrace(
 	// prerequisites to this function were not met. Namely that either trace[len(trace)-1].Height < targetBlock.Height
 	// or that trace[i].Hash() != targetBlock.Hash()
 	return nil, nil, errNoDivergence
-
 }
 
 // getTargetBlockOrLatest gets the latest height, if it is greater than the target height then it queries

light/detector_test.go

@@ -1,6 +1,7 @@
 package light_test
 
 import (
+	"errors"
 	"testing"
 	"time"
 
@@ -428,3 +429,122 @@ func TestClientDivergentTraces4(t *testing.T) {
 	assert.Error(t, err)
 	assert.Equal(t, 1, len(c.Witnesses()))
 }
+
+func TestClientDivergentHeadersNoTraces(t *testing.T) {
+	primaryHeaders, primaryVals, primaryKeys := genMockNodeWithKeys(chainID, 10, 5, 2, bTime)
+	primary := mockp.New(chainID, primaryHeaders, primaryVals)
+
+	firstBlock, err := primary.LightBlock(ctx, 1)
+	require.NoError(t, err)
+
+	keysAtH10 := primaryKeys[10]
+	valsAtH10 := primaryVals[10]
+	nextValsAtH11 := primaryKeys[11].ToValidators(2, 0)
+
+	// generate a header with an app hash that diverges from the primary
+	makeDivergentHeader := func(appHashSeed string) *types.SignedHeader {
+		return keysAtH10.GenSignedHeader(
+			chainID, 10,
+			primaryHeaders[10].Time, nil,
+			valsAtH10, nextValsAtH11,
+			hash(appHashSeed), hash("cons_hash"), hash("results_hash"),
+			0, len(keysAtH10),
+		)
+	}
+	divergentHeader1 := makeDivergentHeader("divergent_app_1")
+	divergentHeader2 := makeDivergentHeader("divergent_app_2")
+	require.NotEqual(t, primaryHeaders[10].Hash(), divergentHeader1.Hash())
+	require.NotEqual(t, primaryHeaders[10].Hash(), divergentHeader2.Hash())
+
+	// generate a witness that has this divergent app hash but the same
+	// proposer priority hash
+	makeSparseWitness := func(divergent *types.SignedHeader) *mockp.Mock {
+		return mockp.New(
+			chainID,
+			map[int64]*types.SignedHeader{1: primaryHeaders[1], 10: divergent},
+			map[int64]*types.ValidatorSet{1: primaryVals[1], 10: valsAtH10},
+		)
+	}
+
+	// run for multiple iterations since this is exercising a race, 100
+	// typically causes it
+	const iterations = 100
+	for i := 0; i < iterations; i++ {
+		witness1 := makeSparseWitness(divergentHeader1)
+		witness2 := makeSparseWitness(divergentHeader2)
+
+		c, err := light.NewClient(
+			ctx,
+			chainID,
+			light.TrustOptions{
+				Height: 1,
+				Hash:   firstBlock.Hash(),
+				Period: 4 * time.Hour,
+			},
+			primary,
+			[]provider.Provider{witness1, witness2},
+			dbs.New(dbm.NewMemDB(), chainID),
+			light.Logger(log.TestingLogger()),
+		)
+		require.NoError(t, err)
+
+		// try and validate the block using our two witnesses that have
+		// diverged from the primary, this should error
+		_, err = c.VerifyLightBlockAtHeight(ctx, 10, bTime.Add(1*time.Hour))
+		require.Error(t, err)
+	}
+}
+
+func TestClientDivergentProposerPriorities(t *testing.T) {
+	const (
+		latestHeight = int64(10)
+		valSize      = 5
+	)
+
+	primaryHeaders, primaryValidators, _ := genMockNodeWithKeys(chainID, latestHeight, valSize, 2, bTime)
+	primary := mockp.New(chainID, primaryHeaders, primaryValidators)
+
+	witnessHeaders := make(map[int64]*types.SignedHeader, len(primaryHeaders))
+	witnessValidators := make(map[int64]*types.ValidatorSet, len(primaryValidators))
+	for height, header := range primaryHeaders {
+		witnessHeaders[height] = header
+	}
+	for height, valSet := range primaryValidators {
+		witnessValidators[height] = valSet
+	}
+
+	// Keep header hash identical while changing only proposer priorities in the validator set.
+	witnessValidators[latestHeight] = primaryValidators[latestHeight].CopyIncrementProposerPriority(1)
+	require.Equal(t, primaryHeaders[latestHeight].Hash(), witnessHeaders[latestHeight].Hash())
+	require.Equal(t, primaryValidators[latestHeight].Hash(), witnessValidators[latestHeight].Hash())
+	require.NotEqual(t,
+		primaryValidators[latestHeight].ProposerPriorityHash(),
+		witnessValidators[latestHeight].ProposerPriorityHash(),
+	)
+
+	witness := mockp.New(chainID, witnessHeaders, witnessValidators)
+	c, err := light.NewClient(
+		ctx,
+		chainID,
+		light.TrustOptions{
+			Period: 4 * time.Hour,
+			Height: 1,
+			Hash:   primaryHeaders[1].Hash(),
+		},
+		primary,
+		[]provider.Provider{witness},
+		dbs.New(dbm.NewMemDB(), chainID),
+		light.Logger(log.TestingLogger()),
+		light.MaxRetryAttempts(1),
+	)
+	require.NoError(t, err)
+
+	_, err = c.VerifyLightBlockAtHeight(ctx, latestHeight, bTime.Add(1*time.Hour))
+	require.Error(t, err)
+
+	var divergeErr light.ErrProposerPrioritiesDiverge
+	require.True(t, errors.As(err, &divergeErr), "expected ErrProposerPrioritiesDiverge, got %T: %v", err, err)
+	assert.Equal(t, 0, divergeErr.WitnessIndex)
+	assert.Equal(t, primaryValidators[latestHeight].ProposerPriorityHash(), divergeErr.PrimaryHash)
+	assert.Equal(t, witnessValidators[latestHeight].ProposerPriorityHash(), divergeErr.WitnessHash)
+}