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

Drop beacon_chain pubkey to index map cache #17

Draft
wants to merge 5 commits into
base: unstable
Choose a base branch
from

Conversation

dapplion
Copy link
Owner

Issue Addressed

With current spec, the pubkey to index map could be a global cache. Right now this cache is duplicated in:

  • once in BeaconChain struct
  • one copy per BeaconState instance

This PR drops the copy in the BeaconChain struct, making its consumers use some state instead.

Comment on lines +1147 to +1153
/// Rebase state pubkey_cache on some existing state to prevent re-populating the cache on the
/// first process_deposit call.
fn rebase_caches(&self, state: &mut BeaconState<E>) {
if let Ok((_, first_state)) = self.state_cache.lock().iter().exactly_one() {
state.rebase_caches_on(first_state);
}
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

@michaelsproul is it possible to access the head state here?
(2) Those the state_cache always include the head state?
(3) Is the state_cache guaranteed to never be empty?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, on unstable that state_cache is just for historic states. It would be better named historic_state_cache.

The state_cache on tree-states can guarantee that the finalized state is always present. See:

https://github.com/sigp/lighthouse/blob/928915c71885a2439f8fa3b3a22ef6b39e978339/beacon_node/store/src/state_cache.rs#L35

(the Option is just for first-time initialization)

In summary:

  1. No, but you could get the finalized state on tree-states. I think that would work?
  2. Definitely not on unstable (head is not historic), and probably yes (but not guaranteed) on tree-states.
  3. On unstable, no; on tree-states, yes.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Might be best to postpone this PR for post tree-states then

@@ -1529,7 +1529,8 @@ mod release_tests {
let (harness, _) = sync_contribution_test_state::<MainnetEthSpec>(1).await;

let op_pool = OperationPool::<MainnetEthSpec>::new();
let state = harness.get_current_state();
let mut state = harness.get_current_state();
state.update_pubkey_cache().unwrap();
Copy link
Owner Author

Choose a reason for hiding this comment

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

@michaelsproul could not figure out where the caches are lost. The genesis state in the harness builds the caches but they are lost along the way. Maybe while advancing the blocks?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure, can dig into this if it's still a problem later

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.

2 participants