Optimize pivot block selector on PoS networks #4488
Merged
Conversation
This file contains 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
Renaming to make clear everywhere that the forkchoice is not verified, so it will be clear in case there will be a future event for a verified forkchoice. Finalized block hash no more optional. Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
On PoS network we use a pivot block sent by the Consensus Layer, so we do not need peers, and so all the logic for selecting the pivot block from peers has been moved from FastSyncActions to PivotSelectorFromPeers. We do not need anymore the TransictionPeerSelector, and the --fast-sync-min-peers applies only to PoW networks. Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
fab-10
changed the title
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
fab-10
added
mainnet
syncing
doc-change-required
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
# Conflicts: # CHANGELOG.md # besu/src/main/java/org/hyperledger/besu/controller/BesuControllerBuilder.java
2 tasks
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
# Conflicts: # besu/src/main/java/org/hyperledger/besu/controller/BesuControllerBuilder.java
Renaming to make clear everywhere that the forkchoice is not verified, so it will be clear in case there will be a future event for a verified forkchoice. Finalized block hash no more optional. Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
…ze-snap-sync3 Signed-off-by: garyschulte <garyschulte@gmail.com>
garyschulte
force-pushed
the
optimize-snap-sync3
branch
from
01279f0 to
ecc7a8a
Compare
Signed-off-by: Justin Florentine <justin+github@florentine.us>
jflo
force-pushed
the
optimize-snap-sync3
branch
from
ecc7a8a to
d350adc
Compare
Signed-off-by: Justin Florentine <justin+github@florentine.us>
jflo
approved these changes
Oct 6, 2022
garyschulte
approved these changes
Oct 6, 2022
Comment on lines
-72
to
73
| .waitForSuitablePeers(FastSyncState.EMPTY_SYNC_STATE) | ||
| CompletableFuture.completedFuture(FastSyncState.EMPTY_SYNC_STATE) | ||
| .thenCompose(syncActions::selectPivotBlock) |
There was a problem hiding this comment.
nit, but rather than composing a static completed future, couldn't this be reduced to
fastSyncActions.selectPivotBlock(fastSyncState)
There was a problem hiding this comment.
I did this way to avoid that an exception threw from fastSyncActions.selectPivotBlock(fastSyncState) escape the future chain, so I can handle all the exception in the same place, instead of putting an extra try...catch around fastSyncActions.selectPivotBlock(fastSyncState), do you have another idea on how to improve this?
Comment on lines
-97
to
+96
| .waitForSuitablePeers(fastSyncState) | ||
| CompletableFuture.completedFuture(fastSyncState) |
There was a problem hiding this comment.
same comment about unnecessary compose
| .whenComplete( | ||
| (blockHeader, throwable) -> { | ||
| if (throwable != null) { | ||
| LOG.debug("Error downloading block header by hash {}", hash); |
There was a problem hiding this comment.
it might be useful to log the throwable too
| ethContext, | ||
| metricsSystem, | ||
| currentState.getPivotBlockNumber().getAsLong(), | ||
| syncConfig.getFastSyncMinimumPeerCount(), |
There was a problem hiding this comment.
this is going to default to 5 since we no longer override this value to 1 for PoS networks?
There was a problem hiding this comment.
on PoS this is never called, since we always have the pivot block hash
rolandtyler
removed
the
doc-change-required
eum602
pushed a commit
to lacchain/besu
that referenced
this pull request
Nov 3, 2023
* Refactor to optimize pivot block selector on PoS networks On PoS network we use a pivot block sent by the Consensus Layer, so we do not need peers, and so all the logic for selecting the pivot block from peers has been moved from FastSyncActions to PivotSelectorFromPeers. We do not need anymore the TransictionPeerSelector, and the --fast-sync-min-peers applies only to PoW networks. Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
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.
PR description
This PR is built on top of #4487 so check it first
This is the 3rd PR of a series to optimize the initial sync on PoS networks, that is ongoing here #4462, but since it is quite big, I am splitting it in smaller PRs to simplify the code review.
On PoS network we use as pivot block, the last finalized block sent by the Consensus Layer, so we do
not need peers, and so all the logic for selecting the pivot block from peers
has been moved from
FastSyncActionstoPivotSelectorFromPeers.We do not need anymore the
TransictionPeerSelector, and the--fast-sync-min-peersapplies only to PoW networks.
Fixed Issue(s)
Documentation
doc-change-requiredlabel to this PR ifupdates are required.
Changelog