From 7a64fe44eb9d3c6e3ff4e9830d4e828f248ef2a0 Mon Sep 17 00:00:00 2001 From: Giulio rebuffo Date: Mon, 8 Aug 2022 11:26:34 +0200 Subject: [PATCH] UX improvement for pre-merge sync with Teku (#4955) Co-authored-by: giuliorebuffo --- ethdb/privateapi/ethbackend.go | 75 +++++++++++++++---------------- turbo/engineapi/fork_validator.go | 2 +- 2 files changed, 36 insertions(+), 41 deletions(-) diff --git a/ethdb/privateapi/ethbackend.go b/ethdb/privateapi/ethbackend.go index fe9b93dfb..4d095d992 100644 --- a/ethdb/privateapi/ethbackend.go +++ b/ethdb/privateapi/ethbackend.go @@ -313,23 +313,14 @@ func (s *EthBackendServer) EngineNewPayloadV1(ctx context.Context, req *types2.E } block := types.NewBlockFromStorage(blockHash, &header, transactions, nil) - possibleStatus, err := s.getPayloadStatusFromHashIfPossible(blockHash, req.BlockNumber, header.ParentHash, true) + possibleStatus, err := s.getPayloadStatusFromHashIfPossible(blockHash, req.BlockNumber, header.ParentHash, nil, true) if err != nil { return nil, err } if possibleStatus != nil { return convertPayloadStatus(possibleStatus), nil } - // If another payload is already commissioned then we just reply with syncing - if s.stageLoopIsBusy() { - // We are still syncing a commissioned payload - // TODO(yperbasis): not entirely correct since per the spec: - // The process of validating a payload on the canonical chain MUST NOT be affected by an active sync process on a side branch of the block tree. - // For example, if side branch B is SYNCING but the requisite data for validating a payload from canonical branch A is available, client software MUST initiate the validation process. - // https://github.com/ethereum/execution-apis/blob/v1.0.0-alpha.6/src/engine/specification.md#payload-validation - log.Debug("[NewPayload] stage loop is busy") - return &remote.EnginePayloadStatus{Status: remote.EngineStatus_SYNCING}, nil - } + s.lock.Lock() defer s.lock.Unlock() @@ -347,7 +338,7 @@ func (s *EthBackendServer) EngineNewPayloadV1(ctx context.Context, req *types2.E } // Check if we can make out a status from the payload hash/head hash. -func (s *EthBackendServer) getPayloadStatusFromHashIfPossible(blockHash common.Hash, blockNumber uint64, parentHash common.Hash, newPayload bool) (*engineapi.PayloadStatus, error) { +func (s *EthBackendServer) getPayloadStatusFromHashIfPossible(blockHash common.Hash, blockNumber uint64, parentHash common.Hash, forkchoiceMessage *engineapi.ForkChoiceMessage, newPayload bool) (*engineapi.PayloadStatus, error) { // Determine which prefix to use for logs var prefix string if newPayload { @@ -361,7 +352,7 @@ func (s *EthBackendServer) getPayloadStatusFromHashIfPossible(blockHash common.H } if s.hd == nil { - return nil, nil + return nil, fmt.Errorf("headerdownload is nil") } tx, err := s.db.BeginRo(s.ctx) @@ -369,6 +360,14 @@ func (s *EthBackendServer) getPayloadStatusFromHashIfPossible(blockHash common.H return nil, err } defer tx.Rollback() + // Some Consensus layer clients sometimes sends us repeated FCUs and make Erigon print a gazillion logs. + // E.G teku sometimes will end up spamming fcu on the terminal block if it has not synced to that point. + if forkchoiceMessage != nil && + forkchoiceMessage.FinalizedBlockHash == rawdb.ReadForkchoiceFinalized(tx) && + forkchoiceMessage.HeadBlockHash == rawdb.ReadForkchoiceHead(tx) && + forkchoiceMessage.SafeBlockHash == rawdb.ReadForkchoiceSafe(tx) { + return &engineapi.PayloadStatus{Status: remote.EngineStatus_VALID, LatestValidHash: blockHash}, nil + } header, err := rawdb.ReadHeaderByHash(tx, blockHash) if err != nil { @@ -437,28 +436,31 @@ func (s *EthBackendServer) getPayloadStatusFromHashIfPossible(blockHash common.H if parent == nil && s.hd.PosStatus() == headerdownload.Syncing { return &engineapi.PayloadStatus{Status: remote.EngineStatus_SYNCING}, nil } - - return nil, nil - } - - if header == nil { - if s.hd.PosStatus() == headerdownload.Syncing { - return &engineapi.PayloadStatus{Status: remote.EngineStatus_SYNCING}, nil - + } else { + if header == nil { + if s.hd.PosStatus() == headerdownload.Syncing { + return &engineapi.PayloadStatus{Status: remote.EngineStatus_SYNCING}, nil + } + return nil, nil + } + + headHash := rawdb.ReadHeadBlockHash(tx) + if err != nil { + return nil, err + } + + // We add the extra restriction blockHash != headHash for the FCU case of canonicalHash == blockHash + // because otherwise (when FCU points to the head) we want go to stage headers + // so that it calls writeForkChoiceHashes. + if blockHash != headHash && canonicalHash == blockHash { + return &engineapi.PayloadStatus{Status: remote.EngineStatus_VALID, LatestValidHash: blockHash}, nil } - return nil, nil } - headHash := rawdb.ReadHeadBlockHash(tx) - if err != nil { - return nil, err - } - - // We add the extra restriction blockHash != headHash for the FCU case of canonicalHash == blockHash - // because otherwise (when FCU points to the head) we want go to stage headers - // so that it calls writeForkChoiceHashes. - if blockHash != headHash && canonicalHash == blockHash { - return &engineapi.PayloadStatus{Status: remote.EngineStatus_VALID, LatestValidHash: blockHash}, nil + // If another payload is already commissioned then we just reply with syncing + if s.stageLoopIsBusy() { + log.Debug(fmt.Sprintf("[%s] stage loop is busy", prefix)) + return &engineapi.PayloadStatus{Status: remote.EngineStatus_SYNCING}, nil } return nil, nil @@ -526,18 +528,11 @@ func (s *EthBackendServer) EngineForkChoiceUpdatedV1(ctx context.Context, req *r FinalizedBlockHash: gointerfaces.ConvertH256ToHash(req.ForkchoiceState.FinalizedBlockHash), } - status, err := s.getPayloadStatusFromHashIfPossible(forkChoice.HeadBlockHash, 0, common.Hash{}, false) + status, err := s.getPayloadStatusFromHashIfPossible(forkChoice.HeadBlockHash, 0, common.Hash{}, &forkChoice, false) if err != nil { return nil, err } - if status == nil && s.stageLoopIsBusy() { - log.Debug("[ForkChoiceUpdated] stage loop is busy") - return &remote.EngineForkChoiceUpdatedReply{ - PayloadStatus: &remote.EnginePayloadStatus{Status: remote.EngineStatus_SYNCING}, - }, nil - } - s.lock.Lock() defer s.lock.Unlock() diff --git a/turbo/engineapi/fork_validator.go b/turbo/engineapi/fork_validator.go index f5a2a61a0..225977cf1 100644 --- a/turbo/engineapi/fork_validator.go +++ b/turbo/engineapi/fork_validator.go @@ -186,7 +186,7 @@ func (fv *ForkValidator) ValidatePayload(tx kv.RwTx, header *types.Header, body return } // MakesBodyCanonical do not support PoS. - if has && len(sb.body.Transactions) > 0 { + if has { status = remote.EngineStatus_ACCEPTED return }