Skip to content

Commit de1cecb

Browse files
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 <[email protected]> Co-authored-by: Marius van der Wijden <[email protected]>
1 parent 692bfd1 commit de1cecb

File tree

3 files changed

+35
-19
lines changed

3 files changed

+35
-19
lines changed

eth/catalyst/api.go

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import (
3131
"github.com/ethereum/go-ethereum/core/rawdb"
3232
"github.com/ethereum/go-ethereum/core/types"
3333
"github.com/ethereum/go-ethereum/eth"
34+
"github.com/ethereum/go-ethereum/eth/downloader"
3435
"github.com/ethereum/go-ethereum/log"
3536
"github.com/ethereum/go-ethereum/node"
3637
"github.com/ethereum/go-ethereum/rpc"
@@ -274,23 +275,7 @@ func (api *ConsensusAPI) NewPayloadV1(params beacon.ExecutableDataV1) (beacon.Pa
274275
// update after legit payload executions.
275276
parent := api.eth.BlockChain().GetBlock(block.ParentHash(), block.NumberU64()-1)
276277
if parent == nil {
277-
// Stash the block away for a potential forced forckchoice update to it
278-
// at a later time.
279-
api.remoteBlocks.put(block.Hash(), block.Header())
280-
281-
// Although we don't want to trigger a sync, if there is one already in
282-
// progress, try to extend if with the current payload request to relieve
283-
// some strain from the forkchoice update.
284-
if err := api.eth.Downloader().BeaconExtend(api.eth.SyncMode(), block.Header()); err == nil {
285-
log.Debug("Payload accepted for sync extension", "number", params.Number, "hash", params.BlockHash)
286-
return beacon.PayloadStatusV1{Status: beacon.SYNCING}, nil
287-
}
288-
// Either no beacon sync was started yet, or it rejected the delivered
289-
// payload as non-integratable on top of the existing sync. We'll just
290-
// have to rely on the beacon client to forcefully update the head with
291-
// a forkchoice update request.
292-
log.Warn("Ignoring payload with missing parent", "number", params.Number, "hash", params.BlockHash, "parent", params.ParentHash)
293-
return beacon.PayloadStatusV1{Status: beacon.ACCEPTED}, nil
278+
return api.delayPayloadImport(block)
294279
}
295280
// We have an existing parent, do some sanity checks to avoid the beacon client
296281
// triggering too early
@@ -311,6 +296,13 @@ func (api *ConsensusAPI) NewPayloadV1(params beacon.ExecutableDataV1) (beacon.Pa
311296
log.Warn("Invalid timestamp", "parent", block.Time(), "block", block.Time())
312297
return api.invalid(errors.New("invalid timestamp"), parent), nil
313298
}
299+
// Another cornercase: if the node is in snap sync mode, but the CL client
300+
// tries to make it import a block. That should be denied as pushing something
301+
// into the database directly will conflict with the assumptions of snap sync
302+
// that it has an empty db that it can fill itself.
303+
if api.eth.SyncMode() != downloader.FullSync {
304+
return api.delayPayloadImport(block)
305+
}
314306
if !api.eth.BlockChain().HasBlockAndState(block.ParentHash(), block.NumberU64()-1) {
315307
api.remoteBlocks.put(block.Hash(), block.Header())
316308
log.Warn("State not available, ignoring new payload")
@@ -345,6 +337,30 @@ func computePayloadId(headBlockHash common.Hash, params *beacon.PayloadAttribute
345337
return out
346338
}
347339

340+
// delayPayloadImport stashes the given block away for import at a later time,
341+
// either via a forkchoice update or a sync extension. This method is meant to
342+
// be called by the newpayload command when the block seems to be ok, but some
343+
// prerequisite prevents it from being processed (e.g. no parent, or nap sync).
344+
func (api *ConsensusAPI) delayPayloadImport(block *types.Block) (beacon.PayloadStatusV1, error) {
345+
// Stash the block away for a potential forced forkchoice update to it
346+
// at a later time.
347+
api.remoteBlocks.put(block.Hash(), block.Header())
348+
349+
// Although we don't want to trigger a sync, if there is one already in
350+
// progress, try to extend if with the current payload request to relieve
351+
// some strain from the forkchoice update.
352+
if err := api.eth.Downloader().BeaconExtend(api.eth.SyncMode(), block.Header()); err == nil {
353+
log.Debug("Payload accepted for sync extension", "number", block.NumberU64(), "hash", block.Hash())
354+
return beacon.PayloadStatusV1{Status: beacon.SYNCING}, nil
355+
}
356+
// Either no beacon sync was started yet, or it rejected the delivered
357+
// payload as non-integratable on top of the existing sync. We'll just
358+
// have to rely on the beacon client to forcefully update the head with
359+
// a forkchoice update request.
360+
log.Warn("Ignoring payload with missing parent", "number", block.NumberU64(), "hash", block.Hash(), "parent", block.ParentHash())
361+
return beacon.PayloadStatusV1{Status: beacon.ACCEPTED}, nil
362+
}
363+
348364
// invalid returns a response "INVALID" with the latest valid hash supplied by latest or to the current head
349365
// if no latestValid block was provided.
350366
func (api *ConsensusAPI) invalid(err error, latestValid *types.Block) beacon.PayloadStatusV1 {

eth/catalyst/api_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -403,7 +403,7 @@ func startEthService(t *testing.T, genesis *core.Genesis, blocks []*types.Block)
403403
t.Fatal("can't create node:", err)
404404
}
405405

406-
ethcfg := &ethconfig.Config{Genesis: genesis, Ethash: ethash.Config{PowMode: ethash.ModeFake}, SyncMode: downloader.SnapSync, TrieTimeout: time.Minute, TrieDirtyCache: 256, TrieCleanCache: 256}
406+
ethcfg := &ethconfig.Config{Genesis: genesis, Ethash: ethash.Config{PowMode: ethash.ModeFake}, SyncMode: downloader.FullSync, TrieTimeout: time.Minute, TrieDirtyCache: 256, TrieCleanCache: 256}
407407
ethservice, err := eth.New(n, ethcfg)
408408
if err != nil {
409409
t.Fatal("can't create eth service:", err)

eth/handler.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ func newHandler(config *handlerConfig) (*handler, error) {
249249
// out a way yet where nodes can decide unilaterally whether the network is new
250250
// or not. This should be fixed if we figure out a solution.
251251
if atomic.LoadUint32(&h.snapSync) == 1 {
252-
log.Warn("Fast syncing, discarded propagated block", "number", blocks[0].Number(), "hash", blocks[0].Hash())
252+
log.Warn("Snap syncing, discarded propagated block", "number", blocks[0].Number(), "hash", blocks[0].Hash())
253253
return 0, nil
254254
}
255255
if h.merger.TDDReached() {

0 commit comments

Comments
 (0)