Add `execution_optimistic` field to API responses #10389

rkapka · 2022-03-17T18:01:55Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Adds a new execution_optimistic field to API responses. The design is based on ethereum/beacon-APIs#190

# Conflicts: # beacon-chain/rpc/eth/debug/debug_test.go

terencechain · 2022-03-17T21:01:44Z

beacon-chain/rpc/eth/beacon/blocks.go

@@ -67,6 +67,10 @@ func (bs *Server) GetBlockHeader(ctx context.Context, req *ethpbv1.BlockRequest)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "Could not determine if block root is canonical: %v", err)
 	}
+	isOptimistic, err := bs.HeadFetcher.IsOptimistic(ctx)


This is wrong.

The spec says:

- description: "True if the response references an unverified execution payload. Optimistic information may be invalidated at a later time. If the field is not present, assume the False value."

The isOptimistic should reference to the responded block header not to the current head. Same for other places

You're right. I changed the code to use IsOptimisticForRoot everywhere. The only two places where we look at the head are head and chain_reorg events.

terencechain · 2022-03-18T16:00:47Z

beacon-chain/blockchain/process_block.go

@@ -229,7 +229,7 @@ func (s *Service) onBlock(ctx context.Context, signed block.SignedBeaconBlock, b
 		if err := s.cfg.ForkChoiceStore.Prune(ctx, fRoot); err != nil {
 			return errors.Wrap(err, "could not prune proto array fork choice nodes")
 		}
-		isOptimistic, err := s.cfg.ForkChoiceStore.IsOptimistic(ctx, s.headRoot())
+		isOptimistic, err := s.cfg.ForkChoiceStore.IsOptimistic(ctx, fRoot)


I wonder if this is better inside the goroutine, I don't think it makes a big difference but something worth considering

The call above locks on notifyFockchoiceUpdate so this one is guaranteed to return immediately.

terencechain · 2022-03-18T16:18:22Z

beacon-chain/rpc/eth/beacon/state.go

+	_, blocks, err := bs.BeaconDB.BlocksBySlot(ctx, st.Slot())
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "Could not get blocks for state slot: %v", err)
+	}
+	isOptimistic, err := bs.blocksAreOptimistic(ctx, blocks)
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "Could not check if blocks are optimistic: %v", err)
+	}


Here's a better and more accurate algorithm:

header := st.LatestBlockHeader() header.StateRoot = stateRoot[:] headRoot, err := header.HashTreeRoot() if err != nil { return nil, errors.Wrap(err, "error while computing block root using state data") } isOptimistic, err = bs.HeadFetcher.IsOptimisticForRoot(ctx, headRoot) if err != nil { return false, errors.Wrap(err, "could not check if block is optimistic") }

terencechain · 2022-03-22T02:51:29Z

beacon-chain/rpc/eth/beacon/blocks.go

+				"Block proposal received via RPC")
+			bs.BlockNotifier.BlockFeed().Send(&feed.Event{
+				Type: blockfeed.ReceivedBlock,
+				Data: &blockfeed.ReceivedBlockData{SignedBlock: wrappedPhase0Blk, IsOptimistic: isOptimistic},


phase0 and altair blocks will always be non-optimistic. Optimistic is a concept introduced for Bellatrix

Suggested change

Data: &blockfeed.ReceivedBlockData{SignedBlock: wrappedPhase0Blk, IsOptimistic: isOptimistic},

Data: &blockfeed.ReceivedBlockData{SignedBlock: wrappedPhase0Blk, IsOptimistic: false},

+1, in addition the check for isOptimistic should not be there.

terencechain · 2022-03-22T02:51:43Z

beacon-chain/rpc/eth/beacon/blocks.go

+				"Block proposal received via RPC")
+			bs.BlockNotifier.BlockFeed().Send(&feed.Event{
+				Type: blockfeed.ReceivedBlock,
+				Data: &blockfeed.ReceivedBlockData{SignedBlock: wrappedAltairBlk, IsOptimistic: isOptimistic},


Suggested change

Data: &blockfeed.ReceivedBlockData{SignedBlock: wrappedAltairBlk, IsOptimistic: isOptimistic},

Data: &blockfeed.ReceivedBlockData{SignedBlock: wrappedAltairBlk, IsOptimistic: false},

terencechain · 2022-03-22T02:53:10Z

beacon-chain/rpc/eth/beacon/blocks.go

@@ -327,6 +366,7 @@ func (bs *Server) GetBlockV2(ctx context.Context, req *ethpbv2.BlockRequestV2) (
 				Message:   &ethpbv2.SignedBeaconBlockContainerV2_Phase0Block{Phase0Block: v1Blk.Block},
 				Signature: v1Blk.Signature,
 			},
+			ExecutionOptimistic: isOptimistic,


Suggested change

ExecutionOptimistic: isOptimistic,

ExecutionOptimistic: false,

terencechain · 2022-03-22T02:53:18Z

beacon-chain/rpc/eth/beacon/blocks.go

@@ -349,6 +389,7 @@ func (bs *Server) GetBlockV2(ctx context.Context, req *ethpbv2.BlockRequestV2) (
 				Message:   &ethpbv2.SignedBeaconBlockContainerV2_AltairBlock{AltairBlock: v2Blk},
 				Signature: blk.Signature(),
 			},
+			ExecutionOptimistic: isOptimistic,


Suggested change

ExecutionOptimistic: isOptimistic,

ExecutionOptimistic: false,

terencechain · 2022-03-22T02:54:09Z

beacon-chain/rpc/eth/beacon/blocks.go

+	root, err := blk.Block().HashTreeRoot()
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "Could not get block root: %v", err)
+	}
+	isOptimistic, err := bs.HeadFetcher.IsOptimisticForRoot(ctx, root)
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "Could not check if block is optimistic: %v", err)
+	}


Move these before bellatrixBlk, err := blk.PbBellatrixBlock()

You can save HTR if it's phase 0 or Altair

terencechain · 2022-03-22T02:54:59Z

beacon-chain/rpc/eth/beacon/blocks.go

@@ -577,7 +635,8 @@ func (bs *Server) ListBlockAttestations(ctx context.Context, req *ethpbv1.BlockR
 			return nil, status.Errorf(codes.Internal, "Could not get signed beacon block: %v", err)
 		}
 		return &ethpbv1.BlockAttestationsResponse{
-			Data: v2Blk.Body.Attestations,
+			Data:                v2Blk.Body.Attestations,
+			ExecutionOptimistic: isOptimistic,


Suggested change

ExecutionOptimistic: isOptimistic,

ExecutionOptimistic: false,

terencechain · 2022-03-22T02:58:23Z

beacon-chain/rpc/eth/helpers/sync.go

@@ -35,3 +37,21 @@ func ValidateSync(ctx context.Context, syncChecker sync.Checker, headFetcher blo
 	}
 	return status.Error(codes.Unavailable, "Syncing to latest head, not ready to respond")
 }
+
+func IsOptimistic(ctx context.Context, st state.BeaconState, headFetcher blockchain.HeadFetcher) (bool, error) {


terencechain · 2022-03-22T03:01:48Z

beacon-chain/rpc/eth/validator/validator.go

@@ -109,9 +109,15 @@ func (vs *Server) GetAttesterDuties(ctx context.Context, req *ethpbv1.AttesterDu
 		return nil, status.Errorf(codes.Internal, "Could not get dependent root: %v", err)
 	}

+	isOptimistic, err := rpchelpers.IsOptimistic(ctx, s, vs.HeadFetcher)


You want to move this before s, err = advanceState(ctx, s, req.Epoch, currentEpoch) or else state s gets mutated

terencechain · 2022-03-22T03:02:27Z

beacon-chain/rpc/eth/validator/validator.go

@@ -171,9 +177,15 @@ func (vs *Server) GetProposerDuties(ctx context.Context, req *ethpbv1.ProposerDu
 		return nil, status.Errorf(codes.Internal, "Could not get dependent root: %v", err)
 	}

+	isOptimistic, err := rpchelpers.IsOptimistic(ctx, s, vs.HeadFetcher)


Same. Move this before advanceState(ctx, s, req.Epoch, currentEpoch)

terencechain · 2022-03-22T03:03:22Z

beacon-chain/rpc/eth/validator/validator.go

@@ -245,8 +257,15 @@ func (vs *Server) GetSyncCommitteeDuties(ctx context.Context, req *ethpbv2.SyncC
 	} else if err != nil {
 		return nil, status.Errorf(codes.Internal, "Could not get duties: %v", err)
 	}
+
+	isOptimistic, err := rpchelpers.IsOptimistic(ctx, st, vs.HeadFetcher)


This one is good

potuz · 2022-03-22T09:29:11Z

beacon-chain/blockchain/head.go

+		isOptimistic, err := s.IsOptimistic(ctx)
+		if err != nil {
+			return errors.Wrap(err, "could not check if node is optimistically synced")
+		}
 		s.cfg.StateNotifier.StateFeed().Send(&feed.Event{


There's a short time lapse between we set the head and the optimistic status of the node is settled. How much of a race issue can there be here if we return optimistic when soon after the head will become VALID?

I can't say how event subscribers will use this info. Is there an easy way to sync things? If not, I would just not worry about it at this point.

potuz · 2022-03-22T09:30:56Z

beacon-chain/blockchain/head.go

+	isOptimistic, err := s.IsOptimistic(ctx)
+	if err != nil {
+		return errors.Wrap(err, "could not check if node is optimistically synced")
+	}
 	s.cfg.StateNotifier.StateFeed().Send(&feed.Event{


Same here, is this function being called right after setting head or right after the return from the engine?

Is this a question for me?

potuz · 2022-03-22T09:40:54Z

beacon-chain/blockchain/process_block.go

@@ -229,7 +229,7 @@ func (s *Service) onBlock(ctx context.Context, signed block.SignedBeaconBlock, b
 		if err := s.cfg.ForkChoiceStore.Prune(ctx, fRoot); err != nil {
 			return errors.Wrap(err, "could not prune proto array fork choice nodes")
 		}
-		isOptimistic, err := s.cfg.ForkChoiceStore.IsOptimistic(ctx, s.headRoot())
+		isOptimistic, err := s.cfg.ForkChoiceStore.IsOptimistic(ctx, fRoot)


The call above locks on notifyFockchoiceUpdate so this one is guaranteed to return immediately.

potuz · 2022-03-22T09:45:03Z

beacon-chain/rpc/eth/beacon/blocks.go

@@ -126,6 +132,12 @@ func (bs *Server) ListBlockHeaders(ctx context.Context, req *ethpbv1.BlockHeader
 		if err != nil {
 			return nil, status.Errorf(codes.Internal, "Could not determine if block root is canonical: %v", err)
 		}
+		if !isOptimistic {


Here there's an assumption that all blocks in blks have the same optimistic status. Is this guaranteed to hold?

If at least one block is optimistic, then isOptimistic will become true and stay this way.

potuz · 2022-03-22T09:47:21Z

beacon-chain/rpc/eth/beacon/blocks.go

+				"Block proposal received via RPC")
+			bs.BlockNotifier.BlockFeed().Send(&feed.Event{
+				Type: blockfeed.ReceivedBlock,
+				Data: &blockfeed.ReceivedBlockData{SignedBlock: wrappedPhase0Blk, IsOptimistic: isOptimistic},


+1, in addition the check for isOptimistic should not be there.

beacon-chain/rpc/eth/beacon/state.go

beacon-chain/rpc/eth/helpers/sync.go

terencechain · 2022-03-22T17:57:12Z

beacon-chain/rpc/eth/beacon/blocks.go

 		// Do not block proposal critical path with debug logging or block feed updates.
 		defer func() {
 			log.WithField("blockRoot", fmt.Sprintf("%#x", bytesutil.Trunc(root[:]))).Debugf(
 				"Block proposal received via RPC")
 			bs.BlockNotifier.BlockFeed().Send(&feed.Event{
 				Type: blockfeed.ReceivedBlock,
-				Data: &blockfeed.ReceivedBlockData{SignedBlock: wrappedBellatrixBlk},
+				Data: &blockfeed.ReceivedBlockData{SignedBlock: wrappedBellatrixBlk, IsOptimistic: false},


This one is bellatrix block, have we decided to just return false anyway?
Because we don't care about this endpoint?

@potuz mentioned that a proposed block is always optimistic. My thinking is that in this case we should always return true here. I must have copy/pasted false by mistake.

Is it? Why would a validator propose an optimistic block? That's a violation of the spec

cc @potuz

# Conflicts: # beacon-chain/rpc/eth/beacon/blocks.go # proto/eth/v1/beacon_chain.pb.go # proto/eth/v2/beacon_state.pb.go

rkapka added 6 commits March 16, 2022 17:45

protos

3a7943a

grpc

b63b1e8

middleware

28d8270

event protos

93ce171

event backend

45c97b2

event middleware

7164fe3

rkapka added API Api related tasks Merge PRs related to the great milestone the merge labels Mar 17, 2022

rkapka requested a review from a team as a code owner March 17, 2022 18:01

rkapka requested review from prestonvanloon, terencechain and nisdas March 17, 2022 18:01

rkapka added 3 commits March 17, 2022 19:02

Merge branch 'develop' into api-execution-optimistic

ed0e66b

fix tests

67be852

Merge branch '__develop' into api-execution-optimistic

ce50ba4

# Conflicts: # beacon-chain/rpc/eth/debug/debug_test.go

rauljordan previously approved these changes Mar 17, 2022

View reviewed changes

terencechain requested changes Mar 17, 2022

View reviewed changes

use IsOptimisticForRoot with proper block instead of header

25722c6

rkapka dismissed rauljordan’s stale review via 25722c6 March 18, 2022 13:25

terencechain reviewed Mar 18, 2022

View reviewed changes

rkapka added 4 commits March 21, 2022 18:16

better algorithm

a663286

Merge branch '__develop' into api-execution-optimistic

a49a623

return proper error

1cbcd47

Merge branch 'develop' into api-execution-optimistic

d842f39

terencechain reviewed Mar 22, 2022

View reviewed changes

potuz reviewed Mar 22, 2022

View reviewed changes

rkapka added 2 commits March 22, 2022 15:44

review

2bfafc7

Merge branch '__develop' into api-execution-optimistic

c3541fa

terencechain reviewed Mar 22, 2022

View reviewed changes

optimistic return on bellatrix proposal

a77362b

potuz previously approved these changes Mar 25, 2022

View reviewed changes

Merge branch '__develop' into api-execution-optimistic

a181dac

# Conflicts: # beacon-chain/rpc/eth/beacon/blocks.go # proto/eth/v1/beacon_chain.pb.go # proto/eth/v2/beacon_state.pb.go

rkapka dismissed potuz’s stale review via a181dac March 25, 2022 09:23

potuz approved these changes Mar 25, 2022

View reviewed changes

rkapka added the OK to merge label Mar 25, 2022

terencechain approved these changes Mar 25, 2022

View reviewed changes

prylabs-bulldozer bot added 3 commits March 25, 2022 15:08

Merge refs/heads/develop into api-execution-optimistic

5ace770

Merge refs/heads/develop into api-execution-optimistic

6f82ab9

Merge refs/heads/develop into api-execution-optimistic

918dc99

prylabs-bulldozer bot merged commit ed07f4b into develop Mar 25, 2022

delete-merged-branch bot deleted the api-execution-optimistic branch March 25, 2022 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `execution_optimistic` field to API responses #10389

Add `execution_optimistic` field to API responses #10389

rkapka commented Mar 17, 2022

terencechain Mar 17, 2022

rkapka Mar 18, 2022

terencechain Mar 18, 2022

potuz Mar 22, 2022

terencechain Mar 18, 2022

terencechain Mar 22, 2022

potuz Mar 22, 2022

terencechain Mar 22, 2022

terencechain Mar 22, 2022

terencechain Mar 22, 2022

terencechain Mar 22, 2022

terencechain Mar 22, 2022

terencechain Mar 22, 2022

terencechain Mar 22, 2022

terencechain Mar 22, 2022

terencechain Mar 22, 2022

potuz Mar 22, 2022

rkapka Mar 22, 2022

potuz Mar 22, 2022

rkapka Mar 22, 2022

potuz Mar 22, 2022

potuz Mar 22, 2022

rkapka Mar 22, 2022

potuz Mar 22, 2022

terencechain Mar 22, 2022

rkapka Mar 23, 2022

terencechain Mar 23, 2022

	Data: &blockfeed.ReceivedBlockData{SignedBlock: wrappedPhase0Blk, IsOptimistic: isOptimistic},
	Data: &blockfeed.ReceivedBlockData{SignedBlock: wrappedPhase0Blk, IsOptimistic: false},

	ExecutionOptimistic: isOptimistic,
	ExecutionOptimistic: false,

Add execution_optimistic field to API responses #10389

Add execution_optimistic field to API responses #10389

Conversation

rkapka commented Mar 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `execution_optimistic` field to API responses #10389

Add `execution_optimistic` field to API responses #10389