Skip to content

only viable head is invalid #11117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
potuz merged 19 commits into from
Aug 2, 2022
Merged

only viable head is invalid #11117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
bcbdf43
failing onBlock syncing
potuz Jul 27, 2022
534d1ea
passing merge check
potuz Jul 28, 2022
bd0c5f2
failing signature verification
potuz Jul 28, 2022
4cad80e
still failing block signature
potuz Jul 30, 2022
6ce8999
mock full bellatrix blocks
potuz Jul 30, 2022
f8890ff
working unit test
potuz Jul 30, 2022
a5778c5
return error from FCU if head fails to update
potuz Jul 30, 2022
d794b14
move bellatrix block generator
potuz Jul 31, 2022
8e1d12e
remove bellatrix signature function
potuz Jul 31, 2022
9ccb63a
Add liveness unit tests
potuz Jul 31, 2022
84f0c8f
Merge branch 'develop' into invalid-head
potuz Jul 31, 2022
98c27db
revert removal of sync_aggregate.go
potuz Jul 31, 2022
2eb203b
gaz
potuz Jul 31, 2022
164dbb4
Terence's suggestion
potuz Jul 31, 2022
908dc3e
go fmt
potuz Jul 31, 2022
18e1444
Merge branch 'develop' into invalid-head
terencechain Aug 1, 2022
3ecd852
Nishant's suggestion
potuz Aug 2, 2022
12ca2cf
Merge branch 'develop' into invalid-head
nisdas Aug 2, 2022
2ce8c91
Fix build
terencechain Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions beacon-chain/blockchain/chain_info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package blockchain

import (
"bytes"
"context"
"time"

Expand Down Expand Up @@ -299,13 +300,14 @@ func (s *Service) ForkChoicer() forkchoice.ForkChoicer {

// IsOptimistic returns true if the current head is optimistic.
func (s *Service) IsOptimistic(ctx context.Context) (bool, error) {
s.headLock.RLock()
defer s.headLock.RUnlock()
if slots.ToEpoch(s.CurrentSlot()) < params.BeaconConfig().BellatrixForkEpoch {
return false, nil
}
s.headLock.RLock()
headRoot := s.head.root
s.headLock.RUnlock()

return s.IsOptimisticForRoot(ctx, s.head.root)
return s.IsOptimisticForRoot(ctx, headRoot)
Copy link
Contributor Author

@potuz potuz Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsOptimisticForRoot now requires a lock.

}

// IsFinalized returns true if the input root is finalized.
Expand All @@ -332,7 +334,17 @@ func (s *Service) IsOptimisticForRoot(ctx context.Context, root [32]byte) (bool,
return false, err
}
if ss == nil {
return false, errInvalidNilSummary
// if the requested root is the headroot we should treat the
// node as optimistic. This can happen if we pruned INVALID
// nodes and no viable head is available.
headRoot, err := s.HeadRoot(ctx)
Copy link
Contributor

@nisdas nisdas Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this work, we have a head for which there is no state summary/block in the db ?

Copy link
Contributor Author

@potuz potuz Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and even worse, it can be different than forkchoice's

if err != nil {
return true, err
}
if bytes.Equal(headRoot, root[:]) {
return true, nil
}
return true, errInvalidNilSummary
}

validatedCheckpoint, err := s.cfg.BeaconDB.LastValidatedCheckpoint(ctx)
Expand Down
8 changes: 6 additions & 2 deletions beacon-chain/blockchain/execution_engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,12 @@ func (s *Service) notifyForkchoiceUpdate(ctx context.Context, arg *notifyForkcho

r, err := s.cfg.ForkChoiceStore.Head(ctx, s.justifiedBalances.balances)
if err != nil {
log.WithError(err).Error("Could not get head root")
return nil, nil
log.WithFields(logrus.Fields{
"slot": headBlk.Slot(),
"blockRoot": fmt.Sprintf("%#x", bytesutil.Trunc(headRoot[:])),
"invalidCount": len(invalidRoots),
}).Warn("Pruned invalid blocks, could not update head root")
return nil, invalidBlock{error: ErrInvalidPayload, root: arg.headRoot, invalidAncestorRoots: invalidRoots}
Copy link
Contributor Author

@potuz potuz Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will log here the pruned blocks, this is a code smell for a few reasons:

  1. It repeats the log below, it can be done by having two different log messages the only difference between this one and the one below in the regular pruning on invalid blocks is that the one below contains the new head root (that we failed to compute here). I didn't care much about the repetition because this code should not really happen in runtime unless some pretty extreme situation.
  2. This is a fairly sensitive change. On the one hand it fixes a bad bug: we were returning nil, allowing onBlock to succeed without any error when our head was INVALID! on the other hand it changes the return path of a recursive function, care must be taken by reviewers in all possible paths here.

Copy link
Collaborator

@terencechain terencechain Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what is bothering me a little bit. We are returning nil for every err != nil under ErrInvalidPayloadStatus. Should we change most of them to invalidBlock{error: ErrInvalidPayload, root: arg.headRoot, invalidAncestorRoots: invalidRoots}? Ok to do this in another PR

Copy link
Contributor Author

@potuz potuz Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It bothers me too, but we used to return errors on everything and it was bad rendering the node locked with minor things. I feel this situation shouldn't happen often so it's kinda safe to put it here, I haven't thought deeply on the other errors here.

Copy link
Contributor

@nisdas nisdas Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we only add the invalid block error here ?

invalidBlock{error: ErrInvalidPayload, root: arg.headRoot, invalidAncestorRoots: invalidRoots}

In this whole conditional branch, this is the only location where we return the error.

}
b, err := s.getBlock(ctx, r)
if err != nil {
Expand Down
Loading