-
-
Notifications
You must be signed in to change notification settings - Fork 451
fix: align voluntary exit pruning with inclusion rules #9214
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
base: unstable
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -373,10 +373,10 @@ export class OpPool { | |
| const finalizedEpoch = headState.finalizedCheckpoint.epoch; | ||
|
|
||
| for (const [key, voluntaryExit] of this.voluntaryExits.entries()) { | ||
| // VoluntaryExit messages signed in the previous fork become invalid and can never be included in any future | ||
| // block, so just drop as the head state advances into the next fork. | ||
| if (this.config.getForkSeq(computeStartSlotAtEpoch(voluntaryExit.message.epoch)) < headStateFork) { | ||
| const voluntaryExitFork = this.config.getForkSeq(computeStartSlotAtEpoch(voluntaryExit.message.epoch)); | ||
| if (!isVoluntaryExitSignatureIncludable(headStateFork, voluntaryExitFork)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Pruning now defers entirely to Useful? React with 👍 / 👎. |
||
| this.voluntaryExits.delete(key); | ||
| continue; | ||
| } | ||
|
|
||
| // TODO: Improve this simplistic condition | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| import {describe, expect, it} from "vitest"; | ||
| import {createBeaconConfig, createChainForkConfig, defaultChainConfig} from "@lodestar/config"; | ||
| import {BeaconStateView, computeStartSlotAtEpoch} from "@lodestar/state-transition"; | ||
| import {phase0, ssz} from "@lodestar/types"; | ||
| import {OpPool} from "../../../../src/chain/opPools/opPool.js"; | ||
| import {createCachedBeaconStateTest} from "../../../utils/cachedBeaconState.js"; | ||
| import {generateState} from "../../../utils/state.js"; | ||
|
|
||
| const chainConfig = createChainForkConfig({ | ||
| ...defaultChainConfig, | ||
| ALTAIR_FORK_EPOCH: 1, | ||
| BELLATRIX_FORK_EPOCH: 2, | ||
| CAPELLA_FORK_EPOCH: 3, | ||
| DENEB_FORK_EPOCH: 4, | ||
| ELECTRA_FORK_EPOCH: 5, | ||
| }); | ||
|
|
||
| describe("OpPool voluntary exits", () => { | ||
| it("keeps previous-fork exits that are still includable before deneb", () => { | ||
| const headSlot = computeStartSlotAtEpoch(chainConfig.BELLATRIX_FORK_EPOCH); | ||
| const {pool, headBlock, headState} = createPoolContext(headSlot); | ||
|
|
||
| pool.insertVoluntaryExit(createVoluntaryExit(chainConfig.ALTAIR_FORK_EPOCH)); | ||
| pool.pruneAll(headBlock, headState); | ||
|
|
||
| expect(pool.getAllVoluntaryExits()).toHaveLength(1); | ||
| }); | ||
|
|
||
| it("prunes exits whose signatures are no longer includable before deneb", () => { | ||
| const headSlot = computeStartSlotAtEpoch(chainConfig.BELLATRIX_FORK_EPOCH); | ||
| const {pool, headBlock, headState} = createPoolContext(headSlot); | ||
|
|
||
| pool.insertVoluntaryExit(createVoluntaryExit(0)); | ||
| pool.pruneAll(headBlock, headState); | ||
|
|
||
| expect(pool.getAllVoluntaryExits()).toHaveLength(0); | ||
| }); | ||
|
|
||
| it("keeps older-fork exits after deneb because signatures remain perpetually valid", () => { | ||
| const headSlot = computeStartSlotAtEpoch(chainConfig.ELECTRA_FORK_EPOCH); | ||
| const {pool, headBlock, headState} = createPoolContext(headSlot); | ||
|
|
||
| pool.insertVoluntaryExit(createVoluntaryExit(chainConfig.ALTAIR_FORK_EPOCH)); | ||
| pool.pruneAll(headBlock, headState); | ||
|
|
||
| expect(pool.getAllVoluntaryExits()).toHaveLength(1); | ||
| }); | ||
| }); | ||
|
|
||
| function createPoolContext(headSlot: number) { | ||
| const state = generateState({slot: headSlot}, chainConfig); | ||
| const config = createBeaconConfig(chainConfig, state.genesisValidatorsRoot); | ||
| const headBlock = config.getForkTypes(headSlot).SignedBeaconBlock.defaultValue(); | ||
| headBlock.message.slot = headSlot; | ||
|
|
||
| return { | ||
| pool: new OpPool(config), | ||
| headBlock, | ||
| headState: new BeaconStateView(createCachedBeaconStateTest(state, config)), | ||
| }; | ||
| } | ||
|
|
||
| function createVoluntaryExit(epoch: number): phase0.SignedVoluntaryExit { | ||
| return { | ||
| ...ssz.phase0.SignedVoluntaryExit.defaultValue(), | ||
| message: { | ||
| validatorIndex: 0, | ||
| epoch, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to determine the fork sequence for a voluntary exit is repeated in multiple places (here and in
getSlashingsAndExits). Extracting this into a private helper method would improve maintainability and ensure consistency across the pool's operations.