Remove EnableEIP4881 flag #13826

saolyn · 2024-03-28T15:44:08Z

What type of PR is this?

Cleanup

What does this PR do? Why is it needed?

The EIP4881 deposit tree is the default and we have deprecated the EnableEIP4881 flag.
This PR removes the old flag and makes the deposit tree the default option.

Which issues(s) does this PR fix?

Part of #11928

Other notes for review

nisdas · 2024-04-19T05:57:58Z

beacon-chain/cache/depositsnapshot/deposit_fetcher.go

+
+// RemovePendingDeposit from the database. The deposit is indexed by the
+// Index. This method does nothing if deposit ptr is nil.
+func (c *Cache) RemovePendingDeposit(ctx context.Context, d *ethpb.Deposit) {


This particular method hasn't been used anywhere for a while, I think its safe to just remove this + all the related tests

nisdas · 2024-04-19T06:02:30Z

beacon-chain/cache/depositsnapshot/deposit_tree.go

+	if err != nil {
+		return [32]byte{}, nil, err
+	}
+	if finalizedDeposits > 0 && i <= finalizedIdx {


Not exactly related to the issue that we resolved but if you provide an invalid index(non-existent in tree) as an argument to this method, this particular method will panic. We should also be asserting that any provided index is less than the deposit count.

nisdas · 2024-04-19T06:10:54Z

config/features/config.go

@@ -242,11 +241,6 @@ func ConfigureBeaconChain(ctx *cli.Context) error {
 		logEnabled(disableResourceManager)
 		cfg.DisableResourceManager = true
 	}
-	cfg.EnableEIP4881 = true
-	if ctx.IsSet(DisableEIP4881.Name) {


You need to remove the DisableEIP4881 and deprecate it too since it won't do anything

nisdas · 2024-04-24T09:19:30Z

beacon-chain/cache/depositsnapshot/deposit_tree.go

@@ -112,7 +112,7 @@ func (d *DepositTree) getProof(index uint64) ([32]byte, [][32]byte, error) {
 	if err != nil {
 		return [32]byte{}, nil, err
 	}
-	if finalizedDeposits > 0 && i <= finalizedIdx {
+	if finalizedDeposits > 0 && i <= finalizedIdx || index >= d.depositCount {


can you do the index check at the very beginning ? Just after we check the deposit count

* Remove EnableEIP4881 flag * Gaz * Fix missing error handler * Remove old tree and fix tests * Gaz * Fix build import * Replace depositcache * Add pendingDeposit tests * Nishant's fix * Fix unsafe uint64 to int * Fix other unsafe uint64 to int * Remove: RemovePendingDeposit * Deprecate and remove DisableEIP4881 flag * Check: index not greater than deposit count * Move index check

@terencechain

* GET * POST * Revert "Auxiliary commit to revert individual files from 615feb1" This reverts commit 55cf071c684019f3d6124179154c10b2277fda49. * comment fix * deepsource config values block protos get_committee_indices proto and ssz attestation interface Revert "Auxiliary commit to revert individual files from deadb21" This reverts commit 32ad5009537bc5ec0e6caf9f52143d380d00be85. todos get_attesting_indices Revert "Auxiliary commit to revert individual files from dd27897" This reverts commit f39644ed3cb6f3964fc6c86fdf4bd5de2a9668c8. beacon spec changes Fix pending attestation. Build ok Don't return error that can be internally handled (#13887) Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com> consistent auth token for validator apis (#13747) * wip * fixing tests * adding more tests especially to handle legacy * fixing linting * fixing deepsource issues and flags * fixing some deepsource issues,pathing issues, and logs * some review items * adding additional review feedback * updating to follow updates from ethereum/keymanager-APIs#74 * adjusting functions to match changes in keymanagers PR * Update validator/rpc/auth_token.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update validator/rpc/auth_token.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * Update validator/rpc/auth_token.go Co-authored-by: Radosław Kapka <rkapka@wp.pl> * review feedback --------- Co-authored-by: Radosław Kapka <rkapka@wp.pl> Use correct port for health check in Beacon API e2e evaluator (#13892) Change example.org DNS record (#13904) DNS Changed for this record causing tests to fail Co-authored-by: Radosław Kapka <rkapka@wp.pl> Electra beacon config (#13907) * Update spectests to v1.5.0 * Add electra config * Fix tests in beacon-chain/rpc/eth/config * gofmt Simplify prune invalid by reusing existing fork choice store call (#13878) Do not remove blobs DB in slasher. (#13881) Refactor beacon-chain/core/helpers tests to be black box (#13906) spectests: fail hard on missing test folders (#13913) Revert "zig: Update zig to recent main branch commit (#13142)" (#13908) This reverts commit b24b60d. Remove EnableEIP4881 flag (#13826) * Remove EnableEIP4881 flag * Gaz * Fix missing error handler * Remove old tree and fix tests * Gaz * Fix build import * Replace depositcache * Add pendingDeposit tests * Nishant's fix * Fix unsafe uint64 to int * Fix other unsafe uint64 to int * Remove: RemovePendingDeposit * Deprecate and remove DisableEIP4881 flag * Check: index not greater than deposit count * Move index check Protobufs for Electra devnet-0 (#13905) * block protos * proto and ssz * stubs * Enable Electra spec test * Pull in EIP-7251 protobuf changes From: #13903 * All EIP7549 containers are passing * All EIP7251 containers passing * including changes from eip7002 * Everything passing except for beacon state hash tree root * fixing eletra state to use electra payload * Fix minimal test. Skip beacon state test * Perston's feedback --------- Co-authored-by: rkapka <radoslaw.kapka@gmail.com> Co-authored-by: james-prysm <james@prysmaticlabs.com> use [32]byte keys in the filesystem cache (#13885) Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com> Electra: full beacon config (#13918) * Electra: full beacon config Fix TestGetSpec * Fix beacon config spec compliance test so that it properly loads the config from spec tests. Tests failing for now. * fix tests and comply with spec presets beacon-chain/cache: Convert tests to cache_test blackbox testing (#13920) * beacon-chain/cache: convert to blackbox tests (package cache_test) * Move balanceCacheKey to its own file to satisify go fuzz build Electra: Minor proto changes, cloner additions (#13923) * Electra: more proto changes * Roundtrip test for cloners Electra: HTR util for DepositReceipt and ExecutionLayerWithdrawalRequest (#13924) * Electra: HTR utils for DepositReceipts and ExecutionLayerWithdrawalRequests * Tests for HTR utils Electra: beacon-chain/core/helpers (#13921) * Electra helpers * Electra helper tests and other fixes * @terencechain feedback Electra: add electra version Electra: consensus types gocognit exclusion @potuz's suggestion build fix interfaces for indexed att and slashing indexed att usage BuildSignedBeaconBlockFromExecutionPayload slashing usage grpc stubs remove unused methods Electra attestation interfaces

* Remove EnableEIP4881 flag * Gaz * Fix missing error handler * Remove old tree and fix tests * Gaz * Fix build import * Replace depositcache * Add pendingDeposit tests * Nishant's fix * Fix unsafe uint64 to int * Fix other unsafe uint64 to int * Remove: RemovePendingDeposit * Deprecate and remove DisableEIP4881 flag * Check: index not greater than deposit count * Move index check

* Remove EnableEIP4881 flag * Gaz * Fix missing error handler * Remove old tree and fix tests * Gaz * Fix build import * Replace depositcache * Add pendingDeposit tests * Nishant's fix * Fix unsafe uint64 to int * Fix other unsafe uint64 to int * Remove: RemovePendingDeposit * Deprecate and remove DisableEIP4881 flag * Check: index not greater than deposit count * Move index check (cherry picked from commit c8d6f47)

* Remove EnableEIP4881 flag * Gaz * Fix missing error handler * Remove old tree and fix tests * Gaz * Fix build import * Replace depositcache * Add pendingDeposit tests * Nishant's fix * Fix unsafe uint64 to int * Fix other unsafe uint64 to int * Remove: RemovePendingDeposit * Deprecate and remove DisableEIP4881 flag * Check: index not greater than deposit count * Move index check

saolyn and others added 4 commits March 28, 2024 13:18

Remove EnableEIP4881 flag

7667eee

Gaz

1fe248e

Fix missing error handler

8c14ee5

Merge branch 'develop' into cleanup-eip4881

5f8ae09

saolyn marked this pull request as ready for review April 3, 2024 13:48

saolyn requested a review from a team as a code owner April 3, 2024 13:48

saolyn requested review from nalepae, prestonvanloon and nisdas April 3, 2024 13:48

saolyn added 2 commits April 3, 2024 16:50

Remove old tree and fix tests

e9a9241

Gaz

ac35abe

saolyn force-pushed the cleanup-eip4881 branch from bacd02a to ac35abe Compare April 3, 2024 14:57

Fix build import

620ed8b

saolyn force-pushed the cleanup-eip4881 branch from b20fc39 to 620ed8b Compare April 3, 2024 15:02

saolyn and others added 6 commits April 3, 2024 17:06

Replace depositcache

9de364d

Add pendingDeposit tests

678652d

Nishant's fix

8c1caef

Fix unsafe uint64 to int

d16d0f9

Fix other unsafe uint64 to int

a424a0c

Merge branch 'develop' into cleanup-eip4881

3843fea

prestonvanloon previously approved these changes Apr 18, 2024

View reviewed changes

nisdas reviewed Apr 19, 2024

View reviewed changes

saolyn added 3 commits April 19, 2024 14:55

Remove: RemovePendingDeposit

8bc6be6

Deprecate and remove DisableEIP4881 flag

21e5781

Check: index not greater than deposit count

9f6930a

saolyn dismissed prestonvanloon’s stale review via 9f6930a April 19, 2024 13:05

saolyn added 2 commits April 19, 2024 15:05

Merge branch 'develop' into cleanup-eip4881

5b39511

Merge branch 'develop' into cleanup-eip4881

3244e9f

nisdas reviewed Apr 24, 2024

View reviewed changes

Move index check

661882b

nisdas approved these changes Apr 24, 2024

View reviewed changes

nisdas added this pull request to the merge queue Apr 24, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 24, 2024

nisdas added this pull request to the merge queue Apr 24, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 24, 2024

nisdas added this pull request to the merge queue Apr 24, 2024

Merged via the queue into develop with commit c8d6f47 Apr 24, 2024
16 of 17 checks passed

nisdas deleted the cleanup-eip4881 branch April 24, 2024 14:03

prestonvanloon mentioned this pull request Apr 29, 2024

Spectest: v1.5.0-alpha.1 #13934

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove EnableEIP4881 flag #13826

Remove EnableEIP4881 flag #13826

saolyn commented Mar 28, 2024 •

edited

Loading

nisdas Apr 19, 2024

nisdas Apr 19, 2024

nisdas Apr 19, 2024

nisdas Apr 24, 2024

Remove EnableEIP4881 flag #13826

Remove EnableEIP4881 flag #13826

Conversation

saolyn commented Mar 28, 2024 • edited Loading

nisdas Apr 19, 2024

Choose a reason for hiding this comment

nisdas Apr 19, 2024

Choose a reason for hiding this comment

nisdas Apr 19, 2024

Choose a reason for hiding this comment

nisdas Apr 24, 2024

Choose a reason for hiding this comment

saolyn commented Mar 28, 2024 •

edited

Loading