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.

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(header/p2p): implement GetVerifiedRangeByHeight #1305

Merged
merged 8 commits into from
Nov 30, 2022

Conversation

@vgonkivs vgonkivs added area:header Extended header area:p2p kind:feat Attached to feature PRs labels Nov 1, 2022
@vgonkivs vgonkivs self-assigned this Nov 1, 2022
@vgonkivs vgonkivs force-pushed the implement_get_verified_range branch 2 times, most recently from 1e0c0ea to 53d9e8b Compare November 1, 2022 11:13
@vgonkivs vgonkivs force-pushed the implement_get_verified_range branch 3 times, most recently from 725069d to 78e5373 Compare November 29, 2022 10:32
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

LGTM, module Adjacent verification.

@vgonkivs and I discussed in sync how to proceed with this work. Once we merge it, Slava will continue working on Verified implementation with retries and penalties. Then we will migrate Syncer over the Verified range and then deprecate the regular GetRange

header/interface.go Outdated Show resolved Hide resolved
header/p2p/session.go Outdated Show resolved Hide resolved
header/store/store.go Outdated Show resolved Hide resolved
header/p2p/exchange.go Outdated Show resolved Hide resolved
header/core/exchange.go Outdated Show resolved Hide resolved
header/interface.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #1305 (4fdd06f) into main (c71c036) will increase coverage by 0.51%.
The diff coverage is 69.88%.

@@            Coverage Diff             @@
##             main    #1305      +/-   ##
==========================================
+ Coverage   55.35%   55.87%   +0.51%     
==========================================
  Files         182      186       +4     
  Lines       11072    11463     +391     
==========================================
+ Hits         6129     6405     +276     
- Misses       4328     4429     +101     
- Partials      615      629      +14     
Impacted Files Coverage Δ
header/core/exchange.go 40.90% <0.00%> (-9.10%) ⬇️
header/interface.go 0.00% <ø> (ø)
header/local/exchange.go 61.90% <0.00%> (-6.52%) ⬇️
header/store/store.go 62.26% <0.00%> (-3.74%) ⬇️
header/p2p/peer_tracker.go 57.35% <57.35%> (ø)
header/p2p/session.go 72.48% <72.48%> (ø)
header/p2p/exchange.go 67.95% <89.28%> (-1.23%) ⬇️
header/p2p/peer_stats.go 93.54% <93.54%> (ø)
nodebuilder/header/constructors.go 79.16% <100.00%> (+7.73%) ⬆️
header/p2p/server.go 66.37% <0.00%> (-5.31%) ⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

header/local/exchange.go Outdated Show resolved Hide resolved
header/interface.go Outdated Show resolved Hide resolved
header/interface.go Outdated Show resolved Hide resolved
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.

Are we going to get a follow-up PR that will use GetVerifiedRange in place of GetRange at some point? (in the syncer, for example)

header/local/exchange.go Outdated Show resolved Hide resolved
header/core/exchange.go Outdated Show resolved Hide resolved
header/local/exchange.go Outdated Show resolved Hide resolved
header/p2p/exchange.go Outdated Show resolved Hide resolved
header/store/store.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member

@renaynay, check the top comment on my previous review to answer the your question

@renaynay
Copy link
Member

ack @Wondertan

Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
@vgonkivs vgonkivs merged commit 8a6de42 into celestiaorg:main Nov 30, 2022
@vgonkivs vgonkivs deleted the implement_get_verified_range branch January 9, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:header Extended header area:p2p kind:feat Attached to feature PRs
Projects
No open projects
Status: Done
5 participants