[WIP] feat: Refactor pubkeyCache into finalized and unfinalized cache #5937
Conversation
|
|
| addPubkey(index: ValidatorIndex, pubkey: Uint8Array): void { | ||
| this.pubkey2index.set(pubkey, index); | ||
| this.index2pubkey[index] = bls.PublicKey.fromBytes(pubkey, CoordType.jacobian); // Optimize for aggregation | ||
| this.unfinalizedPubkey2index.set(toMemoryEfficientHexStr(pubkey), index); |
There was a problem hiding this comment.
This entire feature should be fork gated. If this feature in merged to unstable (best option) it must be developed in a way that before eip6110 activation there is 0 performance penalty both in compute and memory footprint. The simplest path would be to assume that before eip6110 activation all pubkeys being added to this function are finalized
There was a problem hiding this comment.
That makes perfect sense! Will address it
| } | ||
|
|
||
| getValidatorIndex(pubkey: Uint8Array | PubkeyHex): ValidatorIndex | undefined { | ||
| return this.globalPubkey2index.get(pubkey) || this.unfinalizedPubkey2index.get(toMemoryEfficientHexStr(pubkey)); |
There was a problem hiding this comment.
See comment below about feature gate-ing at zero cost
| @@ -838,3 +901,9 @@ export function createEmptyEpochCacheImmutableData( | |||
| index2pubkey: [], | |||
| }; | |||
| } | |||
|
|
|||
| export function createEmptyCarryoverData(): CarryoverData { | |||
There was a problem hiding this comment.
Could you explain in detail the rationale of this new CarryoverData struct?
- How does it differentatiate from the rest of caches and sidecars?
- Why must this be a separate object to existing ones?
In case of doubt it's best to not change APIs to minimize the diff and cognitive load to mantainers
There was a problem hiding this comment.
Could you explain in detail the rationale of this new
CarryoverDatastruct?
- How does it differentatiate from the rest of caches and sidecars?
- Why must this be a separate object to existing ones?
In case of doubt it's best to not change APIs to minimize the diff and cognitive load to mantainers
With the new unfinalized cache introduced in EpochCache, I was thinking if 1) EpochCache.createFromState() should allow callers passing in an existed unfinalized cache from other state or should 2) createFromState() create an empty unfinalized cache every time.
I checked every caller of createFromState(), and it seems like we are only creating EpochCache from a “fresh” state ie. no unfinalized cache. However we cannot guarantee if future development will demand creating EpochCache from a CachedBeaconState. It wouldn’t make sense to have a new instance of EpochCache with empty unfinalized cache in this case.
I leaned towards the second option. Instead of having the populating logic specifically for unfinalized cache, I think it makes more sense to have a generic container CarryoverData that contains any data including unfinalizedPubkey2index that need to be populated during createFromState. It is almost identical to EpochCacheImmutableData, in fact it would be perfectly fine if we put unfinalizedPubkey2index in EpochCacheImmutableData but this would defeat the purpose as it is unfinalized and mutable hence a separate container.
I am actually open for 1) because we can always make the additional change later on when we encounter the need for it.
There was a problem hiding this comment.
Got it. You should mantain the same assumptions of the EpochCache design, otherwise it feels like scope creep. I would prefer to discuss changes on the EpochCache usage assumptions in another thread (issue).
|
Glad to see this feature moving! Since there won't be spec tests for this sorts of caches soon it would be very valuable to introduce tests to assert the invariants of the cache. Getting it wrong would cause consensus bugs, so best to start investing in testing infra early |
ensi321
changed the title
|
Closing the PR as this feature will be developed in a dedicated 6110 branch |
Motivation
To refactor the current
pubkeyCacheby:pubkey2IndexinEpochCachethat is fork specific and will be cloned on state.clone()pubkey2Indexandindex2PubkeyinEpochCachesuch that they only contain validators that are confirmed active or in the activation queue.This is a prerequisite to fixing #5366 .
Description
UnfinalizedPubkeyIndexMapwhich is an alias of immutable.Mappubkey2Indexandindex2Pubkeyare renamed toglobalPubkey2indexandglobalIndex2pubkey.unfinalizedPubkey2Index: UnfinalizedPubkeyIndexMapin theEpochCacheand will be included during state cloningglobalPubkey2indexandglobalIndex2pubkeyhappens duringafterProcessEpoch(). At the same time,UnfinalizedPubkeyIndexMapis pruned for every pubkeys that are inserted into global caches.CarryoverDatato specifically allow caller to pass unfinalized cache intoEpochCache.createFromState()if desired