[PAN-2312] Validate DAO block #939
Conversation
Make GetHeadersFromPeer task stricter in validating response matches.
Add ability to disable automatic running of submitted tasks and the ability to manually run pending tasks.
Add logging, add constructor with defaults
There was a problem hiding this comment.
Generally looks good. Couple of nits and just need to make sure we stop checking peers if they disconnect before getting to the fork block.
In terms of helping fast sync to pick the right block, ideally this check would be performed before the peer is considered available (at least in the case of them being ahead of the Dao block on initial connection). Otherwise there's a (quite small) risk that we'd connect to 5 ETC peers at about the same time and fast sync would pick one of their blocks before this check has a chance to run. In any case that's almost certainly a separate PR.
I do like that the PeerValidator sets things up so we can make this a more generic test in the future (e.g. to detect the many varied forks around Ropsten Constantinople) and potentially even dynamically based on bad blocks we detect.
...ain/java/tech/pegasys/pantheon/ethereum/eth/manager/task/AbstractGetHeadersFromPeerTask.java
Outdated
Show resolved
Hide resolved
| disconnectPeer(ethPeer); | ||
| } | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
Do we need this to be } else if (!peer.isDisconnected()) {?
There was a problem hiding this comment.
yes - good catch. we need to handle disconnects so we don't repeatedly check disconnected peers.
| @@ -116,6 +123,9 @@ public MainnetPantheonController( | |||
| metricsSystem); | |||
| final SyncState syncState = | |||
| new SyncState(blockchain, ethProtocolManager.ethContext().getEthPeers()); | |||
| final LabelledMetric<OperationTimer> ethTasksTimer = | |||
| metricsSystem.createLabelledTimer( | |||
| MetricCategory.SYNCHRONIZER, "task", "Internal processing tasks", "taskName"); | |||
There was a problem hiding this comment.
What was the reason for pushing this up to here? Seems odd to have only done it for MainnetPantheonController and not clique & IBFT, plus we're generally trying to push common construction code out of the controller.
There was a problem hiding this comment.
Because we need it to construct any EthTask's and DaoForkPeerValidator uses an EthTask. I think the real fix is to keep working on MetricsSystem to allow metrics to be created idempotently so we can then push this timer creation into AbstractEthTask where it's used. I have some in-progress work on that which I can come back to.
There was a problem hiding this comment.
ahhh, yeah that's very unfortunate.
There was a problem hiding this comment.
Just remembered, I added some caching for metrics back when I did the EVM metrics but had to revert it, so 525aa19#diff-b95472676d731050caa643c3ebe592b1 may be useful.
Co-Authored-By: mbaxter <[email protected]>
Rework some test implementations to just use mocks.
PR description
This PR adds a
DaoForkPeerValidatorthat will run for every connected eth peer when pantheon is run against mainnet. The validator checks if the peer is ready to be validated by looking at the peer's estimated chain height. If the height is past the DAO block, it will request and check the block from the peer at the DAO fork. If this block is on the wrong side of the fork, the peer will be disconnected.Some secondary changes in this PR include:
BlockHeadersMessage.getHeadersto return a listAbstractGetHeadersFromPeerTaskto perform stricter matching on responses: if a response contains more headers than were requested, we don't consider this response a match to our requestPeerRequestTaskconfigurable