Submission pruning improvements #204
Conversation
KonradStaniec
requested review from
vitsalis and
SebastianElvis
and removed request for
vitsalis
| @@ -26,20 +26,17 @@ message SubmissionKey { | |||
| repeated TransactionKey key = 1; | |||
| } | |||
|
|
|||
| enum EpochStatus { | |||
| enum BtcStatus { | |||
There was a problem hiding this comment.
Why is this renamed? Actually I prefer the previous name 😓
There was a problem hiding this comment.
yea, it is beacouse it is used in two contexts now i.e
- when we are checking submission status
- and later when we prune submissions and decide which submission is the best we also use this enum to mark epoch status.
Having is this use the same enum, enables easy state comparison like bestSubmissionStatus > currentEpoch.Status, to check is we should change epoch status.
So I kinda agree previous type name was a bit better, but imo having it to be more generic and covering more cases is worth it in terms of code simplicity.
x/btccheckpoint/keeper/keeper.go
Outdated
| SubmissionBtcInfo struct { | ||
| SubmissionKey types.SubmissionKey | ||
| // Depth of the oldest btc header of the submission | ||
| OldestBlockDepth uint64 | ||
|
|
||
| // Depth of the youngest btc header of the submission | ||
| YoungestBlockDepth uint64 | ||
|
|
||
| // Index of the latest transaction in youngest submission block | ||
| LatestTxIndex uint32 | ||
| } | ||
|
|
||
| EpochChangesSummary struct { | ||
| SubmissionsToDelete []*types.SubmissionKey | ||
| SubmissionsToKeep []*types.SubmissionKey | ||
| EpochBestSubmission *SubmissionBtcInfo | ||
| BestSubmissionIdx int | ||
| } | ||
|
|
||
| EpochInfo struct { | ||
| bestSubmission *SubmissionBtcInfo | ||
| } |
There was a problem hiding this comment.
Maybe a better place to define these structs is types/. The current keeper.go is already long enough and it's better to make it only include keeper-related functions.
There was a problem hiding this comment.
Also, is it possible to make EpochInfo and SubmissionBtcInfo protobuf objects? The ZoneConcierge module may use them for the timestamp proofs.
There was a problem hiding this comment.
👍 on moving SubmissionBtcInfo and it relted methods to types. It seems this is pretty well defined type to public.
As for EpochInfo and EpochChangesSummary those are rather implementation details which I would rather make package private in keeper.
As for protobuf, I would prefer to have types as simple structs until this really will be required.
x/btccheckpoint/keeper/keeper.go
Outdated
| var depth = initial | ||
| for _, tk := range sk.Key { | ||
| d, err := k.headerDepth(ctx, tk.Hash) | ||
| // HappenedAfter returns true if `this` submission happened after `that` submission |
There was a problem hiding this comment.
As above, HappenedAfter, SubmissionDepth, and IsBetterThan can also be moved to types/
| } | ||
| } | ||
|
|
||
| func (k Keeper) checkConfirmed(ctx sdk.Context) { | ||
| func (k Keeper) clearEpochData( |
There was a problem hiding this comment.
When will we invoke this function? Looks like it removes every submission in an epoch...
There was a problem hiding this comment.
There are two cases when this could happen:
- all submissions in epoch landed on forks or were evicted by btc light client
- parent epoch lost all submissions
| @@ -22,7 +23,7 @@ func BlockCreationResultToProofs(inputs []*dg.BlockCreationResult) []*btcctypes. | |||
| var spvs []*btcctypes.BTCSpvProof | |||
|
|
|||
| for _, input := range inputs { | |||
| headerBytes := input.HeaderBytes | |||
| headerBytes, _ := bbn.NewBTCHeaderBytesFromBytes(input.HeaderBytes) | |||
There was a problem hiding this comment.
We should panic here if the error is not nil, right?
| // a bit of special case, when parent epoch lost all its submissions but current | ||
| // epoch was already in submitted/confirmed states. |
There was a problem hiding this comment.
When will we face this situation? Looks like this will revert a lot of things...
There was a problem hiding this comment.
So to be honest, this situation can only happen in most fresh blocks and it is probably dependent on our choice of parameters (epoch length, w, k etc) but we need to handle it nevertheless.
It is worth remember that we always require that older epoch have always at least one submission deeper on the main btc chain than the new submission for the newer epoch. We also want to preserve this property if some reorgs happen. This is necessary to avoid out of order checkpoints.
So this would happen in case like:
epoch 1 - submission with depth 2 on main chain
epoch 2 - submission with depth 1 on main chain
epoch 3 - submission with depth 0 on main chain
Now when re-org happen, that will make epoch 1 submission land on fork, also need to clear submissions in epochs 2 and 3, as those no longer have submission which is their parent (i.e is deeper on the btc main chain)
| @@ -130,6 +131,39 @@ func GenerateMessageWithRandomSubmitter(blockResults []*dg.BlockCreationResult) | |||
| return &msg | |||
| } | |||
|
|
|||
| func generateMessageWithRandomSubmitterForEpoch() *btcctypes.MsgInsertBTCSpvProof { | |||
There was a problem hiding this comment.
Could we move this to datagen? This will be useful for vigilante as well.
* Add pruning of submissions
This pr makes btccheckpoint module feature complete (except rewards) and simplifies the logic around keeping track which submission is in what state.
New features:
This breaking change as (I have created breaking label):
BTCSpvProofto useBTCHeaderBytescustom type for confirming headers