Propagate FCU invalid error (#10997)

* Wrap fcu error

* Wrap fcu error

* Wrap error better

* More test

* Add else

* Potuz feedback

* Propagate the correct root for fcu

* Always return true for invalid

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
This commit is contained in:
terencechain
2022-07-09 13:51:03 -07:00
committed by GitHub
parent 6695e7c756
commit 7fd2c08be3
11 changed files with 116 additions and 61 deletions

View File

@@ -4,9 +4,9 @@ import "github.com/pkg/errors"
var (
// ErrInvalidPayload is returned when the payload is invalid
ErrInvalidPayload = errors.New("received an INVALID payload from execution engine")
ErrInvalidPayload = invalidBlock{error: errors.New("received an INVALID payload from execution engine")}
// ErrInvalidBlockHashPayloadStatus is returned when the payload has invalid block hash.
ErrInvalidBlockHashPayloadStatus = errors.New("received an INVALID_BLOCK_HASH payload from execution engine")
ErrInvalidBlockHashPayloadStatus = invalidBlock{error: errors.New("received an INVALID_BLOCK_HASH payload from execution engine")}
// ErrUndefinedExecutionEngineError is returned when the execution engine returns an error that is not defined
ErrUndefinedExecutionEngineError = errors.New("received an undefined ee error")
// errNilFinalizedInStore is returned when a nil finalized checkpt is returned from store.
@@ -30,7 +30,7 @@ var (
// errWSBlockNotFoundInEpoch is returned when a block is not found in the WS cache or DB within epoch.
errWSBlockNotFoundInEpoch = errors.New("weak subjectivity root not found in db within epoch")
// errNotDescendantOfFinalized is returned when a block is not a descendant of the finalized checkpoint
errNotDescendantOfFinalized = invalidBlock{errors.New("not descendant of finalized checkpoint")}
errNotDescendantOfFinalized = invalidBlock{error: errors.New("not descendant of finalized checkpoint")}
)
// An invalid block is the block that fails state transition based on the core protocol rules.
@@ -41,16 +41,17 @@ var (
// The block violates certain fork choice rules (before finalized slot, not finalized ancestor)
type invalidBlock struct {
error
root [32]byte
}
type invalidBlockError interface {
Error() string
InvalidBlock() bool
BlockRoot() [32]byte
}
// InvalidBlock returns true for `invalidBlock`.
func (e invalidBlock) InvalidBlock() bool {
return true
// BlockRoot returns the invalid block root.
func (e invalidBlock) BlockRoot() [32]byte {
return e.root
}
// IsInvalidBlock returns true if the error has `invalidBlock`.
@@ -58,9 +59,22 @@ func IsInvalidBlock(e error) bool {
if e == nil {
return false
}
d, ok := e.(invalidBlockError)
_, ok := e.(invalidBlockError)
if !ok {
return IsInvalidBlock(errors.Unwrap(e))
}
return d.InvalidBlock()
return true
}
// InvalidBlockRoot returns the invalid block root. If the error
// doesn't have an invalid blockroot. [32]byte{} is returned.
func InvalidBlockRoot(e error) [32]byte {
if e == nil {
return [32]byte{}
}
d, ok := e.(invalidBlockError)
if !ok {
return [32]byte{}
}
return d.BlockRoot()
}

View File

@@ -8,10 +8,18 @@ import (
)
func TestIsInvalidBlock(t *testing.T) {
require.Equal(t, false, IsInvalidBlock(ErrInvalidPayload))
err := invalidBlock{ErrInvalidPayload}
require.Equal(t, true, IsInvalidBlock(ErrInvalidPayload)) // Already wrapped.
err := invalidBlock{error: ErrInvalidPayload}
require.Equal(t, true, IsInvalidBlock(err))
newErr := errors.Wrap(err, "wrap me")
require.Equal(t, true, IsInvalidBlock(newErr))
}
func TestInvalidBlockRoot(t *testing.T) {
require.Equal(t, [32]byte{}, InvalidBlockRoot(ErrUndefinedExecutionEngineError))
require.Equal(t, [32]byte{}, InvalidBlockRoot(ErrInvalidPayload))
err := invalidBlock{error: ErrInvalidPayload, root: [32]byte{'a'}}
require.Equal(t, [32]byte{'a'}, InvalidBlockRoot(err))
}

View File

@@ -39,19 +39,22 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, arg *notifyForkcho
headBlk := arg.headBlock
if headBlk == nil || headBlk.IsNil() || headBlk.Body().IsNil() {
return nil, errors.New("nil head block")
log.Error("Head block is nil")
return nil, nil
}
// Must not call fork choice updated until the transition conditions are met on the Pow network.
isExecutionBlk, err := blocks.IsExecutionBlock(headBlk.Body())
if err != nil {
return nil, errors.Wrap(err, "could not determine if block is execution block")
log.WithError(err).Error("Could not determine if head block is execution block")
return nil, nil
}
if !isExecutionBlk {
return nil, nil
}
headPayload, err := headBlk.Body().ExecutionPayload()
if err != nil {
return nil, errors.Wrap(err, "could not get execution payload")
log.WithError(err).Error("Could not get execution payload for head block")
return nil, nil
}
finalizedHash := s.ForkChoicer().FinalizedPayloadBlockHash()
justifiedHash := s.ForkChoicer().JustifiedPayloadBlockHash()
@@ -64,7 +67,8 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, arg *notifyForkcho
nextSlot := s.CurrentSlot() + 1 // Cache payload ID for next slot proposer.
hasAttr, attr, proposerId, err := s.getPayloadAttribute(ctx, arg.headState, nextSlot)
if err != nil {
return nil, errors.Wrap(err, "could not get payload attribute")
log.WithError(err).Error("Could not get head payload attribute")
return nil, nil
}
payloadID, lastValidHash, err := s.cfg.ExecutionEngineCaller.ForkchoiceUpdated(ctx, fcs, attr)
@@ -77,29 +81,38 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, arg *notifyForkcho
"headPayloadBlockHash": fmt.Sprintf("%#x", bytesutil.Trunc(headPayload.BlockHash)),
"finalizedPayloadBlockHash": fmt.Sprintf("%#x", bytesutil.Trunc(finalizedHash[:])),
}).Info("Called fork choice updated with optimistic block")
return payloadID, s.optimisticCandidateBlock(ctx, headBlk)
err := s.optimisticCandidateBlock(ctx, headBlk)
if err != nil {
log.WithError(err).Error("Optimistic block failed to be candidate")
}
return payloadID, nil
case powchain.ErrInvalidPayloadStatus:
newPayloadInvalidNodeCount.Inc()
headRoot := arg.headRoot
invalidRoots, err := s.ForkChoicer().SetOptimisticToInvalid(ctx, headRoot, bytesutil.ToBytes32(headBlk.ParentRoot()), bytesutil.ToBytes32(lastValidHash))
if err != nil {
return nil, err
log.WithError(err).Error("Could not set head root to invalid")
return nil, nil
}
if err := s.removeInvalidBlockAndState(ctx, invalidRoots); err != nil {
return nil, err
log.WithError(err).Error("Could not remove invalid block and state")
return nil, nil
}
r, err := s.cfg.ForkChoiceStore.Head(ctx, s.justifiedBalances.balances)
if err != nil {
return nil, err
log.WithError(err).Error("Could not get head root")
return nil, nil
}
b, err := s.getBlock(ctx, r)
if err != nil {
return nil, err
log.WithError(err).Error("Could not get head block")
return nil, nil
}
st, err := s.cfg.StateGen.StateByRoot(ctx, r)
if err != nil {
return nil, err
log.WithError(err).Error("Could not get head state")
return nil, nil
}
pid, err := s.notifyForkchoiceUpdate(ctx, &notifyForkchoiceUpdateArg{
headState: st,
@@ -107,7 +120,7 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, arg *notifyForkcho
headBlock: b.Block(),
})
if err != nil {
return nil, err
return nil, err // Returning err because it's recursive here.
}
if err := s.saveHead(ctx, r, b, st); err != nil {
@@ -120,15 +133,17 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, arg *notifyForkcho
"invalidCount": len(invalidRoots),
"newHeadRoot": fmt.Sprintf("%#x", bytesutil.Trunc(r[:])),
}).Warn("Pruned invalid blocks")
return pid, ErrInvalidPayload
return pid, invalidBlock{error: ErrInvalidPayload, root: arg.headRoot}
default:
return nil, errors.WithMessage(ErrUndefinedExecutionEngineError, err.Error())
log.WithError(err).Error(ErrUndefinedExecutionEngineError)
return nil, nil
}
}
forkchoiceUpdatedValidNodeCount.Inc()
if err := s.cfg.ForkChoiceStore.SetOptimisticToValid(ctx, arg.headRoot); err != nil {
return nil, errors.Wrap(err, "could not set block to valid")
log.WithError(err).Error("Could not set head root to valid")
return nil, nil
}
if hasAttr { // If the forkchoice update call has an attribute, update the proposer payload ID cache.
var pId [8]byte
@@ -173,14 +188,14 @@ func (s *Service) notifyNewPayload(ctx context.Context, postStateVersion int,
body := blk.Block().Body()
enabled, err := blocks.IsExecutionEnabledUsingHeader(postStateHeader, body)
if err != nil {
return false, errors.Wrap(invalidBlock{err}, "could not determine if execution is enabled")
return false, errors.Wrap(invalidBlock{error: err}, "could not determine if execution is enabled")
}
if !enabled {
return true, nil
}
payload, err := body.ExecutionPayload()
if err != nil {
return false, errors.Wrap(invalidBlock{err}, "could not get execution payload")
return false, errors.Wrap(invalidBlock{error: err}, "could not get execution payload")
}
lastValidHash, err := s.cfg.ExecutionEngineCaller.NewPayload(ctx, payload)
switch err {
@@ -212,10 +227,10 @@ func (s *Service) notifyNewPayload(ctx context.Context, postStateVersion int,
"blockRoot": fmt.Sprintf("%#x", root),
"invalidCount": len(invalidRoots),
}).Warn("Pruned invalid blocks")
return false, invalidBlock{ErrInvalidPayload}
return false, ErrInvalidPayload
case powchain.ErrInvalidBlockHashPayloadStatus:
newPayloadInvalidNodeCount.Inc()
return false, invalidBlock{ErrInvalidBlockHashPayloadStatus}
return false, ErrInvalidBlockHashPayloadStatus
default:
return false, errors.WithMessage(ErrUndefinedExecutionEngineError, err.Error())
}

View File

@@ -75,10 +75,6 @@ func Test_NotifyForkchoiceUpdate(t *testing.T) {
newForkchoiceErr error
errString string
}{
{
name: "nil block",
errString: "nil head block",
},
{
name: "phase0 block",
blk: func() interfaces.BeaconBlock {
@@ -192,6 +188,10 @@ func Test_NotifyForkchoiceUpdate(t *testing.T) {
_, err = service.notifyForkchoiceUpdate(ctx, arg)
if tt.errString != "" {
require.ErrorContains(t, tt.errString, err)
if tt.errString == ErrInvalidPayload.Error() {
require.Equal(t, true, IsInvalidBlock(err))
require.Equal(t, tt.headRoot, InvalidBlockRoot(err)) // Head root should be invalid. Not block root!
}
} else {
require.NoError(t, err)
}
@@ -330,7 +330,9 @@ func Test_NotifyForkchoiceUpdateRecursive_Protoarray(t *testing.T) {
headRoot: brg,
}
_, err = service.notifyForkchoiceUpdate(ctx, a)
require.ErrorIs(t, ErrInvalidPayload, err)
require.Equal(t, true, IsInvalidBlock(err))
require.Equal(t, brf, InvalidBlockRoot(err))
// Ensure Head is D
headRoot, err = fcs.Head(ctx, service.justifiedBalances.balances)
require.NoError(t, err)
@@ -473,7 +475,9 @@ func Test_NotifyForkchoiceUpdateRecursive_DoublyLinkedTree(t *testing.T) {
headRoot: brg,
}
_, err = service.notifyForkchoiceUpdate(ctx, a)
require.ErrorIs(t, ErrInvalidPayload, err)
require.Equal(t, true, IsInvalidBlock(err))
require.Equal(t, brf, InvalidBlockRoot(err))
// Ensure Head is D
headRoot, err = fcs.Head(ctx, service.justifiedBalances.balances)
require.NoError(t, err)

View File

@@ -66,7 +66,7 @@ func (s *Service) validateMergeBlock(ctx context.Context, b interfaces.SignedBea
if !valid {
err := fmt.Errorf("invalid TTD, configTTD: %s, currentTTD: %s, parentTTD: %s",
params.BeaconConfig().TerminalTotalDifficulty, mergeBlockTD, mergeBlockParentTD)
return invalidBlock{err}
return invalidBlock{error: err}
}
log.WithFields(logrus.Fields{

View File

@@ -97,7 +97,7 @@ func (s *Service) onBlock(ctx context.Context, signed interfaces.SignedBeaconBlo
ctx, span := trace.StartSpan(ctx, "blockChain.onBlock")
defer span.End()
if err := wrapper.BeaconBlockIsNil(signed); err != nil {
return invalidBlock{err}
return invalidBlock{error: err}
}
b := signed.Block()
@@ -118,7 +118,7 @@ func (s *Service) onBlock(ctx context.Context, signed interfaces.SignedBeaconBlo
}
postState, err := transition.ExecuteStateTransition(ctx, preState, signed)
if err != nil {
return invalidBlock{err}
return invalidBlock{error: err}
}
postStateVersion, postStateHeader, err := getStateVersionAndPayload(postState)
if err != nil {
@@ -184,7 +184,9 @@ func (s *Service) onBlock(ctx context.Context, signed interfaces.SignedBeaconBlo
if err != nil {
log.WithError(err).Warn("Could not update head")
}
s.notifyEngineIfChangedHead(ctx, headRoot)
if err := s.notifyEngineIfChangedHead(ctx, headRoot); err != nil {
return err
}
if err := s.pruneCanonicalAttsFromPool(ctx, blockRoot, signed); err != nil {
return err
@@ -293,7 +295,7 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []interfaces.SignedBeac
}
if err := wrapper.BeaconBlockIsNil(blks[0]); err != nil {
return invalidBlock{err}
return invalidBlock{error: err}
}
b := blks[0].Block()
@@ -341,7 +343,7 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []interfaces.SignedBeac
set, preState, err = transition.ExecuteStateTransitionNoVerifyAnySig(ctx, preState, b)
if err != nil {
return invalidBlock{err}
return invalidBlock{error: err}
}
// Save potential boundary states.
if slots.IsEpochStart(preState.Slot()) {
@@ -362,7 +364,7 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []interfaces.SignedBeac
}
verify, err := sigSet.Verify()
if err != nil {
return invalidBlock{err}
return invalidBlock{error: err}
}
if !verify {
return errors.New("batch block signature verification failed")
@@ -587,7 +589,7 @@ func (s *Service) validateMergeTransitionBlock(ctx context.Context, stateVersion
// Skip validation if block has an empty payload.
payload, err := blk.Block().Body().ExecutionPayload()
if err != nil {
return invalidBlock{err}
return invalidBlock{error: err}
}
if bellatrix.IsEmptyPayload(payload) {
return nil

View File

@@ -114,7 +114,7 @@ func (s *Service) VerifyFinalizedBlkDescendant(ctx context.Context, root [32]byt
bytesutil.Trunc(root[:]), fSlot, bytesutil.Trunc(bFinalizedRoot),
bytesutil.Trunc(fRoot[:]))
tracing.AnnotateError(span, err)
return invalidBlock{err}
return invalidBlock{error: err}
}
return nil
}
@@ -129,7 +129,7 @@ func (s *Service) verifyBlkFinalizedSlot(b interfaces.BeaconBlock) error {
}
if finalizedSlot >= b.Slot() {
err = fmt.Errorf("block is equal or earlier than finalized block, slot %d < slot %d", b.Slot(), finalizedSlot)
return invalidBlock{err}
return invalidBlock{error: err}
}
return nil
}

View File

@@ -166,33 +166,35 @@ func (s *Service) UpdateHead(ctx context.Context) error {
}).Debug("Head changed due to attestations")
}
s.headLock.RUnlock()
s.notifyEngineIfChangedHead(ctx, newHeadRoot)
if err := s.notifyEngineIfChangedHead(ctx, newHeadRoot); err != nil {
return err
}
return nil
}
// This calls notify Forkchoice Update in the event that the head has changed
func (s *Service) notifyEngineIfChangedHead(ctx context.Context, newHeadRoot [32]byte) {
func (s *Service) notifyEngineIfChangedHead(ctx context.Context, newHeadRoot [32]byte) error {
s.headLock.RLock()
if newHeadRoot == [32]byte{} || s.headRoot() == newHeadRoot {
s.headLock.RUnlock()
return
return nil
}
s.headLock.RUnlock()
if !s.hasBlockInInitSyncOrDB(ctx, newHeadRoot) {
log.Debug("New head does not exist in DB. Do nothing")
return // We don't have the block, don't notify the engine and update head.
return nil // We don't have the block, don't notify the engine and update head.
}
newHeadBlock, err := s.getBlock(ctx, newHeadRoot)
if err != nil {
log.WithError(err).Error("Could not get new head block")
return
return nil
}
headState, err := s.cfg.StateGen.StateByRoot(ctx, newHeadRoot)
if err != nil {
log.WithError(err).Error("Could not get state from db")
return
return nil
}
arg := &notifyForkchoiceUpdateArg{
headState: headState,
@@ -201,11 +203,12 @@ func (s *Service) notifyEngineIfChangedHead(ctx context.Context, newHeadRoot [32
}
_, err = s.notifyForkchoiceUpdate(s.ctx, arg)
if err != nil {
log.WithError(err).Error("could not notify forkchoice update")
return err
}
if err := s.saveHead(ctx, newHeadRoot, newHeadBlock, headState); err != nil {
log.WithError(err).Error("could not save head")
}
return nil
}
// This processes fork choice attestations from the pool to account for validator votes and fork choice.

View File

@@ -131,7 +131,7 @@ func TestNotifyEngineIfChangedHead(t *testing.T) {
service, err := NewService(ctx, opts...)
require.NoError(t, err)
service.cfg.ProposerSlotIndexCache = cache.NewProposerPayloadIDsCache()
service.notifyEngineIfChangedHead(ctx, service.headRoot())
require.NoError(t, service.notifyEngineIfChangedHead(ctx, service.headRoot()))
hookErr := "could not notify forkchoice update"
invalidStateErr := "Could not get state from db"
require.LogsDoNotContain(t, hook, invalidStateErr)
@@ -139,7 +139,7 @@ func TestNotifyEngineIfChangedHead(t *testing.T) {
gb, err := wrapper.WrappedSignedBeaconBlock(util.NewBeaconBlock())
require.NoError(t, err)
require.NoError(t, service.saveInitSyncBlock(ctx, [32]byte{'a'}, gb))
service.notifyEngineIfChangedHead(ctx, [32]byte{'a'})
require.NoError(t, service.notifyEngineIfChangedHead(ctx, [32]byte{'a'}))
require.LogsContain(t, hook, invalidStateErr)
hook.Reset()
@@ -164,7 +164,7 @@ func TestNotifyEngineIfChangedHead(t *testing.T) {
state: st,
}
service.cfg.ProposerSlotIndexCache.SetProposerAndPayloadIDs(2, 1, [8]byte{1})
service.notifyEngineIfChangedHead(ctx, r1)
require.NoError(t, service.notifyEngineIfChangedHead(ctx, r1))
require.LogsDoNotContain(t, hook, invalidStateErr)
require.LogsDoNotContain(t, hook, hookErr)
@@ -182,7 +182,7 @@ func TestNotifyEngineIfChangedHead(t *testing.T) {
state: st,
}
service.cfg.ProposerSlotIndexCache.SetProposerAndPayloadIDs(2, 1, [8]byte{1})
service.notifyEngineIfChangedHead(ctx, r1)
require.NoError(t, service.notifyEngineIfChangedHead(ctx, r1))
require.LogsDoNotContain(t, hook, invalidStateErr)
require.LogsDoNotContain(t, hook, hookErr)
vId, payloadID, has := service.cfg.ProposerSlotIndexCache.GetProposerPayloadIDs(2)
@@ -192,7 +192,7 @@ func TestNotifyEngineIfChangedHead(t *testing.T) {
// Test zero headRoot returns immediately.
headRoot := service.headRoot()
service.notifyEngineIfChangedHead(ctx, [32]byte{})
require.NoError(t, service.notifyEngineIfChangedHead(ctx, [32]byte{}))
require.Equal(t, service.headRoot(), headRoot)
}

View File

@@ -170,8 +170,12 @@ func (s *Service) processPendingBlocks(ctx context.Context) error {
if err := s.cfg.chain.ReceiveBlock(ctx, b, blkRoot); err != nil {
if blockchain.IsInvalidBlock(err) {
tracing.AnnotateError(span, err)
s.setBadBlock(ctx, blkRoot)
r := blockchain.InvalidBlockRoot(err)
if r != [32]byte{} {
s.setBadBlock(ctx, r) // Setting head block as bad.
} else {
s.setBadBlock(ctx, blkRoot)
}
}
log.Debugf("Could not process block from slot %d: %v", b.Block().Slot(), err)

View File

@@ -31,8 +31,13 @@ func (s *Service) beaconBlockSubscriber(ctx context.Context, msg proto.Message)
if err := s.cfg.chain.ReceiveBlock(ctx, signed, root); err != nil {
if blockchain.IsInvalidBlock(err) {
interop.WriteBlockToDisk(signed, true /*failed*/)
s.setBadBlock(ctx, root)
r := blockchain.InvalidBlockRoot(err)
if r != [32]byte{} {
s.setBadBlock(ctx, r) // Setting head block as bad.
} else {
interop.WriteBlockToDisk(signed, true /*failed*/)
s.setBadBlock(ctx, root)
}
}
return err
}