refactor(consensus): separate sending votes from "pick votes" logic #2659
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cason
approved these changes
Mar 19, 2024
There was a problem hiding this comment.
Good refactoring. Separating the logic for picking a vote to send to a peer from the actual sending logic is a good improvement.
I left some comments with suggestions. I would separate the most common logic, pick a vote from the current height, from the catchup logic, that takes votes from previous heights.
melekes
reviewed
Mar 20, 2024
cason
reviewed
Mar 20, 2024
4 tasks
cason
reviewed
Mar 20, 2024
Co-authored-by: Daniel <daniel.cason@informal.systems>
…tHasVote` --> `sendVoteSetHasVote`
cason
reviewed
Mar 20, 2024
20 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 17, 2024
…gic (#2663) This was done by @ebuchman as part of a refactoring spree :-) The goal of this is to improve testability of `gossipDataRoutine` by separating out the logic for picking which block part to send. We introduced a new pure function, `pickPartToSend`, which returns a block part, and then call the new `SendPartSetHasPart` method to send it and update the peer state. This greatly simplifies the loop in `gossipDataRoutine`, and allows the `pickPartToSend` function to be independently tested. Note for reviewers: * This PR is built on top of #2659 * This is pure refactoring, no logic change was intended. * This PR can be reviewed commit by commit --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Ethan Buchman <ethan@coinculture.info>
mergify bot
pushed a commit
that referenced
this pull request
May 9, 2024
…gic (#2663) This was done by @ebuchman as part of a refactoring spree :-) The goal of this is to improve testability of `gossipDataRoutine` by separating out the logic for picking which block part to send. We introduced a new pure function, `pickPartToSend`, which returns a block part, and then call the new `SendPartSetHasPart` method to send it and update the peer state. This greatly simplifies the loop in `gossipDataRoutine`, and allows the `pickPartToSend` function to be independently tested. Note for reviewers: * This PR is built on top of #2659 * This is pure refactoring, no logic change was intended. * This PR can be reviewed commit by commit --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Ethan Buchman <ethan@coinculture.info> (cherry picked from commit 163f2d5) # Conflicts: # internal/consensus/reactor.go
Merged
4 tasks
|
@mergify backport v1.x |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
May 9, 2024
…2659) This is done by @adizere , @ebuchman , and @sergio-mena as part of a refactoring spree :-) The goal of this is to improve testability of gossipVotesRoutine by separating out the logic for picking which vote to send. We introduced a new pure function, pickVoteToSend, which returns a vote, and then call the new SendVoteSetHasVote method to send it. This greatly simplifies the loop in gossipVoteRoutine, and allows the pickVoteToSend function to be independently tested. Note for reviewers: * This is pure refactoring, no logic change was intended. * This PR can be reviewed commit by commit --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Daniel <daniel.cason@informal.systems> (cherry picked from commit 34e41c5)
4 tasks
melekes
pushed a commit
that referenced
this pull request
May 9, 2024
…backport #2659) (#3047) This is done by @adizere , @ebuchman , and @sergio-mena as part of a refactoring spree :-) The goal of this is to improve testability of gossipVotesRoutine by separating out the logic for picking which vote to send. We introduced a new pure function, pickVoteToSend, which returns a vote, and then call the new SendVoteSetHasVote method to send it. This greatly simplifies the loop in gossipVoteRoutine, and allows the pickVoteToSend function to be independently tested. Note for reviewers: * This is pure refactoring, no logic change was intended. * This PR can be reviewed commit by commit --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #2659 done by [Mergify](https://mergify.com). Co-authored-by: Sergio Mena <sergio@informal.systems>
melekes
added a commit
that referenced
this pull request
May 9, 2024
…gic (backport #2663) (#3046) This was done by @ebuchman as part of a refactoring spree :-) The goal of this is to improve testability of `gossipDataRoutine` by separating out the logic for picking which block part to send. We introduced a new pure function, `pickPartToSend`, which returns a block part, and then call the new `SendPartSetHasPart` method to send it and update the peer state. This greatly simplifies the loop in `gossipDataRoutine`, and allows the `pickPartToSend` function to be independently tested. Note for reviewers: * This PR is built on top of #2659 * This is pure refactoring, no logic change was intended. * This PR can be reviewed commit by commit --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #2663 done by [Mergify](https://mergify.com). --------- Co-authored-by: Daniel <daniel.cason@informal.systems> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is done by @adizere , @ebuchman , and @sergio-mena as part of a refactoring spree :-)
The goal of this is to improve testability of gossipVotesRoutine by separating out the logic for picking which vote to send. We introduced a new pure function, pickVoteToSend, which returns a vote, and then call the new SendVoteSetHasVote method to send it. This greatly simplifies the loop in gossipVoteRoutine, and allows the pickVoteToSend function to be independently tested.
Note for reviewers:
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments