fix: align voluntary exit pruning with inclusion rules#9214
fix: align voluntary exit pruning with inclusion rules#9214solanaXpeter wants to merge 2 commits intoChainSafe:unstablefrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the voluntary exit pruning logic in OpPool by utilizing isVoluntaryExitSignatureIncludable and introduces unit tests to ensure correct behavior across various fork epochs. Feedback suggests extracting the fork sequence retrieval logic into a helper method to minimize code duplication.
| // 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)); |
There was a problem hiding this comment.
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.
| const voluntaryExitFork = this.config.getForkSeq(computeStartSlotAtEpoch(voluntaryExit.message.epoch)); | |
| const voluntaryExitFork = this.getVoluntaryExitFork(voluntaryExit); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60827a49e3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // 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.
Drop pre-Capella exits when entering Deneb+
Pruning now defers entirely to isVoluntaryExitSignatureIncludable(), but that helper returns true for every exit once stateFork >= deneb, so pre-Capella signatures can survive indefinitely until finalized. This conflicts with the actual signature domain rule (getDomainForVoluntaryExit in packages/config/src/genesisConfig/index.ts fixes Deneb+ voluntary-exit verification to the Capella domain), meaning exits that were valid in Bellatrix/Capella gossip can become invalid at Deneb while still being kept and later selected for block production (which skips re-verifying exit signatures). On short-gap/custom fork schedules or during prolonged non-finality, this can lead to proposing blocks with invalid voluntary exits.
Useful? React with 👍 / 👎.
Motivation
OpPoolcurrently prunes voluntary exits using older cross-fork rules even when the exit signature remains includable. That can drop valid exits from the pool before block production, including Deneb+ exits covered by EIP-7044 and builder voluntary exits after gloas.Description
isVoluntaryExitSignatureIncludable()when pruning voluntary exits fromOpPoolAI Assistance Disclosure