Validate header chain in downloader #1222
Conversation
|
|
||
| private boolean isParentValid(BlockHeader header) { | ||
| Block parent = blockchain.getBlockByHash(header.getParentHash()); | ||
| if (parent == null) { |
There was a problem hiding this comment.
we were getting exception in this case before
is it ok we will pass it since this change?
the only case when it's ok that we don't have parent is genesis.
There was a problem hiding this comment.
Are you sure the block parent is in the blockchain already here? If it is still 'on the way' wouldn't this skip parent validation completely?
| * Runs parent header validation and returns after first occurrence of invalid header | ||
| */ | ||
| ValidatedHeaders validateChain(List<HeaderElement> chain) { | ||
| if (chain.size() <= MAX_CHAIN_LEN || parentHeaderValidator == null) |
There was a problem hiding this comment.
Could you, please, add comment why we could pass <= MAX_CHAIN_LEN
There was a problem hiding this comment.
I could add a comment. But this is a result of a trade off between bad design and changes in legacy code. And I think I've found the way out.
| @@ -726,7 +726,6 @@ private boolean isValid(Repository repo, Block block) { | |||
| boolean isValid = true; | |||
|
|
|||
| if (!block.isGenesis()) { | |||
| isValid = isValid(block.getHeader()); | |||
There was a problem hiding this comment.
May be it makes sense to leave this check if it's not too heavy?
Or leave it here by default and add a flag to skip the check as soon as it is not needed?
There was a problem hiding this comment.
You know, I think I'll leave it here and revert related changes made in BlockLoader.
|
@Nashatyrev please, take a look again |
|
I like this variant much more. |
What's done
Fixes a problem that has been discovered on Ropsten net during the mess happened around Constantinople HF. The fix is covered with unit tests and ready for merge. Testing on Ropsten's data went well.
The problem
BlockDownloaderdidn't run header validations that depends on parent block (like difficulty check). These validations used to be run inBlockchainImplbefore block started to be imported. Thus, if there is a peer with invalid chain in a list of active peers,BlockDownloadercould stick to that chain and never do a re-branch and the sync gets stuck as a result.The fix
Parent header checks have been moved to
BlockDownloaderand now all checks are run there. Hence, preventingBlockDownloaderfrom going down the wrong chain.