feat: retry heights when adding proposal#1724
Closed
rach-id wants to merge 4 commits intofeature/recoveryfrom
Closed
feat: retry heights when adding proposal#1724rach-id wants to merge 4 commits intofeature/recoveryfrom
rach-id wants to merge 4 commits intofeature/recoveryfrom
Conversation
…mitment # Conflicts: # consensus/propagation/reactor.go
rach-id
requested review from
evan-forbes and
rootulp
and removed request for
a team
Contributor
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
consensus/propagation/reactor.go:91
- Changing the ticker frequency from 2 seconds to 30 seconds is a significant update; ensure that this change aligns with the intended catchup behavior and that tests appropriately cover the new timing.
ticker := time.NewTicker(30 * time.Second)
consensus/propagation/have_wants.go:35
- [nitpick] Commenting out the error log may hide useful diagnostic information when a have part for an unknown proposal is received; consider including a debug log or a brief comment explaining the rationale for silencing this error.
//blockProp.Logger.Error("received have part for unknown proposal", "peer", peer, "height", height, "round", round)
Comment on lines
+27
to
+34
| func (blockProp *Reactor) retryWants(height int64, round int32) { | ||
| if !blockProp.started.Load() { | ||
| return | ||
| } | ||
| data := blockProp.unfinishedHeights() | ||
| peers := blockProp.getPeers() | ||
| for _, prop := range data { | ||
| height, round := prop.compactBlock.Proposal.Height, prop.compactBlock.Proposal.Round | ||
|
|
||
| // don't re-request parts for any round on the current height | ||
| if height == currentHeight { | ||
| continue | ||
| for { | ||
| blockProp.Logger.Debug("retrying", "height", height, "round", round) |
There was a problem hiding this comment.
The new retryWants function uses an infinite loop with a sleep delay, which may lead to unbounded retries if the missing parts never resolve. Consider adding a maximum retry limit or timeout to prevent potential resource exhaustion.
rach-id
commented
Apr 11, 2025
| // this call simulates getting a commitment for a proposal of a higher | ||
| // height | ||
| n2.retryWants(2) | ||
| n2.retryWants(prop.Height, prop.Round) |
Member
Author
There was a problem hiding this comment.
this should be changed to better tests where we try:
- catchup multiple heights at the same time
- retry the same height
This file contains hidden or 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
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.
Description
Retries heights when adding a proposal while having a longer catchup frequency of 30s.
Also, retries heights the moment the propagation reactor starts processing in case it received any commitment during block/state sync.
PR checklist
.changelog(we useunclog to manage our changelog)
docs/orspec/) and code comments