From 381c66caf0c1510ae84c135feb2835f86fdca6fc Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Tue, 17 May 2022 11:32:55 +0200 Subject: [PATCH] eth/catalyst: set the correct LatestValidHash (#24855) * eth/catalyst: set the correct LatestValidHash * eth/catalyst: core: return LVH during reorg, rework invalid teminal block * eth/catalyst: nitpicks --- core/beacon/errors.go | 13 ++++++++----- core/blockchain.go | 28 +++++++++++++++------------- eth/catalyst/api.go | 32 +++++++++++++++----------------- eth/catalyst/api_test.go | 2 +- 4 files changed, 39 insertions(+), 36 deletions(-) diff --git a/core/beacon/errors.go b/core/beacon/errors.go index 83d5eebd5..ea51a0714 100644 --- a/core/beacon/errors.go +++ b/core/beacon/errors.go @@ -16,7 +16,10 @@ package beacon -import "github.com/ethereum/go-ethereum/rpc" +import ( + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/rpc" +) var ( // VALID is returned by the engine API in the following calls: @@ -38,13 +41,13 @@ var ( // - newPayloadV1: if the payload was accepted, but not processed (side chain) ACCEPTED = "ACCEPTED" - INVALIDBLOCKHASH = "INVALID_BLOCK_HASH" - INVALIDTERMINALBLOCK = "INVALID_TERMINAL_BLOCK" + INVALIDBLOCKHASH = "INVALID_BLOCK_HASH" GenericServerError = rpc.CustomError{Code: -32000, ValidationError: "Server error"} UnknownPayload = rpc.CustomError{Code: -32001, ValidationError: "Unknown payload"} InvalidTB = rpc.CustomError{Code: -32002, ValidationError: "Invalid terminal block"} - STATUS_INVALID = ForkChoiceResponse{PayloadStatus: PayloadStatusV1{Status: INVALID}, PayloadID: nil} - STATUS_SYNCING = ForkChoiceResponse{PayloadStatus: PayloadStatusV1{Status: SYNCING}, PayloadID: nil} + STATUS_INVALID = ForkChoiceResponse{PayloadStatus: PayloadStatusV1{Status: INVALID}, PayloadID: nil} + STATUS_SYNCING = ForkChoiceResponse{PayloadStatus: PayloadStatusV1{Status: SYNCING}, PayloadID: nil} + INVALID_TERMINAL_BLOCK = PayloadStatusV1{Status: INVALID, LatestValidHash: &common.Hash{}} ) diff --git a/core/blockchain.go b/core/blockchain.go index a0ec305d8..7ab15d7f4 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -1480,7 +1480,8 @@ func (bc *BlockChain) insertChain(chain types.Blocks, verifySeals, setHead bool) } else { // We're post-merge and the parent is pruned, try to recover the parent state log.Debug("Pruned ancestor", "number", block.Number(), "hash", block.Hash()) - return it.index, bc.recoverAncestors(block) + _, err := bc.recoverAncestors(block) + return it.index, err } // First block is future, shove it (and all children) to the future queue (unknown ancestor) case errors.Is(err, consensus.ErrFutureBlock) || (errors.Is(err, consensus.ErrUnknownAncestor) && bc.futureBlocks.Contains(it.first().ParentHash())): @@ -1849,7 +1850,8 @@ func (bc *BlockChain) insertSideChain(block *types.Block, it *insertIterator) (i // recoverAncestors finds the closest ancestor with available state and re-execute // all the ancestor blocks since that. // recoverAncestors is only used post-merge. -func (bc *BlockChain) recoverAncestors(block *types.Block) error { +// We return the hash of the latest block that we could correctly validate. +func (bc *BlockChain) recoverAncestors(block *types.Block) (common.Hash, error) { // Gather all the sidechain hashes (full blocks may be memory heavy) var ( hashes []common.Hash @@ -1864,18 +1866,18 @@ func (bc *BlockChain) recoverAncestors(block *types.Block) error { // If the chain is terminating, stop iteration if bc.insertStopped() { log.Debug("Abort during blocks iteration") - return errInsertionInterrupted + return common.Hash{}, errInsertionInterrupted } } if parent == nil { - return errors.New("missing parent") + return common.Hash{}, errors.New("missing parent") } // Import all the pruned blocks to make the state available for i := len(hashes) - 1; i >= 0; i-- { // If the chain is terminating, stop processing blocks if bc.insertStopped() { log.Debug("Abort during blocks processing") - return errInsertionInterrupted + return common.Hash{}, errInsertionInterrupted } var b *types.Block if i == 0 { @@ -1884,10 +1886,10 @@ func (bc *BlockChain) recoverAncestors(block *types.Block) error { b = bc.GetBlock(hashes[i], numbers[i]) } if _, err := bc.insertChain(types.Blocks{b}, false, false); err != nil { - return err + return b.ParentHash(), err } } - return nil + return block.Hash(), nil } // collectLogs collects the logs that were generated or removed during @@ -2090,16 +2092,16 @@ func (bc *BlockChain) InsertBlockWithoutSetHead(block *types.Block) error { // SetCanonical rewinds the chain to set the new head block as the specified // block. It's possible that the state of the new head is missing, and it will // be recovered in this function as well. -func (bc *BlockChain) SetCanonical(head *types.Block) error { +func (bc *BlockChain) SetCanonical(head *types.Block) (common.Hash, error) { if !bc.chainmu.TryLock() { - return errChainStopped + return common.Hash{}, errChainStopped } defer bc.chainmu.Unlock() // Re-execute the reorged chain in case the head state is missing. if !bc.HasState(head.Root()) { - if err := bc.recoverAncestors(head); err != nil { - return err + if latestValidHash, err := bc.recoverAncestors(head); err != nil { + return latestValidHash, err } log.Info("Recovered head state", "number", head.Number(), "hash", head.Hash()) } @@ -2107,7 +2109,7 @@ func (bc *BlockChain) SetCanonical(head *types.Block) error { start := time.Now() if head.ParentHash() != bc.CurrentBlock().Hash() { if err := bc.reorg(bc.CurrentBlock(), head); err != nil { - return err + return common.Hash{}, err } } bc.writeHeadBlock(head) @@ -2130,7 +2132,7 @@ func (bc *BlockChain) SetCanonical(head *types.Block) error { context = append(context, []interface{}{"age", common.PrettyAge(timestamp)}...) } log.Info("Chain head was updated", context...) - return nil + return head.Hash(), nil } func (bc *BlockChain) updateFutureBlocks() { diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 1640902db..152576660 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -132,14 +132,14 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa } if td.Cmp(ttd) < 0 || (block.NumberU64() > 0 && ptd.Cmp(ttd) > 0) { log.Error("Refusing beacon update to pre-merge", "number", block.NumberU64(), "hash", update.HeadBlockHash, "diff", block.Difficulty(), "age", common.PrettyAge(time.Unix(int64(block.Time()), 0))) - return beacon.ForkChoiceResponse{PayloadStatus: beacon.PayloadStatusV1{Status: beacon.INVALIDTERMINALBLOCK}, PayloadID: nil}, nil + return beacon.ForkChoiceResponse{PayloadStatus: beacon.INVALID_TERMINAL_BLOCK, PayloadID: nil}, nil } } if rawdb.ReadCanonicalHash(api.eth.ChainDb(), block.NumberU64()) != update.HeadBlockHash { // Block is not canonical, set head. - if err := api.eth.BlockChain().SetCanonical(block); err != nil { - return beacon.STATUS_INVALID, err + if latestValid, err := api.eth.BlockChain().SetCanonical(block); err != nil { + return beacon.ForkChoiceResponse{PayloadStatus: beacon.PayloadStatusV1{Status: beacon.INVALID, LatestValidHash: &latestValid}}, err } } else { // If the head block is already in our canonical chain, the beacon client is @@ -176,6 +176,14 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa return beacon.STATUS_INVALID, errors.New("safe head not canonical") } } + + valid := func(id *beacon.PayloadID) beacon.ForkChoiceResponse { + return beacon.ForkChoiceResponse{ + PayloadStatus: beacon.PayloadStatusV1{Status: beacon.VALID, LatestValidHash: &update.HeadBlockHash}, + PayloadID: id, + } + } + // If payload generation was requested, create a new block to be potentially // sealed by the beacon client. The payload will be requested later, and we // might replace it arbitrarily many times in between. @@ -186,25 +194,15 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa data, err := api.assembleBlock(update.HeadBlockHash, payloadAttributes) if err != nil { log.Error("Failed to create sealing payload", "err", err) - return api.validForkChoiceResponse(nil), err // valid setHead, invalid payload + return valid(nil), err // valid setHead, invalid payload } id := computePayloadId(update.HeadBlockHash, payloadAttributes) api.localBlocks.put(id, data) log.Info("Created payload for sealing", "id", id, "elapsed", time.Since(start)) - return api.validForkChoiceResponse(&id), nil - } - return api.validForkChoiceResponse(nil), nil -} - -// validForkChoiceResponse returns the ForkChoiceResponse{VALID} -// with the latest valid hash and an optional payloadID. -func (api *ConsensusAPI) validForkChoiceResponse(id *beacon.PayloadID) beacon.ForkChoiceResponse { - currentHash := api.eth.BlockChain().CurrentBlock().Hash() - return beacon.ForkChoiceResponse{ - PayloadStatus: beacon.PayloadStatusV1{Status: beacon.VALID, LatestValidHash: ¤tHash}, - PayloadID: id, + return valid(&id), nil } + return valid(nil), nil } // ExchangeTransitionConfigurationV1 checks the given configuration against @@ -291,7 +289,7 @@ func (api *ConsensusAPI) NewPayloadV1(params beacon.ExecutableDataV1) (beacon.Pa ) if td.Cmp(ttd) < 0 { log.Warn("Ignoring pre-merge payload", "number", params.Number, "hash", params.BlockHash, "td", td, "ttd", ttd) - return beacon.PayloadStatusV1{Status: beacon.INVALIDTERMINALBLOCK}, nil + return beacon.INVALID_TERMINAL_BLOCK, nil } if block.Time() <= parent.Time() { log.Warn("Invalid timestamp", "parent", block.Time(), "block", block.Time()) diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index 96dd340d7..2ae6d2cd5 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -136,7 +136,7 @@ func TestSetHeadBeforeTotalDifficulty(t *testing.T) { } if resp, err := api.ForkchoiceUpdatedV1(fcState, nil); err != nil { t.Errorf("fork choice updated should not error: %v", err) - } else if resp.PayloadStatus.Status != beacon.INVALIDTERMINALBLOCK { + } else if resp.PayloadStatus.Status != beacon.INVALID_TERMINAL_BLOCK.Status { t.Errorf("fork choice updated before total terminal difficulty should be INVALID") } }