Skip to content
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.

Sign up for GitHub

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

Jump to bottom

service/header: implement basic syncer #212

Merged
merged 11 commits into from
Nov 18, 2021
Merged

service/header: implement basic syncer #212

merged 11 commits into from
Nov 18, 2021

Conversation

Wondertan
Copy link
Member

Content

  • Improved logging
  • LocalExchange helper
  • Syncer

Refs

Didn't found respective issue syncing

Notes

There is so much to improve and multiple ways to implement header synchronization. I was pulling my head apart with implementation to cover all the edge cases and to be bandwidth cheap, but realized that I need more time which we don't have, so I made a simplest possible syncing for now which should just work for most cases.

@Wondertan Wondertan self-assigned this Nov 18, 2021
renaynay
renaynay reviewed Nov 18, 2021
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM minus some questions

service/header/local_exchange.go Show resolved Hide resolved
service/header/sync.go Outdated Show resolved Hide resolved
service/header/sync.go Outdated Show resolved Hide resolved
service/header/sync.go Show resolved Hide resolved
service/header/local_exchange.go Show resolved Hide resolved
renaynay
renaynay previously approved these changes Nov 18, 2021
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gg

liamsi
liamsi previously approved these changes Nov 18, 2021
View reviewed changes
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I'm a bit confused by the validator logic and suggest to change the comment but optional.

service/header/sync.go Outdated Show resolved Hide resolved
service/header/sync.go Show resolved Hide resolved
service/header/sync.go Show resolved Hide resolved
@Wondertan Wondertan dismissed stale reviews from liamsi and renaynay via aa0ba97 November 18, 2021 17:02
@Wondertan @liamsi
Update service/header/sync.go
c05afff 
Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>
@Wondertan Wondertan merged commit b5b9cab into main Nov 18, 2021
@Wondertan Wondertan deleted the hlib/header-sync branch November 18, 2021 17:09
@Wondertan Wondertan restored the hlib/header-sync branch January 7, 2022 14:40
@Wondertan Wondertan deleted the hlib/header-sync branch January 7, 2022 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@renaynay renaynay renaynay approved these changes

@liamsi liamsi liamsi approved these changes

@mattdf mattdf Awaiting requested review from mattdf

Assignees

@Wondertan Wondertan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Wondertan @liamsi @renaynay