only viable head is invalid #11117

potuz · 2022-07-27T14:37:00Z

This PR ensures that in the situation described in the linked issues, where after pruning INVALID blocks, no viable head is possible, we satisfy the following properties

Stay in optimistic mode Opti-sync: extend optimistic node definition ethereum/consensus-specs#2955
Are able to continue syncing blocks (Reviewers: during Init sync this needs verification)
Are able to restart the beacon node in these conditions (Reviewers: this needs verification)

This is achieved by allowing head to refer to an INVALID block, both in Forkchoice and in blockchain-service.head. Even though both store and forkchoice can have an INVALID root as head, the node itself and the blocks/states would have been removed from forkchoice and database.

Moreover, as a result of this prunning the blockchain.head and the forkchoice's cached head may differ, depending on wether the INVALID status was a result of notifyNewPayload (no block is inserted to forkchoice) or notifyForkchoiceUpdated (the block is inserted in forkchoice and not saved in blockchain.head)

Another danger is that any call to forkchoice.Head() while on this status will result in an error since there are no viable tips.

Fixes #10782
Fixes #10777

potuz · 2022-07-31T00:52:41Z

beacon-chain/blockchain/chain_info.go

+	s.headLock.RUnlock()

-	return s.IsOptimisticForRoot(ctx, s.head.root)
+	return s.IsOptimisticForRoot(ctx, headRoot)


IsOptimisticForRoot now requires a lock.

potuz · 2022-07-31T00:55:22Z

beacon-chain/blockchain/chain_info.go

+		headRoot, err := s.HeadRoot(ctx)
+		if err == nil && bytes.Equal(headRoot, root[:]) {
+			return true, nil
+		}
+		return true, errInvalidNilSummary


I feel it's safer to return true by default, even erroring out. The first if here ensures that if the head root is not in forkchoice and not in DB then we return that we are optimistic. The only reason I can currently think this may happen is only if we have pruned it and the headroot is invalid.

potuz · 2022-07-31T00:59:18Z

beacon-chain/blockchain/execution_engine.go

+					"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}


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

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.

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.

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

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.

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.

potuz · 2022-07-31T01:05:09Z

beacon-chain/blockchain/process_block_test.go

 	require.Equal(t, true, IsInvalidBlock(err))
 }
+
+// See the description in #10777 and #10782 for the full setup


The tests added here cover the main circumstances of a single branch being pruned and no viable head being available. It checks the behavior when the INVALID return comes from NotifyNewPayload and from FCU. Note however that in order to trigger justification change, at least one INVALID block needs to be imported to forkchoice, so this requires at least one INVALID block to be returned as SYNCING/ACCEPTED by notify_newPayload.

These tests do not cover initial sync which can be added if requested, but the paths seem to be contained to the execution engine functions, so it should be similar from onBlock to onBlockBatch. They also do not cover rebooting from this condition. This is a bit more dangerous since our starting head would be non-existent.

potuz · 2022-07-31T01:07:57Z

testing/util/altair.go

 		}
 	}

+	syncCommitteeBits := make(bitfield.Bitvector512, 64)


This function prepared a full SyncAggregate, but the signature would fail at the fork. Fixing that perhaps is probable, but since this is not used anywhere on any test, I decided that it was easier to just return the empty participation case.

See below: I had to reuse the previous behavior because of the monitor's test

potuz · 2022-07-31T01:14:04Z

testing/util/bellatrix.go

+		return nil, err
+	}
+	blockHash := indexToHash(uint64(slot))
+	newExecutionPayload := &enginev1.ExecutionPayload{


This can be improved by including transactions and configuring some of the fields that are hardcoded here.

potuz · 2022-07-31T01:14:54Z

testing/util/block.go

 	NumAttestations      uint64
 	NumDeposits          uint64
 	NumVoluntaryExits    uint64
+	NumTransactions      uint64 // Only for post Bellatrix blocks


this is unused now

potuz · 2022-07-31T01:20:02Z

testing/util/bellatrix.go

 )

-// BlockSignatureBellatrix calculates the post-state root of the block and returns the signature.
-func BlockSignatureBellatrix(


This function is exactly the same as the non-bellatrix version therefore I removed it.

potuz · 2022-07-31T01:21:12Z

testing/util/helpers.go

 func BlockSignature(
 	bState state.BeaconState,
-	block *ethpb.BeaconBlock,
+	block interface{},


This is to use this function to sign Altair and Bellatrix blocks as well.

potuz · 2022-07-31T01:22:35Z

testing/util/sync_aggregate.go

-	"github.com/prysmaticlabs/prysm/time/slots"
-)
-
-func generateSyncAggregate(st state.BeaconState, privs []bls.SecretKey, parentRoot [32]byte) (*ethpb.SyncAggregate, error) {


This function may be salvaged, the signature here is wrong and is never really used. Perhaps it can be saved in a way that it would sign correctly on fork transition, but since it's not used I figured it was better removed.

I had to reuse it because it turned out to be used by the monitor indirectly in tests

terencechain · 2022-07-31T16:50:23Z

beacon-chain/core/altair/block.go

 	votedKeys, votedIndices, didntVoteIndices, err := FilterSyncCommitteeVotes(s, sync)
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrap(err, "could not filter sync committee votes")


how is this related to the PR? I don't mind the changes, I'm just curious

ah I see, you made it better because you had to work through GenerateFullBlockAltair for tests

Yeah, things were failing and I didn't know where

beacon-chain/blockchain/chain_info.go

Co-authored-by: terencechain <terence@prysmaticlabs.com>

nisdas · 2022-08-02T07:26:54Z

beacon-chain/blockchain/chain_info.go

+		// 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)


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

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

nisdas · 2022-08-02T07:35:31Z

beacon-chain/blockchain/execution_engine.go

+					"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}


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.

beacon-chain/core/altair/block.go

mkalinin · 2022-08-02T10:14:52Z

Another danger is that any call to forkchoice.Head() while on this status will result in an error since there are no viable tips.

Why will it result in an error? According to the specification, get_head should return store.justified_checkpoint.root if no viable tip is available

potuz · 2022-08-02T10:35:27Z

Another danger is that any call to forkchoice.Head() while on this status will result in an error since there are no viable tips.

Why will it result in an error? According to the specification, get_head should return store.justified_checkpoint.root if no viable tip is available

This is implementation dependant. The specs cannot say what a function does or doesn't do. In this case the computed head will be the justified checkpoint but we return an error because it's not viable for head

Co-authored-by: Nishant Das <nishdas93@gmail.com>

terencechain · 2022-08-02T14:13:42Z

Chatted with @potuz offline. The ideal solution would be:

Implement this method in fork choice

(f *forkchocice) AreAllTipsInvalid() bool {}

Then in IsOptmistic, we add the following breaking:

func (s *Service) IsOptimistic(ctx context.Context) (bool, error) {
   if s.cfg.ForkChoiceStore.AreAllTipsInvalid {
      return true, nil
   }
}

That can be done in another PR. For now, I'm going to move forward with this one

potuz added 9 commits July 27, 2022 11:31

failing onBlock syncing

bcbdf43

passing merge check

534d1ea

failing signature verification

bd0c5f2

still failing block signature

4cad80e

mock full bellatrix blocks

6ce8999

working unit test

f8890ff

return error from FCU if head fails to update

a5778c5

move bellatrix block generator

d794b14

remove bellatrix signature function

8e1d12e

potuz commented Jul 31, 2022

View reviewed changes

potuz added 2 commits July 30, 2022 23:36

Add liveness unit tests

9ccb63a

Merge branch 'develop' into invalid-head

84f0c8f

potuz marked this pull request as ready for review July 31, 2022 10:43

potuz requested a review from a team as a code owner July 31, 2022 10:44

potuz requested review from james-prysm, saolyn and symbolpunk July 31, 2022 10:44

potuz added 2 commits July 31, 2022 09:34

revert removal of sync_aggregate.go

98c27db

gaz

2eb203b

potuz added the Ready For Review label Jul 31, 2022

terencechain reviewed Jul 31, 2022

View reviewed changes

beacon-chain/blockchain/chain_info.go Outdated Show resolved Hide resolved

potuz and others added 2 commits July 31, 2022 14:35

Terence's suggestion

164dbb4

Co-authored-by: terencechain <terence@prysmaticlabs.com>

go fmt

908dc3e

terencechain previously approved these changes Aug 1, 2022

View reviewed changes

Merge branch 'develop' into invalid-head

18e1444

nisdas reviewed Aug 2, 2022

View reviewed changes

Nishant's suggestion

3ecd852

Co-authored-by: Nishant Das <nishdas93@gmail.com>

potuz dismissed terencechain’s stale review via 3ecd852 August 2, 2022 10:49

nisdas and others added 2 commits August 2, 2022 19:05

Merge branch 'develop' into invalid-head

12ca2cf

Fix build

2ce8c91

terencechain approved these changes Aug 2, 2022

View reviewed changes

nisdas approved these changes Aug 2, 2022

View reviewed changes

potuz merged commit 4b46dea into develop Aug 2, 2022

delete-merged-branch bot deleted the invalid-head branch August 2, 2022 14:55

only viable head is invalid #11117

only viable head is invalid #11117

Uh oh!

Conversation

potuz commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mkalinin commented Aug 2, 2022

Uh oh!

potuz commented Aug 2, 2022

Uh oh!

terencechain commented Aug 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

potuz commented Jul 27, 2022 •

edited

Loading