refactor: payload envelope input -- pruneBelowParent#9326
Conversation
- Updated the `pruneBelowParent` method to improve the deletion of payload inputs. Now, it checks if the cached FULL variant is an ancestor of the current parent block before evicting entries, ensuring only relevant inputs are removed. (according to issue ChainSafe#9318)
- Modified tests to reflect changes in the `pruneBelowParent` method, ensuring it correctly handles ancestor checks using `isDescendant`. Added a new test case to verify that non-ancestor entries on forks remain intact during pruning.
There was a problem hiding this comment.
Code Review
This pull request refactors the pruneBelowParent logic in SeenPayloadEnvelopeInput to use forkChoice.isDescendant for more precise pruning of ancestor blocks, ensuring that entries on other forks are preserved. Unit tests have been updated to reflect this change and include a new test case for fork handling. Feedback suggests using PayloadStatus.PENDING instead of PayloadStatus.FULL to improve robustness in identifying ancestors, particularly for the Gloas fork, and notes a typo in the source code comments.
| // Check if the cached FULL cariant is an ancestor of the current parent block | ||
| this.forkChoice.isDescendant( | ||
| input.blockRootHex, | ||
| PayloadStatus.FULL, | ||
| parentBlock.blockRoot, | ||
| parentBlock.payloadStatus | ||
| ) |
There was a problem hiding this comment.
The check for ancestor status should use PayloadStatus.PENDING instead of PayloadStatus.FULL. In the Gloas fork, every block root has a PENDING variant, but the FULL variant is only created once the execution payload is available. If a block in the cache is an ancestor but hasn't transitioned to FULL status in fork choice (e.g., if it was processed optimistically or if the node switched to a fork that doesn't require its payload), using PayloadStatus.FULL will cause isDescendant to return false, preventing the entry from being pruned until finalization. Using PayloadStatus.PENDING is more robust as it correctly identifies the block as an ancestor regardless of its payload status.
Also, there is a typo in the comment: "cariant" should be "variant".
// Check if the cached block is an ancestor of the current parent block.
// Using PENDING status for the ancestor is more robust as every Gloas block
// has a PENDING variant, while the FULL variant may not exist yet or at all.
this.forkChoice.isDescendant(
input.blockRootHex,
PayloadStatus.PENDING,
parentBlock.blockRoot,
parentBlock.payloadStatus
)| parentBlock.payloadStatus | ||
| ) | ||
| ) { | ||
| this.evictPayloadInput(input); |
There was a problem hiding this comment.
please keep the log inside this if similar to line 144 above
There was a problem hiding this comment.
I fixed in the latest commits.
| // Check if the cached FULL variant is an ancestor of the current parent block | ||
| this.forkChoice.isDescendant( | ||
| input.blockRootHex, | ||
| PayloadStatus.FULL, |
There was a problem hiding this comment.
maybe PayloadStatus.PENDING
we want to also clean the one without payload
so that the next time it loops less items
There was a problem hiding this comment.
I added both FULL and PENDING conditions.
352d517 to
6d4fe72
Compare
|
@twoeths thanks for the review, I changed according to your comments and also added some tests as well. please have a look again. |
twoeths
left a comment
There was a problem hiding this comment.
looks good to me
- in unfinality time, we may have a lot of PayloadEnvelopeInputs but the check if
isDescendant()is cheap there, the for loop will return early - should be very cheap in the normal condition too
@wemeetagain may want to review this
Motivation
According to #9318
Description
Change from iterating all the ancestor to only cache in
seenPayloadEnvelopeInput/pruneBelowParentCloses #9318
AI Assistance Disclosure
collaborate with claude code