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
confirm pivot block - removed disconnect based on chain height estimate #6889
base: main
Are you sure you want to change the base?
Changes from 14 commits
e64112f
e44fcc2
da0454a
9a374ab
77b9ea7
d01a423
500843d
c825c47
5ebcdac
8f5ac92
7db08e9
ea60f72
d3d15f6
75da10f
efe7a64
755d41b
2ea6a32
a3dceb4
20dbba0
2d81237
8352324
47fae0a
94f3a3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,7 +74,8 @@ public SyncTargetManager( | |
protected CompletableFuture<Optional<EthPeer>> selectBestAvailableSyncTarget() { | ||
final BlockHeader pivotBlockHeader = fastSyncState.getPivotBlockHeader().get(); | ||
final EthPeers ethPeers = ethContext.getEthPeers(); | ||
final Optional<EthPeer> maybeBestPeer = ethPeers.bestPeerWithHeightEstimate(); | ||
// just use the default (merge) comparator | ||
final Optional<EthPeer> maybeBestPeer = ethPeers.bestPeer(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this might be a step back thinking about PoS networks, no much use for the comparison using TTD since this is basically a constant after chain switches to PoS. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it actually evaluates to the same thing in the end so I'm reverting this line change. |
||
if (maybeBestPeer.isEmpty()) { | ||
throttledLog( | ||
LOG::debug, | ||
|
@@ -93,19 +94,12 @@ protected CompletableFuture<Optional<EthPeer>> selectBestAvailableSyncTarget() { | |
return completedFuture(Optional.empty()); | ||
} else { | ||
final EthPeer bestPeer = maybeBestPeer.get(); | ||
if (bestPeer.chainState().getEstimatedHeight() < pivotBlockHeader.getNumber()) { | ||
LOG.info( | ||
"Best peer {} has chain height {} below pivotBlock height {}. Waiting for better peers. Current {} of max {}", | ||
maybeBestPeer.map(EthPeer::getLoggableId).orElse("none"), | ||
maybeBestPeer.map(p -> p.chainState().getEstimatedHeight()).orElse(-1L), | ||
pivotBlockHeader.getNumber(), | ||
ethPeers.peerCount(), | ||
ethPeers.getMaxPeers()); | ||
ethPeers.disconnectWorstUselessPeer(); | ||
return completedFuture(Optional.empty()); | ||
} else { | ||
return confirmPivotBlockHeader(bestPeer); | ||
} | ||
LOG.debug( | ||
"attempting to confirm pivot block {} with best peer {}", | ||
pivotBlockHeader.getNumber(), | ||
bestPeer); | ||
|
||
return confirmPivotBlockHeader(bestPeer); | ||
} | ||
} | ||
|
||
|
@@ -142,6 +136,11 @@ private CompletableFuture<Optional<EthPeer>> confirmPivotBlockHeader(final EthPe | |
pivotBlockHeader.toLogString()); | ||
return confirmPivotBlockHeader(bestPeer); | ||
} else { | ||
LOG.debug( | ||
"Confirmed pivot block {} with peer {}", | ||
pivotBlockHeader.getNumber(), | ||
bestPeer.getLoggableId()); | ||
bestPeer.chainState().updateHeightEstimate(pivotBlockHeader.getNumber()); | ||
return CompletableFuture.completedFuture(Optional.of(bestPeer)); | ||
} | ||
}) | ||
|
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.
My suggestion is, we should probably eliminate this TrailingPeer validation (since there's no garantee that peers are going to have their chain height estimate up-to-date) but for test purpose we can easily skip the trailing validation by assigning Long.MAX_VALUE to the second param of the TrailingPeerRequirements constructor