Prune EthereumSignatures and EthereumEventVoteRecords #517
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #517 +/- ##
==========================================
+ Coverage 29.90% 30.01% +0.10%
==========================================
Files 47 48 +1
Lines 6503 6987 +484
==========================================
+ Hits 1945 2097 +152
- Misses 4402 4708 +306
- Partials 156 182 +26
|
…ubmitter's event nonce
module/x/gravity/abci.go
Outdated
| @@ -27,6 +27,8 @@ func BeginBlocker(ctx sdk.Context, k keeper.Keeper) { | |||
| // EndBlocker is called at the end of every block | |||
| func EndBlocker(ctx sdk.Context, k keeper.Keeper) { | |||
| outgoingTxSlashing(ctx, k) | |||
| // Do we need to concern ourselves with future slashing windows for this pruning? | |||
There was a problem hiding this comment.
Can you elaborate on this question?
There was a problem hiding this comment.
there is a window of blocks for which the chain waits to prune batch votes so that validators have time to sign them before getting slashed. At the time when I commented this I didn't fully understand how vote records work for events as distinct from signatures for outgoing TXs. There's no reason to keep event vote records around after the last observed event nonce has incremented so I'll remove this comment.
There was a problem hiding this comment.
Now that you have me thinking about it, I think there's certainly an argument to be made for a slashing window for the oracle function as well. It's as necessary to complete the bridge transaction lifecycle as signatures are. From this perspective, we may want to keep a sliding window's worth of history for both.
module/x/gravity/abci.go
Outdated
| panic(err) | ||
| } | ||
| eventNonce := event.GetEventNonce() | ||
| if eventNonce <= lastObservedEventNonce { |
There was a problem hiding this comment.
Shouldn't this be < rather than <=?
There was a problem hiding this comment.
If the nonce has been canonically observed there is no need to keep new votes for it right?
There was a problem hiding this comment.
One thing I have noticed while helping validators diagnose issues with their orchestrators is that it's valuable to have evidence they have or have not been submitting things correctly. If we prune everything every time, it's very hard to query for this sort of information. I think leaving some small bit of history before pruning might be worthwhile.
|
Need to look into replacing the scan iteration when getting the vote attestation map into a lookup |
module/x/gravity/keeper/keeper.go
Outdated
| @@ -464,6 +464,7 @@ func (k Keeper) SetOutgoingTx(ctx sdk.Context, outgoing types.OutgoingTx) { | |||
| // DeleteOutgoingTx deletes a given outgoingtx | |||
| func (k Keeper) DeleteOutgoingTx(ctx sdk.Context, storeIndex []byte) { | |||
| ctx.KVStore(k.storeKey).Delete(types.MakeOutgoingTxKey(storeIndex)) | |||
| k.DeleteEthereumSignatures(ctx, storeIndex) | |||
There was a problem hiding this comment.
I'm not certain if it matters in practice here, but I would suggest cleaning up the "child" records (the signatures) before deleting the "parent" record (the outgoing TX).
CompletedOutgoingTxsstore. When anOutgoingTxis observed or timed out, it is moved to this new store and the original deleted. This new store is used for slashing. A completedOutgoingTxis pruned after it leaves the slashing window. Signer sets are a special case, theirOutgoingTxis still used for slashing.EthereumSignatureat the time ofCompletedOutgoingTxdeletion for each type, which occurs after the slashing has passedEthereumEventVoteRecords with nonces older than the last observed event nonce.CompletedOutgoingTxsandEventVoteRecordsqueriesOutgoingTxsin the results returned by theUnsigned*Txsqueries so that new signers can sign completed txs and not get slashed.Closes #431
Closes #349