Capella beacon block #11566

rkapka · 2022-10-21T16:49:40Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

adds all the necessary Capella block consensus types
handles version.Capella for block processing
new tests for Capella

This reverts commit 081ab74e88fb7d0e3f6a81e00fe5e89483b41f90.

potuz · 2022-10-24T11:52:27Z

beacon-chain/sync/rpc_chunked_response.go

+		if err != nil {
+			return err
+		}
+		obtainedCtx = digest[:]


Why is obtainedCtx assigned in each case instead of after the switch statement?

potuz · 2022-10-24T12:04:09Z

consensus-types/blocks/types.go

+	deposits                      []*eth.Deposit
+	voluntaryExits                []*eth.SignedVoluntaryExit
+	syncAggregate                 *eth.SyncAggregate
+	executionPayload              *engine.ExecutionPayload


Can't these be in a single interface executionPayload that has different implementations? Otherwise each fork we'll have to add a possibly nil pointer to the BeaconBlockBody

terencechain

I quickly glanced through this. It looks fine
Is there any reason to use a mega PR (2000+) lines diff versus splitting this into more management PRs (e.g. 500 lines) by various components. You'll make @prestonvanloon's day :D

It also might be easier to root cause the e2e failure that way

rkapka · 2022-10-26T10:40:13Z

I quickly glanced through this. It looks fine Is there any reason to use a mega PR (2000+) lines diff versus splitting this into more management PRs (e.g. 500 lines) by various components. You'll make @prestonvanloon's day :D

I think we agreed that splitting PRs does more harm than good.

It also might be easier to root cause the e2e failure that way

Current e2e failures cannot be attributed to this PR because it's not merged yet. Or do you mean potential failures that this PR might cause? 😅

terencechain · 2022-10-26T13:25:44Z

I think we agreed that splitting PRs does more harm than good.

Why is this?

potuz · 2022-10-27T18:49:50Z

validator/client/propose.go

@@ -145,7 +145,7 @@ func (v *validator) ProposeBlock(ctx context.Context, slot types.Slot, pubKey [f
 		trace.Int64Attribute("numAttestations", int64(len(blk.Block().Body().Attestations()))),
 	)

-	if blk.Version() == version.Bellatrix {
+	if blk.Version() > version.Altair {


Do we want to log here also the number of withdrawals included?

potuz · 2022-10-27T18:53:08Z

testing/util/block.go

+			BaseFeePerGas: make([]byte, fieldparams.RootLength),
+			BlockHash:     make([]byte, fieldparams.RootLength),
+			Transactions:  make([][]byte, 0),
+			ExtraData:     make([]byte, 0),


Withdrawals?

potuz · 2022-10-27T18:55:39Z

proto/prysm/v1alpha1/cloners_test.go

+				WithdrawalIndex:  456,
+				ValidatorIndex:   456,


Suggested change

WithdrawalIndex: 456,

ValidatorIndex: 456,

WithdrawalIndex: 123,

ValidatorIndex: 456,

Because it's closer to reality: withdrawals are sequential in a block or the block is invalid, it should be 124 though, not 123 :)

potuz · 2022-10-27T18:58:43Z

proto/prysm/v1alpha1/cloners_test.go

 func genAttestation() *v1alpha1.Attestation {
 	return &v1alpha1.Attestation{
 		AggregationBits: bytes(32),
 		Data:            genAttData(),
-		Signature:       bytes(32),
+		Signature:       bytes(96),


What is this thing?

potuz · 2022-10-27T18:59:35Z

proto/prysm/v1alpha1/cloners_test.go

@@ -360,35 +360,95 @@ func TestCopyBeaconBlockBodyBellatrix(t *testing.T) {
 }

 func TestCopySignedBlindedBeaconBlockBellatrix(t *testing.T) {
-	sbb := genSignedBeaconBlockBellatrix()
+	sbb := genSignedBlindedBeaconBlockBellatrix()


What are these unrelated fixes?

potuz · 2022-10-27T19:02:33Z

beacon-chain/core/transition/transition_no_verify_sig.go

@@ -243,7 +243,7 @@ func ProcessOperationsNoVerifyAttsSigs(
 		if err != nil {
 			return nil, err
 		}
-	case version.Altair, version.Bellatrix:
+	case version.Altair, version.Bellatrix, version.Capella:


Suggested change

case version.Altair, version.Bellatrix, version.Capella:

case version.Altair, version.Bellatrix

Better to leave transition changes out of this PR.

This reverts commit 1ff3a4c864923f5c180aa015aa087a2814498b42.

terencechain · 2022-10-28T13:40:15Z

consensus-types/blocks/execution_test.go

@@ -81,6 +81,76 @@ func TestWrapExecutionPayloadHeader_SSZ(t *testing.T) {
 	assert.NoError(t, wsb.UnmarshalSSZ(encoded))
 }

+func TestWrapExecutionPayloadCapella(t *testing.T) {
+	data := &enginev1.ExecutionPayloadCapella{GasUsed: 54}


Personally, I think there's great value to fill in all the fields and test they are not nil

terencechain · 2022-10-28T13:40:39Z

consensus-types/blocks/execution_test.go

@@ -81,6 +81,76 @@ func TestWrapExecutionPayloadHeader_SSZ(t *testing.T) {
 	assert.NoError(t, wsb.UnmarshalSSZ(encoded))
 }

+func TestWrapExecutionPayloadCapella(t *testing.T) {
+	data := &enginev1.ExecutionPayloadCapella{GasUsed: 54}
+	wsb, err := blocks.WrappedExecutionPayloadCapella(data)


wsb wrapped signed block? this is not a block

potuz · 2022-10-31T17:35:04Z

consensus-types/blocks/proto.go

+			if b.executionPayloadHeader != nil {
+				ph, ok = b.executionPayloadHeader.Proto().(*enginev1.ExecutionPayloadHeader)
+				if !ok {
+					return nil, errPayloadHeaderWrongType


Can we test for this? there are four instances that we may hit these errors.

potuz · 2022-10-31T17:37:23Z

proto/prysm/v1alpha1/cloners_test.go

+				WithdrawalIndex:  456,
+				ValidatorIndex:   456,


Because it's closer to reality: withdrawals are sequential in a block or the block is invalid, it should be 124 though, not 123 :)

rkapka added 6 commits October 20, 2022 23:14

in progress

2e356b6

done, no tests yet

6511fbe

fix ToBlinded()

cf7cd2a

Revert "Auxiliary commit to revert individual files from 2e356b6"

49b0655

This reverts commit 081ab74e88fb7d0e3f6a81e00fe5e89483b41f90.

tests

dda154a

Merge branch '__develop' into capella-block

19b61dc

rkapka marked this pull request as ready for review October 21, 2022 18:34

rkapka requested a review from a team as a code owner October 21, 2022 18:34

rkapka requested review from rauljordan, terencechain and nisdas October 21, 2022 18:34

rkapka added 4 commits October 24, 2022 10:55

fix tests

241c081

Merge branch '__develop' into capella-block

32dd4f8

one more fix

b357390

and one more

fa99628

rkapka added Ready For Review labels Oct 24, 2022

potuz reviewed Oct 24, 2022

View reviewed changes

rkapka added 9 commits October 24, 2022 15:05

review

e8441f4

fix proto_test

5c0ed0e

another fix

aeea110

do not return error when nil object is wrapped

555d55d

allow nil payload in body.Proto()

92e6668

Merge branch '__develop' into capella-block

0e6abcf

correctly assert error

dc8d0f4

nil checks in body.Execution()

8b03145

Merge branch 'develop' into capella-block

c562467

terencechain reviewed Oct 25, 2022

View reviewed changes

terencechain and others added 2 commits October 27, 2022 08:36

Merge branch 'develop' into capella-block

84cf3d0

Merge branch 'develop' into capella-block

141e20c

potuz reviewed Oct 27, 2022

View reviewed changes

simplify PR

5736c1f

rkapka force-pushed the capella-block branch from 03992ad to 5736c1f Compare October 28, 2022 09:20

rkapka added 2 commits October 28, 2022 11:21

Revert "Auxiliary commit to revert individual files from 5736c1f"

75f535a

This reverts commit 1ff3a4c864923f5c180aa015aa087a2814498b42.

fix slice sizes in cloner tests

541e010

terencechain reviewed Oct 28, 2022

View reviewed changes

better payload tests

a6a599d

terencechain previously approved these changes Oct 31, 2022

View reviewed changes

Merge branch 'develop' into capella-block

393e12f

potuz reviewed Oct 31, 2022

View reviewed changes

review

979f443

rkapka dismissed terencechain’s stale review via 979f443 October 31, 2022 17:51

potuz approved these changes Oct 31, 2022

View reviewed changes

rkapka added the OK to merge label Oct 31, 2022

prylabs-bulldozer bot merged commit ffac232 into develop Oct 31, 2022

delete-merged-branch bot deleted the capella-block branch October 31, 2022 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capella beacon block #11566

Capella beacon block #11566

rkapka commented Oct 21, 2022 •

edited

Loading

potuz Oct 24, 2022

potuz Oct 24, 2022

terencechain left a comment

rkapka commented Oct 26, 2022 •

edited

Loading

terencechain commented Oct 26, 2022

potuz Oct 27, 2022

potuz Oct 27, 2022

potuz Oct 27, 2022

rkapka Oct 28, 2022

potuz Oct 31, 2022

potuz Oct 27, 2022

potuz Oct 27, 2022

potuz Oct 27, 2022

terencechain Oct 28, 2022

terencechain Oct 28, 2022

potuz Oct 31, 2022

potuz Oct 31, 2022

	case version.Altair, version.Bellatrix, version.Capella:
	case version.Altair, version.Bellatrix

Capella beacon block #11566

Capella beacon block #11566

Conversation

rkapka commented Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain left a comment

Choose a reason for hiding this comment

rkapka commented Oct 26, 2022 • edited Loading

terencechain commented Oct 26, 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

rkapka commented Oct 21, 2022 •

edited

Loading

rkapka commented Oct 26, 2022 •

edited

Loading