dont interrupt sync if ballots in the layer were ignored or rejected #5729
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5729 +/- ##
=========================================
- Coverage 80.0% 80.0% -0.1%
=========================================
Files 279 279
Lines 29009 29032 +23
=========================================
+ Hits 23224 23240 +16
- Misses 4182 4186 +4
- Partials 1603 1606 +3 ☔ View full report in Codecov by Sentry. |
| nested := &BatchError{} | ||
| if errors.As(err, &nested) && nested.Ignore() { | ||
| return true | ||
| } |
There was a problem hiding this comment.
I'm not sure I fully understand that logic: a batch containing an ignored hash will trigger all parent batches to be ignored as well? Can BatchErrors be nested arbitrarily deep?
There was a problem hiding this comment.
it depends on the type of the object. for atxs batch error can be as long as atx chain, for ballots this is limited by the tortoise window size.
the logic here, if all dependencies are "ignored" then ignore then don't try to download this object as well.
| // and added to the tortoise. | ||
| decoded, err := h.tortoise.DecodeBallot(b.ToTortoiseData()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode ballot id %s err %w", b.ID().AsHash32().ShortString(), err) | ||
| return nil, fmt.Errorf( | ||
| "%w: failed to decode ballot id %s. %v", |
There was a problem hiding this comment.
Since Go 1.20 it is possible to wrap multiple errors into one:
| "%w: failed to decode ballot id %s. %v", | |
| "%w: failed to decode ballot id %s. %w", |
There was a problem hiding this comment.
i remember but i don't find it useful, there are errors that simply add more context and errors that are a part of the api
| require.ErrorIs(t, th.HandleSyncedBallot(context.Background(), b.ID().AsHash32(), peer, data), expected) | ||
| err := th.HandleSyncedBallot(context.Background(), b.ID().AsHash32(), peer, data) | ||
| require.ErrorIs(t, err, fetch.ErrIgnore) | ||
| require.Contains(t, err.Error(), expected.Error()) |
There was a problem hiding this comment.
With the suggested change above this becomes:
| require.Contains(t, err.Error(), expected.Error()) | |
| require.ErrorIs(t, err, expected) |
dshulyak
changed the title
dshulyak
changed the title
|
reopened from local branch #5761 |
related: #5701
in the implementation sync loop will repeatedly asks for objects even if it can't process them.
in this pr we allow sync to make progress if the remaining objects in the layer can't be processed with retries.