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

Implement consensus context optimisation #2371

Closed
michaelsproul opened this issue May 27, 2021 · 2 comments
Closed

Implement consensus context optimisation #2371

michaelsproul opened this issue May 27, 2021 · 2 comments
Assignees
Labels
A1 consensus An issue/PR that touches consensus code, such as state_processing or block verification. optimization Something to make Lighthouse run more efficiently. RFC Request for comment

Comments

@michaelsproul
Copy link
Member

Description

Throughout state processing, we quite often make use of a pattern where we pass in a pre-computed value in order to avoid recomputing it, e.g. the state_root passed to per_slot_processing or the total_active_balance passed to various rewards processing functions. There are more opportunities for such optimisations, including the proposer index (#598) and other properties of the validator set (unslashed_participating_indices for the 3 different flags).

My idea for neatly generalising this pattern and enabling more of these optimisations is to add a single context struct to contain these values, and pass it around between all relevant state transition functions:

pub struct ConsensusContext<T: EthSpec> {
    /// Slot to act as an identifier/safeguard
    slot: Slot,
    state_root: Option<Hash256>,
    proposer_index: Option<u64>,
    ...
}

The signature of per_slot_processing would change as follows:

pub fn per_slot_processing<T: EthSpec>(
    state: &mut BeaconState<T>,
    context: &mut ConsensusContext<T>,
    spec: &ChainSpec,
) -> Result<Option<EpochProcessingSummary>, Error>;

The context would have constructors/builders for making a context with 0 or more pre-computed values. If you have the state_root handy, you feed it in, or if you have the proposer index from the BeaconChain's proposer index cache, you supply that.

let mut ctxt = ConsensusContext::new(slot)
    .set_state_root(state_root)
    .set_proposer_index(proposer_index);

Then in the consensus code when these values are required they are fetched via memoising accessors that either a) use the cached value if it exists, or b) compute the value from scratch and add it to the context for future use.

impl<T: EthSpec> ConsensusContext<T> {
    pub fn get_beacon_proposer_index(
        &mut self,
        state: &BeaconState<T>,
        spec: &ChainSpec
    ) -> Result<usize, Error> {
        if let Some(proposer_index) = self.proposer_index {
            return Ok(proposer_index);
        }
        let proposer_index = state.get_beacon_proposer_index(self.slot, spec)?;
        self.proposer_index = Some(proposer_index)
        Ok(proposer_index)
    }
}

Alternatives

  • As an alternative we could instead add these optional memoising caches to the BeaconState, but I think that would be messier, and would introduce cache invalidation bugs that don't exist with an ephemeral cache like the ConsensusContext
  • Plumb parameters around manually. Also messier, introduces friction for new optimisations, no memoisation logic.
@michaelsproul michaelsproul added RFC Request for comment t Consensus & Verification A1 optimization Something to make Lighthouse run more efficiently. labels May 27, 2021
@paulhauner
Copy link
Member

I've just come across this again after forgetting about it.

There's some optimizations in #2416 which cover unslashed_participating_indices and total balances, but only for Altair. That implements a ParticipationCache which would be a good candidate to include in ConsensusContext.

@michaelsproul
Copy link
Member Author

I've implemented this while working on #2806

bors bot pushed a commit that referenced this issue Oct 15, 2022
## Issue Addressed

Closes #2371

## Proposed Changes

Backport some changes from `tree-states` that remove duplicated calculations of the `proposer_index`.

With this change the proposer index should be calculated only once for each block, and then plumbed through to every place it is required.

## Additional Info

In future I hope to add more data to the consensus context that is cached on a per-epoch basis, like the effective balances of validators and the base rewards.

There are some other changes to remove indexing in tests that were also useful for `tree-states` (the `tree-states` types don't implement `Index`).
@michaelsproul michaelsproul added the consensus An issue/PR that touches consensus code, such as state_processing or block verification. label Nov 9, 2022
macladson pushed a commit to macladson/lighthouse that referenced this issue Jan 5, 2023
## Issue Addressed

Closes sigp#2371

## Proposed Changes

Backport some changes from `tree-states` that remove duplicated calculations of the `proposer_index`.

With this change the proposer index should be calculated only once for each block, and then plumbed through to every place it is required.

## Additional Info

In future I hope to add more data to the consensus context that is cached on a per-epoch basis, like the effective balances of validators and the base rewards.

There are some other changes to remove indexing in tests that were also useful for `tree-states` (the `tree-states` types don't implement `Index`).
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
Closes sigp#2371

Backport some changes from `tree-states` that remove duplicated calculations of the `proposer_index`.

With this change the proposer index should be calculated only once for each block, and then plumbed through to every place it is required.

In future I hope to add more data to the consensus context that is cached on a per-epoch basis, like the effective balances of validators and the base rewards.

There are some other changes to remove indexing in tests that were also useful for `tree-states` (the `tree-states` types don't implement `Index`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A1 consensus An issue/PR that touches consensus code, such as state_processing or block verification. optimization Something to make Lighthouse run more efficiently. RFC Request for comment
Projects
None yet
Development

No branches or pull requests

2 participants