Fix for not marking peer as low quality too soon if there is only one peer connected#7067
Fix for not marking peer as low quality too soon if there is only one peer connected#7067ManiBAJPAI22 wants to merge 10 commits into
Conversation
| } | ||
| peerLastFailures++; | ||
|
|
||
| if (peerLastFailures > AllowedInvalidResponses) |
There was a problem hiding this comment.
Please cleanup indentation
Indentations updated
Update SnapSyncFeed.cs
|
Indentations managed |
|
|
||
| return SyncResponseHandlingResult.OK; | ||
| } | ||
| else |
There was a problem hiding this comment.
Won't this mark peer as lesser quality unless all failures were that peer? (i.e. if its less than all, it will also mark as Lesser)
The key changes are: Introduced a new boolean variable peerIsOnlyFailure and initialized it to true. While iterating through _resultLog, if a failure is encountered from a peer other than the current peer, peerIsOnlyFailure is set to false. When checking if peerLastFailures is greater than AllowedInvalidResponses, the code first checks the value of peerIsOnlyFailure. If peerIsOnlyFailure is true, it means the current peer is solely responsible for all the failures, so it updates the pivot and returns SyncResponseHandlingResult.OK. If peerIsOnlyFailure is false, it means there are failures from other peers as well, so it marks the current peer as SyncResponseHandlingResult.LesserQuality.
Update/2 with new boolean variable peerIsOnlyFailure SnapSyncFeed.cs
|
I've updated the code to address the concern regarding the peer quality evaluation logic. I nearly missed that detail, so thank you for pointing it out. |
|
@benaadams @asdacap |
| } | ||
|
|
||
| public SyncResponseHandlingResult AnalyzeResponsePerPeer(AddRangeResult result, PeerInfo peer) | ||
| { |
There was a problem hiding this comment.
Indentation is still wrong.
There was a problem hiding this comment.
Sorry for the delayed response.
I guess now Indentations are managed.
Indentations managed
Update SnapSyncFeed.cs
|
Should put a unit tests for this case though. |
|
Yeah checking on it ! |
|
Closing — the unit test asdacap requested in Sept 2024 was never added, and the branch is ~600 days stale. If #6803 still reproduces, please reopen with a focused fix and tests. |
Fixes #6803
Please choose one of the keywords above to refer to the issue this PR solves followed by the issue number (e.g. Fixes #000). If no issue number, remove the line. Also, remove everything marked optional that is not applicable. Remove this note after reading.
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Optional. Remove if not applicable.
Documentation
Requires documentation update
If yes, link the PR to the docs update or the issue with the details labeled
docs. Remove if not applicable.Requires explanation in Release Notes
If yes, fill in the details here. Remove if not applicable.
Remarks
_If the number of consecutive failures from the current peer (peerLastFailures) exceeds the AllowedInvalidResponses threshold, and all the recent failures are from this peer
(allLastFailures == peerLastFailures), the peer is marked as LesserQuality.In a scenario where there is only one peer connected, this condition will be met quickly, leading to the peer being marked as
LesserQualityeven though there are no other peers to compare with.To address this issue and improve the behavior in situations with a single peer, we can modify the logic to update the pivot and retry with the same peer before marking it as
LesserQuality, when the current peer is the only one with failures, instead of immediately marking it asLesserQuality, the code will update the pivot, clear the result log, and log a message indicating that the pivot has been updated and the peer will be retried.The
SyncResponseHandlingResult.OKis returned, allowing the sync process to retry with the same peer after updating the pivot.If there are other peers with failures
(allLastFailures != peerLastFailures), then the original logic of marking the current peer as LesserQuality is preserved.This change should improve the behavior in scenarios with a single peer by giving the peer another chance after updating the pivot, before marking it as
LesserQuality. _