Skip to content

[FIX] blacklist for longer in case of header validation failure #840

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Dec 8, 2020
Merged

[FIX] blacklist for longer in case of header validation failure #840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/main/resources/application.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,9 @@ mantis {
# are controlled by a single party, eg. private networks)
blacklist-duration = 200.seconds

# Duration for high offense blacklisting of a peer. Blacklisting reason include: header validation failure.
critical-blacklist-duration = 240.minutes

# Retry interval when not having enough peers to start fast-sync
start-retry-interval = 5.seconds

Expand Down
16 changes: 11 additions & 5 deletions src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSync.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,8 +408,8 @@ class FastSync(
blockchain.getChainWeightByHash(header.parentHash).toRight(ParentChainWeightNotFound(header))
}

private def handleRewind(header: BlockHeader, peer: Peer, N: Int): Unit = {
blacklist(peer.id, blacklistDuration, "block header validation failed")
private def handleRewind(header: BlockHeader, peer: Peer, N: Int, duration: FiniteDuration): Unit = {
blacklist(peer.id, duration, "block header validation failed")
if (header.number <= syncState.safeDownloadTarget) {
discardLastBlocks(header.number, N)
syncState = syncState.updateDiscardedBlocks(header, N)
Expand All @@ -431,14 +431,20 @@ class FastSync(
// we blacklist peer just in case we got malicious peer which would send us bad blocks, forcing us to rollback
// to genesis
log.warning("Parent chain weight not found for block {}, not processing rest of headers", header.idTag)
handleRewind(header, peer, syncConfig.fastSyncBlockValidationN)
handleRewind(header, peer, syncConfig.fastSyncBlockValidationN, syncConfig.blacklistDuration)
case HeadersProcessingFinished =>
processSyncing()
case ImportedPivotBlock =>
updatePivotBlock(ImportedLastBlock)
case ValidationFailed(header, peerToBlackList) =>
log.warning(s"validation fo header ${header.idTag} failed")
handleRewind(header, peerToBlackList, syncConfig.fastSyncBlockValidationN)
log.warning(s"validation of header ${header.idTag} failed")
// pow validation failure indicate that either peer is malicious or it is on wrong fork
handleRewind(
header,
peerToBlackList,
syncConfig.fastSyncBlockValidationN,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@KonradStaniec KonradStaniec Dec 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

syncConfig.criticalBlacklistDuration
)
}
} else {
blacklist(peer.id, blacklistDuration, "error in block headers response")
Expand Down
2 changes: 2 additions & 0 deletions src/main/scala/io/iohk/ethereum/utils/Config.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ object Config {
doFastSync: Boolean,
peersScanInterval: FiniteDuration,
blacklistDuration: FiniteDuration,
criticalBlacklistDuration: FiniteDuration,
startRetryInterval: FiniteDuration,
syncRetryInterval: FiniteDuration,
peerResponseTimeout: FiniteDuration,
Expand Down Expand Up @@ -137,6 +138,7 @@ object Config {
doFastSync = syncConfig.getBoolean("do-fast-sync"),
peersScanInterval = syncConfig.getDuration("peers-scan-interval").toMillis.millis,
blacklistDuration = syncConfig.getDuration("blacklist-duration").toMillis.millis,
criticalBlacklistDuration = syncConfig.getDuration("critical-blacklist-duration").toMillis.millis,
startRetryInterval = syncConfig.getDuration("start-retry-interval").toMillis.millis,
syncRetryInterval = syncConfig.getDuration("sync-retry-interval").toMillis.millis,
peerResponseTimeout = syncConfig.getDuration("peer-response-timeout").toMillis.millis,
Expand Down
1 change: 1 addition & 0 deletions src/test/scala/io/iohk/ethereum/blockchain/sync/TestSyncConfig.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ trait TestSyncConfig extends SyncConfigBuilder {
pivotBlockOffset = 500,
branchResolutionRequestSize = 2,
blacklistDuration = 5.seconds,
criticalBlacklistDuration = 10.seconds,
syncRetryInterval = 1.second,
checkForNewBlockInterval = 1.milli,
startRetryInterval = 500.milliseconds,
Expand Down