light: remove max retry attempts from client and add to provider (#6054)

CHANGELOG_PENDING.md

@@ -28,10 +28,13 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi
   - [proto/p2p] Renamed `DefaultNodeInfo` and `DefaultNodeInfoOther` to `NodeInfo` and `NodeInfoOther` (@erikgrinaker)
   - [proto/p2p] Rename `NodeInfo.default_node_id` to `node_id` (@erikgrinaker)
   - [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)
+  - [store] \#5848 Remove block store state in favor of using the db iterators directly (@cmwaters)  
+  - [state] \#5864 Use an iterator when pruning state (@cmwaters)
   - [types] \#6023 Remove `tm2pb.Header`, `tm2pb.BlockID`, `tm2pb.PartSetHeader` and `tm2pb.NewValidatorUpdate`.
     - Each of the above types has a `ToProto` and `FromProto` method or function which replaced this logic.
   - [rpc/client/http] \#6022 Change `timeout` type to `time.Duration` in `NewWithTimeout`
+  - [light] \#6054 Move `MaxRetryAttempt` option from client to provider.
+    - `NewWithOptions` now sets the max retry attempts and timeouts (@cmwaters)
 
 - Blockchain Protocol
 

light/client.go

@@ -21,8 +21,7 @@ const (
 	sequential mode = iota + 1
 	skipping
 
-	defaultPruningSize      = 1000
-	defaultMaxRetryAttempts = 10
+	defaultPruningSize = 1000
 	// For verifySkipping, when using the cache of headers from the previous batch,
 	// they will always be at a height greater than 1/2 (normal verifySkipping) so to
 	// find something in between the range, 9/16 is used.
@@ -90,14 +89,6 @@ func Logger(l log.Logger) Option {
 	}
 }
 
-// MaxRetryAttempts option can be used to set max attempts before replacing
-// primary with a witness.
-func MaxRetryAttempts(max uint16) Option {
-	return func(c *Client) {
-		c.maxRetryAttempts = max
-	}
-}
-
 // MaxClockDrift defines how much new header's time can drift into
 // the future. Default: 10s.
 func MaxClockDrift(d time.Duration) Option {
@@ -116,7 +107,6 @@ type Client struct {
 	trustingPeriod   time.Duration // see TrustOptions.Period
 	verificationMode mode
 	trustLevel       tmmath.Fraction
-	maxRetryAttempts uint16 // see MaxRetryAttempts option
 	maxClockDrift    time.Duration
 
 	// Mutex for locking during changes of the light clients providers
@@ -202,7 +192,6 @@ func NewClientFromTrustedStore(
 		trustingPeriod:   trustingPeriod,
 		verificationMode: skipping,
 		trustLevel:       DefaultTrustLevel,
-		maxRetryAttempts: defaultMaxRetryAttempts,
 		maxClockDrift:    defaultMaxClockDrift,
 		primary:          primary,
 		witnesses:        witnesses,

light/client_test.go

@@ -768,7 +768,6 @@ func TestClientReplacesPrimaryWithWitnessIfPrimaryIsUnavailable(t *testing.T) {
 		[]provider.Provider{fullNode, fullNode},
 		dbs.New(dbm.NewMemDB()),
 		light.Logger(log.TestingLogger()),
-		light.MaxRetryAttempts(1),
 	)
 
 	require.NoError(t, err)
@@ -946,7 +945,6 @@ func TestClientRemovesWitnessIfItSendsUsIncorrectHeader(t *testing.T) {
 		[]provider.Provider{badProvider1, badProvider2},
 		dbs.New(dbm.NewMemDB()),
 		light.Logger(log.TestingLogger()),
-		light.MaxRetryAttempts(1),
 	)
 	// witness should have behaved properly -> no error
 	require.NoError(t, err)
@@ -1086,7 +1084,6 @@ func TestClientEnsureValidHeadersAndValSets(t *testing.T) {
 			badNode,
 			[]provider.Provider{badNode, badNode},
 			dbs.New(dbm.NewMemDB()),
-			light.MaxRetryAttempts(1),
 		)
 		require.NoError(t, err)
 

light/detector_test.go

@@ -56,7 +56,6 @@ func TestLightClientAttackEvidence_Lunatic(t *testing.T) {
 		[]provider.Provider{witness},
 		dbs.New(dbm.NewMemDB()),
 		light.Logger(log.TestingLogger()),
-		light.MaxRetryAttempts(1),
 	)
 	require.NoError(t, err)
 
@@ -138,7 +137,6 @@ func TestLightClientAttackEvidence_Equivocation(t *testing.T) {
 			[]provider.Provider{witness},
 			dbs.New(dbm.NewMemDB()),
 			light.Logger(log.TestingLogger()),
-			light.MaxRetryAttempts(1),
 			verificationOption,
 		)
 		require.NoError(t, err)
@@ -193,7 +191,6 @@ func TestClientDivergentTraces1(t *testing.T) {
 		[]provider.Provider{witness},
 		dbs.New(dbm.NewMemDB()),
 		light.Logger(log.TestingLogger()),
-		light.MaxRetryAttempts(1),
 	)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "does not match primary")
@@ -217,7 +214,6 @@ func TestClientDivergentTraces2(t *testing.T) {
 		[]provider.Provider{deadNode, deadNode, primary},
 		dbs.New(dbm.NewMemDB()),
 		light.Logger(log.TestingLogger()),
-		light.MaxRetryAttempts(1),
 	)
 	require.NoError(t, err)
 
@@ -252,7 +248,6 @@ func TestClientDivergentTraces3(t *testing.T) {
 		[]provider.Provider{witness},
 		dbs.New(dbm.NewMemDB()),
 		light.Logger(log.TestingLogger()),
-		light.MaxRetryAttempts(1),
 	)
 	require.NoError(t, err)
 

light/provider/http/http.go

@@ -18,20 +18,28 @@ 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
-	timeout          = 5 * time.Second
+	defaultMaxRetryAttempts = 10
+	defaultTimeout          = 5 * time.Second
 )
 
 // http provider uses an RPC client to obtain the necessary information.
 type http struct {
 	chainID string
 	client  rpcclient.RemoteClient
+
+	maxRetryAttempts int
 }
 
 // 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. The 5s timeout is used for all requests.
 func New(chainID, remote string) (provider.Provider, error) {
+	return NewWithOptions(chainID, remote, defaultMaxRetryAttempts, defaultTimeout)
+}
+
+// NewWithOptions is an extension to creating a new http provider that allows the addition
+// of a specified timeout and maxRetryAttempts
+func NewWithOptions(chainID, remote string, maxRetryAttempts int, timeout time.Duration) (provider.Provider, error) {
 	// Ensure URL scheme is set (default HTTP) when not provided.
 	if !strings.Contains(remote, "://") {
 		remote = "http://" + remote
@@ -42,14 +50,19 @@ func New(chainID, remote string) (provider.Provider, error) {
 		return nil, err
 	}
 
-	return NewWithClient(chainID, httpClient), nil
+	return NewWithClientAndOptions(chainID, httpClient, maxRetryAttempts), nil
 }
 
-// NewWithClient allows you to provide a custom client.
 func NewWithClient(chainID string, client rpcclient.RemoteClient) provider.Provider {
+	return NewWithClientAndOptions(chainID, client, defaultMaxRetryAttempts)
+}
+
+// NewWithClient allows you to provide a custom client.
+func NewWithClientAndOptions(chainID string, client rpcclient.RemoteClient, maxRetryAttempts int) provider.Provider {
 	return &http{
-		client:  client,
-		chainID: chainID,
+		client:           client,
+		chainID:          chainID,
+		maxRetryAttempts: maxRetryAttempts,
 	}
 }
 
@@ -108,7 +121,7 @@ func (p *http) validatorSet(ctx context.Context, height *int64) (*types.Validato
 	)
 
 	for len(vals) != total && page <= maxPages {
-		for attempt := 1; attempt <= maxRetryAttempts; attempt++ {
+		for attempt := 1; attempt <= p.maxRetryAttempts; attempt++ {
 			res, err := p.client.Validators(ctx, height, &page, &perPage)
 			if err != nil {
 				// TODO: standardize errors on the RPC side
@@ -116,7 +129,7 @@ func (p *http) validatorSet(ctx context.Context, height *int64) (*types.Validato
 					return nil, provider.ErrLightBlockNotFound
 				}
 				// if we have exceeded retry attempts then return no response error
-				if attempt == maxRetryAttempts {
+				if attempt == p.maxRetryAttempts {
 					return nil, provider.ErrNoResponse
 				}
 				// else we wait and try again with exponential backoff
@@ -153,7 +166,7 @@ func (p *http) validatorSet(ctx context.Context, height *int64) (*types.Validato
 }
 
 func (p *http) signedHeader(ctx context.Context, height *int64) (*types.SignedHeader, error) {
-	for attempt := 1; attempt <= maxRetryAttempts; attempt++ {
+	for attempt := 1; attempt <= p.maxRetryAttempts; attempt++ {
 		commit, err := p.client.Commit(ctx, height)
 		if err != nil {
 			// TODO: standardize errors on the RPC side

statesync/stateprovider.go

@@ -71,7 +71,7 @@ func NewLightClientStateProvider(
 	}
 
 	lc, err := light.NewClient(ctx, chainID, trustOptions, providers[0], providers[1:],
-		lightdb.New(dbm.NewMemDB()), light.Logger(logger), light.MaxRetryAttempts(5))
+		lightdb.New(dbm.NewMemDB()), light.Logger(logger))
 	if err != nil {
 		return nil, err
 	}