[PAN-2333] Use only fully validated peers for fast sync pivot selection #21
Conversation
Signed-off-by: Meredith Baxter <[email protected]>
Signed-off-by: Meredith Baxter <[email protected]>
Signed-off-by: Meredith Baxter <[email protected]>
Signed-off-by: Meredith Baxter <[email protected]>
Signed-off-by: Meredith Baxter <[email protected]>
Signed-off-by: Meredith Baxter <[email protected]>
Signed-off-by: Meredith Baxter <[email protected]>
…ble-to-synchronizer
| ethProtocolManager = createEthProtocolManager(protocolContext, fastSyncEnabled); | ||
| ethProtocolManager = | ||
| createEthProtocolManager( | ||
| protocolContext, fastSyncEnabled, createPeerValidators(protocolSchedule)); |
There was a problem hiding this comment.
Move validator management into EthProtocolManager so that every EthPeer can track its validation state.
| @@ -52,28 +55,6 @@ public Istanbul64ProtocolManager( | |||
| ethereumWireProtocolConfiguration); | |||
| } | |||
|
|
|||
| public Istanbul64ProtocolManager( | |||
| @@ -86,6 +90,30 @@ protected boolean removeEldestEntry(final Map.Entry<Hash, Boolean> eldest) { | |||
| })); | |||
| this.chainHeadState = new ChainState(); | |||
| this.onStatusesExchanged.set(onStatusesExchanged); | |||
| for (PeerValidator peerValidator : peerValidators) { | |||
| validationStatus.put(peerValidator, false); | |||
There was a problem hiding this comment.
Track validation status within each EthPeer.
| } | ||
|
|
||
| @Override | ||
| public CompletableFuture<Boolean> validatePeer(final EthPeer ethPeer) { | ||
| public CompletableFuture<Boolean> validatePeer( | ||
| final EthContext ethContext, final EthPeer ethPeer) { |
There was a problem hiding this comment.
Validators are constructed before EthProtocolManager, so EthContext must be passed in.
| } | ||
|
|
||
| public static RespondingEthPeer create( |
There was a problem hiding this comment.
Replace factory methods with a Builder.
| @@ -90,6 +95,11 @@ public EthProtocolManager( | |||
|
|
|||
| this.blockBroadcaster = new BlockBroadcaster(ethContext); | |||
|
|
|||
| // Run validators | |||
| for (final PeerValidator peerValidator : this.peerValidators) { | |||
There was a problem hiding this comment.
Question: How fast is the validation ? Is it worth validating in parallel ?
There was a problem hiding this comment.
The runValidator method just sets up a listener, so I think we should be fine with a plain loop here.
| @@ -113,15 +114,15 @@ public FastSyncActions( | |||
| private CompletableFuture<FastSyncState> selectPivotBlockFromPeers() { | |||
| return ethContext | |||
| .getEthPeers() | |||
| .bestPeerWithHeightEstimate() | |||
| .bestPeerMatchingCriteria(this::canPeerDeterminePivotBlock) | |||
| } | ||
|
|
||
| @Test | ||
| public void checkPeer_doesNotScheduleFutureCheckWhenPeerNotReadyAndDisconnected() { | ||
| PeerValidator validator = mock(PeerValidator.class); |
There was a problem hiding this comment.
(suggestion) make the variable final
| EthProtocolManager ethProtocolManager = EthProtocolManagerTestUtil.create(); | ||
| EthProtocolManagerTestUtil.disableEthSchedulerAutoRun(ethProtocolManager); | ||
| EthPeer peer = EthProtocolManagerTestUtil.createPeer(ethProtocolManager).getEthPeer(); | ||
| EthPeer peer = |
There was a problem hiding this comment.
(suggestion) make the variable final
| verify(runner, never()).disconnectPeer(eq(peer)); | ||
| verify(runner, times(0)).scheduleNextCheck(eq(peer)); | ||
| } | ||
|
|
||
| @Test | ||
| public void checkPeer_handlesInvalidPeer() { | ||
| PeerValidator validator = mock(PeerValidator.class); |
There was a problem hiding this comment.
(suggestion) make the variable final
| EthProtocolManager ethProtocolManager = EthProtocolManagerTestUtil.create(); | ||
| EthProtocolManagerTestUtil.disableEthSchedulerAutoRun(ethProtocolManager); | ||
| EthPeer peer = EthProtocolManagerTestUtil.createPeer(ethProtocolManager).getEthPeer(); | ||
| EthPeer peer = |
There was a problem hiding this comment.
(suggestion) make the variable final
| } | ||
|
|
||
| @Test | ||
| public void checkPeer_handlesValidPeer() { | ||
| PeerValidator validator = mock(PeerValidator.class); |
There was a problem hiding this comment.
(suggestion) make the variable final
| EthProtocolManager ethProtocolManager = EthProtocolManagerTestUtil.create(); | ||
| EthProtocolManagerTestUtil.disableEthSchedulerAutoRun(ethProtocolManager); | ||
| EthPeer peer = EthProtocolManagerTestUtil.createPeer(ethProtocolManager).getEthPeer(); | ||
| EthPeer peer = |
There was a problem hiding this comment.
(suggestion) make the variable final
Signed-off-by: Meredith Baxter <[email protected]>
PR description
Expose
EthPeervalidation state so that theSynchronizercan choose peers based on whether or not they have been fully validated. This allows us to use only fully validated peers when choosing a pivot block.