GetBlock() - Simple optimisations #9541

jmozah · 2021-09-07T10:49:31Z

What type of PR is this?
Optimisation

What does this PR do? Why is it needed?
During debugging of #8943, it was found that some of the operations done
during GetBlock() can be optimised further. This PR does the simple optimisations under a flag.

Which issues(s) does this PR fix?
Related to #8943

Other notes for review
The flag is --enable-get-block-optimizations, and it is disabled by default.

codecov · 2021-09-07T11:05:45Z

Codecov Report

Merging #9541 (84088db) into develop (393549a) will decrease coverage by 0.43%.
The diff coverage is 18.36%.

❗ Current head 84088db differs from pull request most recent head af11581. Consider uploading reports for the commit af11581 to get more accurate results

@@             Coverage Diff             @@
##           develop    #9541      +/-   ##
===========================================
- Coverage    62.34%   61.90%   -0.44%     
===========================================
  Files          572      565       -7     
  Lines        45457    44750     -707     
===========================================
- Hits         28339    27703     -636     
+ Misses       12568    12555      -13     
+ Partials      4550     4492      -58

prestonvanloon · 2021-09-07T18:43:09Z

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

+
+	eg.Go(func() error {
+		// Pack ETH1 deposits which have not been included in the beacon chain.
+		localDeposits, err := vs.deposits(ctx, head, eth1Data)


Shouldn't you use egctx here?

Suggested change

localDeposits, err := vs.deposits(ctx, head, eth1Data)

localDeposits, err := vs.deposits(egctx, head, eth1Data)

prestonvanloon · 2021-09-07T18:43:22Z

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

+
+	eg.Go(func() error {
+		// Pack aggregated attestations which have not been included in the beacon chain.
+		localAtts, err := vs.packAttestations(ctx, head)


Here too

Suggested change

localAtts, err := vs.packAttestations(ctx, head)

localAtts, err := vs.packAttestations(egctx, head)

shared/featureconfig/flags.go

prestonvanloon · 2021-09-07T18:45:31Z

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

-	// This is very likely to happen because BlockTimeByHeight returns the last block AT OR BEFORE the specified time.
-	if lastBlockByEarliestValidTime.Time < earliestValidTime {
-		lastBlockByEarliestValidTime.Number = big.NewInt(0).Add(lastBlockByEarliestValidTime.Number, big.NewInt(1))
+	if !featureconfig.Get().EnableGetBlockOptimizations {


Why is this branch omitted when the flag is on?

This branch seems to not do anything here.
lastBlockByEarliestValidTime is never used anywhere inside the code.
I need a confirmation if my thinking is right.
WDYT @nisdas

That is a bit more complicated, as that value was used for our inRangeVotes method. This was removed previously when we had the voting bug and defaulted to independent voting compared to voting by majority. Do we ever expect to add the vote by majority feature back ? @prestonvanloon

My 2 cents: Even if want to add the feature back sometime later. Let's remove unused code for now.

Discussed offline, its better to just remove it now. Performing the extra BlockByTimeStamp call is expensive and can increase block proposal times needlessly.

prestonvanloon · 2021-09-07T18:46:35Z

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

 	dbpb "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1"
+	eth "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1"
 	ethpb "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1"


These are the same imports, please use only one

terencechain

Can we implement benchmarks on before and after? We should know how much this improves

Numbers from tracing instrumentation will be helpful too

nisdas · 2021-09-10T03:14:48Z

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

@@ -230,16 +232,9 @@ func (vs *Server) buildPhase0BlockData(ctx context.Context, req *ethpb.BlockRequ
 		return nil, fmt.Errorf("could not get ETH1 data: %v", err)
 	}

-	// Pack ETH1 deposits which have not been included in the beacon chain.
-	deposits, err := vs.deposits(ctx, head, eth1Data)
+	deposits, atts, err := vs.depositAndPackAttestations(ctx, head, eth1Data)


it is a bit odd to group deposit and attestation packing into one method. But I do understand that it makes it cleaner when applying the flag.

jmozah · 2021-09-13T07:32:23Z

Can we implement benchmarks on before and after? We should know how much this improves

Numbers from tracing instrumentation will be helpful too

@terencechain yeah we can do that.

jmozah · 2021-09-13T15:28:19Z

Can we implement benchmarks on before and after? We should know how much this improves
Numbers from tracing instrumentation will be helpful too

@terencechain yeah we can do that.

@terencechain On the second thought, i think it will be nice to run the instrumentation and monitor block proposal times, after we fix the rest of the fixes related to getBlock(). What say?

terencechain · 2021-09-13T15:40:22Z

Can we implement benchmarks on before and after? We should know how much this improves
Numbers from tracing instrumentation will be helpful too

@terencechain yeah we can do that.

@terencechain On the second thought, i think it will be nice to run the instrumentation and monitor block proposal times, after we fix the rest of the fixes related to getBlock(). What say?

That's fine with me. Let's wait to merge this until we can monitor block proposal time then

Another method here (easier) is to run getblock benchmark test on both featureflag on/off

jmozah · 2021-09-13T17:29:18Z

Another method here (easier) is to run getblock benchmark test on both featureflag on/off

Thats a nice idea. I will do this.

jmozah · 2021-09-13T17:49:33Z

Another method here (easier) is to run getblock benchmark test on both featureflag on/off

Thats a nice idea. I will do this.

Decided offline with @terencechain to keep this PR open until we fix other issues in GetBlock()

jmozah · 2021-09-16T13:42:02Z

This PR will be closed once all the tracking issues are closed.

…abs/prysm into getblock_optimization

nisdas · 2021-09-29T07:33:07Z

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

+		}
+		// Increment the earliest block if the original block's time is before valid time.
+		// This is very likely to happen because BlockTimeByHeight returns the last block AT OR BEFORE the specified time.
+		if lastBlockByEarliestValidTime.Time < earliestValidTime {


if this value isnt being used anywhere you can remove it, no need to gate it behind the flag.

Makes sense. Done.

…abs/prysm into getblock_optimization

terencechain · 2021-09-30T15:48:17Z

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

@@ -278,6 +271,70 @@ func (vs *Server) ProposeBeaconBlock(ctx context.Context, req *ethpb.GenericSign
 	return vs.proposeGenericBeaconBlock(ctx, blk)
 }

+func (vs *Server) depositAndPackAttestations(ctx context.Context, head state.BeaconState, eth1Data *ethpb.Eth1Data) ([]*ethpb.Deposit, []*ethpb.Attestation, error) {


Suggested change

func (vs *Server) depositAndPackAttestations(ctx context.Context, head state.BeaconState, eth1Data *ethpb.Eth1Data) ([]*ethpb.Deposit, []*ethpb.Attestation, error) {

func (vs *Server) packDepositsAndAttestations(ctx context.Context, head state.BeaconState, eth1Data *ethpb.Eth1Data) ([]*ethpb.Deposit, []*ethpb.Attestation, error) {

terencechain · 2021-09-30T15:48:32Z

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

+	return deposits, atts, nil
+}
+
+func (vs *Server) optimizedDepositAndPackAttestations(ctx context.Context, head state.BeaconState, eth1Data *ethpb.Eth1Data) ([]*ethpb.Deposit, []*ethpb.Attestation, error) {


Suggested change

func (vs *Server) optimizedDepositAndPackAttestations(ctx context.Context, head state.BeaconState, eth1Data *ethpb.Eth1Data) ([]*ethpb.Deposit, []*ethpb.Attestation, error) {

func (vs *Server) optimizedPackDepositsAndAttestations(ctx context.Context, head state.BeaconState, eth1Data *ethpb.Eth1Data) ([]*ethpb.Deposit, []*ethpb.Attestation, error) {

terencechain · 2021-09-30T15:49:28Z

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

+		if err != nil {
+			return status.Errorf(codes.Internal, "Could not get ETH1 deposits: %v", err)
+		}
+		// if the original context is cancelled, then cancel this routine toobazel run //:gazelle -- fix`


Suggested change

// if the original context is cancelled, then cancel this routine toobazel run //:gazelle -- fix`

// if the original context is cancelled, then cancel this routine too

terencechain · 2021-09-30T15:52:47Z

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

@@ -551,7 +605,7 @@ func (vs *Server) deposits(
 	if vs.MockEth1Votes || !vs.Eth1InfoFetcher.IsConnectedToETH1() {
 		return []*ethpb.Deposit{}, nil
 	}
-	// Need to fetch if the deposits up to the state's latest eth 1 data matches
+	// Need to fetch if the deposits up to the state's latest ethpb 1 data matches


Suggested change

// Need to fetch if the deposits up to the state's latest ethpb 1 data matches

// Need to fetch if the deposits up to the state's latest eth 1 data matches

terencechain · 2021-09-30T15:53:07Z

config/features/config.go

@@ -50,6 +50,7 @@ type Flags struct {
 	EnableOptimizedBalanceUpdate        bool // EnableOptimizedBalanceUpdate uses an updated method of performing balance updates.
 	EnableDoppelGanger                  bool // EnableDoppelGanger enables doppelganger protection on startup for the validator.
 	EnableHistoricalSpaceRepresentation bool // EnableHistoricalSpaceRepresentation enables the saving of registry validators in separate buckets to save space
+	EnableGetBlockOptimizations         bool // EnableGetBlockOptimizations optimizes some elements of the GetBlock() function


Suggested change

EnableGetBlockOptimizations bool // EnableGetBlockOptimizations optimizes some elements of the GetBlock() function

EnableGetBlockOptimizations bool // EnableGetBlockOptimizations optimizes some elements of the GetBlock() function.

Lgtm

…abs/prysm into getblock_optimization

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

Co-authored-by: Nishant Das <nishdas93@gmail.com>

GetBlock() optimizations

ca67599

jmozah requested a review from nisdas September 7, 2021 10:49

jmozah self-assigned this Sep 7, 2021

jmozah requested a review from a team as a code owner September 7, 2021 10:49

jmozah requested review from terencechain and rkapka and removed request for a team September 7, 2021 10:49

prestonvanloon reviewed Sep 7, 2021

View reviewed changes

jmozah added 2 commits September 8, 2021 19:42

addressed preston's comments

060b3bf

Merge branch 'develop' into getblock_optimization

2abdabd

jmozah added the Ready For Review label Sep 9, 2021

terencechain previously requested changes Sep 9, 2021

View reviewed changes

nisdas reviewed Sep 10, 2021

View reviewed changes

Merge branch 'develop' into getblock_optimization

d2affc8

Merge branch 'develop' into getblock_optimization

f6ff9f9

jmozah added 2 commits September 13, 2021 22:59

Merge branch 'develop' into getblock_optimization

b87927c

fix fmt

a83338b

Merge branch 'develop' into getblock_optimization

2421f00

jmozah removed the Ready For Review label Sep 13, 2021

jmozah added 4 commits September 23, 2021 12:29

added benchmarks

c88e3bd

Merge branch 'getblock_optimization' of https://github.com/prysmaticl…

a721f0c

…abs/prysm into getblock_optimization

resolve conflicts

0c4f8a8

fix deepsource

f5b9c27

jmozah added the Ready For Review label Sep 29, 2021

nisdas reviewed Sep 29, 2021

View reviewed changes

jmozah added 2 commits September 30, 2021 10:03

remove unnesesary check

731f1bb

Merge branch 'getblock_optimization' of https://github.com/prysmaticl…

f7725db

…abs/prysm into getblock_optimization

jmozah changed the title ~~GetBlock() optimizations~~ GetBlock() - Simple optimisations Sep 30, 2021

jmozah mentioned this pull request Sep 30, 2021

GetBlock() Optimisations #9708

Closed

6 tasks

Merge branch 'develop' into getblock_optimization

623be05

jmozah requested review from nisdas, a team and kasey and removed request for rkapka, terencechain, nisdas, a team and kasey September 30, 2021 15:39

terencechain reviewed Sep 30, 2021

View reviewed changes

Merge branch 'develop' into getblock_optimization

030ca7c

nisdas previously approved these changes Oct 1, 2021

View reviewed changes

jmozah added 2 commits October 2, 2021 12:53

fixed terence's comments

409edc1

Merge branch 'getblock_optimization' of https://github.com/prysmaticl…

106e2bb

…abs/prysm into getblock_optimization

jmozah dismissed nisdas’s stale review via 106e2bb October 2, 2021 07:23

Merge branch 'develop' into getblock_optimization

64e0464

nisdas reviewed Oct 5, 2021

View reviewed changes

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go Outdated Show resolved Hide resolved

Update beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

5b89c65

Co-authored-by: Nishant Das <nishdas93@gmail.com>

nisdas approved these changes Oct 5, 2021

View reviewed changes

Merge branch 'develop' into getblock_optimization

af11581

nisdas merged commit 362dfa6 into develop Oct 5, 2021

delete-merged-branch bot deleted the getblock_optimization branch October 5, 2021 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GetBlock() - Simple optimisations #9541

GetBlock() - Simple optimisations #9541

jmozah commented Sep 7, 2021 •

edited

Loading

codecov bot commented Sep 7, 2021 •

edited

Loading

prestonvanloon Sep 7, 2021

prestonvanloon Sep 7, 2021

prestonvanloon Sep 7, 2021

jmozah Sep 8, 2021

nisdas Sep 8, 2021

jmozah Sep 9, 2021

nisdas Sep 10, 2021

prestonvanloon Sep 7, 2021

terencechain left a comment

nisdas Sep 10, 2021

jmozah commented Sep 13, 2021

jmozah commented Sep 13, 2021

terencechain commented Sep 13, 2021

jmozah commented Sep 13, 2021

jmozah commented Sep 13, 2021

jmozah commented Sep 16, 2021

nisdas Sep 29, 2021

jmozah Sep 30, 2021 •

edited

Loading

terencechain Sep 30, 2021

terencechain Sep 30, 2021

terencechain Sep 30, 2021

terencechain Sep 30, 2021

terencechain Sep 30, 2021

	localDeposits, err := vs.deposits(ctx, head, eth1Data)
	localDeposits, err := vs.deposits(egctx, head, eth1Data)

	localAtts, err := vs.packAttestations(ctx, head)
	localAtts, err := vs.packAttestations(egctx, head)

	func (vs Server) depositAndPackAttestations(ctx context.Context, head state.BeaconState, eth1Data ethpb.Eth1Data) ([]ethpb.Deposit, []ethpb.Attestation, error) {
	func (vs Server) packDepositsAndAttestations(ctx context.Context, head state.BeaconState, eth1Data ethpb.Eth1Data) ([]ethpb.Deposit, []ethpb.Attestation, error) {

	func (vs Server) optimizedDepositAndPackAttestations(ctx context.Context, head state.BeaconState, eth1Data ethpb.Eth1Data) ([]ethpb.Deposit, []ethpb.Attestation, error) {
	func (vs Server) optimizedPackDepositsAndAttestations(ctx context.Context, head state.BeaconState, eth1Data ethpb.Eth1Data) ([]ethpb.Deposit, []ethpb.Attestation, error) {

	// if the original context is cancelled, then cancel this routine toobazel run //:gazelle -- fix`
	// if the original context is cancelled, then cancel this routine too

	// Need to fetch if the deposits up to the state's latest ethpb 1 data matches
	// Need to fetch if the deposits up to the state's latest eth 1 data matches

	EnableGetBlockOptimizations bool // EnableGetBlockOptimizations optimizes some elements of the GetBlock() function
	EnableGetBlockOptimizations bool // EnableGetBlockOptimizations optimizes some elements of the GetBlock() function.

GetBlock() - Simple optimisations #9541

GetBlock() - Simple optimisations #9541

Conversation

jmozah commented Sep 7, 2021 • edited Loading

codecov bot commented Sep 7, 2021 • edited Loading

Codecov Report

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

terencechain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmozah commented Sep 13, 2021

jmozah commented Sep 13, 2021

terencechain commented Sep 13, 2021

jmozah commented Sep 13, 2021

jmozah commented Sep 13, 2021

jmozah commented Sep 16, 2021

Choose a reason for hiding this comment

jmozah Sep 30, 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

jmozah commented Sep 7, 2021 •

edited

Loading

codecov bot commented Sep 7, 2021 •

edited

Loading

jmozah Sep 30, 2021 •

edited

Loading