From b54743edbf32ce13798d09d5459cb59150ed3799 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Mon, 4 Jan 2021 12:48:39 -0800 Subject: [PATCH] Add mutex to params/config (#8160) * add mutex to params/config * split config files into test/prod * add tags checker * add regression test * remove debug info * update bazel config * go fmt * make sure that conditional file is kept by gazelle * update build tag: test -> develop * gazelle * remove redundant import * update deps.md (per Nishant's suggestion) Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> --- DEPENDENCIES.md | 11 ++++++++ shared/params/BUILD.bazel | 6 ++++ shared/params/checktags_test.go | 11 ++++++++ shared/params/config.go | 26 ----------------- shared/params/config_test.go | 16 +++++++++-- shared/params/config_utils_develop.go | 40 +++++++++++++++++++++++++++ shared/params/config_utils_prod.go | 31 +++++++++++++++++++++ 7 files changed, 113 insertions(+), 28 deletions(-) create mode 100644 shared/params/checktags_test.go create mode 100644 shared/params/config_utils_develop.go create mode 100644 shared/params/config_utils_prod.go diff --git a/DEPENDENCIES.md b/DEPENDENCIES.md index 03f78b67a..c5a6a673d 100644 --- a/DEPENDENCIES.md +++ b/DEPENDENCIES.md @@ -67,3 +67,14 @@ bazel run //:gazelle -- update-repos -from_file=go.mod -to_macro=deps.bzl%prysm_ The deps.bzl file should have been updated with the dependency and any transitive dependencies. Do NOT add new `go_repository` to the WORKSPACE file. All dependencies should live in deps.bzl. + +## Running tests + +To enable conditional compilation and custom configuration for tests (where compiled code has more +debug info, while not being completely optimized), we rely on Go's build tags/constraints mechanism +(see official docs on [build constraints](https://golang.org/pkg/go/build/#hdr-Build_Constraints)). +Therefore, whenever using `go test`, do not forget to pass in extra build tag, eg: + +```bash +go test ./beacon-chain/sync/initial-sync -tags develop +``` diff --git a/shared/params/BUILD.bazel b/shared/params/BUILD.bazel index 68d95f9d0..8a77d0606 100644 --- a/shared/params/BUILD.bazel +++ b/shared/params/BUILD.bazel @@ -5,6 +5,8 @@ go_library( name = "go_default_library", srcs = [ "config.go", + "config_utils_develop.go", # keep + "config_utils_prod.go", "io_config.go", "loader.go", "mainnet_config.go", @@ -29,6 +31,7 @@ go_test( name = "go_default_test", size = "small", srcs = [ + "checktags_test.go", "config_test.go", "loader_test.go", ], @@ -37,9 +40,12 @@ go_test( "@eth2_spec_tests_minimal//:test_data", ], embed = [":go_default_library"], + gotags = ["develop"], + race = "on", deps = [ "//shared/testutil/assert:go_default_library", "//shared/testutil/require:go_default_library", + "@com_github_sirupsen_logrus//:go_default_library", "@io_bazel_rules_go//go/tools/bazel:go_default_library", ], ) diff --git a/shared/params/checktags_test.go b/shared/params/checktags_test.go new file mode 100644 index 000000000..07bc61ea8 --- /dev/null +++ b/shared/params/checktags_test.go @@ -0,0 +1,11 @@ +// +build !develop + +package params + +import ( + log "github.com/sirupsen/logrus" +) + +func init() { + log.Fatal("Tests in this package require extra build tag: re-run with `-tags develop`") +} diff --git a/shared/params/config.go b/shared/params/config.go index dff0f8a3f..8c156b9c2 100644 --- a/shared/params/config.go +++ b/shared/params/config.go @@ -3,8 +3,6 @@ package params import ( "time" - - "github.com/mohae/deepcopy" ) // BeaconChainConfig contains constant configs for node to participate in beacon chain. @@ -126,27 +124,3 @@ type BeaconChainConfig struct { // Weak subjectivity values. SafetyDecay uint64 // SafetyDecay is defined as the loss in the 1/3 consensus safety margin of the casper FFG mechanism. } - -var beaconConfig = MainnetConfig() - -// BeaconConfig retrieves beacon chain config. -func BeaconConfig() *BeaconChainConfig { - return beaconConfig -} - -// OverrideBeaconConfig by replacing the config. The preferred pattern is to -// call BeaconConfig(), change the specific parameters, and then call -// OverrideBeaconConfig(c). Any subsequent calls to params.BeaconConfig() will -// return this new configuration. -func OverrideBeaconConfig(c *BeaconChainConfig) { - beaconConfig = c -} - -// Copy returns a copy of the config object. -func (c *BeaconChainConfig) Copy() *BeaconChainConfig { - config, ok := deepcopy.Copy(*c).(BeaconChainConfig) - if !ok { - config = *beaconConfig - } - return &config -} diff --git a/shared/params/config_test.go b/shared/params/config_test.go index 2efc1b07c..e40954e8e 100644 --- a/shared/params/config_test.go +++ b/shared/params/config_test.go @@ -11,7 +11,7 @@ import ( // to make sure that previous test case has already been completed and check can be run. var testOverrideBeaconConfigExecuted bool -func TestOverrideBeaconConfig(t *testing.T) { +func TestConfig_OverrideBeaconConfig(t *testing.T) { // Ensure that param modifications are safe. params.SetupTestConfigCleanup(t) cfg := params.BeaconConfig() @@ -23,7 +23,7 @@ func TestOverrideBeaconConfig(t *testing.T) { testOverrideBeaconConfigExecuted = true } -func TestOverrideBeaconConfigTestTeardown(t *testing.T) { +func TestConfig_OverrideBeaconConfigTestTeardown(t *testing.T) { if !testOverrideBeaconConfigExecuted { t.Skip("State leak can occur only if state mutating test has already completed") } @@ -32,3 +32,15 @@ func TestOverrideBeaconConfigTestTeardown(t *testing.T) { t.Fatal("Parameter update has been leaked out of previous test") } } + +func TestConfig_DataRace(t *testing.T) { + for i := 0; i < 10; i++ { + go func() { + cfg := params.BeaconConfig() + params.OverrideBeaconConfig(cfg) + }() + go func() uint64 { + return params.BeaconConfig().MaxDeposits + }() + } +} diff --git a/shared/params/config_utils_develop.go b/shared/params/config_utils_develop.go new file mode 100644 index 000000000..500d6dfa5 --- /dev/null +++ b/shared/params/config_utils_develop.go @@ -0,0 +1,40 @@ +// +build develop + +package params + +import ( + "sync" + + "github.com/mohae/deepcopy" +) + +var beaconConfig = MainnetConfig() +var beaconConfigLock sync.RWMutex + +// BeaconConfig retrieves beacon chain config. +func BeaconConfig() *BeaconChainConfig { + beaconConfigLock.RLock() + defer beaconConfigLock.RUnlock() + return beaconConfig +} + +// OverrideBeaconConfig by replacing the config. The preferred pattern is to +// call BeaconConfig(), change the specific parameters, and then call +// OverrideBeaconConfig(c). Any subsequent calls to params.BeaconConfig() will +// return this new configuration. +func OverrideBeaconConfig(c *BeaconChainConfig) { + beaconConfigLock.Lock() + defer beaconConfigLock.Unlock() + beaconConfig = c +} + +// Copy returns a copy of the config object. +func (c *BeaconChainConfig) Copy() *BeaconChainConfig { + beaconConfigLock.RLock() + defer beaconConfigLock.RUnlock() + config, ok := deepcopy.Copy(*c).(BeaconChainConfig) + if !ok { + config = *beaconConfig + } + return &config +} diff --git a/shared/params/config_utils_prod.go b/shared/params/config_utils_prod.go new file mode 100644 index 000000000..90f716b50 --- /dev/null +++ b/shared/params/config_utils_prod.go @@ -0,0 +1,31 @@ +// +build !develop + +package params + +import ( + "github.com/mohae/deepcopy" +) + +var beaconConfig = MainnetConfig() + +// BeaconConfig retrieves beacon chain config. +func BeaconConfig() *BeaconChainConfig { + return beaconConfig +} + +// OverrideBeaconConfig by replacing the config. The preferred pattern is to +// call BeaconConfig(), change the specific parameters, and then call +// OverrideBeaconConfig(c). Any subsequent calls to params.BeaconConfig() will +// return this new configuration. +func OverrideBeaconConfig(c *BeaconChainConfig) { + beaconConfig = c +} + +// Copy returns a copy of the config object. +func (c *BeaconChainConfig) Copy() *BeaconChainConfig { + config, ok := deepcopy.Copy(*c).(BeaconChainConfig) + if !ok { + config = *beaconConfig + } + return &config +}