-
Notifications
You must be signed in to change notification settings - Fork 804
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use retry switching peer for world state download tasks #5508
Conversation
Signed-off-by: Fabio Di Fabio <[email protected]>
Signed-off-by: Fabio Di Fabio <[email protected]>
Signed-off-by: Fabio Di Fabio <[email protected]>
Signed-off-by: Fabio Di Fabio <[email protected]>
|
super( | ||
ethContext, | ||
metricsSystem, | ||
data -> data.proofs().isEmpty() && data.slots().isEmpty(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bad peer will be a peer with this kind of result
I’m not sure you are checking if the response is like that in order to switch to another peer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check this other PR #5509, that checks and records the empty response in the AbstractRetryingPeerTask
Signed-off-by: Fabio Di Fabio <[email protected]>
Signed-off-by: Fabio Di Fabio <[email protected]>
…e-retry-switching-peer # Conflicts: # ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetAccountRangeFromPeerTask.java # ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetBytecodeFromPeerTask.java # ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetStorageRangeFromPeerTask.java # ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetTrieNodeFromPeerTask.java # ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/RetryingGetNodeDataFromPeerTask.java
…e-retry-switching-peer
Signed-off-by: Fabio Di Fabio <[email protected]>
…e-retry-switching-peer
Signed-off-by: Fabio Di Fabio <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff looks pretty straightforward. Couple of small comments. Don't want to approve while the parent PR is in flight
@@ -155,7 +166,8 @@ public void completesWhenPeersAreTemporarilyUnavailable() | |||
final T requestedData = generateDataToBeRequested(); | |||
|
|||
// Execute task and wait for response | |||
final EthTask<T> task = createTask(requestedData); | |||
// +2 is required for switching peers tasks where the max retries = number of peers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useful comment 👍
@@ -320,7 +320,9 @@ public Stream<EthPeer> streamAvailablePeers() { | |||
} | |||
|
|||
public Stream<EthPeer> streamBestPeers() { | |||
return streamAvailablePeers().sorted(getBestChainComparator().reversed()); | |||
return streamAvailablePeers() | |||
.filter(EthPeer::isFullyValidated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this required?
Just a question. A peer that is useless for the worldstate part may not be for the blockchain part. Is there not a risk of losing valid nodes for a certain type of data by marking useless when it could still help us. For example Besu and Nethermind which do not provide Snapsync data but remain valid for the blockchain. Should not have a more complex notion of useless ? |
testing currently the PR with my snapsync boost feature |
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Blocked by #6609 - without that we would disconnect peers, because we are trying peers that do not serve snap data. |
@fab-10 Closing this, reopen if think we still need it |
PR description
Built on top of #5509, so please check it first. Link to only see diff again #5509
With this PR all the world state download tasks used by snap sync are updated to use the retry switching peer strategy, to reduce the chance that we keep sending requests to the same bad peer.
relates to #5415 and #5271