diff --git a/CHANGELOG.md b/CHANGELOG.md index 913f1d1e995..283a22cc728 100644 --- a/CHANGELOG.md +++ b/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)) 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") +} diff --git a/light/detector.go b/light/detector.go index 0429bd1aad7..930644a5a48 100644 --- a/light/detector.go +++ b/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 diff --git a/light/detector_test.go b/light/detector_test.go index d8afacefd89..4afceeaf888 100644 --- a/light/detector_test.go +++ b/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) +}