-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Closing in favour of #205 |
Overview