Fix statesync

.changelog/unreleased/bug-fixes/0016-abc-light-proposer-priorities.md

@@ -0,0 +1,2 @@
+- `[light]` Cross-check proposer priorities in retrieved validator sets
+  ([\#ABC-0016](https://github.com/cometbft/cometbft/security/advisories/GHSA-g5xx-c4hv-9ccc))

.changelog/unreleased/improvements/0016-abc-types-validator-set.md

@@ -0,0 +1,2 @@
+- `[types]` Check that proposer is one of the validators in `ValidateBasic`
+  ([\#ABC-0016](https://github.com/cometbft/cometbft/security/advisories/GHSA-g5xx-c4hv-9ccc))

light/client.go

@@ -384,7 +384,7 @@ func (c *Client) initializeWithTrustOptions(ctx context.Context, options TrustOp
 	}
 
 	// 3) Cross-verify with witnesses to ensure everybody has the same state.
-	if err := c.compareFirstHeaderWithWitnesses(ctx, l.SignedHeader); err != nil {
+	if err := c.compareFirstLightBlockWithWitnesses(ctx, l); err != nil {
 		return err
 	}
 
@@ -1126,9 +1126,9 @@ func (c *Client) findNewPrimary(ctx context.Context, height int64, remove bool)
 	return nil, lastError
 }
 
-// compareFirstHeaderWithWitnesses compares h with all witnesses. If any
+// compareFirstLightBlockWithWitnesses compares light block l with all witnesses. If any
 // witness reports a different header than h, the function returns an error.
-func (c *Client) compareFirstHeaderWithWitnesses(ctx context.Context, h *types.SignedHeader) error {
+func (c *Client) compareFirstLightBlockWithWitnesses(ctx context.Context, l *types.LightBlock) error {
 	compareCtx, cancel := context.WithCancel(ctx)
 	defer cancel()
 
@@ -1141,7 +1141,7 @@ func (c *Client) compareFirstHeaderWithWitnesses(ctx context.Context, h *types.S
 
 	errc := make(chan error, len(c.witnesses))
 	for i, witness := range c.witnesses {
-		go c.compareNewHeaderWithWitness(compareCtx, errc, h, witness, i)
+		go c.compareNewLightBlockWithWitness(compareCtx, errc, l, witness, i)
 	}
 
 	witnessesToRemove := make([]int, 0, len(c.witnesses))
@@ -1153,31 +1153,37 @@ func (c *Client) compareFirstHeaderWithWitnesses(ctx context.Context, h *types.S
 		switch e := err.(type) {
 		case nil:
 			continue
-		case errConflictingHeaders:
-			c.logger.Error(fmt.Sprintf(`Witness #%d has a different header. Please check primary is correct
-and remove witness. Otherwise, use the different primary`, e.WitnessIndex), "witness", c.witnesses[e.WitnessIndex])
+		case ErrConflictingHeaders:
+			c.logger.Error("Witness reports a conflicting header. "+
+				"Please check if the primary is correct or use a different witness.",
+				"witness", c.witnesses[e.WitnessIndex], "err", err)
 			return err
 		case errBadWitness:
 			// If witness sent us an invalid header, then remove it
-			c.logger.Info("witness sent an invalid light block, removing...",
+			c.logger.Info("Witness sent an invalid light block, removing...",
 				"witness", c.witnesses[e.WitnessIndex],
 				"err", err)
 			witnessesToRemove = append(witnessesToRemove, e.WitnessIndex)
+		case ErrProposerPrioritiesDiverge:
+			c.logger.Error("Witness reports conflicting proposer priorities. "+
+				"Please check if the primary is correct or use a different witness.",
+				"witness", c.witnesses[e.WitnessIndex], "err", err)
+			return err
 		default: // benign errors can be ignored with the exception of context errors
 			if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
 				return err
 			}
 
 			// the witness either didn't respond or didn't have the block. We ignore it.
-			c.logger.Info("error comparing first header with witness. You may want to consider removing the witness",
+			c.logger.Info("Error comparing first header with witness. You may want to consider removing the witness",
 				"err", err)
 		}
 
 	}
 
 	// remove witnesses that have misbehaved
 	if err := c.removeWitnesses(witnessesToRemove); err != nil {
-		c.logger.Error("failed to remove witnesses", "err", err, "witnessesToRemove", witnessesToRemove)
+		c.logger.Error("Failed to remove witnesses", "err", err, "witnessesToRemove", witnessesToRemove)
 	}
 
 	return nil

light/client_test.go

@@ -32,11 +32,13 @@ var (
 	bTime, _ = time.Parse(time.RFC3339, "2006-01-02T15:04:05Z")
 	h1       = keys.GenSignedHeader(chainID, 1, bTime, nil, vals, vals,
 		hash("app_hash"), hash("cons_hash"), hash("results_hash"), 0, len(keys))
-	// 3/3 signed
-	h2 = keys.GenSignedHeaderLastBlockID(chainID, 2, bTime.Add(30*time.Minute), nil, vals, vals,
+	// 3/3 signed.
+	vals2 = vals.CopyIncrementProposerPriority(1)
+	h2    = keys.GenSignedHeaderLastBlockID(chainID, 2, bTime.Add(30*time.Minute), nil, vals2, vals2,
 		hash("app_hash"), hash("cons_hash"), hash("results_hash"), 0, len(keys), types.BlockID{Hash: h1.Hash()})
-	// 3/3 signed
-	h3 = keys.GenSignedHeaderLastBlockID(chainID, 3, bTime.Add(1*time.Hour), nil, vals, vals,
+	// 3/3 signed.
+	vals3 = vals2.CopyIncrementProposerPriority(1)
+	h3    = keys.GenSignedHeaderLastBlockID(chainID, 3, bTime.Add(1*time.Hour), nil, vals3, vals3,
 		hash("app_hash"), hash("cons_hash"), hash("results_hash"), 0, len(keys), types.BlockID{Hash: h2.Hash()})
 	trustPeriod  = 4 * time.Hour
 	trustOptions = light.TrustOptions{
@@ -46,9 +48,9 @@ var (
 	}
 	valSet = map[int64]*types.ValidatorSet{
 		1: vals,
-		2: vals,
-		3: vals,
-		4: vals,
+		2: vals2,
+		3: vals3,
+		4: vals.CopyIncrementProposerPriority(1),
 	}
 	headerSet = map[int64]*types.SignedHeader{
 		1: h1,
@@ -58,7 +60,7 @@ var (
 		3: h3,
 	}
 	l1       = &types.LightBlock{SignedHeader: h1, ValidatorSet: vals}
-	l2       = &types.LightBlock{SignedHeader: h2, ValidatorSet: vals}
+	l2       = &types.LightBlock{SignedHeader: h2, ValidatorSet: vals2}
 	fullNode = mockp.New(
 		chainID,
 		headerSet,
@@ -913,13 +915,13 @@ func TestClientRemovesWitnessIfItSendsUsIncorrectHeader(t *testing.T) {
 		chainID,
 		map[int64]*types.SignedHeader{
 			1: h1,
-			2: keys.GenSignedHeaderLastBlockID(chainID, 2, bTime.Add(30*time.Minute), nil, vals, vals,
+			2: keys.GenSignedHeaderLastBlockID(chainID, 2, bTime.Add(30*time.Minute), nil, vals2, vals2,
 				hash("app_hash2"), hash("cons_hash"), hash("results_hash"),
 				len(keys), len(keys), types.BlockID{Hash: h1.Hash()}),
 		},
 		map[int64]*types.ValidatorSet{
 			1: vals,
-			2: vals,
+			2: vals2,
 		},
 	)
 	// header is empty
@@ -931,7 +933,7 @@ func TestClientRemovesWitnessIfItSendsUsIncorrectHeader(t *testing.T) {
 		},
 		map[int64]*types.ValidatorSet{
 			1: vals,
-			2: vals,
+			2: vals2,
 		},
 	)
 
@@ -1155,3 +1157,56 @@ func TestClientHandlesContexts(t *testing.T) {
 	require.Error(t, err)
 	require.True(t, errors.Is(err, context.Canceled))
 }
+
+// TestClientErrorsDifferentProposerPriorities tests the case where the witness
+// sends us a light block with a validator set with different proposer priorities.
+func TestClientErrorsDifferentProposerPriorities(t *testing.T) {
+	primary := mockp.New(
+		chainID,
+		map[int64]*types.SignedHeader{
+			1: h1,
+			2: h2,
+		},
+		map[int64]*types.ValidatorSet{
+			1: vals,
+			2: vals2,
+		},
+	)
+	witness := mockp.New(
+		chainID,
+		map[int64]*types.SignedHeader{
+			1: h1,
+			2: h2,
+		},
+		map[int64]*types.ValidatorSet{
+			1: vals,
+			2: vals,
+		},
+	)
+
+	// Proposer priorities in vals and vals2 are different.
+	// This is because vals2 = vals.CopyIncrementProposerPriority(1)
+	require.Equal(t, vals.Hash(), vals2.Hash())
+	require.NotEqual(t, vals.ProposerPriorityHash(), vals2.ProposerPriorityHash())
+
+	c, err := light.NewClient(
+		ctx,
+		chainID,
+		trustOptions,
+		fullNode,
+		[]provider.Provider{primary, witness},
+		dbs.New(dbm.NewMemDB(), chainID),
+		light.Logger(log.TestingLogger()),
+		light.MaxRetryAttempts(1),
+	)
+	// witness should have behaved properly -> no error
+	require.NoError(t, err)
+	assert.EqualValues(t, 2, len(c.Witnesses()))
+
+	// witness behaves incorrectly, but we can't prove who's guilty -> error
+	_, err = c.VerifyLightBlockAtHeight(ctx, 2, bTime.Add(2*time.Hour))
+	require.Error(t, err)
+
+	// witness left in the list
+	assert.EqualValues(t, 2, len(c.Witnesses()))
+}

light/detector.go

@@ -31,7 +31,8 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig
 	}
 	var (
 		headerMatched      bool
-		lastVerifiedHeader = primaryTrace[len(primaryTrace)-1].SignedHeader
+		lastVerifiedBlock  = primaryTrace[len(primaryTrace)-1]
+		lastVerifiedHeader = lastVerifiedBlock.SignedHeader
 		witnessesToRemove  = make([]int, 0)
 	)
 	c.logger.Debug("Running detector against trace", "finalizeBlockHeight", lastVerifiedHeader.Height,
@@ -48,7 +49,7 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig
 	// and compare it with the header from the primary
 	errc := make(chan error, len(c.witnesses))
 	for i, witness := range c.witnesses {
-		go c.compareNewHeaderWithWitness(ctx, errc, lastVerifiedHeader, witness, i)
+		go c.compareNewLightBlockWithWitness(ctx, errc, lastVerifiedBlock, witness, i)
 	}
 
 	// handle errors from the header comparisons as they come in
@@ -58,7 +59,7 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig
 		switch e := err.(type) {
 		case nil: // at least one header matched
 			headerMatched = true
-		case errConflictingHeaders:
+		case ErrConflictingHeaders:
 			// We have conflicting headers. This could possibly imply an attack on the light client.
 			// First we need to verify the witness's header using the same skipping verification and then we
 			// need to find the point that the headers diverge and examine this for any evidence of an attack.
@@ -79,6 +80,10 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig
 			c.logger.Info("witness returned an error during header comparison, removing...",
 				"witness", c.witnesses[e.WitnessIndex], "err", err)
 			witnessesToRemove = append(witnessesToRemove, e.WitnessIndex)
+		case ErrProposerPrioritiesDiverge:
+			c.logger.Info("witness reported validator set with different proposer priorities",
+				"witness", c.witnesses[e.WitnessIndex], "err", err)
+			return e
 		default:
 			// Benign errors which can be ignored unless there was a context
 			// canceled
@@ -104,17 +109,19 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig
 	return ErrFailedHeaderCrossReferencing
 }
 
-// compareNewHeaderWithWitness takes the verified header from the primary and compares it with a
+// compareNewLightBlockWithWitness takes the verified header from the primary and compares it with a
 // header from a specified witness. The function can return one of three errors:
 //
-// 1: errConflictingHeaders -> there may have been an attack on this light client
+// 1: ErrConflictingHeaders -> there may have been an attack on this light client
 // 2: errBadWitness -> the witness has either not responded, doesn't have the header or has given us an invalid one
 //
 //	Note: In the case of an invalid header we remove the witness
 //
 // 3: nil -> the hashes of the two headers match
-func (c *Client) compareNewHeaderWithWitness(ctx context.Context, errc chan error, h *types.SignedHeader,
-	witness provider.Provider, witnessIndex int) {
+func (c *Client) compareNewLightBlockWithWitness(ctx context.Context, errc chan error, l *types.LightBlock,
+	witness provider.Provider, witnessIndex int,
+) {
+	h := l.SignedHeader
 
 	lightBlock, err := witness.LightBlock(ctx, h.Height)
 	switch err {
@@ -150,7 +157,7 @@ func (c *Client) compareNewHeaderWithWitness(ctx context.Context, errc chan erro
 		// witness' last header is below the primary's header. We check the times to see if the blocks
 		// have conflicting times
 		if !lightBlock.Time.Before(h.Time) {
-			errc <- errConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex}
+			errc <- ErrConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex}
 			return
 		}
 
@@ -175,7 +182,7 @@ func (c *Client) compareNewHeaderWithWitness(ctx context.Context, errc chan erro
 		// the witness still doesn't have a block at the height of the primary.
 		// Check if there is a conflicting time
 		if !lightBlock.Time.Before(h.Time) {
-			errc <- errConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex}
+			errc <- ErrConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex}
 			return
 		}
 
@@ -197,7 +204,13 @@ func (c *Client) compareNewHeaderWithWitness(ctx context.Context, errc chan erro
 	}
 
 	if !bytes.Equal(h.Hash(), lightBlock.Hash()) {
-		errc <- errConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex}
+		errc <- ErrConflictingHeaders{Block: lightBlock, WitnessIndex: witnessIndex}
+	}
+
+	// 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}
 	}
 
 	c.logger.Debug("Matching header received by witness", "height", h.Height, "witness", witnessIndex)
@@ -245,7 +258,7 @@ func (c *Client) handleConflictingHeaders(
 	if primaryBlock.Commit.Round != witnessTrace[len(witnessTrace)-1].Commit.Round {
 		c.logger.Info("The light client has detected, and prevented, an attempted amnesia attack." +
 			" We think this attack is pretty unlikely, so if you see it, that's interesting to us." +
-			" Can you let us know by opening an issue through https://github.com/tendermint/tendermint/issues/new?")
+			" Can you let us know by opening an issue through https://github.com/cometbft/cometbft/issues/new?")
 	}
 
 	// This may not be valid because the witness itself is at fault. So now we reverse it, examining the

light/errors.go

@@ -75,20 +75,36 @@ var ErrLightClientAttack = errors.New(`attempted attack detected.
 // continue running the light client.
 var ErrNoWitnesses = errors.New("no witnesses connected. please reset light client")
 
-// ----------------------------- INTERNAL ERRORS ---------------------------------
-
 // ErrConflictingHeaders is thrown when two conflicting headers are discovered.
-type errConflictingHeaders struct {
+type ErrConflictingHeaders struct {
 	Block        *types.LightBlock
 	WitnessIndex int
 }
 
-func (e errConflictingHeaders) Error() string {
+func (e ErrConflictingHeaders) Error() string {
 	return fmt.Sprintf(
 		"header hash (%X) from witness (%d) does not match primary",
 		e.Block.Hash(), e.WitnessIndex)
 }
 
+// ErrProposerPrioritiesDiverge is thrown when two conflicting headers are
+// discovered, but the error is non-attributable comparing to ErrConflictingHeaders.
+// The difference is in validator set proposer priorities, which may change
+// with every round of consensus.
+type ErrProposerPrioritiesDiverge struct {
+	WitnessHash  []byte
+	WitnessIndex int
+	PrimaryHash  []byte
+}
+
+func (e ErrProposerPrioritiesDiverge) Error() string {
+	return fmt.Sprintf(
+		"validator set's proposer priority hashes do not match: witness[%d]=%X, primary=%X",
+		e.WitnessIndex, e.WitnessHash, e.PrimaryHash)
+}
+
+// ----------------------------- INTERNAL ERRORS ---------------------------------
+
 // errBadWitness is returned when the witness either does not respond or
 // responds with an invalid header.
 type errBadWitness struct {