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 <m.vanderwijden@live.de>

Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de>

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
+		return api.delayPayloadImport(block)
-		// 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
 	}
 	// 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 {

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 := &ethconfig.Config{Genesis: genesis, Ethash: ethash.Config{PowMode: ethash.ModeFake}, SyncMode: downloader.SnapSync, TrieTimeout: time.Minute, TrieDirtyCache: 256, TrieCleanCache: 256}
+	ethcfg := &ethconfig.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)

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() {