Native Blocks Ep. 2 - Switch usages to new package #10885

rkapka · 2022-06-15T14:00:09Z

Previous episode: #10837

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR replace all usages of the old wrapper package with the new blocks package. It accomplishes the following tasks from notion.so/prysmaticlabs/Native-Beacon-Block-c862730f431545b6a57333eaa50025d2:

Make these new consensus types implement corresponding existing interfaces: interfaces.SignedBeaconBlock, interfaces.BeaconBlock, interfaces.BeaconBlockBody
Find all usages of the current block-related functions from /consensus-types/wrapper and replace them with calls to the new code.
Find all usages of the current functions in tests, test helpers and tools, and replace them with calls to the new code.

# Conflicts: # beacon-chain/sync/subscriber_beacon_blocks.go

rkapka · 2022-06-15T18:10:29Z

consensus-types/blocks/proto.go

@@ -188,7 +188,7 @@ func (b *BeaconBlockBody) Proto() (proto.Message, error) {

 func initSignedBlockFromProtoPhase0(pb *eth.SignedBeaconBlock) (*SignedBeaconBlock, error) {
 	if pb == nil {
-		return nil, errNilBlock


I decided to relax this assertion because of tests, especially fuzz. Fuzz test randomizing can result in Block or Block.Body being nil, which results in errors and failed tests. I don't know if there is a way to overcome this. Additionally, returning an error means that all test setups, not only fuzz tests, need to set Block and Block.Body fields even if they don't need them. Furthermore, these functions are not exported, and so this change is not too risky.

I mean even with an error, this shouldn't cause a fuzz test to fail. The fuzz test should be able to handle nil blocks gracefully. Is this what you are seeing ?

The fuzzer generates nil values and then the fuzz test calls NewSignedBeaconBlock which returns an error. This makes the whole test fail.

The job of the fuzzer is to find crashes though, having a nil block shouldnt be an issue. We should be gracefully handling it, can you link which fuzz test causes issues ?

rkapka · 2022-06-15T18:11:43Z

consensus-types/blocks/getters_test.go

@@ -312,3 +338,29 @@ func Test_BeaconBlockBody_HashTreeRoot(t *testing.T) {
 	require.NoError(t, err)
 	assert.DeepEqual(t, expectedHTR, actualHTR)
 }
+
+func hydrateSignedBeaconBlock() *eth.SignedBeaconBlock {


I can't use utils.HydrateXXX functions here because that will result in a cyclic dependency. The utils package uses functions from this package.

This reverts commit ecc84e4.

rkapka · 2022-06-16T13:29:39Z

consensus-types/blocks/proto.go

 func (b *SignedBeaconBlock) Proto() (proto.Message, error) {
 	if b == nil {
-		return nil, errNilBlock
+		return nil, nil


Why I think this change is fine (especially for BeaconBlock.Proto() and BeaconBlockBody.Proto()):

Converting a nil block to a nil proto equivalent makes sense.

SignedBeaconBlock.Copy() uses Proto() internally and will not work if the block or body are nil. This issue came up in several tests. And to me it makes sense to be able to copy a block even if some part of it is nil.

Are there any possible cases where the signed beacon block is ever nil ? Ex: its possible to initialize a block which is nil.

# Conflicts: # beacon-chain/blockchain/chain_info_test.go # beacon-chain/blockchain/execution_engine_test.go # beacon-chain/blockchain/head.go # beacon-chain/blockchain/head_test.go # beacon-chain/blockchain/init_sync_process_block_test.go # beacon-chain/blockchain/new_slot_test.go # beacon-chain/blockchain/process_attestation_test.go # beacon-chain/blockchain/process_block_test.go # beacon-chain/blockchain/receive_attestation_test.go # beacon-chain/blockchain/receive_block_test.go # beacon-chain/blockchain/service_test.go # beacon-chain/blockchain/weak_subjectivity_checks_test.go # beacon-chain/rpc/eth/beacon/blocks_test.go # beacon-chain/rpc/eth/beacon/state_test.go # beacon-chain/rpc/eth/beacon/sync_committee_test.go # beacon-chain/rpc/eth/beacon/validator_test.go # beacon-chain/rpc/eth/debug/debug_test.go # beacon-chain/rpc/eth/validator/validator_test.go # beacon-chain/rpc/prysm/v1alpha1/beacon/assignments_test.go # beacon-chain/rpc/prysm/v1alpha1/beacon/attestations_test.go # beacon-chain/rpc/prysm/v1alpha1/beacon/blocks_test.go # beacon-chain/rpc/prysm/v1alpha1/beacon/committees_test.go # beacon-chain/rpc/prysm/v1alpha1/beacon/validators_test.go # beacon-chain/rpc/prysm/v1alpha1/debug/block_test.go # beacon-chain/rpc/prysm/v1alpha1/debug/state_test.go # beacon-chain/rpc/prysm/v1alpha1/validator/attester_test.go # beacon-chain/rpc/prysm/v1alpha1/validator/blocks_test.go # beacon-chain/rpc/prysm/v1alpha1/validator/proposer_execution_payload_test.go # beacon-chain/rpc/prysm/v1alpha1/validator/proposer_test.go # beacon-chain/rpc/statefetcher/fetcher_test.go # beacon-chain/state/stategen/getter_test.go # beacon-chain/state/stategen/migrate_test.go # beacon-chain/state/stategen/replay_test.go # beacon-chain/state/stategen/service_test.go # beacon-chain/state/stategen/setter_test.go # beacon-chain/sync/initial-sync/blocks_fetcher_test.go # beacon-chain/sync/initial-sync/blocks_fetcher_utils_test.go # beacon-chain/sync/initial-sync/blocks_queue_test.go # beacon-chain/sync/initial-sync/initial_sync_test.go # beacon-chain/sync/initial-sync/round_robin_test.go # beacon-chain/sync/initial-sync/service_test.go # beacon-chain/sync/pending_attestations_queue_test.go # beacon-chain/sync/pending_blocks_queue_test.go # beacon-chain/sync/rpc_beacon_blocks_by_range_test.go # beacon-chain/sync/rpc_beacon_blocks_by_root_test.go # beacon-chain/sync/rpc_status_test.go # beacon-chain/sync/service_test.go # beacon-chain/sync/sync_fuzz_test.go # beacon-chain/sync/validate_aggregate_proof_test.go # beacon-chain/sync/validate_beacon_attestation_test.go # beacon-chain/sync/validate_beacon_blocks_test.go

# Conflicts: # beacon-chain/blockchain/service_test.go # consensus-types/blocks/BUILD.bazel # consensus-types/blocks/getters_test.go # validator/client/validator.go

rkapka · 2022-06-28T17:23:50Z

This PR is currently marked as WIP because the code changes undergo testing in a dev cluster.

prestonvanloon · 2022-07-25T18:11:00Z

consensus-types/blocks/proto.go

 func (b *BeaconBlock) Proto() (proto.Message, error) {
 	if b == nil {
-		return nil, errNilBlock
+		return nil, nil
 	}


Any reason for this change? I think i saw a comment about it relating to fuzz testing, but I see this change only some structs and not others.

See #10885 (comment)

# Conflicts: # beacon-chain/blockchain/BUILD.bazel # beacon-chain/blockchain/pow_block_test.go # beacon-chain/blockchain/process_block.go # beacon-chain/core/blocks/BUILD.bazel # beacon-chain/core/blocks/payload.go # beacon-chain/core/blocks/payload_test.go # beacon-chain/db/kv/blocks_test.go # beacon-chain/rpc/eth/beacon/blocks.go # consensus-types/interfaces/beacon_block.go

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

blah

# Conflicts: # testing/util/bellatrix.go # testing/util/helpers.go

rkapka added 15 commits June 13, 2022 13:02

panic in SizeSSZ

ffc7020

moving slowly

3d8ece1

adapt old code to new interfaces

4fcff54

return interfaces from factory functions

f865365

replace the rest of WrappedSignedBeaconBlock

d9be7bc

WrappedBeaconBlock

59cb845

WrappedBeaconBlockBody

b99135f

miscellaneous

0e489a8

Test_BeaconBlockIsNil

77e9d55

replace usages of BeaconBlockIsNil

940fa81

replace usages of mutator

ca8d2ba

fix all build errors

08c2ef3

fix some more issues

58b587f

mutator changes

f8dc4cd

Merge branch '__develop' into native-blocks-swap-usage

3085e96

# Conflicts: # beacon-chain/sync/subscriber_beacon_blocks.go

rkapka requested a review from a team as a code owner June 15, 2022 14:00

rkapka requested review from potuz, terencechain and james-prysm June 15, 2022 14:00

rkapka changed the title ~~Native Blocks Ep. 2 - Switch usages to new package~~ [WIP] Native Blocks Ep. 2 - Switch usages to new package Jun 15, 2022

relax assertions when initializing

dbe1550

rkapka commented Jun 15, 2022

View reviewed changes

rkapka added 6 commits June 15, 2022 20:32

revert changes in object_mapping.go

267f728

allow calling Proto on nil

ecc84e4

Revert "allow calling Proto on nil"

a554535

This reverts commit ecc84e4.

modify Copy and Proto methods

cede04b

remove unused var

da3c5df

fix block batch tests

31effdb

rkapka commented Jun 16, 2022

View reviewed changes

rkapka force-pushed the native-blocks-swap-usage branch from 7194864 to 42368bc Compare June 24, 2022 09:26

rkapka added 2 commits June 24, 2022 13:19

fix last test

5f2685b

rkapka force-pushed the native-blocks-swap-usage branch from b7b42c1 to 9064b3d Compare June 27, 2022 13:22

rkapka added 2 commits June 28, 2022 17:06

Merge branch '__develop' into native-blocks-swap-usage

e220583

# Conflicts: # beacon-chain/blockchain/service_test.go # consensus-types/blocks/BUILD.bazel # consensus-types/blocks/getters_test.go # validator/client/validator.go

import fix

931b2a6

prestonvanloon reviewed Jul 25, 2022

View reviewed changes

rkapka added 3 commits July 28, 2022 18:11

broken

610937b

working

88116c4

rkapka dismissed a stale review via 88116c4 July 28, 2022 20:09

rkapka requested a review from rauljordan as a code owner July 28, 2022 20:09

rkapka added 2 commits July 28, 2022 22:24

test fixes

1356fe0

Merge branch '__develop' into native-blocks-swap-usage

903be1b

rkapka changed the title ~~[WIP] Native Blocks Ep. 2 - Switch usages to new package~~ Native Blocks Ep. 2 - Switch usages to new package Aug 1, 2022

Merge branch '__develop' into native-blocks-swap-usage

03ea561

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

rauljordan previously approved these changes Aug 2, 2022

View reviewed changes

rkapka dismissed rauljordan’s stale review via 8d287c2 August 2, 2022 13:47

reduce complexity of processPendingBlocks

af5b903

rkapka force-pushed the native-blocks-swap-usage branch from 8d287c2 to af5b903 Compare August 2, 2022 13:49

rauljordan previously approved these changes Aug 2, 2022

View reviewed changes

rauljordan added the OK to merge label Aug 2, 2022

rkapka added 2 commits August 2, 2022 16:58

simplified

5def3c5

Merge branch '__develop' into native-blocks-swap-usage

74a44ce

# Conflicts: # testing/util/bellatrix.go # testing/util/helpers.go

rauljordan approved these changes Aug 2, 2022

View reviewed changes

prylabs-bulldozer bot merged commit 879e310 into develop Aug 2, 2022

delete-merged-branch bot deleted the native-blocks-swap-usage branch August 2, 2022 15:30

rkapka mentioned this pull request Aug 3, 2022

Native Blocks Ep. 3 - Remove old code #11158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native Blocks Ep. 2 - Switch usages to new package #10885

Native Blocks Ep. 2 - Switch usages to new package #10885

rkapka commented Jun 15, 2022

rkapka Jun 15, 2022

nisdas Jun 17, 2022

rkapka Jun 17, 2022

nisdas Jun 17, 2022

rkapka Jun 15, 2022

rkapka Jun 16, 2022 •

edited

Loading

nisdas Jun 23, 2022

rkapka commented Jun 28, 2022

prestonvanloon Jul 25, 2022

rkapka Jul 28, 2022

Native Blocks Ep. 2 - Switch usages to new package #10885

Native Blocks Ep. 2 - Switch usages to new package #10885

Conversation

rkapka commented Jun 15, 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

rkapka Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka Jun 16, 2022 •

edited

Loading