feat(store): heightSub doesn't require adjacent headers #197
Conversation
| @@ -34,8 +34,17 @@ func (hs *heightSub[H]) Height() uint64 { | |||
| } | |||
|
|
|||
| // SetHeight sets the new head height for heightSub. | |||
| // Only the higher height can be set, otherwise no-op. | |||
There was a problem hiding this comment.
Before this change heightSub was vulnerable against setting lower height. I bet we had a place that did this but by dropping adjacency requirement better to make it resilient against this. Objections?
On the other side with a new headers sorting in store.Store.Append we cannot set lower header anymore (even if 2 parallel calls to .Append will happen we still write them sequentially).
(hm, what if we write [100 .. 150) and then [90..100))
| from, to := headers[0].Height(), headers[ln-1].Height() | ||
| if height+1 != from && height != 0 { // height != 0 is needed to enable init from any height and not only 1 |
There was a problem hiding this comment.
This is what I mentioned in our Slack conversation. We don't wait adjacent headers anymore + don't fail.
| @@ -319,6 +321,10 @@ func (s *Store[H]) Append(ctx context.Context, headers ...H) error { | |||
| head = *headPtr | |||
| } | |||
|
|
|||
| slices.SortFunc(headers, func(a, b H) int { | |||
There was a problem hiding this comment.
Sort headers in ascending order before processing.
| req <- h // reqs must always be buffered, so this won't block | ||
| } | ||
| delete(hs.heightReqs, height) | ||
| for _, h := range headers { |
There was a problem hiding this comment.
This is one of the places where we assume continuous range of headers. If headers doesn't have gaps then headers[height-from] works fine (line 123 above).
I will revert this change, this was done purely for the discussion.
cristaloleg
requested review from
tzdybal,
renaynay,
Wondertan,
vgonkivs,
gupadhyaya,
walldiss and
ramin
as code owners
renaynay
changed the title
|
Closing in favour of #205 |
Overview