light: minor fixes / standardising errors (#5716)

## Description

I'm just doing a self audit of the light client. There's a few things I've changed

- Validate trust level in `VerifyNonAdjacent` function
- Make errNoWitnesses public (it's something people running software on top of a light client should be able to parse)
- Remove `ChainID` check of witnesses on start up. We do this already when we compare the first header with witnesses
- Remove `ChainID()` from provider interface

Closes: #4538

light/client.go

@@ -219,15 +219,7 @@ func NewClientFromTrustedStore(
 
 	// Validate the number of witnesses.
 	if len(c.witnesses) < 1 {
-		return nil, errNoWitnesses{}
-	}
-
-	// Verify witnesses are all on the same chain.
-	for i, w := range witnesses {
-		if w.ChainID() != chainID {
-			return nil, fmt.Errorf("witness #%d: %v is on another chain %s, expected %s",
-				i, w, w.ChainID(), chainID)
-		}
+		return nil, ErrNoWitnesses
 	}
 
 	// Validate trust level.
@@ -455,7 +447,7 @@ func (c *Client) VerifyLightBlockAtHeight(ctx context.Context, height int64, now
 		return nil, errors.New("negative or zero height")
 	}
 
-	// Check if the light block already verified.
+	// Check if the light block is already verified.
 	h, err := c.TrustedLightBlock(height)
 	if err == nil {
 		c.logger.Info("Header has already been verified", "height", height, "hash", hash2str(h.Hash()))
@@ -634,7 +626,7 @@ func (c *Client) verifySequential(
 
 				// If some intermediate header is invalid, replace the primary and try
 				// again.
-				c.logger.Error("primary sent invalid header -> replacing", "err", err)
+				c.logger.Error("primary sent invalid header -> replacing", "err", err, "primary", c.primary)
 				replaceErr := c.replacePrimaryProvider()
 				if replaceErr != nil {
 					c.logger.Error("Can't replace primary", "err", replaceErr)
@@ -768,7 +760,7 @@ func (c *Client) verifySkippingAgainstPrimary(
 
 		// If some intermediate header is invalid, replace the primary and try
 		// again.
-		c.logger.Error("primary sent invalid header -> replacing", "err", err)
+		c.logger.Error("primary sent invalid header -> replacing", "err", err, "primary", c.primary)
 		replaceErr := c.replacePrimaryProvider()
 		if replaceErr != nil {
 			c.logger.Error("Can't replace primary", "err", replaceErr)
@@ -853,7 +845,7 @@ func (c *Client) Witnesses() []provider.Provider {
 // Cleanup removes all the data (headers and validator sets) stored. Note: the
 // client must be stopped at this point.
 func (c *Client) Cleanup() error {
-	c.logger.Info("Removing all the data")
+	c.logger.Info("Removing all light blocks")
 	c.latestTrustedBlock = nil
 	return c.trustedStore.Prune(0)
 }
@@ -932,7 +924,7 @@ func (c *Client) backwards(
 			"newHeight", interimHeader.Height,
 			"newHash", hash2str(interimHeader.Hash()))
 		if err := VerifyBackwards(interimHeader, verifiedHeader); err != nil {
-			c.logger.Error("primary sent invalid header -> replacing", "err", err)
+			c.logger.Error("primary sent invalid header -> replacing", "err", err, "primary", c.primary)
 			if replaceErr := c.replacePrimaryProvider(); replaceErr != nil {
 				c.logger.Error("Can't replace primary", "err", replaceErr)
 				// return original error
@@ -968,7 +960,7 @@ func (c *Client) replacePrimaryProvider() error {
 	defer c.providerMutex.Unlock()
 
 	if len(c.witnesses) <= 1 {
-		return errNoWitnesses{}
+		return ErrNoWitnesses
 	}
 	c.primary = c.witnesses[0]
 	c.witnesses = c.witnesses[1:]
@@ -985,7 +977,7 @@ func (c *Client) lightBlockFromPrimary(ctx context.Context, height int64) (*type
 	l, err := c.primary.LightBlock(ctx, height)
 	c.providerMutex.Unlock()
 	if err != nil {
-		c.logger.Debug("Error on light block request from primary", "error", err)
+		c.logger.Debug("Error on light block request from primary", "error", err, "primary", c.primary)
 		replaceErr := c.replacePrimaryProvider()
 		if replaceErr != nil {
 			return nil, fmt.Errorf("%v. Tried to replace primary but: %w", err.Error(), replaceErr)
@@ -1003,7 +995,7 @@ func (c *Client) compareFirstHeaderWithWitnesses(ctx context.Context, h *types.S
 	defer cancel()
 
 	if len(c.witnesses) < 1 {
-		return errNoWitnesses{}
+		return ErrNoWitnesses
 	}
 
 	errc := make(chan error, len(c.witnesses))

light/detector.go

@@ -41,7 +41,7 @@ func (c *Client) detectDivergence(ctx context.Context, primaryTrace []*types.Lig
 	defer c.providerMutex.Unlock()
 
 	if len(c.witnesses) == 0 {
-		return errNoWitnesses{}
+		return ErrNoWitnesses
 	}
 
 	// launch one goroutine per witness to retrieve the light block of the target height

light/errors.go

@@ -42,9 +42,10 @@ func (e ErrInvalidHeader) Error() string {
 
 // ErrFailedHeaderCrossReferencing is returned when the detector was not able to cross reference the header
 // with any of the connected witnesses.
-var ErrFailedHeaderCrossReferencing = errors.New("all witnesses have either not responded, don't have the " +
-	" blocks or sent invalid blocks. You should look to change your witnesses" +
-	"  or review the light client's logs for more information")
+var ErrFailedHeaderCrossReferencing = errors.New(
+	`all witnesses have either not responded, don't have the blocks or sent invalid blocks. 
+	You should look to change your witnesses or review the light client's logs for more information`,
+)
 
 // ErrVerificationFailed means either sequential or skipping verification has
 // failed to verify from header #1 to header #2 due to some reason.
@@ -65,10 +66,15 @@ func (e ErrVerificationFailed) Error() string {
 
 // ErrLightClientAttack is returned when the light client has detected an attempt
 // to verify a false header and has sent the evidence to either a witness or primary.
-var ErrLightClientAttack = errors.New("attempted attack detected." +
-	" Light client received valid conflicting header from witness." +
-	" Unable to verify header. Evidence has been sent to both providers." +
-	" Check logs for full evidence and trace")
+var ErrLightClientAttack = errors.New(`attempted attack detected.
+	Light client received valid conflicting header from witness.
+	Unable to verify header. Evidence has been sent to both providers.
+	Check logs for full evidence and trace`,
+)
+
+// ErrNoWitnesses means that there are not enough witnesses connected to
+// continue running the light client.
+var ErrNoWitnesses = errors.New("no witnesses connected. please reset light client")
 
 // ----------------------------- INTERNAL ERRORS ---------------------------------
 
@@ -84,14 +90,6 @@ func (e errConflictingHeaders) Error() string {
 		e.Block.Hash(), e.WitnessIndex)
 }
 
-// errNoWitnesses means that there are not enough witnesses connected to
-// continue running the light client.
-type errNoWitnesses struct{}
-
-func (e errNoWitnesses) Error() string {
-	return "no witnesses connected. please reset light client"
-}
-
 // errBadWitness is returned when the witness either does not respond or
 // responds with an invalid header.
 type errBadWitness struct {

light/provider/http/http.go

@@ -51,11 +51,6 @@ func NewWithClient(chainID string, client rpcclient.RemoteClient) provider.Provi
 	}
 }
 
-// ChainID returns a chainID this provider was configured with.
-func (p *http) ChainID() string {
-	return p.chainID
-}
-
 func (p *http) String() string {
 	return fmt.Sprintf("http{%s}", p.client.Remote())
 }

light/provider/mock/deadmock.go

@@ -3,6 +3,7 @@ package mock
 import (
 	"context"
 	"errors"
+	"fmt"
 
 	"github.com/tendermint/tendermint/light/provider"
 	"github.com/tendermint/tendermint/types"
@@ -11,17 +12,17 @@ import (
 var errNoResp = errors.New("no response from provider")
 
 type deadMock struct {
-	chainID string
+	id string
 }
 
-// NewDeadMock creates a mock provider that always errors.
-func NewDeadMock(chainID string) provider.Provider {
-	return &deadMock{chainID: chainID}
+// NewDeadMock creates a mock provider that always errors. id is used in case of multiple providers.
+func NewDeadMock(id string) provider.Provider {
+	return &deadMock{id: id}
 }
 
-func (p *deadMock) ChainID() string { return p.chainID }
-
-func (p *deadMock) String() string { return "deadMock" }
+func (p *deadMock) String() string {
+	return fmt.Sprintf("DeadMock-%s", p.id)
+}
 
 func (p *deadMock) LightBlock(_ context.Context, height int64) (*types.LightBlock, error) {
 	return nil, errNoResp

light/provider/mock/mock.go

@@ -11,7 +11,7 @@ import (
 )
 
 type Mock struct {
-	chainID          string
+	id               string
 	headers          map[int64]*types.SignedHeader
 	vals             map[int64]*types.ValidatorSet
 	evidenceToReport map[string]types.Evidence // hash => evidence
@@ -21,20 +21,15 @@ var _ provider.Provider = (*Mock)(nil)
 
 // New creates a mock provider with the given set of headers and validator
 // sets.
-func New(chainID string, headers map[int64]*types.SignedHeader, vals map[int64]*types.ValidatorSet) *Mock {
+func New(id string, headers map[int64]*types.SignedHeader, vals map[int64]*types.ValidatorSet) *Mock {
 	return &Mock{
-		chainID:          chainID,
+		id:               id,
 		headers:          headers,
 		vals:             vals,
 		evidenceToReport: make(map[string]types.Evidence),
 	}
 }
 
-// ChainID returns the blockchain ID.
-func (p *Mock) ChainID() string {
-	return p.chainID
-}
-
 func (p *Mock) String() string {
 	var headers strings.Builder
 	for _, h := range p.headers {
@@ -46,7 +41,7 @@ func (p *Mock) String() string {
 		fmt.Fprintf(&vals, " %X", v.Hash())
 	}
 
-	return fmt.Sprintf("Mock{headers: %s, vals: %v}", headers.String(), vals.String())
+	return fmt.Sprintf("Mock{id: %s, headers: %s, vals: %v}", p.id, headers.String(), vals.String())
 }
 
 func (p *Mock) LightBlock(_ context.Context, height int64) (*types.LightBlock, error) {

light/provider/provider.go

@@ -9,9 +9,6 @@ import (
 // Provider provides information for the light client to sync (verification
 // happens in the client).
 type Provider interface {
-	// ChainID returns the blockchain ID.
-	ChainID() string
-
 	// LightBlock returns the LightBlock that corresponds to the given
 	// height.
 	//

light/store/db/db.go

@@ -238,7 +238,7 @@ func (s *dbs) Prune(size uint16) error {
 		append(s.lbKey(1<<63-1), byte(0x00)),
 	)
 	if err != nil {
-		return err
+		panic(err)
 	}
 	defer itr.Close()
 

light/verifier.go

@@ -46,6 +46,10 @@ func VerifyNonAdjacent(
 		return errors.New("headers must be non adjacent in height")
 	}
 
+	if err := ValidateTrustLevel(trustLevel); err != nil {
+		return err
+	}
+
 	if HeaderExpired(trustedHeader, trustingPeriod, now) {
 		return ErrOldHeaderExpired{trustedHeader.Time.Add(trustingPeriod), now}
 	}
@@ -57,14 +61,15 @@ func VerifyNonAdjacent(
 		return ErrInvalidHeader{err}
 	}
 
-	// Ensure that +`trustLevel` (default 1/3) or more of last trusted validators signed correctly.
+	// Ensure that +`trustLevel` (default 1/3) or more in voting power of the last trusted validator
+	// set signed correctly.
 	err := trustedVals.VerifyCommitLightTrusting(trustedHeader.ChainID, untrustedHeader.Commit, trustLevel)
 	if err != nil {
 		switch e := err.(type) {
 		case types.ErrNotEnoughVotingPowerSigned:
 			return ErrNewValSetCantBeTrusted{e}
 		default:
-			return e
+			return ErrInvalidHeader{e}
 		}
 	}
 
@@ -186,14 +191,16 @@ func HeaderExpired(h *types.SignedHeader, trustingPeriod time.Duration, now time
 //  c) that the LastBlockID hash of the trusted header is the same as the hash
 //  of the trusted header
 //
-//  For any of these cases ErrInvalidHeader is returned.
+// For any of these cases ErrInvalidHeader is returned.
+// NOTE: This does not check whether the trusted header has expired or not.
 func VerifyBackwards(untrustedHeader, trustedHeader *types.Header) error {
 	if err := untrustedHeader.ValidateBasic(); err != nil {
 		return ErrInvalidHeader{err}
 	}
 
 	if untrustedHeader.ChainID != trustedHeader.ChainID {
-		return ErrInvalidHeader{errors.New("header belongs to another chain")}
+		return ErrInvalidHeader{fmt.Errorf("new header belongs to a different chain (%s != %s)",
+			untrustedHeader.ChainID, trustedHeader.ChainID)}
 	}
 
 	if !untrustedHeader.Time.Before(trustedHeader.Time) {