Conversation
Codecov Report
@@ Coverage Diff @@
## master #6687 +/- ##
==========================================
- Coverage 65.99% 60.89% -5.10%
==========================================
Files 234 297 +63
Lines 20269 28229 +7960
==========================================
+ Hits 13376 17191 +3815
- Misses 5825 9304 +3479
- Partials 1068 1734 +666
|
internal/statesync/syncer.go
Outdated
| errNoSnapshots = errors.New("no suitable snapshots found") | ||
| // errDeadlineExceeded is returned by Sync() when the timeout for retrieving | ||
| // tendermint state or the commit is exceeded | ||
| errDeadlineExceeded = errors.New("deadline to retrieve state and commit exceeded") |
There was a problem hiding this comment.
This already is an error value in the standard library, can we just use that one?
There was a problem hiding this comment.
The reason why I made a separate error is what happens if someone uses a deadline in the parent context. If it gets triggered we have (AFAIK and context is relatively new to me so I don't know too much) no idea whether the deadline was from the parent context or the sub context we use for retrieving the state and commit information. If it is a parent context deadline then we should stop completely. If it is a sub context deadline, we should drop the snapshot but try again with another snapshot. Does that make sense?
There was a problem hiding this comment.
I see that this used on line 287 and 295 of this file. In either of those cases, the parent or the child could have timed out and we sill return this error. We still can't tell which one timed out when we percolate this error up but perhaps I'm not totally understanding?
There was a problem hiding this comment.
Ok good catch. I think I've added the adequate clause to distinguish between parent context errors and child context errors
internal/statesync/syncer.go
Outdated
| state, err := s.stateProvider.State(pctx, snapshot.Height) | ||
| if err != nil { | ||
| if err == context.DeadlineExceeded { | ||
| return sm.State{}, nil, errDeadlineExceeded |
There was a problem hiding this comment.
we wrap the error that we return using the %w format directive. I'm not sure there's a need for this extra case. The caller can use error.Is(err, Context.DeadlineExceeded).
| errNoSnapshots = errors.New("no suitable snapshots found") | ||
| // errStateCommitTimeout is returned by Sync() when the timeout for retrieving | ||
| // tendermint state or the commit is exceeded | ||
| errStateCommitTimeout = errors.New("timed out trying to retrieve state and commit") |
This is the update to master for #6685 which fixes incorrect handling of contexts within the light client.