Handle Invalid Blocks in Initial Sync

[nisdas]

Currently we panic if we receive an invalid block in initial sync. Ex: It causes a transition with an invalid state root, or it has an invalid attestation,deposit,etc .

We should instead continue syncing even though the block is bad rather than panicking, Currently we panic, so as to be able to debug any potential testnet bugs more easily. This issue is to track the implementation of an invalid block handler.

[mkinney]

Do you know at what block it dies?

Or, do you know how to simulate the failure?

I've started a few syncs after running clear-db.

[mkinney]

One of the syncs go to here:

[2019-10-31 03:28:16] INFO initial-sync: Processing block 7552/194026 - estimated time remaining 4h10m38s blocksPerSecond=12.4 peers=3/8
[2019-10-31 03:28:16] INFO initial-sync: Processing block 7553/194026 - estimated time remaining 4h9m37s blocksPerSecond=12.4 peers=3/8
panic: could not process block from fork choice service: pre state of slot 7553 does not exist
goroutine 302 [running]:
github.com/prysmaticlabs/prysm/beacon-chain/sync/initial-sync.(*InitialSync).Start(0xc0009c80f0)
beacon-chain/sync/initial-sync/service.go:106 +0x97d
created by github.com/prysmaticlabs/prysm/shared.(*ServiceRegistry).StartAll
shared/service_registry.go:44 +0x23e

[mkinney]

Here's a diff... and re-trying a sync (after clear-db):

diff --git a/beacon-chain/sync/initial-sync/service.go b/beacon-chain/sync/initial-sync/service.go
index 247af6f20..c5aaef8bf 100644
--- a/beacon-chain/sync/initial-sync/service.go
+++ b/beacon-chain/sync/initial-sync/service.go
@@ -103,7 +103,8 @@ func (s *InitialSync) Start() {
        }

        if err := s.roundRobinSync(genesis); err != nil {
-               panic(err)
+               //panic(err)
+               log.Errorf("Sync error: %v", err)
        }

        log.Infof("Synced up to slot %d", s.chain.HeadSlot())

[mkinney]

Want me to create a PR for changing the panic to a log?

Want a new flag --stop-on-sync-error?

[terencechain]

This is probably move involved than panic -> log. We might want an invalid block handler

[mkinney]

What would the invalid block handler do? Re-try n times with a backoff amount? Or perhaps something else?

[nisdas]

Ah no we dont want a flag for this, #3880 should handle most of the panics we are seeing now. This is to track the implementation of an invalid block handler. For the purposes of debugging our testnet, we panic if we receive a block that causes an invalid transition. Eventually we will want to handle any invalid blocks properly in mainnet

[rauljordan]

This is something perhaps @farazdagi should keep in mind while designing the block processor in initial sync

[rauljordan]

@farazdagi do your PRs deal with this issue? Do you think we can close this after your PRs are merged in? Maybe add resolves #3898 to the descriptions if that's the case.

[farazdagi]

I think that even the current implementation doesn't panic on invalid blocks (it just tries to re-fetch from another peer). Still, I will double check, and make sure that we have covered all the different cases of invalid blocks (not only if block's parent doesn't match any existing hashes in DB).

Will assign issue to myself, and report back when I have more info.