Merge all block unblinding code into a single `unblinder` struct by rkapka · Pull Request #12240 · OffchainLabs/prysm

rkapka · 2023-04-05T15:25:25Z

What type of PR is this?

Other

What does this PR do? Why is it needed?

This PR creates a new unblinder type responsible for (wait for it...) unblinding blocks. The main purpose of the type is to reduce code duplication and keep unblinding logic in one place. It can easily be extended for Deneb blocks.

This was already merged to 4844 in #12232

) # Conflicts: # beacon-chain/rpc/prysm/v1alpha1/validator/proposer_bellatrix.go # beacon-chain/rpc/prysm/v1alpha1/validator/proposer_bellatrix_test.go # beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella.go # beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella_test.go

terencechain · 2023-04-13T05:00:42Z

beacon-chain/rpc/prysm/v1alpha1/validator/unblinder.go

+		return nil, errors.Wrap(err, "could not set execution")
+	}
+
+	payload, err := u.builder.SubmitBlindedBlock(ctx, sb)


Please don't forget to set SetBLSToExecutionChanges before SubmitBlindedBlock
Also, please add a unit test for this field
See #12263 for detail

# Conflicts: # beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella.go # beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella_test.go

terencechain · 2023-04-13T21:01:21Z

beacon-chain/rpc/prysm/v1alpha1/validator/unblinder.go

 		return nil, errors.Wrap(err, "could not set sync aggregate")
 	}
 	sb.SetSignature(sig[:])
+	if err = sb.SetBLSToExecutionChanges(blsToExecChanges); err != nil && !errors.Is(err, consensusblocks.ErrUnsupportedGetter) {


Wouldn't the error be unsupported Setter here instead of Getter?

This requires some more refactoring, but I can add it.

kasey · 2023-04-13T21:08:39Z

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

+		StateRoot:     make([]byte, fieldparams.RootLength),
+		ReceiptsRoot:  make([]byte, fieldparams.RootLength),
+		LogsBloom:     make([]byte, fieldparams.LogsBloomLength),
+		PrevRandao:    make([]byte, fieldparams.RootLength),


presumably this omits BlockNumber, GasLimit, GasUsed, Timestamp because they are uint64s so they receive zero values, but we may want to always include all fields in functions like this as a matter of course to help verify completeness.

kasey · 2023-04-13T21:09:47Z

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

+		BlockHash:     make([]byte, fieldparams.RootLength),
+		Transactions:  make([][]byte, 0),
+		Withdrawals:   make([]*enginev1.Withdrawal, 0),
+		ExtraData:     make([]byte, 0),


could we put these fields in the same order as the consensus types? That would make this code a little easier to compare to generated protobuf code.

What do you mean exactly? In consensus types we have an ExecutionData interface and a type that wraps a proto object.

james-prysm · 2023-04-14T03:17:42Z

beacon-chain/rpc/prysm/v1alpha1/validator/unblinder.go

+		return nil, errors.Wrap(err, "could not set execution")
+	}
+
+	payload, err := u.builder.SubmitBlindedBlock(ctx, sb)


It might be a good time to add a defensive check for SubmitBlindedBlock and do a version check on the response for the payload. right now the version isn't getting checked on the response and is not part of the executiondata interface which could result in a bad payload.

payload is of type ExecutionData, which doesn't have a version. If we want to change this, it should be in a new PR.

yeah we need a new PR for checking inside I'm not sure if ExecutionData needs it to get the version or not, but at least it should be checked inside

my PR merged in with the check here

james-prysm · 2023-04-14T03:18:51Z

beacon-chain/rpc/prysm/v1alpha1/validator/unblinder.go

+	if err != nil {
+		return nil, errors.Wrap(err, "could not create signed block")
+	}
+	wb.SetSlot(sb.Block().Slot())


is there any pattern we can follow so we don't miss/forget doing these sets? also might be worth breaking into its own function to make this more readable.

I guess the same comment applies to line 66and onwards.
do the settings in sb and wb need to be the same? maybe there can be a check on that too somehow?

or better yet a utility function where wb/sb are passed so it's at least 1 spot ( not sure if it's completed replicated logic however.

I extracted common code

james-prysm · 2023-04-14T03:21:48Z

beacon-chain/rpc/prysm/v1alpha1/validator/unblinder.go

+		return nil, errors.New("builder not configured")
+	}
+
+	agg, err := u.b.Block().Body().SyncAggregate()


might need nil checks in case of panics

this is already checked at the beginning of newUnblinder

rkapka · 2023-05-05T22:43:46Z

Marking as blocked until we have the test builder ready.

# Conflicts: # beacon-chain/blockchain/log.go # beacon-chain/core/blocks/payload.go # beacon-chain/rpc/eth/beacon/BUILD.bazel # beacon-chain/rpc/eth/beacon/blinded_blocks.go # beacon-chain/rpc/eth/beacon/blinded_blocks_test.go # beacon-chain/rpc/eth/beacon/blocks.go # beacon-chain/rpc/prysm/v1alpha1/validator/proposer_bellatrix.go # cmd/prysmctl/p2p/request_blocks.go # consensus-types/blocks/execution.go # consensus-types/blocks/execution_test.go # consensus-types/blocks/types.go

james-prysm · 2023-05-18T17:51:46Z

beacon-chain/rpc/prysm/v1alpha1/validator/unblinder.go

+		return nil, err
+	}
+	return &unblinder{
+		b:       b,


should there be a nil check on builder here?

james-prysm · 2023-05-18T17:53:39Z

beacon-chain/rpc/prysm/v1alpha1/validator/unblinder.go

+	if !u.b.IsBlinded() || u.b.Version() < version.Bellatrix {
+		return u.b, nil
+	}
+	if u.b.IsBlinded() && !u.builder.Configured() {


builder nil check could also be here I guess.

Dismissing my review. I haven't looked at it closely for approve but feel free to merge if others have

james-prysm · 2023-05-22T18:07:53Z

beacon-chain/rpc/prysm/v1alpha1/validator/unblinder.go

+	return wb, nil
+}
+
+func copyBlockData(src interfaces.SignedBeaconBlock, dst interfaces.SignedBeaconBlock) error {


This might be the scariest function in all of this if we forget to add something/populate something.

james-prysm · 2023-05-22T18:08:09Z

beacon-chain/rpc/prysm/v1alpha1/validator/unblinder.go

+		return errors.Wrap(err, "could not set sync aggregate")
+	}
+	dst.SetSignature(sig[:])
+	if err = dst.SetBLSToExecutionChanges(blsToExecChanges); err != nil && !errors.Is(err, consensus_types.ErrUnsupportedField) {


worth adding capella fork marking here in comments?

james-prysm · 2023-05-22T18:11:07Z

beacon-chain/rpc/prysm/v1alpha1/validator/unblinder.go

+	graffiti := src.Block().Body().Graffiti()
+	sig := src.Signature()
+	blsToExecChanges, err := src.Block().Body().BLSToExecutionChanges()
+	if err != nil && !errors.Is(err, consensus_types.ErrUnsupportedField) {


so copy Block will throw an error here if it's an old block that doesn't have this support?

totally missed the !

) Co-authored-by: james-prysm <90280386+james-prysm@users.noreply.github.com>

rkapka added the Ready For Review label Apr 5, 2023

rkapka requested a review from a team as a code owner April 5, 2023 15:25

rkapka requested review from potuz, saolyn and terencechain April 5, 2023 15:25

rkapka added 4 commits April 5, 2023 17:27

build fix

3d50a53

fix tests

a8e5f83

bzl

ce43b80

log gasUsed correctly

1dc7d34

terencechain previously requested changes Apr 13, 2023

View reviewed changes

rkapka added 3 commits April 13, 2023 13:03

add bls changes

4f807f8

test for bls changes

567dcd3

Merge branch '__develop' into unblinder

1be2490

# Conflicts: # beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella.go # beacon-chain/rpc/prysm/v1alpha1/validator/proposer_capella_test.go

terencechain reviewed Apr 13, 2023

View reviewed changes

kasey reviewed Apr 13, 2023

View reviewed changes

rauljordan mentioned this pull request Apr 13, 2023

How do we ensure new consensus field are well tested #12266

Open

james-prysm reviewed Apr 14, 2023

View reviewed changes

rkapka added 2 commits April 14, 2023 16:58

review

5932e6b

Merge branch 'develop' into unblinder

693b686

rkapka added the blocked label May 5, 2023

rkapka added 2 commits May 17, 2023 19:16

fix diff

f6f3ba6

rkapka force-pushed the unblinder branch from 88bffec to 72729ae Compare May 18, 2023 10:53

Merge branch '__develop' into unblinder

8c921f8

rkapka force-pushed the unblinder branch from 72729ae to 8c921f8 Compare May 18, 2023 10:53

fix diff

c47e2b4

rkapka removed the blocked label May 18, 2023

build fixes

6b5e6aa

rkapka force-pushed the unblinder branch from 7b5ff4b to 6b5e6aa Compare May 18, 2023 11:36

rkapka added 4 commits May 18, 2023 13:47

remove unused receiver

27a1890

unify ErrUnsupportedField

56ebe39

update dependencies

477e1f4

Merge branch 'develop' into unblinder

f53ef0b

james-prysm reviewed May 18, 2023

View reviewed changes

rkapka and others added 6 commits May 22, 2023 19:11

Merge branch '__develop' into unblinder

7bee6af

James' review

d8b8239

Merge branch 'develop' into unblinder

da6d0bc

Merge branch 'develop' into unblinder

305d816

Merge branch 'develop' into unblinder

3e8f976

Merge branch 'develop' into unblinder

5805ef4

james-prysm reviewed May 22, 2023

View reviewed changes

james-prysm approved these changes May 22, 2023

View reviewed changes

rkapka merged commit abc81e6 into develop May 23, 2023

rkapka deleted the unblinder branch May 23, 2023 09:38

james-prysm added a commit that referenced this pull request May 23, 2023

Merge all block unblinding code into a single unblinder struct (#12240

95d399a

) Co-authored-by: james-prysm <90280386+james-prysm@users.noreply.github.com>

Conversation

rkapka commented Apr 5, 2023 • 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!

kasey Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rkapka commented May 5, 2023

Uh oh!

james-prysm May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rkapka commented Apr 5, 2023 •

edited

Loading

kasey Apr 13, 2023 •

edited

Loading

james-prysm May 18, 2023 •

edited

Loading