Persist evidence in equivocation handler #8502
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8502 +/- ##
=======================================
Coverage 61.49% 61.49%
=======================================
Files 651 651
Lines 37250 37251 +1
=======================================
+ Hits 22906 22907 +1
Misses 11942 11942
Partials 2402 2402
|
sahith-narahari
requested review from
aaronc,
alessio and
alexanderbez
as code owners
alessio
requested review from
amaury1093,
fdymylja,
jgimeno and
robert-zaremba
amaury1093
added
the
T: State Machine Breaking
| @@ -119,4 +119,5 @@ func (k Keeper) HandleEquivocationEvidence(ctx sdk.Context, evidence *types.Equi | |||
|
|
|||
| k.slashingKeeper.JailUntil(ctx, consAddr, types.DoubleSignJailEndTime) | |||
| k.slashingKeeper.Tombstone(ctx, consAddr) | |||
| k.SetEvidence(ctx, evidence) | |||
There was a problem hiding this comment.
So we're duplicating TM evidences in state, correct? What do you think of #8360 (comment), to keep state lean?
There was a problem hiding this comment.
I feel it's better to have consistency across evidence handling(all being present in store), and being able to query it using evidence queries. wdyt?
There was a problem hiding this comment.
ok, it seems people generally agree about this duplication in #8360.
| @@ -119,4 +119,5 @@ func (k Keeper) HandleEquivocationEvidence(ctx sdk.Context, evidence *types.Equi | |||
|
|
|||
| k.slashingKeeper.JailUntil(ctx, consAddr, types.DoubleSignJailEndTime) | |||
| k.slashingKeeper.Tombstone(ctx, consAddr) | |||
| k.SetEvidence(ctx, evidence) | |||
There was a problem hiding this comment.
I have a question about migrations. I'm working on in-place store migrations #8485, i.e. updating from e.g. v0.41 to v0.42 without doing a genesis dump, just modifying store in-place.
It seems that this PR will make in-place migrations impossible for v0.42, as the migration script doesn't have access to the history of blocks. Is that correct?
There was a problem hiding this comment.
yes, without access to blockInfo it would be possible to migrate evidence store in-place.
There was a problem hiding this comment.
Sorry to block, but it seems like this basically breaks x/upgrade. I think we need to think through the consequences a bit more before we do this and consider the how feasible tendermint/tendermint#5977 will be as an alternative in the near future.
Can someone comment on the where in place upgrades would and would not break with this change?
amaury1093
added
the
A:automerge
Description
closes: #8360
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes