Switch to new sync target if it exceeds the td threshold #1286
Conversation
…oesn't exceed the height threshold.
…d value (accounting for the announced block difficulty being subtracted).
...rc/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/BetterSyncTargetEvaluator.java
Outdated
Show resolved
Hide resolved
| .getBestBlock() | ||
| .getTotalDifficulty() | ||
| .minus(currentPeerChainState.getBestBlock().getTotalDifficulty()); | ||
| if (tdDifference.compareTo(config.downloaderChangeTargetThresholdByTd()) > 0) { |
There was a problem hiding this comment.
The defaults we have for these values don't seem to make much sense. Looks like mainnet is changing by ~10^15 per block but the default threshold is 10^9. And td is tricky because the rate of change should depend on the particular chain. In any case, we should probably try to pick better defaults as part of this PR while we're here.
There was a problem hiding this comment.
That's a particularly astute yet inconvenient observation...
The situation we want to avoid is where our first peer has a very long but low difficulty chain, trying to draw us into a tar pit and spending a long time sync'ing their chain. Currently if their chain height was higher we'd continue syncing to them until we reached their head and only then switch to the actual best peer and begin syncing the right chain.
That said, it takes quite a lot of effort to generate a really long PoW chain because the difficulty will always keep blocks around 15s apart.
I'm inclined to set it to a high value like 10^18 which means the default is almost always switch on chain height before the TD threshold was reached. It would still differentiate between ETH and ETC though we have an explicit Dao block check for that as well now. Having the setting around is potentially quite useful for cases like when the Ropsten fork went wrong - you'd want to set it to something much smaller to ensure you switched to the best chain faster.
There was a problem hiding this comment.
Sounds reasonable. The height threshold is pretty low as well - should we bump that up?
|
|
||
| import org.junit.Test; | ||
|
|
||
| public class BetterSyncTargetEvaluatorTest { |
…1286) Increase total difficulty threshold required to switch sync targets. Increase threshold for changing sync target based on height to 200.
…1286) Increase total difficulty threshold required to switch sync targets. Increase threshold for changing sync target based on height to 200.
PR description
When evaluating if the sync target should be changed, we would only switch if the new best peer's height was better by some amount. We should switch if the difficulty or height exceeds the "better" threshold (as long as the difficulty is at least equal).
Also splits this calculation into a separate class so it can be reused in the pipeline chain download and adds unit tests for it.