besu
besu copied to clipboard
Record empty responses when retrying a peer task
PR description
AbstractRetryingPeerTask
has a way to understand if the result of a try is empty, but it only uses this information to discriminate if the result is a partial one, instead is very useful to also use the emptiness information to demote the peer and eventually disconnect it in case it sends too many useless responses, as done in this PR.
In the making of this PR, I discovered that there were opportunities to improve the code and simplify the writing of retrying tasks, so I refactored and documented the code so that any class extending AbstractRetryingPeerTask
should not set the final task result by themself, but instead implements the emptyResult
and successfulResult
to report the status of the request, so that the final setting of the task result is always a duty of AbstractRetryingPeerTask
, removing the different approaches used before.
relates to #5415 and #5271
Tests
- [ ] Checkpoint Sync
- [ ] Snap Sync
- [ ] Fast Sync
- [ ] Full sync
- [x] I thought about documentation and added the
doc-change-required
label to this PR if updates are required. - [ ] I have considered running
./gradlew acceptanceTestNonMainnet
locally if my PR affects non-mainnet modules. - [ ] I thought about the changelog and included a changelog update if required.
- [x] If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests
SGTM. Do we demote the peer as soon as we get one empty response from it? If so, are there not cases where we receive an empty result but still want to retry with the same peer?
Blocked by #6609 - without that we would disconnect peers, because we are trying peers that do not serve snap data.
@Matilda-Clerke his could be superseded by https://github.com/hyperledger/besu/pull/7602 too
@fab-10 I haven't included any feedback regarding peer performance or quality in the refactor yet, so it should be possible to ensure we provide feedback for empty responses too. I'll take a good look at this PR to make sure I understand the intended behaviour.