From d10a5dcc677bd4ecbe2b8b11df5a728c15b6bbf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Thu, 30 Jun 2022 17:55:31 +0300 Subject: [PATCH 1/4] eth/catalyst: disallow importing blocks via newPayload during snap sync --- eth/catalyst/api.go | 8 +++++++- eth/handler.go | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index acc9c0e66ecb..b7faea34877f 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" @@ -272,8 +273,13 @@ func (api *ConsensusAPI) NewPayloadV1(params beacon.ExecutableDataV1) (beacon.Pa // our live chain. As such, payload execution will not permit reorgs and thus // will not trigger a sync cycle. That is fine though, if we get a fork choice // update after legit payload executions. + // + // A similar cornercase is when 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. parent := api.eth.BlockChain().GetBlock(block.ParentHash(), block.NumberU64()-1) - if parent == nil { + if parent == nil || api.eth.SyncMode() != downloader.FullSync { // Stash the block away for a potential forced forckchoice update to it // at a later time. api.remoteBlocks.put(block.Hash(), block.Header()) diff --git a/eth/handler.go b/eth/handler.go index 43d03924defa..1418c73894c8 100644 --- a/eth/handler.go +++ b/eth/handler.go @@ -249,7 +249,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() { From 5ffae5ed2046848b804d8c691d7966ecd62f449f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Thu, 30 Jun 2022 18:09:41 +0300 Subject: [PATCH 2/4] eth/catalyst: make tests pass by using full sync only --- eth/catalyst/api_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) From 056e6c653a8974d58c30f094c6f2fda2185f4245 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Fri, 1 Jul 2022 12:06:12 +0300 Subject: [PATCH 3/4] eth/catalysts: make the import delay a bit cleaner --- eth/catalyst/api.go | 56 ++++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index b7faea34877f..7aeb2c1e8c2b 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -273,30 +273,9 @@ func (api *ConsensusAPI) NewPayloadV1(params beacon.ExecutableDataV1) (beacon.Pa // our live chain. As such, payload execution will not permit reorgs and thus // will not trigger a sync cycle. That is fine though, if we get a fork choice // update after legit payload executions. - // - // A similar cornercase is when 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. parent := api.eth.BlockChain().GetBlock(block.ParentHash(), block.NumberU64()-1) - if parent == nil || api.eth.SyncMode() != downloader.FullSync { - // 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 + if parent == nil { + return api.delayPayloadImport(block) } // We have an existing parent, do some sanity checks to avoid the beacon client // triggering too early @@ -317,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") @@ -351,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 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", 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 { From ce8be2d88bf6f6a3375e022f1e4e19b60f11cf23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Fri, 1 Jul 2022 12:11:25 +0300 Subject: [PATCH 4/4] eth/catalyst: fix typo Co-authored-by: Marius van der Wijden --- eth/catalyst/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 7aeb2c1e8c2b..cc1627f802fe 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -342,7 +342,7 @@ func computePayloadId(headBlockHash common.Hash, params *beacon.PayloadAttribute // 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 forckchoice update to it + // Stash the block away for a potential forced forkchoice update to it // at a later time. api.remoteBlocks.put(block.Hash(), block.Header())