light/provider/http: fix Validators (#6022)

Closes #6010

CHANGELOG_PENDING.md

@@ -30,6 +30,7 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi
   - [libs/os] Kill() and {Must,}{Read,Write}File() functions have been removed. (@alessio)
   - [store] \#5848 Remove block store state in favor of using the db iterators directly (@cmwaters)
   - [state] \#5864 Use an iterator when pruning state (@cmwaters)
+  - [rpc/client/http] \#6022 Change `timeout` type to `time.Duration` in `NewWithTimeout`
 
 - Blockchain Protocol
 
@@ -42,7 +43,7 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi
 
 - [crypto/ed25519] \#5632 Adopt zip215 `ed25519` verification. (@marbar3778)
 - [privval] \#5603 Add `--key` to `init`, `gen_validator`, `testnet` & `unsafe_reset_priv_validator` for use in generating `secp256k1` keys.
-- [privval] \#5725 Add gRPC support to private validator. 
+- [privval] \#5725 Add gRPC support to private validator.
 - [privval] \#5876 `tendermint show-validator` will query the remote signer if gRPC is being used (@marbar3778)
 - [abci/client] \#5673 `Async` requests return an error if queue is full (@melekes)
 - [mempool] \#5673 Cancel `CheckTx` requests if RPC client disconnects or times out (@melekes)
@@ -62,3 +63,4 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi
 - [privval] \#5638 Increase read/write timeout to 5s and calculate ping interval based on it (@JoeKash)
 - [blockchain/v1] [\#5701](https://github.com/tendermint/tendermint/pull/5701) Handle peers without blocks (@melekes)
 - [blockchain/v1] \#5711 Fix deadlock (@melekes)
+- [light] \#6022 Fix a bug when the number of validators equals 100 (@melekes)

light/provider/http/http.go

@@ -14,10 +14,12 @@ import (
 	"github.com/tendermint/tendermint/types"
 )
 
-// This is very brittle, see: https://github.com/tendermint/tendermint/issues/4740
 var (
+	// This is very brittle, see: https://github.com/tendermint/tendermint/issues/4740
 	regexpMissingHeight = regexp.MustCompile(`height \d+ (must be less than or equal to|is not available)`)
-	maxRetryAttempts    = 10
+
+	maxRetryAttempts = 10
+	timeout          = 5 * time.Second
 )
 
 // http provider uses an RPC client to obtain the necessary information.
@@ -28,14 +30,14 @@ type http struct {
 
 // New creates a HTTP provider, which is using the rpchttp.HTTP client under
 // the hood. If no scheme is provided in the remote URL, http will be used by
-// default.
+// default. The 5s timeout is used for all requests.
 func New(chainID, remote string) (provider.Provider, error) {
 	// Ensure URL scheme is set (default HTTP) when not provided.
 	if !strings.Contains(remote, "://") {
 		remote = "http://" + remote
 	}
 
-	httpClient, err := rpchttp.New(remote, "/websocket")
+	httpClient, err := rpchttp.NewWithTimeout(remote, "/websocket", timeout)
 	if err != nil {
 		return nil, err
 	}
@@ -93,15 +95,21 @@ func (p *http) ReportEvidence(ctx context.Context, ev types.Evidence) error {
 }
 
 func (p *http) validatorSet(ctx context.Context, height *int64) (*types.ValidatorSet, error) {
+	// Since the malicious node could report a massive number of pages, making us
+	// spend a considerable time iterating, we restrict the number of pages here.
+	// => 10000 validators max
+	const maxPages = 100
+
 	var (
-		maxPerPage = 100
-		vals       = []*types.Validator{}
-		page       = 1
+		perPage = 100
+		vals    = []*types.Validator{}
+		page    = 1
+		total   = -1
 	)
 
-	for len(vals)%maxPerPage == 0 {
+	for len(vals) != total && page <= maxPages {
 		for attempt := 1; attempt <= maxRetryAttempts; attempt++ {
-			res, err := p.client.Validators(ctx, height, &page, &maxPerPage)
+			res, err := p.client.Validators(ctx, height, &page, &perPage)
 			if err != nil {
 				// TODO: standardize errors on the RPC side
 				if regexpMissingHeight.MatchString(err.Error()) {
@@ -115,18 +123,28 @@ func (p *http) validatorSet(ctx context.Context, height *int64) (*types.Validato
 				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}
+
+			// Validate response.
+			if len(res.Validators) == 0 {
+				return nil, provider.ErrBadLightBlock{
+					Reason: fmt.Errorf("validator set is empty (height: %d, page: %d, per_page: %d)",
+						height, page, perPage),
 				}
-				return valSet, nil
 			}
+			if res.Total <= 0 {
+				return nil, provider.ErrBadLightBlock{
+					Reason: fmt.Errorf("total number of vals is <= 0: %d (height: %d, page: %d, per_page: %d)",
+						res.Total, height, page, perPage),
+				}
+			}
+
+			total = res.Total
 			vals = append(vals, res.Validators...)
 			page++
 			break
 		}
 	}
+
 	valSet, err := types.ValidatorSetFromExistingValidators(vals)
 	if err != nil {
 		return nil, provider.ErrBadLightBlock{Reason: err}

rpc/client/http/http.go

@@ -118,12 +118,12 @@ func New(remote, wsEndpoint string) (*HTTP, error) {
 
 // NewWithTimeout does the same thing as New, except you can set a Timeout for
 // http.Client. A Timeout of zero means no timeout.
-func NewWithTimeout(remote, wsEndpoint string, timeout uint) (*HTTP, error) {
+func NewWithTimeout(remote, wsEndpoint string, timeout time.Duration) (*HTTP, error) {
 	httpClient, err := jsonrpcclient.DefaultHTTPClient(remote)
 	if err != nil {
 		return nil, err
 	}
-	httpClient.Timeout = time.Duration(timeout) * time.Second
+	httpClient.Timeout = timeout
 	return NewWithClient(remote, wsEndpoint, httpClient)
 }
 

rpc/client/rpc_test.go

@@ -40,7 +40,7 @@ func getHTTPClient() *rpchttp.HTTP {
 	return c
 }
 
-func getHTTPClientWithTimeout(timeout uint) *rpchttp.HTTP {
+func getHTTPClientWithTimeout(timeout time.Duration) *rpchttp.HTTP {
 	rpcAddr := rpctest.GetConfig().RPC.ListenAddress
 	c, err := rpchttp.NewWithTimeout(rpcAddr, "/websocket", timeout)
 	if err != nil {
@@ -497,8 +497,7 @@ func TestTx(t *testing.T) {
 }
 
 func TestTxSearchWithTimeout(t *testing.T) {
-	// Get a client with a time-out of 10 secs.
-	timeoutClient := getHTTPClientWithTimeout(10)
+	timeoutClient := getHTTPClientWithTimeout(10 * time.Second)
 
 	_, _, tx := MakeTxKV()
 	_, err := timeoutClient.BroadcastTxCommit(context.Background(), tx)

types/validator_set.go

@@ -991,16 +991,21 @@ func ValidatorSetFromProto(vp *tmproto.ValidatorSet) (*ValidatorSet, error) {
 	return vals, vals.ValidateBasic()
 }
 
-// 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.
+// 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("validator set is empty")
+	}
 	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,
 	}