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

feat(header/p2p): implement retry mechanism for header/p2p session #1477

Merged
merged 7 commits into from
Dec 23, 2022
Merged

feat(header/p2p): implement retry mechanism for header/p2p session #1477

merged 7 commits into from
Dec 23, 2022

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Dec 12, 2022 •
edited
Loading

Overview

Resolves #1406

  • implement a retry mechanism in case of failed requests;
  • punish peer in case of invalid data;
  • fix issue with peer scoring;

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@vgonkivs vgonkivs added area:header Extended header area:p2p labels Dec 12, 2022
@vgonkivs vgonkivs self-assigned this Dec 12, 2022
@vgonkivs vgonkivs added the kind:feat Attached to feature PRs label Dec 12, 2022
Wondertan
Wondertan reviewed Dec 13, 2022
View reviewed changes
header/p2p/session.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs marked this pull request as ready for review December 13, 2022 14:59
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2022 •
edited
Loading

Codecov Report

Merging #1477 (c3fca79) into main (52048ce) will increase coverage by 0.43%.
The diff coverage is 76.19%.

@@            Coverage Diff             @@
##             main    #1477      +/-   ##
==========================================
+ Coverage   55.37%   55.80%   +0.43%     
==========================================
  Files         199      209      +10     
  Lines       12053    12819     +766     
==========================================
+ Hits         6674     7154     +480     
- Misses       4714     4944     +230     
- Partials      665      721      +56
Impacted Files Coverage Δ
header/p2p/peer_tracker.go 78.57% <66.66%> (+9.12%) ⬆️
header/p2p/session.go 77.85% <74.07%> (+0.86%) ⬆️
header/p2p/exchange.go 75.16% <100.00%> (+0.47%) ⬆️
header/p2p/peer_stats.go 96.96% <100.00%> (+3.42%) ⬆️
nodebuilder/header/constructors.go 75.47% <100.00%> (+0.47%) ⬆️
nodebuilder/share/module.go 58.73% <0.00%> (-18.36%) ⬇️
share/empty.go 86.66% <0.00%> (-13.34%) ⬇️
header/p2p/helpers.go 50.00% <0.00%> (-10.42%) ⬇️
nodebuilder/share/constructors.go 92.10% <0.00%> (-7.90%) ⬇️
nodebuilder/p2p/flags.go 65.82% <0.00%> (-1.65%) ⬇️
... and 50 more

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

@vgonkivs vgonkivs mentioned this pull request Dec 13, 2022
Closed
19 tasks
distractedm1nd
distractedm1nd requested changes Dec 15, 2022
View reviewed changes
header/p2p/peer_tracker.go Outdated Show resolved Hide resolved
header/p2p/peer_tracker.go Outdated Show resolved Hide resolved
header/p2p/session.go Show resolved Hide resolved
header/p2p/session.go Show resolved Hide resolved
@vgonkivs vgonkivs force-pushed the implement_retry_mechanism branch 4 times, most recently from 9a5e2df to a873700 Compare December 19, 2022 14:29
distractedm1nd
distractedm1nd previously approved these changes Dec 20, 2022
View reviewed changes
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

optional suggestion

header/p2p/session.go Show resolved Hide resolved
Wondertan
Wondertan requested changes Dec 21, 2022
View reviewed changes
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, modulo 3 comments

header/p2p/peer_tracker.go Outdated Show resolved Hide resolved
header/p2p/peer_tracker.go Outdated Show resolved Hide resolved
header/p2p/session.go Outdated Show resolved Hide resolved
distractedm1nd
distractedm1nd previously approved these changes Dec 22, 2022
View reviewed changes
@vgonkivs vgonkivs changed the title feat: implement retry mechanism for header/p2p session feat(header/p2p)!: implement retry mechanism for header/p2p session Dec 22, 2022
renaynay
renaynay reviewed Dec 22, 2022
View reviewed changes
header/p2p/peer_stats.go Show resolved Hide resolved
header/p2p/session.go Outdated Show resolved Hide resolved
header/p2p/exchange_test.go Show resolved Hide resolved
header/p2p/exchange_test.go Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Dec 22, 2022
View reviewed changes
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, +1 to Rene's comments

@vgonkivs vgonkivs merged commit f25b5f1 into celestiaorg:main Dec 23, 2022
@vgonkivs vgonkivs changed the title feat(header/p2p)!: implement retry mechanism for header/p2p session feat(header/p2p): implement retry mechanism for header/p2p session Jan 9, 2023
@vgonkivs vgonkivs deleted the implement_retry_mechanism branch January 9, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wondertan Wondertan Wondertan approved these changes

@renaynay renaynay renaynay approved these changes

@distractedm1nd distractedm1nd Awaiting requested review from distractedm1nd distractedm1nd is a code owner

Assignees

@vgonkivs vgonkivs

Labels
area:header Extended header area:p2p kind:feat Attached to feature PRs
Projects
No open projects
Celestia Node
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

improvement(header/p2p): implement retry mechanism for p2p.session
5 participants
@vgonkivs @codecov-commenter @distractedm1nd @Wondertan @renaynay