Do not require a minimum block height when downloading headers or blocks #3911
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
fab-10
force-pushed
the
update-chain-state-post-merge
branch
6 times, most recently
from
7ca3017
to
18f7c53
Compare
fab-10
changed the title
fab-10
force-pushed
the
update-chain-state-post-merge
branch
from
18f7c53
to
e21382e
Compare
macfarla
approved these changes
Jun 16, 2022
| @@ -48,7 +47,6 @@ | |||
| final MetricsSystem metricsSystem) { | |||
| super(ethContext, 3, List::isEmpty, metricsSystem); | |||
| this.protocolSchedule = protocolSchedule; | |||
| this.minimumRequiredBlockNumber = minimumRequiredBlockNumber; | |||
There was a problem hiding this comment.
should minimumRequiredBlockNumber be removed from the constructor also? L47
fab-10
force-pushed
the
update-chain-state-post-merge
branch
from
e21382e
to
8bdeadb
Compare
…esponse Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Peers could have that header, bacause our internal record about the status of the peer could not always be up-to-date, so it is better to avoid setting that constraint when selecting peers to download block headers. 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>
fab-10
force-pushed
the
update-chain-state-post-merge
branch
from
8bdeadb
to
a57c70f
Compare
garyschulte
pushed a commit
that referenced
this pull request
Jul 7, 2022
…cks (#3911) * If needed update peer chain state when processing the block headers response Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> * Do not require a minimum block height when downloading headers or blocks Peers could have that header, bacause our internal record about the status of the peer could not always be up-to-date, so it is better to avoid setting that constraint when selecting peers to download block headers. Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> * Add CHANGELOG Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> * Complete the removal of minimumRequiredBlockNumber from constructors Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Co-authored-by: Sally MacFarlane <sally.macfarlane@consensys.net>
garyschulte
added a commit
that referenced
this pull request
Jul 7, 2022
49e4fd8 -> worldstate not avail (#4069) 6aa8812 -> stateroot mismatch (#4041) 043191a -> jwt auth on websockets (#4039) 90f891b -> do not move head on exec and propose (#4013) 3baa4da -> upgrade for websockets (#4019) 5024c07 -> sepolia TTD (#4024) 5ee9be8 -> heal step in snap (#3920) 261b1e0 -> remove peer block height requirements (#3911) Signed-off-by: garyschulte <garyschulte@gmail.com>
garyschulte
added a commit
that referenced
this pull request
Jul 7, 2022
49e4fd8 -> worldstate not avail (#4069) 6aa8812 -> stateroot mismatch (#4041) 043191a -> jwt auth on websockets (#4039) 90f891b -> do not move head on exec and propose (#4013) b5fa62c -> sync check before processing remote transactions (4035) 3baa4da -> upgrade for websockets (#4019) 5024c07 -> sepolia TTD (#4024) 5ee9be8 -> heal step in snap (#3920) 261b1e0 -> remove peer block height requirements (#3911) Signed-off-by: garyschulte <garyschulte@gmail.com>
Merged
eum602
pushed a commit
to lacchain/besu
that referenced
this pull request
Nov 3, 2023
…cks (hyperledger#3911) * If needed update peer chain state when processing the block headers response Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> * Do not require a minimum block height when downloading headers or blocks Peers could have that header, bacause our internal record about the status of the peer could not always be up-to-date, so it is better to avoid setting that constraint when selecting peers to download block headers. Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> * Add CHANGELOG Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> * Complete the removal of minimumRequiredBlockNumber from constructors Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Co-authored-by: Sally MacFarlane <sally.macfarlane@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.
After The Merge, there will be no more broadcating of the following messages:
NewBlockNewBlockHashesand we used these messages to update the status (height and difficulty) of a peer.
The status of a peer is set at the beginning of the connection, and then update using the above messages, but after the switch to PoS, since there messages are no longer sent, the status of a peer will remain the same for the duration of the connection, and this could mean that Besu is not able to query for block headers or blocks anymore, since some of the messages currently enforce that a peer has minimum height before we send them.
This PR address this issue, removing the constraint that a peer must have a minimum height, before sending block header or block requests, since we actually do not known which is the current status of the peer, and also try to parse peer responses for information that could be useful to update the peer status.
Even if the removal of the constraint is mandatory for the Merge, it could also benefit PoW networks, since assuming that the records keep for any peer is always up-to-date, but this could not be the case, since the peer could send some messages only to a subset of its peers, or some messages could not be processed.
This shold not introduce too much overhead, because Besu tries first with the best peers, the one that should be able to respond to our request.
Signed-off-by: Fabio Di Fabio fabio.difabio@consensys.net
PR description
Fixed Issue(s)
Documentation
doc-change-requiredlabel to this PR ifupdates are required.
Changelog