Method to list incomplete checkpoints #207
Conversation
src/gateway/GatewayRouterFacet.sol
Outdated
|
|
||
| /// @notice garbage collect all checkpoints and related data for heights lower than `newIndex`. | ||
| /// @param newIndex - new retention index | ||
| function pruneBottomUpCheckpoints(uint64 newIndex) external systemActorOnly { |
There was a problem hiding this comment.
When is this function expected to be called externally? The idea is that this function should be called when there is no longer need to store the checkpoint signatures because it has already been submitted up, right?
If this is the case, would you mind adding this description in the comments so we are explcit?
There was a problem hiding this comment.
The idea is that this function should be called when there is no longer need to store the checkpoint signatures because it has already been submitted up, right
correct
There was a problem hiding this comment.
Updated description
src/gateway/GatewayRouterFacet.sol
Outdated
|
|
||
| /// @notice garbage collect all checkpoints and related data for heights lower than `newIndex`. | ||
| /// @param newIndex - new retention index | ||
| function pruneBottomUpCheckpoints(uint64 newIndex) external systemActorOnly { |
There was a problem hiding this comment.
How do we expect to set the newIndex for a specific garbage collection?
There was a problem hiding this comment.
If I understand correctly, the caller should know what checkpoints have been processed in the parent and can be removed in the child
src/lib/LibGatewayActorStorage.sol
Outdated
| /// @notice The height of the last bottom-up checkpoint registered in the parent. | ||
| /// All checkpoint with the height less than or equal to that number can be garbage collected in the child subnet. | ||
| /// @dev Initial retention index is 1. | ||
| uint64 bottomUpCheckpointRetentionIndex; |
There was a problem hiding this comment.
Do you mind explaining here that the retention is needed so the checkpoint and signatures are available so relayers can submit the chceckpoint? It wasn't clear to me the purpose of the RetentionIndex on a first read. Thanks!
There was a problem hiding this comment.
- https://github.com/consensus-shipyard/fendermint/issues/258 wants to retrieve the checkpoints that have not reached quorum yet
- https://github.com/consensus-shipyard/fendermint/issues/250 wants to clear the checkpoints registered (applied) in the parent
retentionIndexis the number of the first incomplete checkpoints we must have to implement checkpointing
All checkpoints with a height less than retentionIndex may be removed from the history if we want.
There was a problem hiding this comment.
I meant in the code, so it is clear :) But no need if you feel it may be redundant or clear once the end-to-end is implemented.
Co-authored-by: adlrocha <6717133+adlrocha@users.noreply.github.com>
There was a problem hiding this comment.
Unless @aakoshh and @cryptoAtwill want to have a final look, I think is ready to 🚢
| for (uint64 h = oldRetentionIndex; h < newRetentionIndex; ) { | ||
| delete s.bottomUpCheckpoints[h]; | ||
| delete s.bottomUpCheckpointInfo[h]; | ||
| delete s.bottomUpCollectedSignatures[h]; |
There was a problem hiding this comment.
We should also clear the bottomUpCheckpointsLegacy while it exists.
There was a problem hiding this comment.
Those two bottom-up checkpoint types are probably misleading. But bottomUpCheckpointsLegacy is not used in implementing the new bottom-up checkpoint mechanism. It is in the code just to build it without removing all libraries, etc.
There was a problem hiding this comment.
Okay, I understand we want to get rid of it, just wanted to highlight that since I'm relying on it in consensus-shipyard/fendermint#286 due to the lack of any alternatives, technically it will have data in it as long as it exists and someone sends a bottom-up message this way.
There was a problem hiding this comment.
It will be fixed in #214
Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
This PR adds a method to list incomplete checkpoints described in consensus-shipyard/ipc#299 and a method to garbage collect completed checkpoints