From 18cc7fb67a3bbf5548b23d944642261e9c80d207 Mon Sep 17 00:00:00 2001 From: Shota Date: Wed, 30 Oct 2024 02:12:36 +0400 Subject: [PATCH] fix race for global vars (#12512) Co-authored-by: shota.silagadze --- .../services/attestation_service_test.go | 26 ++++++++++++++----- .../bls_to_execution_change_service_test.go | 1 - cl/sentinel/gossip.go | 2 ++ cl/sentinel/sentinel.go | 2 ++ 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/cl/phase1/network/services/attestation_service_test.go b/cl/phase1/network/services/attestation_service_test.go index 2482fc4ea2f..60e8f1df401 100644 --- a/cl/phase1/network/services/attestation_service_test.go +++ b/cl/phase1/network/services/attestation_service_test.go @@ -81,7 +81,6 @@ func (t *attestationTestSuite) SetupTest() { netConfig := &clparams.NetworkConfig{} emitters := beaconevents.NewEventEmitter() computeSigningRoot = func(obj ssz.HashableSSZ, domain []byte) ([32]byte, error) { return [32]byte{}, nil } - batchCheckInterval = 1 * time.Millisecond batchSignatureVerifier := NewBatchSignatureVerifier(context.TODO(), nil) go batchSignatureVerifier.Start() ctx, cn := context.WithCancel(context.Background()) @@ -100,9 +99,10 @@ func (t *attestationTestSuite) TestAttestationProcessMessage() { msg *solid.Attestation } tests := []struct { - name string - mock func() - args args + name string + wantErr bool + mock func() + args args }{ { name: "Test attestation with committee index out of range", @@ -118,6 +118,7 @@ func (t *attestationTestSuite) TestAttestationProcessMessage() { subnet: nil, msg: att, }, + wantErr: true, }, { name: "Test attestation with wrong subnet", @@ -136,6 +137,7 @@ func (t *attestationTestSuite) TestAttestationProcessMessage() { subnet: uint64Ptr(1), msg: att, }, + wantErr: true, }, { name: "Test attestation with wrong slot (current_slot < slot)", @@ -155,6 +157,7 @@ func (t *attestationTestSuite) TestAttestationProcessMessage() { subnet: uint64Ptr(1), msg: att, }, + wantErr: true, }, { name: "Attestation is aggregated", @@ -178,6 +181,7 @@ func (t *attestationTestSuite) TestAttestationProcessMessage() { Signature: [96]byte{0, 1, 2, 3, 4, 5}, }, }, + wantErr: true, }, { name: "Attestation is empty", @@ -201,6 +205,7 @@ func (t *attestationTestSuite) TestAttestationProcessMessage() { Signature: [96]byte{0, 1, 2, 3, 4, 5}, }, }, + wantErr: true, }, { name: "invalid signature", @@ -225,6 +230,7 @@ func (t *attestationTestSuite) TestAttestationProcessMessage() { subnet: uint64Ptr(1), msg: att, }, + wantErr: true, }, { name: "block header not found", @@ -249,6 +255,7 @@ func (t *attestationTestSuite) TestAttestationProcessMessage() { subnet: uint64Ptr(1), msg: att, }, + wantErr: true, }, { name: "invalid target block", @@ -276,6 +283,7 @@ func (t *attestationTestSuite) TestAttestationProcessMessage() { subnet: uint64Ptr(1), msg: att, }, + wantErr: true, }, { name: "invalid finality checkpoint", @@ -309,6 +317,7 @@ func (t *attestationTestSuite) TestAttestationProcessMessage() { subnet: uint64Ptr(1), msg: att, }, + wantErr: true, }, { name: "success", @@ -355,9 +364,14 @@ func (t *attestationTestSuite) TestAttestationProcessMessage() { log.Printf("test case: %s", tt.name) t.SetupTest() tt.mock() - err := t.attService.ProcessMessage(tt.args.ctx, tt.args.subnet, &AttestationWithGossipData{Attestation: tt.args.msg, GossipData: nil}) + err := t.attService.ProcessMessage(tt.args.ctx, tt.args.subnet, &AttestationWithGossipData{Attestation: tt.args.msg, GossipData: nil, ImmediateProcess: true}) time.Sleep(time.Millisecond * 60) - t.Require().Error(err, ErrIgnore) + if tt.wantErr { + t.Require().Error(err) + } else { + t.Require().NoError(err) + } + t.True(t.gomockCtrl.Satisfied()) } } diff --git a/cl/phase1/network/services/bls_to_execution_change_service_test.go b/cl/phase1/network/services/bls_to_execution_change_service_test.go index ee0fc6fac76..5fbc55f1543 100644 --- a/cl/phase1/network/services/bls_to_execution_change_service_test.go +++ b/cl/phase1/network/services/bls_to_execution_change_service_test.go @@ -59,7 +59,6 @@ func (t *blsToExecutionChangeTestSuite) SetupTest() { t.emitters = beaconevents.NewEventEmitter() t.beaconCfg = &clparams.BeaconChainConfig{} batchSignatureVerifier := NewBatchSignatureVerifier(context.TODO(), nil) - batchCheckInterval = 1 * time.Millisecond go batchSignatureVerifier.Start() t.service = NewBLSToExecutionChangeService(*t.operationsPool, t.emitters, t.syncedData, t.beaconCfg, batchSignatureVerifier) // mock global functions diff --git a/cl/sentinel/gossip.go b/cl/sentinel/gossip.go index 68f89835ba1..5ed91599768 100644 --- a/cl/sentinel/gossip.go +++ b/cl/sentinel/gossip.go @@ -513,9 +513,11 @@ func (g *GossipManager) Start(ctx context.Context) { logArgs := []interface{}{} g.subscriptions.Range(func(key, value any) bool { sub := value.(*GossipSubscription) + sub.lock.Lock() if sub.topic != nil { logArgs = append(logArgs, sub.topic.String(), sub.subscribed.Load()) } + sub.lock.Unlock() return true }) log.Trace("[Gossip] Subscriptions", "subscriptions", logArgs) diff --git a/cl/sentinel/sentinel.go b/cl/sentinel/sentinel.go index 7fd34df35b2..937b755ccad 100644 --- a/cl/sentinel/sentinel.go +++ b/cl/sentinel/sentinel.go @@ -377,6 +377,8 @@ func (s *Sentinel) isPeerUsefulForAnySubnet(node *enode.Node) bool { s.subManager.subscriptions.Range(func(key, value any) bool { sub := value.(*GossipSubscription) + sub.lock.Lock() + defer sub.lock.Unlock() if sub.sub == nil { return true }