Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: rename caches and related functions to indicate finalized #6528

Closed
wants to merge 9 commits into from

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented Mar 12, 2024

Motivation

EIP 6110 breaks the assumptions that validator indices are always finalized, and thus introduces pubkey caches for unfinalized validators (unfinalizedPubkey2index and unfinalizedIndex2pubkey).

As such, the original pubkey caches also need to update their naming to indicate they only store pubkeys and indices for finalized validators.

Description

Rename the following:

  • epochCache.pubkey2index -> epochCache.finalizedPubkey2index
  • epochCache.index2pubkey -> epochCache.finalizedIndex2pubkey

cc. @g11tech @tuyennhv

Related issue and PR: #6341 #6042

@ensi321 ensi321 marked this pull request as ready for review March 12, 2024 07:53
@ensi321 ensi321 requested a review from a team as a code owner March 12, 2024 07:53
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

❗ No coverage uploaded for pull request base (electra-fork@5808ddb). Click here to learn what that means.
The diff coverage is 65.75%.

❗ Current head f631fd9 differs from pull request most recent head 7dda166. Consider uploading reports for the commit 7dda166 to get more accurate results

Additional details and impacted files
@@               Coverage Diff               @@
##             electra-fork    #6528   +/-   ##
===============================================
  Coverage                ?   61.97%           
===============================================
  Files                   ?      562           
  Lines                   ?    59854           
  Branches                ?     1905           
===============================================
  Hits                    ?    37097           
  Misses                  ?    22716           
  Partials                ?       41           

@ensi321
Copy link
Contributor Author

ensi321 commented Mar 13, 2024

Ignore the spec test failure for now as the consensus-spec is in the progress of migrating 6110 spec test to electra

@ensi321 ensi321 mentioned this pull request Mar 14, 2024
12 tasks
@ensi321 ensi321 force-pushed the nc/6110 branch 2 times, most recently from c8ca410 to 1227f52 Compare March 18, 2024 13:36
@twoeths
Copy link
Contributor

twoeths commented Mar 25, 2024

since this change is quite straightforward, should we merge this PR to unstable instead? then electra-fork should be rebased from unstable, or merge it

@@ -5,7 +5,7 @@ import {
CachedBeaconStateAllForks,
computeEpochAtSlot,
computeStartSlotAtEpoch,
createCachedBeaconState,
createFinalizedCachedBeaconState,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we want this to be renamed to finalized because it has other connotations other than having a finalized pubkey maps

*/
export function loadCachedBeaconState<T extends BeaconStateAllForks & BeaconStateCache>(
export function loadUnfinalizedCachedBeaconState<T extends BeaconStateAllForks & BeaconStateCache>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should stay the original name since the state might not have to do anything with finalized/unfinalized pubkey indices

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should stay the original name since the state might not have to do anything with finalized/unfinalized pubkey indices

I see. Yea I agree the unfinalized here is unrelated but rather it's part of the persistent state cache. Will rename it in the next PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ensi321 what differences between loading a finalized state vs unfinalized state?

Copy link
Contributor Author

@ensi321 ensi321 Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ensi321 what differences between loading a finalized state vs unfinalized state?

@tuyennhv In the context of 6110? It probably doesn't make a difference.

TODO: rename to loadUnfinalizedCachedBeaconState() due to EIP-6110

When you first added this comment in this PR I thought you knew something that I don't 😅

Copy link
Contributor

@twoeths twoeths Apr 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just review the logic, right now when loading any states we use EpochCache.createFromState which set empty unfinalizedPubkey2index which is not correct for unfinalized states

in this function, we should pass in pivotValidatorIndex to decide unfinalizedPubkey2index, hence the name should be loadUnfinalizedCachedBeaconState

unfinalizedPubkey2index should be computed from modifiedValidators see the other comment #6528 (comment)

@g11tech
Copy link
Contributor

g11tech commented Apr 3, 2024

since this change is quite straightforward, should we merge this PR to unstable instead? then electra-fork should be rebased from unstable, or merge it

once this PR is merged into the electra, i can try cherrypicking out into the unstable

add types stub and epoch config

fix types
* Add immutable in the dependencies

* Initial change to pubkeyCache

* Added todos

* Moved unfinalized cache to epochCache

* Move populating finalized cache to afterProcessEpoch

* Specify unfinalized cache during state cloning

* Move from unfinalized to finalized cache in afterProcessEpoch

* Confused myself

* Clean up

* Change logic

* Fix cloning issue

* Clean up redundant code

* Add CarryoverData in epochCtx.createFromState

* Fix typo

* Update usage of pubkeyCache

* Update pubkeyCache usage

* Fix lint

* Fix lint

* Add 6110 to ChainConfig

* Add 6110 to BeaconPreset

* Define 6110 fork and container

* Add V6110 api to execution engine

* Update test

* Add depositReceiptsRoot to process_execution_payload

* State transitioning to EIP6110

* State transitioning to EIP6110

* Light client change in EIP-6110

* Update tests

* produceBlock

* Refactor processDeposit to match the spec

* Implement processDepositReceipt

* Implement 6110 fork guard for pubkeyCache

* Handle changes in eth1 deposit

* Update eth1 deposit test

* Fix typo

* Lint

* Remove embarassing comments

* Address comments

* Modify applyDeposit signature

* Update packages/state-transition/src/cache/epochCache.ts

Co-authored-by: Lion - dapplion <[email protected]>

* Update packages/state-transition/src/cache/epochCache.ts

Co-authored-by: Lion - dapplion <[email protected]>

* Update packages/state-transition/src/cache/pubkeyCache.ts

Co-authored-by: Lion - dapplion <[email protected]>

* Remove old code

* Rename fields in epochCache and immutableData

* Remove CarryoverData

* Move isAfter6110 from var to method

* Fix cyclic import

* Fix operations spec runner

* Fix for spec test

* Fix spec test

* state.depositReceiptsStartIndex to BigInt

* getDeposit requires cached state

* default depositReceiptsStartIndex value in genesis

* Fix pubkeyCache bug

* newUnfinalizedPubkeyIndexMap in createCachedBeaconState

* Lint

* Pass epochCache instead of pubkey2IndexFn in apis

* Address comments

* Add unit test on pubkey cache cloning

* Add unfinalizedPubkeyCacheSize to metrics

* Add unfinalizedPubkeyCacheSize to metrics

* Clean up code

* Add besu to el-interop

* Add 6110 genesis file

* Template for sim test

* Add unit test for getEth1DepositCount

* Update sim test

* Update besudocker

* Finish beacon api calls in sim test

* Update epochCache.createFromState()

* Fix bug unfinalized validators are not finalized

* Add sim test to run a few blocks

* Lint

* Merge branch 'unstable' into 611

* Add more check to sim test

* Update besu docker image instruction

* Update sim test with correct tx

* Address comment + cleanup

* Clean up code

* Properly handle promise rejection

* Lint

* Update packages/beacon-node/src/execution/engine/types.ts

Co-authored-by: Lion - dapplion <[email protected]>

* Update comments

* Accept type undefined in ExecutionPayloadBodyRpc

* Update comment and semantic

* Remove if statement when adding finalized validator

* Comment on repeated insert on finalized cache

* rename createFromState

* Add comment on getPubkey()

* Stash change to reduce diffs

* Stash change to reduce diffs

* Lint

* addFinalizedPubkey on finalized checkpoint

* Update comment

* Use OrderedMap for unfinalized cache

* Pull out logic of deleting pubkeys for batch op

* Add updateUnfinalizedPubkeys in regen

* Update updateUnfinalizedPubkeys logic

* Add comment

* Add metrics for state context caches

* Address comment

* Address comment

* Deprecate eth1Data polling when condition is reached

* Fix conflicts

* Fix sim test

* Lint

* Fix type

* Fix test

* Fix test

* Lint

* Update packages/light-client/src/spec/utils.ts

Co-authored-by: Lion - dapplion <[email protected]>

* Fix spec test

* Address comments

* Improve cache logic on checkpoint finalized

* Update sim test according to new cache logic

* Update comment

* Lint

* Finalized pubkey cache only update once per checkpoint

* Add perf test for updateUnfinalizedPubkeys

* Add perf test for updateUnfinalizedPubkeys

* Tweak params for perf test

* Freeze besu docker image version for 6110

* Add benchmark result

* Use Map instead of OrderedMap. Update benchmark

* Minor optimization

* Minor optimization

* Add memory test for immutable.js

* Update test

* Reduce code duplication

* Lint

* Remove try/catch in updateUnfinalizedPubkeys

* Introduce EpochCache metric

* Add historicalValidatorLengths

* Polish code

* Migrate state-transition unit tests to vitest

* Fix calculation of pivot index

* `historicalValidatorLengths` only activate post 6110

* Update sim test

* Lint

* Update packages/state-transition/src/cache/epochCache.ts

Co-authored-by: Lion - dapplion <[email protected]>

* Improve readability on historicalValidatorLengths

* Update types

* Fix calculation

* Add eth1data poll todo

* Add epochCache.getValidatorCountAtEpoch

* Add todo

* Add getStateIterator for state cache

* Partial commit

* Update perf test

* updateUnfinalizedPubkeys directly modify states from regen

* Update sim test. Lint

* Add todo

* some improvements and a fix for effectiveBalanceIncrements fork safeness

* rename eip6110 to elctra

* fix electra-interop.test.ts

---------

Co-authored-by: Lion - dapplion <[email protected]>
Co-authored-by: gajinder <[email protected]>

lint and tsc

small cleanup

fix rebase issue
@ensi321
Copy link
Contributor Author

ensi321 commented Apr 15, 2024

@tuyennhv Can you check if 7dda166 makes sense to you. Thanks!

for (const validatorIndex of modifiedValidators) {
const validator = validators.getReadonly(validatorIndex);
const pubkey = validator.pubkey;
state.epochCtx.addUnFinalizedPubkey(validatorIndex, pubkey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw error if validatorIndex < cachedSeedState.epochCtx.finalizedPubkey2index.size
(or if addUnFinalizedPubkey already handle it then it's fine)

@wemeetagain
Copy link
Member

setting this as a draft PR for now

@wemeetagain wemeetagain marked this pull request as draft June 6, 2024 18:08
@ensi321
Copy link
Contributor Author

ensi321 commented Dec 10, 2024

Not relevant anymore

@ensi321 ensi321 closed this Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants