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.

Sign up for GitHub

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

feat: prune BlsToExecutionChange opPool with head state #6252

Merged
merged 2 commits into from
Jan 9, 2024
Merged

feat: prune BlsToExecutionChange opPool with head state #6252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11012b8
feat: prune BlsToExecutionChange opPool with head state
twoeths Jan 5, 2024
22801c7
fix: address PR comment
twoeths Jan 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions packages/beacon-node/src/chain/chain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,15 +904,20 @@ export class BeaconChain implements IBeaconChain {
this.logger.verbose("Fork choice justified", {epoch: cp.epoch, root: cp.rootHex});
}

private onForkChoiceFinalized(this: BeaconChain, cp: CheckpointWithHex): void {
private async onForkChoiceFinalized(this: BeaconChain, cp: CheckpointWithHex): Promise<void> {
this.logger.verbose("Fork choice finalized", {epoch: cp.epoch, root: cp.rootHex});
this.seenBlockProposers.prune(computeStartSlotAtEpoch(cp.epoch));

// TODO: Improve using regen here
const headState = this.regen.getStateSync(this.forkChoice.getHead().stateRoot);
const finalizedState = this.regen.getCheckpointStateSync(cp);
const {blockRoot, stateRoot, slot} = this.forkChoice.getHead();
const headState = this.regen.getStateSync(stateRoot);
const headBlock = await this.db.block.get(fromHexString(blockRoot));
if (headBlock == null) {
throw Error(`Head block ${slot} ${headBlock} is not available in database`);
}

if (headState) {
this.opPool.pruneAll(headState, finalizedState);
this.opPool.pruneAll(headBlock, headState);
}
}

Expand Down
34 changes: 22 additions & 12 deletions packages/beacon-node/src/chain/opPools/opPool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ import {
MAX_BLS_TO_EXECUTION_CHANGES,
BLS_WITHDRAWAL_PREFIX,
MAX_ATTESTER_SLASHINGS,
ForkSeq,
} from "@lodestar/params";
import {Epoch, phase0, capella, ssz, ValidatorIndex} from "@lodestar/types";
import {Epoch, phase0, capella, ssz, ValidatorIndex, allForks} from "@lodestar/types";
import {IBeaconDb} from "../../db/index.js";
import {SignedBLSToExecutionChangeVersioned} from "../../util/types.js";
import {BlockType} from "../interface.js";
Expand Down Expand Up @@ -300,11 +301,11 @@ export class OpPool {
/**
* Prune all types of transactions given the latest head state
*/
pruneAll(headState: CachedBeaconStateAllForks, finalizedState: CachedBeaconStateAllForks | null): void {
pruneAll(headBlock: allForks.SignedBeaconBlock, headState: CachedBeaconStateAllForks): void {
this.pruneAttesterSlashings(headState);
this.pruneProposerSlashings(headState);
this.pruneVoluntaryExits(headState);
this.pruneBlsToExecutionChanges(headState, finalizedState);
this.pruneBlsToExecutionChanges(headBlock, headState);
}

/**
Expand Down Expand Up @@ -369,19 +370,28 @@ export class OpPool {
}

/**
* Call after finalizing
* Prune blsToExecutionChanges for validators which have been set with withdrawal
* credentials
* Prune BLS to execution changes that have been applied to the state more than 1 block ago.
* In the worse case where head block is reorged, the same BlsToExecutionChange message can be re-added
* to opPool once gossipsub seen cache TTL passes.
*/
private pruneBlsToExecutionChanges(
headState: CachedBeaconStateAllForks,
finalizedState: CachedBeaconStateAllForks | null
headBlock: allForks.SignedBeaconBlock,
headState: CachedBeaconStateAllForks
): void {
const {config} = headState;
const recentBlsToExecutionChanges =
config.getForkSeq(headBlock.message.slot) >= ForkSeq.capella
? (headBlock as capella.SignedBeaconBlock).message.body.blsToExecutionChanges
: [];

const recentBlsToExecutionChangeIndexes = new Set(
recentBlsToExecutionChanges.map((blsToExecutionChange) => blsToExecutionChange.message.validatorIndex)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This check looks insufficient. The prune handler is only called once per finalization event ~1 time / epoch. Then only the execution changes included in that block will be pruned, with all remaining changes staying there.

A more complete solution, using our state caches:

Attach new cache to the state:

  • appliedBlsToExecutionChangesCurrentEpoch
  • appliedBlsToExecutionChangesPreviousEpoch

During block processing you register the validator index of applied execution changes. During epoch transition you rotate the caches. Then to prune you use the appliedBlsToExecutionChangesPreviousEpoch list to prune those. This will give you the complete list of items to prune if there are no re-orgs deeper than 1 epoch, which is an okay compromise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then only the execution changes included in that block will be pruned, with all remaining changes staying there.

just to clarify, execution changes in the head block is not pruned, others will be pruned. So we store BLSToExecutionChange messages in max 33 slots

A more complete solution, using our state caches

the solution works, however I think it's too much for this feature since we have to apply ImmutableJS to maintain these 2 arrays like in EIP-6110 implementation #6042 Also it's tricky in the edge case

I think there are pros and cons for these 2 solutions:

# BLSToExecutionChange messages in block slot in opPool pros cons
current 3 epochs straightforward have to query the finalized state
no state cache (this PR) 1 slot to 33 slots no state cache is needed, lighthouse runs it since capella cover 1-slot reorg scenario only
state cache (Lion's suggestion) 32 slots to 64 slots cover up to 1-epoch reorg scenario need to have 2 ImmutableJS arrays maintained, also need to think about edge case when finalized checkpoint happens >4s, after clock epoch

I prefer the no state cache approach as it's worked for lighthouse since capella and in the worse case if the reorg happen, it'll not affect block reward (if it's the bn's turn to propose block) and messages will be added back once gossip seenTTL passes anyway, I'd like to know others' opinions cc @wemeetagain @g11tech

Copy link
Member

Choose a reason for hiding this comment

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

I think what @tuyennhv proposed here is sufficient and his reasoning is sound.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks for the table! please monitor metrics after deploying to check the pool is not growing

Copy link
Contributor

Choose a reason for hiding this comment

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

the heuristic is fine i guess, since no one is anyway producing these messages and deep reorgs are rare on mainnet


for (const [key, blsToExecutionChange] of this.blsToExecutionChanges.entries()) {
// TODO CAPELLA: We need the finalizedState to safely prune BlsToExecutionChanges. Finalized state may not be
// available in the cache, so it can be null. Once there's a head only prunning strategy, change
if (finalizedState !== null) {
const validator = finalizedState.validators.getReadonly(blsToExecutionChange.data.message.validatorIndex);
const {validatorIndex} = blsToExecutionChange.data.message;
if (!recentBlsToExecutionChangeIndexes.has(validatorIndex)) {
const validator = headState.validators.getReadonly(validatorIndex);
if (validator.withdrawalCredentials[0] !== BLS_WITHDRAWAL_PREFIX) {
this.blsToExecutionChanges.delete(key);
}
Expand Down
Loading