-
Notifications
You must be signed in to change notification settings - Fork 971
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
header: hardening syncing logic #334
Conversation
aa88d6a
to
2541ac2
Compare
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.
Generally looks very good. Left some comments and agree with @adlerjohn and most of @renaynay's comments.
f816e7c
to
36caec9
Compare
I was changing network interfaces, hibernating, suspending for long, and still, my Light Node was syncing without needing to restart. |
18775e6
to
c614f5b
Compare
Changes since your last reviews
|
Tests are failing due to known #340. |
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.
need to pass over it again tomorrow.
8347ad0
to
318ecff
Compare
There are two broken tests:
|
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.
Okay, let's go 👍
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.
Almost ready.
Co-authored-by: rene <[email protected]>
Co-authored-by: rene <[email protected]>
Co-authored-by: rene <[email protected]>
@renaynay, responded |
@Wondertan sir responded |
@renaynay, time to approve the behemoth beast |
f861652
to
094553a
Compare
One of the suggestions broke the linter and I had to fix it, therefore I need reapprovals @Bidon15 |
094553a
to
5eddee9
Compare
Co-authored-by: rene <[email protected]>
5eddee9
to
96280ba
Compare
Broken test is a flakey one, but will be fixed by a younger bro |
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.
LEMS (let's end my suffering)
Lol, no drama pls |
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.
Looks behemoth to me :D
But seriously, Thanks for tackling this, thanks fort the thorough reviews and for addressing all feedback.
I really hope we can split PRs like this into smaller PRs in the future.
Previously, we were syncing only once on
Start
and were catching up the chain by Pubsub topic. This PR changes that we can sync multiple times during runtime and if we missed some new Headers from PubSub topic, that will trigger sync again to catch-up. Also, this PR starts to verify new non-adjacent headers from PubSub and caches them to later be applied adjacently to avoid unnecessary request of already rcvd headersAlso fixes a variety of minor things.
Depends on #327
Closes: #242
Closes: #326