From 8b687a3e722a9b9274ba9a6fb38f0a326ae8589e Mon Sep 17 00:00:00 2001 From: terence tsao Date: Mon, 20 Mar 2023 15:19:52 -0700 Subject: [PATCH 1/4] Add locks to forkchoice spec testS --- .../spectest/shared/common/forkchoice/builder.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/testing/spectest/shared/common/forkchoice/builder.go b/testing/spectest/shared/common/forkchoice/builder.go index 1e3fc413cd1..39376fd8623 100644 --- a/testing/spectest/shared/common/forkchoice/builder.go +++ b/testing/spectest/shared/common/forkchoice/builder.go @@ -38,6 +38,9 @@ func NewBuilder(t testing.TB, initialState state.BeaconState, initialBlock inter // Tick resets the genesis time to now()-tick and adjusts the slot to the appropriate value. func (bb *Builder) Tick(t testing.TB, tick int64) { + bb.service.ForkChoicer().Lock() + defer bb.service.ForkChoicer().Unlock() + bb.service.SetGenesisTime(time.Unix(time.Now().Unix()-tick, 0)) lastSlot := uint64(bb.lastTick) / params.BeaconConfig().SecondsPerSlot currentSlot := uint64(tick) / params.BeaconConfig().SecondsPerSlot @@ -104,17 +107,26 @@ func (bb *Builder) PoWBlock(pb *ethpb.PowBlock) { // Attestation receives the attestation and updates forkchoice. func (bb *Builder) Attestation(t testing.TB, a *ethpb.Attestation) { + bb.service.ForkChoicer().Lock() + defer bb.service.ForkChoicer().Unlock() + require.NoError(t, bb.service.OnAttestation(context.TODO(), a, params.BeaconNetworkConfig().MaximumGossipClockDisparity)) } // AttesterSlashing receives an attester slashing and feeds it to forkchoice. func (bb *Builder) AttesterSlashing(s *ethpb.AttesterSlashing) { + bb.service.ForkChoicer().Lock() + defer bb.service.ForkChoicer().Unlock() + slashings := []*ethpb.AttesterSlashing{s} bb.service.InsertSlashingsToForkChoiceStore(context.TODO(), slashings) } // Check evaluates the fork choice results and compares them to the expected values. func (bb *Builder) Check(t testing.TB, c *Check) { + bb.service.ForkChoicer().RUnlock() + defer bb.service.ForkChoicer().RUnlock() + if c == nil { return } @@ -144,9 +156,7 @@ func (bb *Builder) Check(t testing.TB, c *Check) { } if c.ProposerBoostRoot != nil { want := fmt.Sprintf("%#x", common.FromHex(*c.ProposerBoostRoot)) - bb.service.ForkChoiceStore().RLock() got := fmt.Sprintf("%#x", bb.service.ForkChoiceStore().ProposerBoost()) - bb.service.ForkChoiceStore().RUnlock() require.DeepEqual(t, want, got) } From b3a4080300e0238f6fcc5b13d285a33a43fc9115 Mon Sep 17 00:00:00 2001 From: terence tsao Date: Mon, 20 Mar 2023 15:23:34 -0700 Subject: [PATCH 2/4] Fix --- testing/spectest/shared/common/forkchoice/builder.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/testing/spectest/shared/common/forkchoice/builder.go b/testing/spectest/shared/common/forkchoice/builder.go index 39376fd8623..f52f6e872dc 100644 --- a/testing/spectest/shared/common/forkchoice/builder.go +++ b/testing/spectest/shared/common/forkchoice/builder.go @@ -124,16 +124,17 @@ func (bb *Builder) AttesterSlashing(s *ethpb.AttesterSlashing) { // Check evaluates the fork choice results and compares them to the expected values. func (bb *Builder) Check(t testing.TB, c *Check) { - bb.service.ForkChoicer().RUnlock() - defer bb.service.ForkChoicer().RUnlock() - if c == nil { return } ctx := context.TODO() + bb.service.ForkChoicer().Lock() require.NoError(t, bb.service.UpdateAndSaveHeadWithBalances(ctx)) + bb.service.ForkChoicer().Unlock() if c.Head != nil { + bb.service.ForkChoicer().RLock() r, err := bb.service.HeadRoot(ctx) + bb.service.ForkChoicer().RUnlock() require.NoError(t, err) require.DeepEqual(t, common.FromHex(c.Head.Root), r) require.Equal(t, primitives.Slot(c.Head.Slot), bb.service.HeadSlot()) @@ -156,7 +157,9 @@ func (bb *Builder) Check(t testing.TB, c *Check) { } if c.ProposerBoostRoot != nil { want := fmt.Sprintf("%#x", common.FromHex(*c.ProposerBoostRoot)) + bb.service.ForkChoicer().RLock() got := fmt.Sprintf("%#x", bb.service.ForkChoiceStore().ProposerBoost()) + bb.service.ForkChoicer().RUnlock() require.DeepEqual(t, want, got) } From 8e8c2f40f3f8b42da58e9c25e207b6d2cf0dd0fd Mon Sep 17 00:00:00 2001 From: terence tsao Date: Mon, 20 Mar 2023 16:56:44 -0700 Subject: [PATCH 3/4] Rm forkchoice store getter --- beacon-chain/blockchain/chain_info.go | 5 ----- beacon-chain/blockchain/chain_info_test.go | 8 +------- testing/spectest/shared/common/forkchoice/builder.go | 2 +- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/beacon-chain/blockchain/chain_info.go b/beacon-chain/blockchain/chain_info.go index 775e752ff85..2246b4b7b0c 100644 --- a/beacon-chain/blockchain/chain_info.go +++ b/beacon-chain/blockchain/chain_info.go @@ -484,8 +484,3 @@ func (s *Service) Ancestor(ctx context.Context, root []byte, slot primitives.Slo func (s *Service) SetGenesisTime(t time.Time) { s.genesisTime = t } - -// ForkChoiceStore returns the fork choice store in the service. -func (s *Service) ForkChoiceStore() forkchoice.ForkChoicer { - return s.cfg.ForkChoiceStore -} diff --git a/beacon-chain/blockchain/chain_info_test.go b/beacon-chain/blockchain/chain_info_test.go index 4c8359e8d42..c9ba86d8109 100644 --- a/beacon-chain/blockchain/chain_info_test.go +++ b/beacon-chain/blockchain/chain_info_test.go @@ -71,12 +71,6 @@ func TestHeadRoot_Nil(t *testing.T) { assert.DeepEqual(t, params.BeaconConfig().ZeroHash[:], headRoot, "Incorrect pre chain start value") } -func TestService_ForkChoiceStore(t *testing.T) { - c := &Service{cfg: &config{ForkChoiceStore: doublylinkedtree.New()}} - p := c.ForkChoiceStore() - require.Equal(t, primitives.Epoch(0), p.FinalizedCheckpoint().Epoch) -} - func TestFinalizedCheckpt_GenesisRootOk(t *testing.T) { ctx := context.Background() beaconDB := testDB.SetupDB(t) @@ -559,7 +553,7 @@ func TestService_IsFinalized(t *testing.T) { ctx := context.Background() c := &Service{cfg: &config{BeaconDB: beaconDB, ForkChoiceStore: doublylinkedtree.New()}} r1 := [32]byte{'a'} - require.NoError(t, c.ForkChoiceStore().UpdateFinalizedCheckpoint(&forkchoicetypes.Checkpoint{ + require.NoError(t, c.ForkChoicer().UpdateFinalizedCheckpoint(&forkchoicetypes.Checkpoint{ Root: r1, })) b := util.NewBeaconBlock() diff --git a/testing/spectest/shared/common/forkchoice/builder.go b/testing/spectest/shared/common/forkchoice/builder.go index f52f6e872dc..4b41a19eafb 100644 --- a/testing/spectest/shared/common/forkchoice/builder.go +++ b/testing/spectest/shared/common/forkchoice/builder.go @@ -158,7 +158,7 @@ func (bb *Builder) Check(t testing.TB, c *Check) { if c.ProposerBoostRoot != nil { want := fmt.Sprintf("%#x", common.FromHex(*c.ProposerBoostRoot)) bb.service.ForkChoicer().RLock() - got := fmt.Sprintf("%#x", bb.service.ForkChoiceStore().ProposerBoost()) + got := fmt.Sprintf("%#x", bb.service.ForkChoicer().ProposerBoost()) bb.service.ForkChoicer().RUnlock() require.DeepEqual(t, want, got) } From e03d2f194b4f3fd412c2f973c866c9a1fdc892a3 Mon Sep 17 00:00:00 2001 From: terence tsao Date: Mon, 20 Mar 2023 17:00:23 -0700 Subject: [PATCH 4/4] Rm useless lock --- testing/spectest/shared/common/forkchoice/builder.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/testing/spectest/shared/common/forkchoice/builder.go b/testing/spectest/shared/common/forkchoice/builder.go index 4b41a19eafb..6399b96d371 100644 --- a/testing/spectest/shared/common/forkchoice/builder.go +++ b/testing/spectest/shared/common/forkchoice/builder.go @@ -132,9 +132,7 @@ func (bb *Builder) Check(t testing.TB, c *Check) { require.NoError(t, bb.service.UpdateAndSaveHeadWithBalances(ctx)) bb.service.ForkChoicer().Unlock() if c.Head != nil { - bb.service.ForkChoicer().RLock() r, err := bb.service.HeadRoot(ctx) - bb.service.ForkChoicer().RUnlock() require.NoError(t, err) require.DeepEqual(t, common.FromHex(c.Head.Root), r) require.Equal(t, primitives.Slot(c.Head.Slot), bb.service.HeadSlot())