[FIX] blacklist for longer in case of header validation failure#840
[FIX] blacklist for longer in case of header validation failure#840KonradStaniec merged 1 commit intodevelopfrom
Conversation
mmrozek
left a comment
mmrozek
left a comment
There was a problem hiding this comment.
One doubt only. Apart from that code LGTM
| handleRewind( | ||
| header, | ||
| peerToBlackList, | ||
| syncConfig.fastSyncBlockValidationN, |
There was a problem hiding this comment.
I am not sure if we shouldn't rewind more blocks in case of critical failure. Some times ago I stuck in the wrong fork and only the increasing number of blocks helped me. WDYT?
There was a problem hiding this comment.
to be honest all those number were derived from old geth pr which also proided some security analysis so i am not eager the change those just before release, also until we will have proper branch resolving during fast sync all we do here i a little hacky either way. So i would leave it as it is.
What does it mean that 2 are on wrong fork "for us"? Which criteria tells that it's wrong? Is it a hard fork we don't want to follow? Or rather if we do we get validation error because of the implementation differences? |
|
Is it a hard fork we don't want to follow. i.e take Thanos, hard fork, it changes how proof of work validation work and we do not want to follow nodes which do not activated it. |
| case ImportedPivotBlock => | ||
| updatePivotBlock(ImportedLastBlock) | ||
| case ValidationFailed(header, peerToBlackList) => | ||
| log.warning(s"validation fo header ${header.idTag} failed") |
There was a problem hiding this comment.
| log.warning(s"validation fo header ${header.idTag} failed") | |
| log.warning(s"validation of header ${header.idTag} failed") |
aakoshh
left a comment
aakoshh
left a comment
There was a problem hiding this comment.
Looks reasonable. I don't fully understand what the default case is, the "missing parent weight".
KonradStaniec
force-pushed
the
fix/longer-blacklist-for-critical
branch
from
46694ea to
e9006ef
Compare
|
this can happen when lets say there is minor fork in the chain. Let say we are on block In this case we also rewind blockchain a little bit, and hope fork will be resolved the next time we will get to this place. |
4 participants
Description
Increases duration for critical offences in fast sync.
Imagine scenario with 3 remote peers - two of them are on wrong fork for us, and only one of them is on correct for, and those two on wrong fork are slightly beter than this on good fork.
Such scenario is possible:
2.we sync up with the second peer on wrong fork, ultimately blacklisting him due to validation error. We rollback N blocks and start from behind
By increasing blacklist time for validation errors we make sure we will have time to try other peers.
btw. I did not introduce it to regular sync as it not easy to grab header validation error there and i would like to avoid big code changes before release