From 861d93df3cf4a4a1d2b8048a3c7e15972aaf9511 Mon Sep 17 00:00:00 2001 From: Wondertan Date: Thu, 26 Jan 2023 23:39:10 +0100 Subject: [PATCH 1/9] fix(header/p2p): add trivial retrying for trustedPeer requested --- libs/header/p2p/exchange.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/libs/header/p2p/exchange.go b/libs/header/p2p/exchange.go index 5ea9b6379d..58ba205e66 100644 --- a/libs/header/p2p/exchange.go +++ b/libs/header/p2p/exchange.go @@ -6,6 +6,7 @@ import ( "fmt" "math/rand" "sort" + "time" logging "github.com/ipfs/go-log/v2" "github.com/libp2p/go-libp2p/core/host" @@ -224,9 +225,21 @@ func (ex *Exchange[H]) performRequest( return nil, fmt.Errorf("no trusted peers") } - //nolint:gosec // G404: Use of weak random number generator - index := rand.Intn(len(ex.trustedPeers)) - return ex.request(ctx, ex.trustedPeers[index], req) + for ctx.Err() == nil { + //nolint:gosec // G404: Use of weak random number generator + index := rand.Intn(len(ex.trustedPeers)) + cctx, cancel := context.WithTimeout(context.Background(), time.Second) + h, err := ex.request(cctx, ex.trustedPeers[index], req) + cancel() + if err != nil { + log.Error(err) + continue + } + return h, nil + + } + + return nil, ctx.Err() } // request sends the HeaderRequest to a remote peer. From 7a6ed0cea4998b9870d650f3929cc57516f5003e Mon Sep 17 00:00:00 2001 From: Wondertan Date: Mon, 30 Jan 2023 12:55:50 +0100 Subject: [PATCH 2/9] use passed ctx --- libs/header/p2p/exchange.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/header/p2p/exchange.go b/libs/header/p2p/exchange.go index 58ba205e66..6eb79b2e4d 100644 --- a/libs/header/p2p/exchange.go +++ b/libs/header/p2p/exchange.go @@ -228,7 +228,7 @@ func (ex *Exchange[H]) performRequest( for ctx.Err() == nil { //nolint:gosec // G404: Use of weak random number generator index := rand.Intn(len(ex.trustedPeers)) - cctx, cancel := context.WithTimeout(context.Background(), time.Second) + cctx, cancel := context.WithTimeout(ctx, time.Second) h, err := ex.request(cctx, ex.trustedPeers[index], req) cancel() if err != nil { From 9182524169278aed1a1cbbba6699d32069e56ed4 Mon Sep 17 00:00:00 2001 From: Wondertan Date: Mon, 30 Jan 2023 16:05:09 +0100 Subject: [PATCH 3/9] timeout configuration --- libs/header/p2p/exchange.go | 40 ++++++++++++++++++++----------------- libs/header/p2p/options.go | 10 +++++++--- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/libs/header/p2p/exchange.go b/libs/header/p2p/exchange.go index 6eb79b2e4d..c6346fa803 100644 --- a/libs/header/p2p/exchange.go +++ b/libs/header/p2p/exchange.go @@ -6,7 +6,6 @@ import ( "fmt" "math/rand" "sort" - "time" logging "github.com/ipfs/go-log/v2" "github.com/libp2p/go-libp2p/core/host" @@ -41,11 +40,15 @@ type Exchange[H header.Header] struct { func NewExchange[H header.Header]( host host.Host, - peers peer.IDSlice, + trustedPeers peer.IDSlice, protocolSuffix string, connGater *conngater.BasicConnectionGater, opts ...Option[ClientParameters], ) (*Exchange[H], error) { + if len(trustedPeers) == 0 { + return nil, fmt.Errorf("no trusted peers") + } + params := DefaultClientParameters() for _, opt := range opts { opt(¶ms) @@ -59,7 +62,7 @@ func NewExchange[H header.Header]( return &Exchange[H]{ host: host, protocolID: protocolID(protocolSuffix), - trustedPeers: peers, + trustedPeers: trustedPeers, peerTracker: newPeerTracker( host, connGater, @@ -78,7 +81,12 @@ func (ex *Exchange[H]) Start(context.Context) error { // Try to pre-connect to trusted peers. // We don't really care if we succeed at this point // and just need any peers in the peerTracker asap - go ex.host.Connect(ex.ctx, peer.AddrInfo{ID: p}) //nolint:errcheck + go func(p peer.ID) { + err := ex.host.Connect(ex.ctx, peer.AddrInfo{ID: p}) //nolint:errcheck + if err != nil { + log.Debugw("err connecting to a bootstrap peer", "err", err, "peer", p) + } + }(p) } go ex.peerTracker.gc() go ex.peerTracker.track() @@ -221,25 +229,21 @@ func (ex *Exchange[H]) performRequest( return make([]H, 0), nil } - if len(ex.trustedPeers) == 0 { - return nil, fmt.Errorf("no trusted peers") - } - - for ctx.Err() == nil { + for { //nolint:gosec // G404: Use of weak random number generator - index := rand.Intn(len(ex.trustedPeers)) - cctx, cancel := context.WithTimeout(ctx, time.Second) - h, err := ex.request(cctx, ex.trustedPeers[index], req) + idx := rand.Intn(len(ex.trustedPeers)) + ctx, cancel := context.WithTimeout(ctx, ex.Params.TrustedPeersRequestTimeout) + + h, err := ex.request(ctx, ex.trustedPeers[idx], req) cancel() - if err != nil { - log.Error(err) + switch err { + default: + log.Debug(err) continue + case context.Canceled, context.DeadlineExceeded, nil: + return h, err } - return h, nil - } - - return nil, ctx.Err() } // request sends the HeaderRequest to a remote peer. diff --git a/libs/header/p2p/options.go b/libs/header/p2p/options.go index a644bc547e..fdceb46a64 100644 --- a/libs/header/p2p/options.go +++ b/libs/header/p2p/options.go @@ -104,13 +104,14 @@ func WithRequestTimeout[T parameters](duration time.Duration) Option[T] { } // ClientParameters is the set of parameters that must be configured for the exchange. +// TODO: #1667 type ClientParameters struct { // the target minimum amount of responses with the same chain head MinResponses int // MaxRequestSize defines the max amount of headers that can be handled at once. - MaxRequestSize uint64 + MaxRequestSize uint64 // TODO: Rename to MaxRangeRequestSize // MaxHeadersPerRequest defines the max amount of headers that can be requested per 1 request. - MaxHeadersPerRequest uint64 + MaxHeadersPerRequest uint64 // TODO: Rename to MaxHeadersPerRangeRequest // MaxAwaitingTime specifies the duration that gives to the disconnected peer to be back online, // otherwise it will be removed on the next GC cycle. MaxAwaitingTime time.Duration @@ -118,7 +119,9 @@ type ClientParameters struct { DefaultScore float32 // RequestTimeout defines a timeout after which the session will try to re-request headers // from another peer. - RequestTimeout time.Duration + RequestTimeout time.Duration // TODO: Rename to RangeRequestTimeout + // TrustedPeersRequestTimeout a timeout for any request to a trusted peer. + TrustedPeersRequestTimeout time.Duration // MaxTrackerSize specifies the max amount of peers that can be added to the peerTracker. MaxPeerTrackerSize int } @@ -132,6 +135,7 @@ func DefaultClientParameters() ClientParameters { MaxAwaitingTime: time.Hour, DefaultScore: 1, RequestTimeout: time.Second * 3, + TrustedPeersRequestTimeout: time.Millisecond * 300, MaxPeerTrackerSize: 100, } } From c70d6394be2121e59646eedd06d43343bc635188 Mon Sep 17 00:00:00 2001 From: Wondertan Date: Mon, 30 Jan 2023 19:55:11 +0100 Subject: [PATCH 4/9] lint --- libs/header/p2p/exchange.go | 2 +- libs/header/p2p/options.go | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/libs/header/p2p/exchange.go b/libs/header/p2p/exchange.go index c6346fa803..8b2614868d 100644 --- a/libs/header/p2p/exchange.go +++ b/libs/header/p2p/exchange.go @@ -82,7 +82,7 @@ func (ex *Exchange[H]) Start(context.Context) error { // We don't really care if we succeed at this point // and just need any peers in the peerTracker asap go func(p peer.ID) { - err := ex.host.Connect(ex.ctx, peer.AddrInfo{ID: p}) //nolint:errcheck + err := ex.host.Connect(ex.ctx, peer.AddrInfo{ID: p}) if err != nil { log.Debugw("err connecting to a bootstrap peer", "err", err, "peer", p) } diff --git a/libs/header/p2p/options.go b/libs/header/p2p/options.go index fdceb46a64..052bd6c144 100644 --- a/libs/header/p2p/options.go +++ b/libs/header/p2p/options.go @@ -129,14 +129,14 @@ type ClientParameters struct { // DefaultClientParameters returns the default params to configure the store. func DefaultClientParameters() ClientParameters { return ClientParameters{ - MinResponses: 2, - MaxRequestSize: 512, - MaxHeadersPerRequest: 64, - MaxAwaitingTime: time.Hour, - DefaultScore: 1, - RequestTimeout: time.Second * 3, + MinResponses: 2, + MaxRequestSize: 512, + MaxHeadersPerRequest: 64, + MaxAwaitingTime: time.Hour, + DefaultScore: 1, + RequestTimeout: time.Second * 3, TrustedPeersRequestTimeout: time.Millisecond * 300, - MaxPeerTrackerSize: 100, + MaxPeerTrackerSize: 100, } } From beb13dbf6f279e5ccdb12f4fd0538f0d9937c977 Mon Sep 17 00:00:00 2001 From: Wondertan Date: Mon, 30 Jan 2023 20:04:33 +0100 Subject: [PATCH 5/9] remove if that breaks test --- libs/header/p2p/exchange.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/libs/header/p2p/exchange.go b/libs/header/p2p/exchange.go index 8b2614868d..1c862f716e 100644 --- a/libs/header/p2p/exchange.go +++ b/libs/header/p2p/exchange.go @@ -45,10 +45,6 @@ func NewExchange[H header.Header]( connGater *conngater.BasicConnectionGater, opts ...Option[ClientParameters], ) (*Exchange[H], error) { - if len(trustedPeers) == 0 { - return nil, fmt.Errorf("no trusted peers") - } - params := DefaultClientParameters() for _, opt := range opts { opt(¶ms) From 0b84e42798e1184bc1939d99611a01249d7d64cd Mon Sep 17 00:00:00 2001 From: Wondertan Date: Tue, 31 Jan 2023 19:40:32 +0100 Subject: [PATCH 6/9] add trusted peers check back and validation check as well --- libs/header/p2p/exchange.go | 5 +++++ libs/header/p2p/options.go | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/libs/header/p2p/exchange.go b/libs/header/p2p/exchange.go index 1c862f716e..04b3ccf0a0 100644 --- a/libs/header/p2p/exchange.go +++ b/libs/header/p2p/exchange.go @@ -225,6 +225,11 @@ func (ex *Exchange[H]) performRequest( return make([]H, 0), nil } + // TODO: Move this check to constructor() + if len(ex.trustedPeers) == 0 { + return nil, fmt.Errorf("no trusted peers") + } + for { //nolint:gosec // G404: Use of weak random number generator idx := rand.Intn(len(ex.trustedPeers)) diff --git a/libs/header/p2p/options.go b/libs/header/p2p/options.go index 052bd6c144..8fa20ee56f 100644 --- a/libs/header/p2p/options.go +++ b/libs/header/p2p/options.go @@ -171,6 +171,10 @@ func (p *ClientParameters) Validate() error { return fmt.Errorf("invalid request timeout for session: "+ "%s. %s: %v", greaterThenZero, providedSuffix, p.RequestTimeout) } + if p.TrustedPeersRequestTimeout == 0 { + return fmt.Errorf("invalid TrustedPeersRequestTimeout: "+ + "%s. %s: %v", greaterThenZero, providedSuffix, p.TrustedPeersRequestTimeout) + } if p.MaxPeerTrackerSize <= 0 { return fmt.Errorf("invalid MaxTrackerSize: %s. %s: %d", greaterThenZero, providedSuffix, p.MaxPeerTrackerSize) } From 253dcd28dd522af00f4b052177cfc3528647e614 Mon Sep 17 00:00:00 2001 From: Wondertan Date: Tue, 31 Jan 2023 19:43:55 +0100 Subject: [PATCH 7/9] add logging suggestion --- libs/header/p2p/exchange.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/header/p2p/exchange.go b/libs/header/p2p/exchange.go index 04b3ccf0a0..0dc78a0138 100644 --- a/libs/header/p2p/exchange.go +++ b/libs/header/p2p/exchange.go @@ -79,7 +79,7 @@ func (ex *Exchange[H]) Start(context.Context) error { // and just need any peers in the peerTracker asap go func(p peer.ID) { err := ex.host.Connect(ex.ctx, peer.AddrInfo{ID: p}) - if err != nil { + if err != nil && err != context.Canceled && err != context.DeadlineExceeded { log.Debugw("err connecting to a bootstrap peer", "err", err, "peer", p) } }(p) From aa496dfe627ebf84be822e0194665988e3744254 Mon Sep 17 00:00:00 2001 From: Wondertan Date: Tue, 31 Jan 2023 19:49:42 +0100 Subject: [PATCH 8/9] use error is --- libs/header/p2p/exchange.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/header/p2p/exchange.go b/libs/header/p2p/exchange.go index 0dc78a0138..c9ec280da6 100644 --- a/libs/header/p2p/exchange.go +++ b/libs/header/p2p/exchange.go @@ -3,6 +3,7 @@ package p2p import ( "bytes" "context" + "errors" "fmt" "math/rand" "sort" @@ -79,7 +80,7 @@ func (ex *Exchange[H]) Start(context.Context) error { // and just need any peers in the peerTracker asap go func(p peer.ID) { err := ex.host.Connect(ex.ctx, peer.AddrInfo{ID: p}) - if err != nil && err != context.Canceled && err != context.DeadlineExceeded { + if err != nil && !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) { log.Debugw("err connecting to a bootstrap peer", "err", err, "peer", p) } }(p) From c434ce9eda44dcffd315321a7e50accf644551ff Mon Sep 17 00:00:00 2001 From: Wondertan Date: Tue, 31 Jan 2023 19:52:42 +0100 Subject: [PATCH 9/9] add issue --- libs/header/p2p/exchange.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/header/p2p/exchange.go b/libs/header/p2p/exchange.go index c9ec280da6..b7e06721fb 100644 --- a/libs/header/p2p/exchange.go +++ b/libs/header/p2p/exchange.go @@ -226,7 +226,7 @@ func (ex *Exchange[H]) performRequest( return make([]H, 0), nil } - // TODO: Move this check to constructor() + // TODO: Move this check to constructor(#1671) if len(ex.trustedPeers) == 0 { return nil, fmt.Errorf("no trusted peers") }