Repository: celestiaorg/celestia-node
Pull request #1591 (2023-01-13 12:12:39)

refactor(shrexeds): removing retry logic from client
<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before submitting your
first PR.
-->

## Overview
This PR removes the retry logic from the shrex/eds client and adds test skipping to test cases that can either be refactored or removed when implementing #1489. This logic is being removed so that ShrexGetter can handle retry/round-robin logic for both the shrex/eds and shrex/nd clients.

<!-- 
Please provide an explanation of the PR, including the appropriate context,
background, goal, and rationale. If there is an issue with this information,
please provide a tl;dr and link the issue. 
-->

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [ ] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or documentation updates
- [ ] Linked issues closed with keywords



Patch:
diff --git a/share/p2p/shrexeds/client.go b/share/p2p/shrexeds/client.go
index 57cdf789..50c5f359 100644
--- a/share/p2p/shrexeds/client.go
+++ b/share/p2p/shrexeds/client.go
@@ -42,52 +42,34 @@ func NewClient(host host.Host, opts ...Option) (*Client, error) {
 	}, nil
 }
 
-// RequestEDS requests the full ODS from one of the given peers and returns the EDS.
-//
-// The peers are requested in a round-robin manner with retries until one of them gives a valid
-// response, blocking until the context is canceled or a valid response is given.
+// RequestEDS requests the ODS from the given peers and returns the EDS upon success.
 func (c *Client) RequestEDS(
 	ctx context.Context,
 	dataHash share.DataHash,
-	peers peer.IDSlice,
+	peers ...peer.ID,
 ) (*rsmt2d.ExtendedDataSquare, error) {
 	req := &p2p_pb.EDSRequest{Hash: dataHash}
 
-	// requests are retried for every peer until a valid response is received
-	excludedPeers := make(map[peer.ID]struct{})
-	for {
-		// if no peers are left, return
-		if len(peers) == len(excludedPeers) {
-			return nil, errNoMorePeers
+	for _, to := range peers {
+		eds, err := c.doRequest(ctx, req, to)
+		if eds != nil {
+			return eds, err
 		}
-
-		for _, to := range peers {
-			// skip over excluded peers
-			if _, ok := excludedPeers[to]; ok {
-				continue
-			}
-			eds, err := c.doRequest(ctx, req, to)
-			if eds != nil {
-				return eds, err
-			}
-			if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) {
-				return nil, ctx.Err()
-			}
-			// some net.Errors also mean the context deadline was exceeded, but yamux/mocknet do not
-			// unwrap to a ctx err
-			var ne net.Error
-			if errors.As(err, &ne) && ne.Timeout() {
-				return nil, context.DeadlineExceeded
-			}
-			if err != nil {
-				// peer has misbehaved, exclude them from round-robin
-				excludedPeers[to] = struct{}{}
-				log.Errorw("client: eds request to peer failed", "peer", to, "hash", dataHash.String())
-			}
-
-			// no eds was found, continue
+		if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) {
+			return nil, ctx.Err()
+		}
+		// some net.Errors also mean the context deadline was exceeded, but yamux/mocknet do not
+		// unwrap to a ctx err
+		var ne net.Error
+		if errors.As(err, &ne) && ne.Timeout() {
+			return nil, context.DeadlineExceeded
+		}
+		if err != nil {
+			log.Errorw("client: eds request to peer failed", "peer", to, "hash", dataHash.String())
 		}
 	}
+
+	return nil, errNoMorePeers
 }
 
 func (c *Client) doRequest(


Test patch:
diff --git a/share/p2p/shrexeds/exchange_test.go b/share/p2p/shrexeds/exchange_test.go
index 21e90297..07814833 100644
--- a/share/p2p/shrexeds/exchange_test.go
+++ b/share/p2p/shrexeds/exchange_test.go
@@ -8,7 +8,6 @@ import (
 	"github.com/ipfs/go-datastore"
 	ds_sync "github.com/ipfs/go-datastore/sync"
 	libhost "github.com/libp2p/go-libp2p/core/host"
-	"github.com/libp2p/go-libp2p/core/peer"
 	mocknet "github.com/libp2p/go-libp2p/p2p/net/mock"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -36,14 +35,14 @@ func TestExchange_RequestEDS(t *testing.T) {
 		err = store.Put(ctx, dah.Hash(), eds)
 		require.NoError(t, err)
 
-		requestedEDS, err := client.RequestEDS(ctx, dah.Hash(), []peer.ID{server.host.ID()})
+		requestedEDS, err := client.RequestEDS(ctx, dah.Hash(), server.host.ID())
 		assert.NoError(t, err)
 		assert.Equal(t, eds.Flattened(), requestedEDS.Flattened())
-
 	})
 
 	// Testcase: EDS is unavailable initially, but is found after multiple requests
 	t.Run("EDS_AvailableAfterDelay", func(t *testing.T) {
+		t.Skip()
 		storageDelay := time.Second
 		eds := share.RandEDS(t, 4)
 		dah := da.NewDataAvailabilityHeader(eds)
@@ -54,7 +53,7 @@ func TestExchange_RequestEDS(t *testing.T) {
 		}()
 
 		now := time.Now()
-		requestedEDS, err := client.RequestEDS(ctx, dah.Hash(), []peer.ID{server.host.ID()})
+		requestedEDS, err := client.RequestEDS(ctx, dah.Hash(), server.host.ID())
 		finished := time.Now()
 
 		assert.Greater(t, finished.Sub(now), storageDelay)
@@ -64,19 +63,21 @@ func TestExchange_RequestEDS(t *testing.T) {
 
 	// Testcase: Invalid request excludes peer from round-robin, stopping request
 	t.Run("EDS_InvalidRequest", func(t *testing.T) {
+		t.Skip()
 		dataHash := []byte("invalid")
-		requestedEDS, err := client.RequestEDS(ctx, dataHash, []peer.ID{server.host.ID()})
+		requestedEDS, err := client.RequestEDS(ctx, dataHash, server.host.ID())
 		assert.ErrorIs(t, err, errNoMorePeers)
 		assert.Nil(t, requestedEDS)
 	})
 
 	// Testcase: Valid request, which server cannot serve, waits forever
 	t.Run("EDS_ValidTimeout", func(t *testing.T) {
+		t.Skip()
 		timeoutCtx, cancel := context.WithTimeout(ctx, time.Second)
 		t.Cleanup(cancel)
 		eds := share.RandEDS(t, 4)
 		dah := da.NewDataAvailabilityHeader(eds)
-		requestedEDS, err := client.RequestEDS(timeoutCtx, dah.Hash(), []peer.ID{server.host.ID()})
+		requestedEDS, err := client.RequestEDS(timeoutCtx, dah.Hash(), server.host.ID())
 		assert.ErrorIs(t, err, timeoutCtx.Err())
 		assert.Nil(t, requestedEDS)
 	})