From 226554ece3201cacdac40cb69e25c376ac990f74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Fri, 1 Jul 2022 14:38:26 +0300 Subject: [PATCH] eth/catalyst: disallow importing blocks via newPayload during snap sync (#25210) * eth/catalyst: disallow importing blocks via newPayload during snap sync * eth/catalyst: make tests pass by using full sync only * eth/catalysts: make the import delay a bit cleaner * eth/catalyst: fix typo Co-authored-by: Marius van der Wijden Co-authored-by: Marius van der Wijden --- eth/catalyst/api.go | 50 ++++++++++++++++++++++++++-------------- eth/catalyst/api_test.go | 2 +- eth/handler.go | 2 +- 3 files changed, 35 insertions(+), 19 deletions(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index acc9c0e66ecb..cc1627f802fe 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -31,6 +31,7 @@ import ( "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/eth" + "github.com/ethereum/go-ethereum/eth/downloader" "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/node" "github.com/ethereum/go-ethereum/rpc" @@ -274,23 +275,7 @@ func (api *ConsensusAPI) NewPayloadV1(params beacon.ExecutableDataV1) (beacon.Pa // update after legit payload executions. parent := api.eth.BlockChain().GetBlock(block.ParentHash(), block.NumberU64()-1) if parent == nil { - // Stash the block away for a potential forced forckchoice update to it - // at a later time. - api.remoteBlocks.put(block.Hash(), block.Header()) - - // Although we don't want to trigger a sync, if there is one already in - // progress, try to extend if with the current payload request to relieve - // some strain from the forkchoice update. - if err := api.eth.Downloader().BeaconExtend(api.eth.SyncMode(), block.Header()); err == nil { - log.Debug("Payload accepted for sync extension", "number", params.Number, "hash", params.BlockHash) - return beacon.PayloadStatusV1{Status: beacon.SYNCING}, nil - } - // Either no beacon sync was started yet, or it rejected the delivered - // payload as non-integratable on top of the existing sync. We'll just - // have to rely on the beacon client to forcefully update the head with - // a forkchoice update request. - log.Warn("Ignoring payload with missing parent", "number", params.Number, "hash", params.BlockHash, "parent", params.ParentHash) - return beacon.PayloadStatusV1{Status: beacon.ACCEPTED}, nil + return api.delayPayloadImport(block) } // We have an existing parent, do some sanity checks to avoid the beacon client // triggering too early @@ -311,6 +296,13 @@ func (api *ConsensusAPI) NewPayloadV1(params beacon.ExecutableDataV1) (beacon.Pa log.Warn("Invalid timestamp", "parent", block.Time(), "block", block.Time()) return api.invalid(errors.New("invalid timestamp"), parent), nil } + // Another cornercase: if the node is in snap sync mode, but the CL client + // tries to make it import a block. That should be denied as pushing something + // into the database directly will conflict with the assumptions of snap sync + // that it has an empty db that it can fill itself. + if api.eth.SyncMode() != downloader.FullSync { + return api.delayPayloadImport(block) + } if !api.eth.BlockChain().HasBlockAndState(block.ParentHash(), block.NumberU64()-1) { api.remoteBlocks.put(block.Hash(), block.Header()) log.Warn("State not available, ignoring new payload") @@ -345,6 +337,30 @@ func computePayloadId(headBlockHash common.Hash, params *beacon.PayloadAttribute return out } +// delayPayloadImport stashes the given block away for import at a later time, +// either via a forkchoice update or a sync extension. This method is meant to +// be called by the newpayload command when the block seems to be ok, but some +// prerequisite prevents it from being processed (e.g. no parent, or nap sync). +func (api *ConsensusAPI) delayPayloadImport(block *types.Block) (beacon.PayloadStatusV1, error) { + // Stash the block away for a potential forced forkchoice update to it + // at a later time. + api.remoteBlocks.put(block.Hash(), block.Header()) + + // Although we don't want to trigger a sync, if there is one already in + // progress, try to extend if with the current payload request to relieve + // some strain from the forkchoice update. + if err := api.eth.Downloader().BeaconExtend(api.eth.SyncMode(), block.Header()); err == nil { + log.Debug("Payload accepted for sync extension", "number", block.NumberU64(), "hash", block.Hash()) + return beacon.PayloadStatusV1{Status: beacon.SYNCING}, nil + } + // Either no beacon sync was started yet, or it rejected the delivered + // payload as non-integratable on top of the existing sync. We'll just + // have to rely on the beacon client to forcefully update the head with + // a forkchoice update request. + log.Warn("Ignoring payload with missing parent", "number", block.NumberU64(), "hash", block.Hash(), "parent", block.ParentHash()) + return beacon.PayloadStatusV1{Status: beacon.ACCEPTED}, nil +} + // invalid returns a response "INVALID" with the latest valid hash supplied by latest or to the current head // if no latestValid block was provided. func (api *ConsensusAPI) invalid(err error, latestValid *types.Block) beacon.PayloadStatusV1 { diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index 6171c14c432c..547b727f1c6a 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -403,7 +403,7 @@ func startEthService(t *testing.T, genesis *core.Genesis, blocks []*types.Block) t.Fatal("can't create node:", err) } - ethcfg := ðconfig.Config{Genesis: genesis, Ethash: ethash.Config{PowMode: ethash.ModeFake}, SyncMode: downloader.SnapSync, TrieTimeout: time.Minute, TrieDirtyCache: 256, TrieCleanCache: 256} + ethcfg := ðconfig.Config{Genesis: genesis, Ethash: ethash.Config{PowMode: ethash.ModeFake}, SyncMode: downloader.FullSync, TrieTimeout: time.Minute, TrieDirtyCache: 256, TrieCleanCache: 256} ethservice, err := eth.New(n, ethcfg) if err != nil { t.Fatal("can't create eth service:", err) diff --git a/eth/handler.go b/eth/handler.go index 221c2f255640..31775e73c300 100644 --- a/eth/handler.go +++ b/eth/handler.go @@ -255,7 +255,7 @@ func newHandler(config *handlerConfig) (*handler, error) { // out a way yet where nodes can decide unilaterally whether the network is new // or not. This should be fixed if we figure out a solution. if atomic.LoadUint32(&h.snapSync) == 1 { - log.Warn("Fast syncing, discarded propagated block", "number", blocks[0].Number(), "hash", blocks[0].Hash()) + log.Warn("Snap syncing, discarded propagated block", "number", blocks[0].Number(), "hash", blocks[0].Hash()) return 0, nil } if h.merger.TDDReached() {