From a974627258aff479d1c2fb9aa7e10be7677b636d Mon Sep 17 00:00:00 2001 From: Nishant Das Date: Sat, 6 Jan 2024 15:29:06 +0800 Subject: [PATCH] Make Aggregating In Parallel The Permanent Default (#13407) * make it the permanent default * gaz --- .../operations/attestations/BUILD.bazel | 1 - .../operations/attestations/kv/BUILD.bazel | 2 -- .../operations/attestations/kv/aggregated.go | 26 +------------------ .../attestations/kv/aggregated_test.go | 6 ----- .../attestations/prepare_forkchoice_test.go | 6 ----- config/features/config.go | 7 ----- config/features/deprecated_flags.go | 6 +++++ config/features/flags.go | 6 ----- 8 files changed, 7 insertions(+), 53 deletions(-) diff --git a/beacon-chain/operations/attestations/BUILD.bazel b/beacon-chain/operations/attestations/BUILD.bazel index 25eb6af0f6..4f85675135 100644 --- a/beacon-chain/operations/attestations/BUILD.bazel +++ b/beacon-chain/operations/attestations/BUILD.bazel @@ -47,7 +47,6 @@ go_test( deps = [ "//async:go_default_library", "//beacon-chain/operations/attestations/kv:go_default_library", - "//config/features:go_default_library", "//config/fieldparams:go_default_library", "//config/params:go_default_library", "//crypto/bls:go_default_library", diff --git a/beacon-chain/operations/attestations/kv/BUILD.bazel b/beacon-chain/operations/attestations/kv/BUILD.bazel index a787ebd6c7..aab6e08232 100644 --- a/beacon-chain/operations/attestations/kv/BUILD.bazel +++ b/beacon-chain/operations/attestations/kv/BUILD.bazel @@ -14,7 +14,6 @@ go_library( visibility = ["//beacon-chain:__subpackages__"], deps = [ "//beacon-chain/core/helpers:go_default_library", - "//config/features:go_default_library", "//config/params:go_default_library", "//consensus-types/primitives:go_default_library", "//crypto/hash:go_default_library", @@ -39,7 +38,6 @@ go_test( ], embed = [":go_default_library"], deps = [ - "//config/features:go_default_library", "//config/fieldparams:go_default_library", "//crypto/bls:go_default_library", "//proto/prysm/v1alpha1:go_default_library", diff --git a/beacon-chain/operations/attestations/kv/aggregated.go b/beacon-chain/operations/attestations/kv/aggregated.go index a322b37dd3..97817b246b 100644 --- a/beacon-chain/operations/attestations/kv/aggregated.go +++ b/beacon-chain/operations/attestations/kv/aggregated.go @@ -7,7 +7,6 @@ import ( "github.com/pkg/errors" "github.com/prysmaticlabs/prysm/v4/beacon-chain/core/helpers" - "github.com/prysmaticlabs/prysm/v4/config/features" "github.com/prysmaticlabs/prysm/v4/consensus-types/primitives" ethpb "github.com/prysmaticlabs/prysm/v4/proto/prysm/v1alpha1" attaggregation "github.com/prysmaticlabs/prysm/v4/proto/prysm/v1alpha1/attestation/aggregation/attestations" @@ -46,30 +45,7 @@ func (c *AttCaches) aggregateUnaggregatedAtts(ctx context.Context, unaggregatedA // Track the unaggregated attestations that aren't able to aggregate. leftOverUnaggregatedAtt := make(map[[32]byte]bool) - if features.Get().AggregateParallel { - leftOverUnaggregatedAtt = c.aggregateParallel(attsByDataRoot, leftOverUnaggregatedAtt) - } else { - for _, atts := range attsByDataRoot { - aggregated, err := attaggregation.AggregateDisjointOneBitAtts(atts) - if err != nil { - return errors.Wrap(err, "could not aggregate unaggregated attestations") - } - if aggregated == nil { - return errors.New("could not aggregate unaggregated attestations") - } - if helpers.IsAggregated(aggregated) { - if err := c.SaveAggregatedAttestations([]*ethpb.Attestation{aggregated}); err != nil { - return err - } - } else { - h, err := hashFn(aggregated) - if err != nil { - return err - } - leftOverUnaggregatedAtt[h] = true - } - } - } + leftOverUnaggregatedAtt = c.aggregateParallel(attsByDataRoot, leftOverUnaggregatedAtt) // Remove the unaggregated attestations from the pool that were successfully aggregated. for _, att := range unaggregatedAtts { diff --git a/beacon-chain/operations/attestations/kv/aggregated_test.go b/beacon-chain/operations/attestations/kv/aggregated_test.go index b83f1303a1..e6ce172e47 100644 --- a/beacon-chain/operations/attestations/kv/aggregated_test.go +++ b/beacon-chain/operations/attestations/kv/aggregated_test.go @@ -9,7 +9,6 @@ import ( "github.com/pkg/errors" fssz "github.com/prysmaticlabs/fastssz" "github.com/prysmaticlabs/go-bitfield" - "github.com/prysmaticlabs/prysm/v4/config/features" "github.com/prysmaticlabs/prysm/v4/crypto/bls" ethpb "github.com/prysmaticlabs/prysm/v4/proto/prysm/v1alpha1" "github.com/prysmaticlabs/prysm/v4/testing/assert" @@ -18,11 +17,6 @@ import ( ) func TestKV_Aggregated_AggregateUnaggregatedAttestations(t *testing.T) { - resetFn := features.InitWithReset(&features.Flags{ - AggregateParallel: true, - }) - defer resetFn() - cache := NewAttCaches() priv, err := bls.RandKey() require.NoError(t, err) diff --git a/beacon-chain/operations/attestations/prepare_forkchoice_test.go b/beacon-chain/operations/attestations/prepare_forkchoice_test.go index 174947fc88..eef9df5b43 100644 --- a/beacon-chain/operations/attestations/prepare_forkchoice_test.go +++ b/beacon-chain/operations/attestations/prepare_forkchoice_test.go @@ -7,7 +7,6 @@ import ( "testing" "github.com/prysmaticlabs/go-bitfield" - "github.com/prysmaticlabs/prysm/v4/config/features" "github.com/prysmaticlabs/prysm/v4/crypto/bls" ethpb "github.com/prysmaticlabs/prysm/v4/proto/prysm/v1alpha1" attaggregation "github.com/prysmaticlabs/prysm/v4/proto/prysm/v1alpha1/attestation/aggregation/attestations" @@ -18,11 +17,6 @@ import ( ) func TestBatchAttestations_Multiple(t *testing.T) { - resetFn := features.InitWithReset(&features.Flags{ - AggregateParallel: true, - }) - defer resetFn() - s, err := NewService(context.Background(), &Config{Pool: NewPool()}) require.NoError(t, err) diff --git a/config/features/config.go b/config/features/config.go index 79d62ed5ab..1eef337901 100644 --- a/config/features/config.go +++ b/config/features/config.go @@ -68,8 +68,6 @@ type Flags struct { PrepareAllPayloads bool // PrepareAllPayloads informs the engine to prepare a block on every slot. - AggregateParallel bool // AggregateParallel aggregates attestations in parallel. - // KeystoreImportDebounceInterval specifies the time duration the validator waits to reload new keys if they have // changed on disk. This feature is for advanced use cases only. KeystoreImportDebounceInterval time.Duration @@ -231,11 +229,6 @@ func ConfigureBeaconChain(ctx *cli.Context) error { logEnabled(prepareAllPayloads) cfg.PrepareAllPayloads = true } - cfg.AggregateParallel = true - if ctx.IsSet(disableAggregateParallel.Name) { - logEnabled(disableAggregateParallel) - cfg.AggregateParallel = false - } if ctx.IsSet(disableResourceManager.Name) { logEnabled(disableResourceManager) cfg.DisableResourceManager = true diff --git a/config/features/deprecated_flags.go b/config/features/deprecated_flags.go index cabffec7f5..6e7a9db990 100644 --- a/config/features/deprecated_flags.go +++ b/config/features/deprecated_flags.go @@ -63,6 +63,11 @@ var ( Usage: deprecatedUsage, Hidden: true, } + deprecatedDisableAggregateParallel = &cli.BoolFlag{ + Name: "disable-aggregate-parallel", + Usage: deprecatedUsage, + Hidden: true, + } ) // Deprecated flags for both the beacon node and validator client. @@ -78,6 +83,7 @@ var deprecatedFlags = []cli.Flag{ deprecatedDisableBuildBlockParallel, deprecatedDisableReorgLateBlocks, deprecatedDisableOptionalEngineMethods, + deprecatedDisableAggregateParallel, } // deprecatedBeaconFlags contains flags that are still used by other components diff --git a/config/features/flags.go b/config/features/flags.go index c7814e8a3e..09c3bdbf3c 100644 --- a/config/features/flags.go +++ b/config/features/flags.go @@ -150,11 +150,6 @@ var ( Name: "disable-registration-cache", Usage: "Temporary flag for disabling the validator registration cache instead of using the DB. Note: registrations do not clear on restart while using the DB.", } - - disableAggregateParallel = &cli.BoolFlag{ - Name: "disable-aggregate-parallel", - Usage: "Disables parallel aggregation of attestations.", - } ) // devModeFlags holds list of flags that are set when development mode is on. @@ -209,7 +204,6 @@ var BeaconChainFlags = append(deprecatedBeaconFlags, append(deprecatedFlags, []c EnableEIP4881, disableResourceManager, DisableRegistrationCache, - disableAggregateParallel, }...)...) // E2EBeaconChainFlags contains a list of the beacon chain feature flags to be tested in E2E.