lodestar icon indicating copy to clipboard operation
lodestar copied to clipboard

feat: implement EIP-6110

Open ensi321 opened this issue 1 year ago • 10 comments

Motivation

To implement EIP-6110 and to modify the existed codebase to accomodate EIP-6110 including:

  • Introduce unfinalized pubkey2Index in EpochCache that is fork specific and will be cloned on state.clone()
  • To modify existed pubkey2Index and index2Pubkey in EpochCache such that they only contain validators that are confirmed active, in the activation queue, or initialized but pending entering activation queue.

Description

Implementation

PubkeyCache

  • Introduce class UnfinalizedPubkeyIndexMap which is an alias of immutable.Map
  • Existed pubkey2Index and index2Pubkey are renamed to finalizedPubkey2index and finalizedIndex2pubkey.
  • Add new field unfinalizedPubkey2Index: UnfinalizedPubkeyIndexMap in the EpochCache and will be included during state cloning
  • Populating finalizedPubkey2index and finalizedIndex2pubkey happens during afterProcessEpoch(). At the same time, UnfinalizedPubkeyIndexMap is pruned for every pubkeys that are inserted into finalized caches.
  • Introduce interfaces to hide finalized and unfinalized caches from callers. Callers to the cache should not interact with finalized and unfinalized cache directly
  • Fork guard to ensure new pubkey cache logic does not get activated before eip6110
  • Add immutabe-js to the dependencies

EIP-6110

New

  • Boilerplate code to add new fork eip6110
  • Update execution engine to use new api -engine_newPayloadV6110 -engine_getPayloadV6110
  • Add upgradeStateToEIP6110
  • Add/update related containers
  • Introduce new constants
    • MAX_DEPOSIT_RECEIPTS_PER_PAYLOAD
    • UNSET_DEPOSIT_RECEIPTS_START_INDEX
  • Add processDepositReceipt() into processOperation()

Modify

  • Update current logic for eth1 deposit by introducing getEth1DepositCount()
  • Extract applyDeposit() logic out of processDeposit()

Testing

  • Add 6110 spec test
  • Add 6110 sim test
  • Add besu and besudocker to EL interop
  • Unit test for getEth1DepositCount()
  • Unit test for state cloning using new pubkey cache
  • Perf test on moving pubkeys from unfinalized to finalized cache on new finalized checkpoint
  • Memory test on unfinalized pubkey cache

Closes #5366

ensi321 avatar Oct 17 '23 11:10 ensi321

Open Point of Discussion There exists an identical PR opened in my personal repo, and it has some old discussions/concerns. I will migrate all points of discussions that are still open to here and will close the old PR imminently. Old PR for reference only: https://github.com/naviechan/lodestar/pull/1

ensi321 avatar Oct 17 '23 12:10 ensi321

My debate for the future of the PR is to merge to unstable or not. Lodestar has historically been eager to merge future fork code into unstable, ensuring that it has absolutely no impact on current forks. This PR implements a feature that still a bit far away but that I'm confident that we have to develop at some point. The arguments are:

  • Merge to unstable: ensures the PR / branch never goes stale, with spec tests we will be always ready for when eventual testing for the PR happen. If the feature has some changes we can do those on unstable
  • Not merge to unstable: 0% chance of disruption to current forks, but the PR / branch is very likely to go stale. If we have some deep structural change between now and a few months it could require to re-write some big parts

I would prefer to merge to unstable, being very confident that we will not have to pay any price in terms of stability of performance to current forks.

ensi321 avatar Oct 17 '23 12:10 ensi321

If we can safely merge to unstable with some good stability guarantees, that's what I would prefer. I think EIP-6110 is mainly in an implementation + testing phase now, so hopefully no large changes. Main concern is if we're doing large changes to Lodestar in the medium-term which is likely.

philknows avatar Oct 17 '23 22:10 philknows

To make this PR easier to review, I have reverted some of the trivial changes to reduce the diffs. Let me know if there is anything else we could do to make this easier🙂

This includes:

  • Rename epoch cache's pubkey2index and index2pubkey to finalizedPubkey2Index and finalizedIndex2Pubkey
  • Restrict callers (esp those from beacon api) from getting direct access to finalizedPubkey2Index and finalizedIndex2Pubkey but to use getPubkey(), getValidatorIndex().
  • Add unfinished unit test for 6110 light client header

Will create a follow-up PR to re-introduce these changes after we sort out all controversial changes in this PR cc. @dapplion @g11tech @tuyennhv

ensi321 avatar Oct 23 '23 11:10 ensi321

Posting the perf test result on updateUnfinalizedPubkeys ie. removing newly finalized pubkeys from unfinalizedPubkey2index and add them to pubkey2Index + index2Pubkey upon new finalized checkpoint. Open to discuss whether the benchmark is acceptable or further optimization should be made. cc. @dapplion @tuyennhv

Background

  • Test is written in updateUnfinalizedPubkeys.test.ts which emulates the cache behaviour in chain. onForkChoiceFinalized()
  • updateUnfinalizedPubkeys should be performed on average ~1 time per epoch. Whenever a new finalized checkpoint is set in ForkChoiceStore post 6110.
  • In post 6110, theoretically newly finalized pubkeys per epoch could reach maximum of 1271 * 32 = 40672. This happens when an epoch full of 30M gas blocks, each deposit in the blocks creates a new validator, and at least 40664 of them fail to reach activation balance. Practically speaking, 1000 pubkeys is already on the higher end depending on the amount of network activities.

Result

Benchmark date from Mon Nov 21 2023 - Intel Core i7-9750H @ 2.60Ghz ✔ updateUnfinalizedPubkeys - updating 10 pubkeys 1496.612 ops/s 668.1760 us/op - 276 runs 3.39 s ✔ updateUnfinalizedPubkeys - updating 100 pubkeys 174.9926 ops/s 5.714528 ms/op - 142 runs 2.19 s ✔ updateUnfinalizedPubkeys - updating 1000 pubkeys 10.17848 ops/s 98.24650 ms/op - 28 runs 3.75 s

Benchmark date from Fri Nov 24 2023 - Apple M2 ✔ updateUnfinalizedPubkeys - updating 10 pubkeys 2878.294 ops/s 347.4280 us/op - 389 runs 2.35 s ✔ updateUnfinalizedPubkeys - updating 100 pubkeys 296.2824 ops/s 3.375158 ms/op - 250 runs 1.79 s ✔ updateUnfinalizedPubkeys - updating 1000 pubkeys 20.07801 ops/s 49.80572 ms/op - 13 runs 1.18 s ✔ updateUnfinalizedPubkeys - updating 40672 pubkeys 0.4405013 ops/s 2.270141 s/op - 13 runs 32.1 s

ensi321 avatar Nov 24 '23 09:11 ensi321

Also want to share the amount of memory used by unfinalizedPubkeyCache aka immutable.js Map when holding [1000, 10000, 100000] pubkeys which I believe is ~80% less than the vanilla map and it scales linearly:

UnfinalizedPubkey2Index 1000 keys - 274956.5 bytes / instance UnfinalizedPubkey2Index 10000 keys - 2591129.3 bytes / instance UnfinalizedPubkey2Index 100000 keys - 27261443.4 bytes / instance

ensi321 avatar Nov 24 '23 09:11 ensi321

Instead you can add a new cache into the state tracking historical validator length counts. On every epoch transition push the current count of validators. Once a state finalizes, retrieve count at index state.epoch - finalized_checkpoint.epoch. Then you don't need to retrieve the finalized state for this purpose.

@dapplion Can you please confirm if #006c26b suits what you are asking before I polish up and test the code?

I believe we only need to maintain historicalValidatorLengths from the latest finalized cp epoch to the current epoch.

Also headState is not a reliable indicator to stop the eth1Data polling so finalizedState is still needed. But I think it shouldn’t be a big deal to stop the polling late if we fail to retrieve finalizedState couple times in a row.

ensi321 avatar Dec 12 '23 13:12 ensi321

@dapplion Can you please confirm if #006c26b suits what you are asking before I polish up and test the code?

Looks good! continue

dapplion avatar Dec 12 '23 13:12 dapplion

Codecov Report

Merging #6042 (2f10313) into electra-fork (9014c8c) will increase coverage by 0.05%. The diff coverage is 65.84%.

Additional details and impacted files
@@               Coverage Diff                @@
##           electra-fork    #6042      +/-   ##
================================================
+ Coverage         61.88%   61.94%   +0.05%     
================================================
  Files               556      559       +3     
  Lines             58108    58740     +632     
  Branches           1830     1844      +14     
================================================
+ Hits              35962    36386     +424     
- Misses            22109    22317     +208     
  Partials             37       37              

codecov[bot] avatar Dec 15 '23 07:12 codecov[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Dec 22 '23 06:12 CLAassistant