[PAN-2917] Rework "in-sync" checks #1720
Conversation
When performing in sync checks, test against the sync target and our best peer. This handles cases where the sync target is temporarily empty, but we are not yet in sync. Also, consolidate logic to determine whether a peer's chain is "better" than our chain and add tests.
| * @return true if this {@link ChainState} represents a better chain than {@code chainToCheck}. | ||
| */ | ||
| public boolean chainIsBetterThan(final ChainHead chainToCheck) { | ||
| return hasHigherDifficultyThan(chainToCheck) || hasLongerChainThan(chainToCheck); |
There was a problem hiding this comment.
If we had perfect information, we would only be checking the total difficulty here. And we do choose the peer we're syncing to by prioritizing total difficulty over height estimates. So, I think we may want to change this logic to just check total difficulty.
However, I'll leave that work for a potential follow-up ticket because I think we need to more fully vet the idea and make this change in isolation. One potential danger in making this change would be: if the estimates we're getting on total difficulty are sparse, we may end up not syncing to peers with new data because their td estimates are out-of-date.
There was a problem hiding this comment.
Unless the computation expense is large I see no harm in leaving the extra check in until we are sure we don't need it.
There was a problem hiding this comment.
Keeping this logic as-is, it's possible that we end up syncing to a lighter-weight but longer chain and wasting effort / disk space. So it's a trade-off between possibly falling behind and possibly doing unnecessary work. But we have state pruning in progress which will address any useless state we might download. And we could also look into pruning chain data from old forks. So, it may make sense to leave the logic as-is ...
There was a problem hiding this comment.
I think this is correct for checking if we've reached our sync target but the method name is misleading. When comparing two chains, the chain is only better if the total difficulty is better.
But once we've picked the peer we want to sync to, it makes sense to import all the blocks it has. The height check makes sense here as we may have received an updated height but not yet an updated total difficulty. Although, that would only occur when first connecting to the peer since NewBlockHashes and NewBlock messages both include the total difficulty - the only time we get height by itself is when we specifically request their best block to find a height upon initial connection. So we could safely use hasHigherDifficultyThan here.
There was a problem hiding this comment.
We actually only get an updated total difficulty estimate when a NewBlock message comes in. NewBlockHashes messages come with block heights. I went with the name chainIsBetterThan because we use this to determine whether or not to sync with a peer.
In any case, because td estimates come in less frequently I'm leaning towards just leaving this as-is for now ...
| @@ -84,11 +85,27 @@ public boolean isInSync() { | |||
| } | |||
|
|
|||
| public boolean isInSync(final long syncTolerance) { | |||
| // Sync target may be temporarily empty while we switch sync targets during a sync, so | |||
| // check both the sync target and our best peer to determine if we're in sync or not | |||
| return isInSyncWithTarget(syncTolerance) && isInSyncWithBestPeer(syncTolerance); | |||
There was a problem hiding this comment.
This change is the main focus of this PR.
| * @return true if this {@link ChainState} represents a better chain than {@code chainToCheck}. | ||
| */ | ||
| public boolean chainIsBetterThan(final ChainHead chainToCheck) { | ||
| return hasHigherDifficultyThan(chainToCheck) || hasLongerChainThan(chainToCheck); |
There was a problem hiding this comment.
Unless the computation expense is large I see no harm in leaving the extra check in until we are sure we don't need it.
PR description
When performing "in-sync" checks, test against the sync target and our best peer. This handles cases where the sync target is temporarily empty, but we are not yet in sync.
Also, consolidate logic to determine whether a peer's chain is "better" than our chain and add tests.