light: move dropout handling and invalid data to the provider (#5308)

light/client.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
-	"math/rand"
 	"time"
 
 	"github.com/tendermint/tendermint/libs/log"
@@ -681,7 +680,7 @@ func (c *Client) verifySkipping(
 			if depth == len(blockCache)-1 {
 				pivotHeight := verifiedBlock.Height + (blockCache[depth].Height-verifiedBlock.
 					Height)*verifySkippingNumerator/verifySkippingDenominator
-				interimBlock, err := c.lightBlockFrom(pivotHeight, source)
+				interimBlock, err := source.LightBlock(pivotHeight)
 				if err != nil {
 					return ErrVerificationFailed{From: verifiedBlock.Height, To: pivotHeight, Reason: err}
 				}
@@ -855,36 +854,6 @@ func (c *Client) updateTrustedLightBlock(l *types.LightBlock) error {
 	return nil
 }
 
-// 0 - latest header
-// Note it does not do retries nor swapping.
-func (c *Client) lightBlockFromWitness(height int64,
-	witness provider.Provider) (*types.LightBlock, *errBadWitness) {
-
-	l, err := witness.LightBlock(height)
-	if err != nil {
-		return nil, &errBadWitness{err, noResponse, -1}
-	}
-	err = c.validateLightBlock(l, height)
-	if err != nil {
-		return nil, &errBadWitness{err, invalidLightBlock, -1}
-	}
-
-	return l, nil
-}
-
-func (c *Client) lightBlockFrom(height int64,
-	source provider.Provider) (*types.LightBlock, error) {
-
-	c.providerMutex.Lock()
-	sourceIsPrimary := (c.primary == source)
-	c.providerMutex.Unlock()
-
-	if sourceIsPrimary {
-		return c.lightBlockFromPrimary(height)
-	}
-	return c.lightBlockFromWitness(height, source)
-}
-
 // backwards verification (see VerifyHeaderBackwards func in the spec) verifies
 // headers before a trusted header. If a sent header is invalid the primary is
 // replaced with another provider and the operation is repeated.
@@ -932,46 +901,44 @@ func (c *Client) compareNewHeaderWithWitnesses(l *types.LightBlock, now time.Tim
 
 	// 1. Make sure AT LEAST ONE witness returns the same header.
 	var headerMatched bool
-	for attempt := uint16(1); attempt <= c.maxRetryAttempts; attempt++ {
-		if len(c.witnesses) == 0 {
-			return errNoWitnesses{}
-		}
 
-		// launch one goroutine per witness
-		errc := make(chan error, len(c.witnesses))
-		for i, witness := range c.witnesses {
-			go c.compareNewHeaderWithWitness(errc, l, witness, i, now)
-		}
+	if len(c.witnesses) == 0 {
+		return errNoWitnesses{}
+	}
 
-		witnessesToRemove := make([]int, 0)
-
-		// handle errors as they come
-		for i := 0; i < cap(errc); i++ {
-			err := <-errc
-
-			switch e := err.(type) {
-			case nil: // at least one header matched
-				headerMatched = true
-			case errBadWitness:
-				c.logger.Info("Bad witness", "witness", c.witnesses[e.WitnessIndex], "err", err)
-				// if witness sent us invalid header / vals, remove it
-				if e.Code == invalidLightBlock {
-					c.logger.Info("Witness sent us invalid header / vals -> removing it", "witness", c.witnesses[e.WitnessIndex])
-					witnessesToRemove = append(witnessesToRemove, e.WitnessIndex)
-				}
-			}
-		}
+	// launch one goroutine per witness
+	errc := make(chan error, len(c.witnesses))
+	for i, witness := range c.witnesses {
+		go c.compareNewHeaderWithWitness(errc, l, witness, i, now)
+	}
 
-		for _, idx := range witnessesToRemove {
-			c.removeWitness(idx)
-		}
+	witnessesToRemove := make([]int, 0)
 
-		if headerMatched {
-			return nil
+	// handle errors as they come
+	for i := 0; i < cap(errc); i++ {
+		err := <-errc
+
+		switch e := err.(type) {
+		case nil: // at least one header matched
+			headerMatched = true
+		case errBadWitness:
+			c.logger.Info("Requested light block from bad witness", "witness", c.witnesses[e.WitnessIndex], "err", err)
+			// if witness sent us invalid header / vals, remove it
+			if e.Code == invalidLightBlock {
+				c.logger.Info("Witness sent us invalid header / vals -> removing it", "witness", c.witnesses[e.WitnessIndex])
+				witnessesToRemove = append(witnessesToRemove, e.WitnessIndex)
+			}
+		default:
+			c.logger.Info("Requested light block from witness but got error", "error", err)
 		}
+	}
 
-		// 2. Otherwise, sleep
-		time.Sleep(backoffTimeout(attempt))
+	for _, idx := range witnessesToRemove {
+		c.removeWitness(idx)
+	}
+
+	if headerMatched {
+		return nil
 	}
 
 	return errors.New("awaiting response from all witnesses exceeded dropout time")
@@ -980,16 +947,21 @@ func (c *Client) compareNewHeaderWithWitnesses(l *types.LightBlock, now time.Tim
 func (c *Client) compareNewHeaderWithWitness(errc chan error, l *types.LightBlock,
 	witness provider.Provider, witnessIndex int, now time.Time) {
 
-	altBlock, err := c.lightBlockFromWitness(l.Height, witness)
+	altBlock, err := witness.LightBlock(l.Height)
 	if err != nil {
-		err.WitnessIndex = witnessIndex
-		errc <- err
+		if _, ok := err.(provider.ErrBadLightBlock); ok {
+			errc <- errBadWitness{Reason: err, Code: invalidLightBlock, WitnessIndex: witnessIndex}
+		} else {
+			errc <- err
+		}
+		// if not a bad light block, then the witness has either not responded or
+		// doesn't have the block -> we ignore
 		return
 	}
 
 	if !bytes.Equal(l.Hash(), altBlock.Hash()) {
 		if bsErr := c.verifySkipping(witness, c.latestTrustedBlock, altBlock, now); bsErr != nil {
-			errc <- errBadWitness{bsErr, invalidLightBlock, witnessIndex}
+			errc <- errBadWitness{Reason: bsErr, Code: invalidLightBlock, WitnessIndex: witnessIndex}
 			return
 		}
 	}
@@ -1061,58 +1033,19 @@ func (c *Client) replacePrimaryProvider() error {
 // at the specified height. Handles dropout by the primary provider by swapping
 // with an alternative provider.
 func (c *Client) lightBlockFromPrimary(height int64) (*types.LightBlock, error) {
-	for attempt := uint16(1); attempt <= c.maxRetryAttempts; attempt++ {
-		c.providerMutex.Lock()
-		l, providerErr := c.primary.LightBlock(height)
-		c.providerMutex.Unlock()
-		if providerErr == nil {
-			err := c.validateLightBlock(l, height)
-			if err != nil {
-				replaceErr := c.replacePrimaryProvider()
-				if replaceErr != nil {
-					return nil, fmt.Errorf("%v. Tried to replace primary but: %w", err.Error(), replaceErr)
-				}
-				// replace primary and request a light block again
-				return c.lightBlockFromPrimary(height)
-			}
-			// valid light block has been received
-			return l, nil
-		}
-		if providerErr == provider.ErrLightBlockNotFound {
-			return nil, providerErr
-		}
-		c.logger.Error("Failed to get signed header from primary", "attempt", attempt, "err", providerErr)
-		time.Sleep(backoffTimeout(attempt))
-	}
-
-	err := c.replacePrimaryProvider()
-	if err != nil {
-		return nil, fmt.Errorf("primary dropped out. Tried to replace but: %w", err)
-	}
-
-	return c.lightBlockFromPrimary(height)
-}
-
-func (c *Client) validateLightBlock(l *types.LightBlock, expectedHeight int64) error {
-	if l == nil {
-		return errors.New("nil header")
-	}
-	err := l.ValidateBasic(c.chainID)
+	c.providerMutex.Lock()
+	l, err := c.primary.LightBlock(height)
+	c.providerMutex.Unlock()
 	if err != nil {
-		return err
-	}
-
-	if expectedHeight > 0 && l.Height != expectedHeight {
-		return fmt.Errorf("height mismatch, got: %d, expected: %d", l.Height, expectedHeight)
+		c.logger.Debug("Error on light block request from primary", "error", err)
+		replaceErr := c.replacePrimaryProvider()
+		if replaceErr != nil {
+			return nil, fmt.Errorf("%v. Tried to replace primary but: %w", err.Error(), replaceErr)
+		}
+		// replace primary and request a light block again
+		return c.lightBlockFromPrimary(height)
 	}
-	return nil
-}
-
-// exponential backoff (with jitter)
-//		0.5s -> 2s -> 4.5s -> 8s -> 12.5 with 1s variation
-func backoffTimeout(attempt uint16) time.Duration {
-	// nolint:gosec // G404: Use of weak random number generator
-	return time.Duration(500*attempt*attempt)*time.Millisecond + time.Duration(rand.Intn(1000))*time.Millisecond
+	return l, err
 }
 
 func hash2str(hash []byte) string {

light/client_test.go

@@ -889,7 +889,6 @@ func TestClientRemovesWitnessIfItSendsUsIncorrectHeader(t *testing.T) {
 		map[int64]*types.SignedHeader{
 			1: h1,
 			2: h2,
-			3: {Header: nil, Commit: nil},
 		},
 		map[int64]*types.ValidatorSet{
 			1: vals,
@@ -927,18 +926,7 @@ func TestClientRemovesWitnessIfItSendsUsIncorrectHeader(t *testing.T) {
 }
 
 func TestClient_TrustedValidatorSet(t *testing.T) {
-	noValSetNode := mockp.New(
-		chainID,
-		headerSet,
-		map[int64]*types.ValidatorSet{
-			1: nil,
-			2: nil,
-			3: nil,
-		},
-	)
-
 	differentVals, _ := types.RandValidatorSet(10, 100)
-
 	badValSetNode := mockp.New(
 		chainID,
 		map[int64]*types.SignedHeader{
@@ -960,8 +948,8 @@ func TestClient_TrustedValidatorSet(t *testing.T) {
 	c, err := light.NewClient(
 		chainID,
 		trustOptions,
-		noValSetNode,
-		[]provider.Provider{badValSetNode, fullNode, fullNode},
+		fullNode,
+		[]provider.Provider{badValSetNode, fullNode},
 		dbs.New(dbm.NewMemDB(), chainID),
 		light.Logger(log.TestingLogger()),
 	)

light/provider/errors.go

@@ -1,9 +1,25 @@
 package provider
 
-import "errors"
+import (
+	"errors"
+	"fmt"
+)
 
 var (
 	// ErrLightBlockNotFound is returned when a provider can't find the
 	// requested header.
 	ErrLightBlockNotFound = errors.New("light block not found")
+	// ErrNoResponse is returned if the provider doesn't respond to the
+	// request in a gieven time
+	ErrNoResponse = errors.New("client failed to respond")
 )
+
+// ErrBadLightBlock is returned when a provider returns an invalid
+// light block.
+type ErrBadLightBlock struct {
+	Reason error
+}
+
+func (e ErrBadLightBlock) Error() string {
+	return fmt.Sprintf("client provided bad signed header: %s", e.Reason.Error())
+}

light/provider/http/http.go

@@ -1,10 +1,11 @@
 package http
 
 import (
-	"errors"
 	"fmt"
+	"math/rand"
 	"regexp"
 	"strings"
+	"time"
 
 	"github.com/tendermint/tendermint/light/provider"
 	rpcclient "github.com/tendermint/tendermint/rpc/client"
@@ -13,7 +14,10 @@ import (
 )
 
 // This is very brittle, see: https://github.com/tendermint/tendermint/issues/4740
-var regexpMissingHeight = regexp.MustCompile(`height \d+ (must be less than or equal to|is not available)`)
+var (
+	regexpMissingHeight = regexp.MustCompile(`height \d+ (must be less than or equal to|is not available)`)
+	maxRetryAttempts    = 10
+)
 
 // http provider uses an RPC client to obtain the necessary information.
 type http struct {
@@ -60,66 +64,95 @@ func (p *http) String() string {
 func (p *http) LightBlock(height int64) (*types.LightBlock, error) {
 	h, err := validateHeight(height)
 	if err != nil {
-		return nil, err
+		return nil, provider.ErrBadLightBlock{Reason: err}
 	}
 
-	commit, err := p.client.Commit(h)
+	sh, err := p.signedHeader(h)
 	if err != nil {
-		// TODO: standardize errors on the RPC side
-		if regexpMissingHeight.MatchString(err.Error()) {
-			return nil, provider.ErrLightBlockNotFound
-		}
 		return nil, err
 	}
 
-	if commit.Header == nil {
-		return nil, errors.New("header is nil")
+	vs, err := p.validatorSet(h)
+	if err != nil {
+		return nil, err
 	}
 
-	// Verify we're still on the same chain.
-	if p.chainID != commit.Header.ChainID {
-		return nil, fmt.Errorf("expected chainID %s, got %s", p.chainID, commit.Header.ChainID)
+	lb := &types.LightBlock{
+		SignedHeader: sh,
+		ValidatorSet: vs,
 	}
 
-	maxPerPage := 100
-	res, err := p.client.Validators(h, nil, &maxPerPage)
+	err = lb.ValidateBasic(p.chainID)
 	if err != nil {
-		// TODO: standardize errors on the RPC side
-		if regexpMissingHeight.MatchString(err.Error()) {
-			return nil, provider.ErrLightBlockNotFound
-		}
-		return nil, err
+		return nil, provider.ErrBadLightBlock{Reason: err}
 	}
 
+	return lb, nil
+}
+
+// ReportEvidence calls `/broadcast_evidence` endpoint.
+func (p *http) ReportEvidence(ev types.Evidence) error {
+	_, err := p.client.BroadcastEvidence(ev)
+	return err
+}
+
+func (p *http) validatorSet(height *int64) (*types.ValidatorSet, error) {
 	var (
-		vals = res.Validators
-		page = 1
+		maxPerPage = 100
+		vals       = []*types.Validator{}
+		page       = 1
 	)
 
-	// Check if there are more validators.
-	for len(res.Validators) == maxPerPage {
-		res, err = p.client.Validators(h, &page, &maxPerPage)
-		if err != nil {
-			return nil, err
-		}
-		if len(res.Validators) > 0 {
+	for len(vals)%maxPerPage == 0 {
+		for attempt := 1; attempt <= maxRetryAttempts; attempt++ {
+			res, err := p.client.Validators(height, &page, &maxPerPage)
+			if err != nil {
+				// TODO: standardize errors on the RPC side
+				if regexpMissingHeight.MatchString(err.Error()) {
+					return nil, provider.ErrLightBlockNotFound
+				}
+				// if we have exceeded retry attempts then return no response error
+				if attempt == maxRetryAttempts {
+					return nil, provider.ErrNoResponse
+				}
+				// else we wait and try again with exponential backoff
+				time.Sleep(backoffTimeout(uint16(attempt)))
+				continue
+			}
+			if len(res.Validators) == 0 { // no more validators left
+				valSet, err := types.ValidatorSetFromExistingValidators(vals)
+				if err != nil {
+					return nil, provider.ErrBadLightBlock{Reason: err}
+				}
+				return valSet, nil
+			}
 			vals = append(vals, res.Validators...)
+			page++
+			break
 		}
-		page++
 	}
-
-	valset := types.NewValidatorSet(vals)
-
-	return &types.LightBlock{
-		SignedHeader: &commit.SignedHeader,
-		ValidatorSet: valset,
-	}, nil
+	valSet, err := types.ValidatorSetFromExistingValidators(vals)
+	if err != nil {
+		return nil, provider.ErrBadLightBlock{Reason: err}
+	}
+	return valSet, nil
 }
 
-// ReportEvidence calls `/broadcast_evidence` endpoint.
-func (p *http) ReportEvidence(ev types.Evidence) error {
-	_, err := p.client.BroadcastEvidence(ev)
-	return err
+func (p *http) signedHeader(height *int64) (*types.SignedHeader, error) {
+	for attempt := 1; attempt <= maxRetryAttempts; attempt++ {
+		commit, err := p.client.Commit(height)
+		if err != nil {
+			// TODO: standardize errors on the RPC side
+			if regexpMissingHeight.MatchString(err.Error()) {
+				return nil, provider.ErrLightBlockNotFound
+			}
+			// we wait and try again with exponential backoff
+			time.Sleep(backoffTimeout(uint16(attempt)))
+			continue
+		}
+		return &commit.SignedHeader, nil
+	}
+	return nil, provider.ErrNoResponse
 }
 
 func validateHeight(height int64) (*int64, error) {
@@ -133,3 +166,10 @@ func validateHeight(height int64) (*int64, error) {
 	}
 	return h, nil
 }
+
+// exponential backoff (with jitter)
+// 0.5s -> 2s -> 4.5s -> 8s -> 12.5 with 1s variation
+func backoffTimeout(attempt uint16) time.Duration {
+	// nolint:gosec // G404: Use of weak random number generator
+	return time.Duration(500*attempt*attempt)*time.Millisecond + time.Duration(rand.Intn(1000))*time.Millisecond
+}

light/provider/http/http_test.go

@@ -66,7 +66,6 @@ func TestProvider(t *testing.T) {
 
 	// let's get the highest block
 	sh, err := p.LightBlock(0)
-
 	require.NoError(t, err)
 	assert.True(t, sh.Height < 1000)
 

light/provider/mock/mock.go

@@ -1,6 +1,7 @@
 package mock
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
@@ -48,23 +49,34 @@ func (p *Mock) String() string {
 }
 
 func (p *Mock) LightBlock(height int64) (*types.LightBlock, error) {
+	var lb *types.LightBlock
 	if height == 0 && len(p.headers) > 0 {
 		sh := p.headers[int64(len(p.headers))]
 		vals := p.vals[int64(len(p.vals))]
-		return &types.LightBlock{
+		lb = &types.LightBlock{
 			SignedHeader: sh,
 			ValidatorSet: vals,
-		}, nil
+		}
+
 	}
 	if _, ok := p.headers[height]; ok {
 		sh := p.headers[height]
 		vals := p.vals[height]
-		return &types.LightBlock{
+		lb = &types.LightBlock{
 			SignedHeader: sh,
 			ValidatorSet: vals,
-		}, nil
+		}
+	}
+	if lb == nil {
+		return nil, provider.ErrLightBlockNotFound
+	}
+	if lb.SignedHeader == nil || lb.ValidatorSet == nil {
+		return nil, provider.ErrBadLightBlock{Reason: errors.New("nil header or vals")}
+	}
+	if err := lb.ValidateBasic(lb.ChainID); err != nil {
+		return nil, provider.ErrBadLightBlock{Reason: err}
 	}
-	return nil, provider.ErrLightBlockNotFound
+	return lb, nil
 }
 
 func (p *Mock) ReportEvidence(ev types.Evidence) error {

types/validator_set.go

@@ -986,9 +986,13 @@ func ValidatorSetFromProto(vp *tmproto.ValidatorSet) (*ValidatorSet, error) {
 
 // ValidatorSetFromExistingValidators takes an existing array of validators and rebuilds
 // the exact same validator set that corresponds to it without changing the proposer priority or power
+// if any of the validators fail validate basic then an empty set is returned.
 func ValidatorSetFromExistingValidators(valz []*Validator) (*ValidatorSet, error) {
-	if len(valz) == 0 {
-		return nil, errors.New("no validators given")
+	for _, val := range valz {
+		err := val.ValidateBasic()
+		if err != nil {
+			return nil, fmt.Errorf("can't create validator set: %w", err)
+		}
 	}
 	vals := &ValidatorSet{
 		Validators: valz,

types/validator_set_test.go

@@ -1436,9 +1436,10 @@ func TestNewValidatorSetFromExistingValidators(t *testing.T) {
 	valSet.IncrementProposerPriority(5)
 
 	newValSet := NewValidatorSet(valSet.Validators)
-	existingValSet, err := ValidatorSetFromExistingValidators(valSet.Validators)
-	require.NoError(t, err)
 	assert.NotEqual(t, valSet, newValSet)
+
+	existingValSet, err := ValidatorSetFromExistingValidators(valSet.Validators)
+	assert.NoError(t, err)
 	assert.Equal(t, valSet, existingValSet)
 	assert.Equal(t, valSet.CopyIncrementProposerPriority(3), existingValSet.CopyIncrementProposerPriority(3))
 }