refactor(consensus): separate sending block parts from "pick part" logic #2663
Conversation
… SendPartSetHasPart
Co-authored-by: Daniel <daniel.cason@informal.systems>
…tHasVote` --> `sendVoteSetHasVote`
| // continue the loop since prs is a copy and not affected by this initialization | ||
| return nil, true // continue OUTER_LOOP | ||
| } | ||
| part := pickPartForCatchup(heightLogger, rs, prs, blockStore) |
There was a problem hiding this comment.
In the original code, if we don't have the part to send, we:
- sleep: time.Sleep(conR.conS.config.PeerGossipSleepDuration)
- continue OUTER_LOOP (we do that in any case, after calling gossipDataForCatchup)
If we have the part, we continue to OUTER_LOOP. If we don't, we don't continue.
Different behavior again.
There was a problem hiding this comment.
This is indeed a different behavior. As commented for line 727, if sending the part to the peer fails, the main routine should sleep. This is achieved with continueLoop = false.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Me and @sergio-mena have discussed the differences in behavior of the original and the refactored code.
The conclusions were added as a comments. A minor refactoring is required, these changes will come in a following commit.
| }) { | ||
| ps.SetHasProposalBlockPart(prs.Height, prs.Round, index) | ||
| } | ||
| continue OUTER_LOOP |
There was a problem hiding this comment.
Observe that we continue even though the send operation fails (i.e., the BlockPart messages is not sent to the peer). Although this behavior can be discussed, this PR is not intended to change the existing behavior, but just to refactor the implementation.
| } else { | ||
| logger.Debug("Sending block part for catchup failed") | ||
| // sleep to avoid retrying too fast | ||
| time.Sleep(conR.conS.config.PeerGossipSleepDuration) |
There was a problem hiding this comment.
Contrary to what reported in the previous comment (line 607), if the send operation does not succeed s (i.e., the BlockPart message is not sent to the peer), the routine sleeps for config.PeerGossipSleepDuration before this method returns. After the call of this method (gossipDataForCatchup), on line 629, the main routine is started again (continue OUTER_LOOP).
When mapped to the new, refactored behavior, this means that the routine does not continue, sleeping at the end of the for loop. If the send operation does succeed, the main routine does continue, i.e., the for loop is restarted without a previous sleep step.
| // continue the loop since prs is a copy and not affected by this initialization | ||
| return nil, true // continue OUTER_LOOP | ||
| } | ||
| part := pickPartForCatchup(heightLogger, rs, prs, blockStore) |
There was a problem hiding this comment.
This is indeed a different behavior. As commented for line 727, if sending the part to the peer fails, the main routine should sleep. This is achieved with continueLoop = false.
| } | ||
| // continue the loop since prs is a copy and not effected by this initialization | ||
| if part, continueLoop := pickPartToSend(logger, conR.conS.blockStore, rs, ps, prs); part != nil { | ||
| if ps.SendPartSetHasPart(part, prs) { | ||
| continue OUTER_LOOP | ||
| } |
There was a problem hiding this comment.
If sending the part fails, we have two different behaviors:
- In line 607, as detailed in the commented there anchored, we should continue (i.e., restart the loop without sleeping)
- In line 727, as detailed in the commented there anchored, we should not continue (i.e, continue the loop, thus sleeping at the end of the loop)
This implementation does not allow for this distinction. In fact, in the absence of an else clause, we only implement the behavior 2.
|
@mergify backport v1.x |
✅ Backports have been created
|
…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
|
@mergify backport v1.x |
✅ Backports have been created
|
…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>
This was done by @ebuchman as part of a refactoring spree :-)
The goal of this is to improve testability of
gossipDataRoutineby 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 newSendPartSetHasPartmethod to send it and update the peer state. This greatly simplifies the loop ingossipDataRoutine, and allows thepickPartToSendfunction to be independently tested.Note for reviewers:
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments