Use Block Interface Across Prysm #8918

nisdas · 2021-05-21T08:24:49Z

What type of PR is this?

Hard Fork Cleanup

What does this PR do? Why is it needed?

Replaces our protobuf block object with a block interface. This is to allow for easier representation of blocks for future
hardforks
Add in new block interface.
Fix all tests which broke due to this change.
Use block interface as much as possible across prysm.

Which issues(s) does this PR fix?

Part of #8638

Other notes for review

into blockInterface

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

…h-sharding into blockInterface

rkapka · 2021-05-26T06:39:54Z

beacon-chain/blockchain/head.go

@@ -104,7 +103,7 @@ func (s *Service) saveHead(ctx context.Context, headRoot [32]byte) error {
 	if err != nil {
 		return err
 	}
-	if newHeadBlock == nil || newHeadBlock.Block == nil {
+	if newHeadBlock == nil || newHeadBlock.IsNil() || newHeadBlock.Block().IsNil() {


How about this change?

func (w WrappedSignedBeaconBlock) IsNil() bool { return w.b == nil || w.Block() == nil || w.Block().IsNil() }

It allows to write a single IsNil() check, and I am afraid most of the time the second check will be forgotten. You also never verify if w.Block() == nil in this PR (I don't either in my suggestions because this was an afterthought) and you already missed several w.Block().IsNil() checks. I'm sure this will happen more often. The downside is that we couple both checks together. Would we ever not care if the underlying beacon block is nil or not though?

The goal was to not add in additional nil block checks unless that particular function requires it. I have tried to strictly mimick existing behaviour and minimize any additional logic changes added into an already large PR. I just wanted to replace what were existing nil checks for a block struct with an interface. Adding in an inner block check would differ from what is the current behaviour (if signedBlock == nil) . We can add it in a follow up PR in that case, it would only be a one line change for both the SignedBeaconBlock and BeaconBlock.

rkapka · 2021-05-26T06:43:48Z

beacon-chain/blockchain/process_attestation_helpers.go

@@ -99,14 +99,14 @@ func (s *Service) verifyBeaconBlock(ctx context.Context, data *ethpb.Attestation
 	}
 	// If the block does not exist in db, check again if block exists in initial sync block cache.
 	// This could happen as the node first syncs to head.
-	if b == nil && s.hasInitSyncBlock(r) {
+	if (b == nil || b.IsNil()) && s.hasInitSyncBlock(r) {


Suggested change

if (b == nil || b.IsNil()) && s.hasInitSyncBlock(r) {

if (b == nil || b.IsNil() || b.Block().IsNil()) && s.hasInitSyncBlock(r) {

rkapka · 2021-05-26T06:46:54Z

beacon-chain/blockchain/service.go

@@ -119,8 +120,8 @@ func (s *Service) Start() {
 		if err != nil {
 			log.Fatalf("Could not fetch finalized cp: %v", err)
 		}
-		if genesisBlock != nil {
-			r, err = genesisBlock.Block.HashTreeRoot()
+		if genesisBlock != nil && !genesisBlock.IsNil() {


Suggested change

if genesisBlock != nil && !genesisBlock.IsNil() {

if genesisBlock != nil && !genesisBlock.IsNil() && !genesisBlock.Block().IsNil() {

rkapka · 2021-05-26T06:47:41Z

beacon-chain/blockchain/service.go

@@ -337,10 +338,10 @@ func (s *Service) saveGenesisData(ctx context.Context, genesisState iface.Beacon
 		return errors.Wrap(err, "could not save genesis data")
 	}
 	genesisBlk, err := s.cfg.BeaconDB.GenesisBlock(ctx)
-	if err != nil || genesisBlk == nil {
+	if err != nil || genesisBlk == nil || genesisBlk.IsNil() {


Suggested change

if err != nil || genesisBlk == nil || genesisBlk.IsNil() {

if err != nil || genesisBlk == nil || genesisBlk.IsNil() || genesisBlk.Block().IsNil() {

rkapka · 2021-05-26T06:48:04Z

beacon-chain/blockchain/service.go

@@ -380,10 +381,10 @@ func (s *Service) initializeChainInfo(ctx context.Context) error {
 	if err != nil {
 		return errors.Wrap(err, "could not get genesis block from db")
 	}
-	if genesisBlock == nil {
+	if genesisBlock == nil || genesisBlock.IsNil() {


Suggested change

if genesisBlock == nil || genesisBlock.IsNil() {

if genesisBlock == nil || genesisBlock.IsNil() || genesisBlock.Block().IsNil() {

rkapka · 2021-05-26T07:06:37Z

beacon-chain/state/stategen/getter.go

@@ -248,7 +248,7 @@ func (s *State) lastAncestorState(ctx context.Context, root [32]byte) (iface.Bea
 	if err != nil {
 		return nil, err
 	}
-	if b == nil {
+	if b == nil || b.IsNil() {


Suggested change

if b == nil || b.IsNil() {

if b == nil || b.IsNil() || b.Block().IsNil() {

rkapka · 2021-05-26T07:06:52Z

beacon-chain/state/stategen/getter.go

@@ -289,7 +289,7 @@ func (s *State) lastAncestorState(ctx context.Context, root [32]byte) (iface.Bea
 		if err != nil {
 			return nil, err
 		}
-		if b == nil {
+		if b == nil || b.IsNil() {


Suggested change

if b == nil || b.IsNil() {

if b == nil || b.IsNil() || b.Block().IsNil() {

rkapka · 2021-05-26T07:09:25Z

beacon-chain/sync/rpc_beacon_blocks_by_root.go

@@ -68,10 +69,10 @@ func (s *Service) beaconBlocksRootRPCHandler(ctx context.Context, msg interface{
 			s.writeErrorResponseToStream(responseCodeServerError, types.ErrGeneric.Error(), stream)
 			return err
 		}
-		if blk == nil {
+		if blk == nil || blk.IsNil() {


Suggested change

if blk == nil || blk.IsNil() {

if blk == nil || blk.IsNil() || blk.Block().IsNil() {

rkapka · 2021-05-26T07:10:05Z

beacon-chain/sync/rpc_status.go

@@ -315,30 +315,30 @@ func (s *Service) validateStatusMessage(ctx context.Context, msg *pb.Status) err
 	if err != nil {
 		return p2ptypes.ErrGeneric
 	}
-	if blk == nil {
+	if blk == nil || blk.IsNil() {


Suggested change

if blk == nil || blk.IsNil() {

if blk == nil || blk.IsNil() || blk.Block().IsNil() {

rkapka · 2021-05-26T07:10:37Z

beacon-chain/sync/rpc_status.go

 		childBlock, err := s.cfg.DB.FinalizedChildBlock(ctx, bytesutil.ToBytes32(msg.FinalizedRoot))
 		if err != nil {
 			return p2ptypes.ErrGeneric
 		}
 		// Is a valid finalized block if no
 		// other child blocks exist yet.
-		if childBlock == nil {
+		if childBlock == nil || childBlock.IsNil() {


Suggested change

if childBlock == nil || childBlock.IsNil() {

if childBlock == nil || childBlock.IsNil() || childBlock.Block().IsNil() {

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

…h-sharding into blockInterface

codecov · 2021-05-26T10:10:29Z

Codecov Report

Merging #8918 (ff3b8a2) into develop (7b55213) will increase coverage by 0.00%.
The diff coverage is 58.72%.

@@           Coverage Diff            @@
##           develop    #8918   +/-   ##
========================================
  Coverage    60.91%   60.92%           
========================================
  Files          529      530    +1     
  Lines        37419    37488   +69     
========================================
+ Hits         22793    22838   +45     
- Misses       11364    11376   +12     
- Partials      3262     3274   +12

nisdas added 9 commits May 21, 2021 13:23

commit initial work

b4e8c2b

checkpoint current work

ee2e4af

gaz

4d82c1a

checkpoint

6872b20

req/resp changes

2fd5e66

initial-sync

e0b54c7

finally works

1ff779a

fix error

b32d16e

fix bugs

96204c7

terencechain mentioned this pull request May 21, 2021

Altair: fork transition and spec tests #8919

Merged

nisdas added 17 commits May 22, 2021 08:15

fix issue

7b8aecc

fix issues

588774b

Merge branch 'develop' of https://github.com/prysmaticlabs/geth-sharding

46be72e

into blockInterface

Merge branch 'develop' of https://github.com/prysmaticlabs/geth-sharding

38a237d

into blockInterface

fix refs

ee33eea

tests

4e60370

more text fixes

f890977

more text fixes

0d12504

more text fixes

237b785

fix tests

db29b95

fix tests

5e234a1

tests

f058514

finally fix builds

78cd401

finally

bd6dcce

comments

cf97730

Merge branch 'develop' into blockInterface

b021a29

fix fuzz

c2919f4

nisdas marked this pull request as ready for review May 25, 2021 16:30

nisdas requested a review from a team as a code owner May 25, 2021 16:30

nisdas requested a review from farazdagi May 25, 2021 16:30

nisdas and others added 15 commits May 26, 2021 13:46

imports

951192c

Apply suggestions from code review

5a706f8

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

Apply suggestions from code review

c550d68

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

Update shared/interfaces/block_interface.go

88b693d

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

Update shared/interfaces/block_wrapper.go

eec67cc

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

Update shared/interfaces/block_interface.go

cb6dd0b

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

imports

07ac83e

fix bad changes

19fb5b0

fix

c267677

Merge branch 'blockInterface' of https://github.com/prysmaticlabs/get…

cda2757

…h-sharding into blockInterface

terence's review

eb05917

terence's review

2792b56

Merge branch 'develop' into blockInterface

778c6ca

fmt

a4a684f

Merge branch 'blockInterface' of https://github.com/prysmaticlabs/get…

a259b9d

…h-sharding into blockInterface

rkapka requested changes May 26, 2021

View reviewed changes

nisdas and others added 5 commits May 26, 2021 16:01

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

8c54db1

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

fix tests

86a3b0e

Merge branch 'blockInterface' of https://github.com/prysmaticlabs/get…

33d3033

…h-sharding into blockInterface

fix

b380223

fix all tests

0798bfe

rkapka approved these changes May 26, 2021

View reviewed changes

Merge branch 'develop' into blockInterface

ff3b8a2

nisdas added the OK to merge label May 26, 2021

prylabs-bulldozer bot merged commit caf9bdb into develop May 26, 2021

delete-merged-branch bot deleted the blockInterface branch May 26, 2021 16:19

terencechain mentioned this pull request May 26, 2021

Block interface clean up #8947

Merged

nisdas mentioned this pull request May 27, 2021

Add Nested Nil Checks To Wrapper #8951

Merged

terencechain mentioned this pull request Jun 2, 2021

Altair: Utilize block interface for clean up #8969

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Block Interface Across Prysm #8918

Use Block Interface Across Prysm #8918

nisdas commented May 21, 2021 •

edited

Loading

rkapka May 26, 2021 •

edited

Loading

nisdas May 26, 2021

rkapka May 26, 2021

rkapka May 26, 2021

rkapka May 26, 2021

rkapka May 26, 2021

rkapka May 26, 2021

rkapka May 26, 2021

rkapka May 26, 2021

rkapka May 26, 2021

rkapka May 26, 2021

codecov bot commented May 26, 2021 •

edited

Loading

	if (b == nil \|\| b.IsNil()) && s.hasInitSyncBlock(r) {
	if (b == nil \|\| b.IsNil() \|\| b.Block().IsNil()) && s.hasInitSyncBlock(r) {

	if genesisBlock != nil && !genesisBlock.IsNil() {
	if genesisBlock != nil && !genesisBlock.IsNil() && !genesisBlock.Block().IsNil() {

	if err != nil \|\| genesisBlk == nil \|\| genesisBlk.IsNil() {
	if err != nil \|\| genesisBlk == nil \|\| genesisBlk.IsNil() \|\| genesisBlk.Block().IsNil() {

	if genesisBlock == nil \|\| genesisBlock.IsNil() {
	if genesisBlock == nil \|\| genesisBlock.IsNil() \|\| genesisBlock.Block().IsNil() {

	if b == nil \|\| b.IsNil() {
	if b == nil \|\| b.IsNil() \|\| b.Block().IsNil() {

	if blk == nil \|\| blk.IsNil() {
	if blk == nil \|\| blk.IsNil() \|\| blk.Block().IsNil() {

	if childBlock == nil \|\| childBlock.IsNil() {
	if childBlock == nil \|\| childBlock.IsNil() \|\| childBlock.Block().IsNil() {

Use Block Interface Across Prysm #8918

Use Block Interface Across Prysm #8918

Conversation

nisdas commented May 21, 2021 • edited Loading

rkapka May 26, 2021 • edited Loading

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

codecov bot commented May 26, 2021 • edited Loading

Codecov Report

nisdas commented May 21, 2021 •

edited

Loading

rkapka May 26, 2021 •

edited

Loading

codecov bot commented May 26, 2021 •

edited

Loading