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

Keep Alive protocol #2251

Merged
merged 9 commits into from
Aug 12, 2020
Merged

Keep Alive protocol #2251

merged 9 commits into from
Aug 12, 2020

Conversation

karknu
Copy link
Contributor

@karknu karknu commented Jun 12, 2020

No description provided.

@semanticphilosopher
Copy link
Contributor

semanticphilosopher commented Jun 16, 2020 via email

@karknu
Copy link
Contributor Author

karknu commented Jun 16, 2020

That is a great idea. I will define a local maxG for now.

@karknu karknu force-pushed the karknu/keep-alive branch 7 times, most recently from 9d35129 to e4991fb Compare June 17, 2020 06:06
@coot
Copy link
Contributor

coot commented Jun 17, 2020

Part of #1416

@karknu karknu force-pushed the karknu/keep-alive branch 4 times, most recently from e317925 to a69f143 Compare July 7, 2020 13:03
@karknu karknu force-pushed the karknu/keep-alive branch 4 times, most recently from f6ed7c2 to dfa6cfe Compare July 28, 2020 17:33
@karknu karknu force-pushed the karknu/keep-alive branch 4 times, most recently from 040548a to 042470e Compare August 5, 2020 11:26
@karknu karknu marked this pull request as ready for review August 6, 2020 08:00
@karknu karknu requested a review from mrBliss August 6, 2020 08:00
@karknu
Copy link
Contributor Author

karknu commented Aug 10, 2020

This branch includes a commit that's in master.

I know, I needed it for testing which was done with a 1.18.0 node.

Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

Looks good, some comments / suggestions follows.

@mrBliss
Copy link
Contributor

mrBliss commented Aug 11, 2020

This branch includes a commit that's in master.

I know, I needed it for testing which was done with a 1.18.0 node.

It makes reviewing this PR harder for me, because that commit touches many consensus files, but I have to ignore these changes. Moreover, I believe it's the cause of the merge conflicts.

@karknu karknu force-pushed the karknu/keep-alive branch 2 times, most recently from d203902 to cc3709c Compare August 11, 2020 11:35
@karknu
Copy link
Contributor Author

karknu commented Aug 11, 2020

It makes reviewing this PR harder for me, because that commit touches many consensus files, but I have to ignore these changes. Moreover, I believe it's the cause of the merge conflicts.

The PR is intended to be reviewed one commit at the time, but in you're case I guess you're mostly interested in reviewing the changes at the consensus layer so that doesn't help. I've rebased on top of master and the extra commit is no longer there.

Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

LGTM

karknu and others added 9 commits August 12, 2020 15:20
Implement smooth switching from slower to faster peers by
temporarily ignoring the concurrency limit for faster peers.
This lets faster peers starve out slower peers and makes it possible
to switch peers without first having to wait for all outstanding
requests to the slower peers to finish.
Give peers with outstanding bytes an advantage when ordering peers.
Add a random cookie to keep-alive messages so that a rouge responder
can't send a MsgKeepAliveResponse before it has read the MsgKeepAlive.
Temporarily increase the keep-alive timeout to 60s.
@karknu
Copy link
Contributor Author

karknu commented Aug 12, 2020

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 12, 2020

@iohk-bors iohk-bors bot merged commit 141a4c1 into master Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants