-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
R4R: Ensure Tendermint Validator Update Invariants #2238
Conversation
Only return ABCI validators that match certain criteria
x/stake/keeper/validator.go
Outdated
if found { | ||
// The validator is new or already exists in the store and must adhere to | ||
// Tendermint invariants. | ||
prevBonded := val.BondHeight < ctx.BlockHeight() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this check is sound, but on the surface is seems correct.
Codecov Report
@@ Coverage Diff @@
## develop #2238 +/- ##
==========================================
Coverage ? 63.75%
==========================================
Files ? 140
Lines ? 8652
Branches ? 0
==========================================
Hits ? 5516
Misses ? 2756
Partials ? 380 |
Can you disable governance txs (so it runs fast) and run test sim Gaia slow locally |
@ValarDragon sure thing! |
Too good to be true! Still seems like an issue. Will look into it. Update: Was able to reproduce with 500 blocks and a seed of |
Ok, so what I have implemented does indeed fix and address the issue at hand, but this may be something else. Perhaps related to #2241. I can follow the events as such, given validatorA:
So validatorA does not go from zero, to non-zero, back to zero as discussed in #2188 it seems, but something else seems to be happening. Correct me if I'm wrong. |
Since the |
Ok! Should we update that issue with my notes? |
Please do! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the design of this fix to be counterintuitive. What's been implemented here is a fix where at the time of retrieving the Tendermint Updates we ad-hoc perform a check on whether it should actually be sent to tendermint for an update or not. This logic is tucked away in the get method (there are no other Get
methods with custom filter logic like this)
A better fix is one where the Tendermint updates simply do not get added to the TendermintUpdates
store if it's not appropriate to do so (see this method
cosmos-sdk/x/stake/keeper/validator.go
Line 637 in 53253aa
bzABCI := k.cdc.MustMarshalBinary(validator.ABCIValidatorZero()) |
To know whether or not a particular validator was bonded in the previous block (and therefore should in fact be removed from tendermint) can be determined using the current height (from the ctx) and the validator's fields BondHeight
and UnbondingHeight
- before the unbonding height has been updated for this transaction if BondHeight > UnbondingHeight && BondHeight > CurrentHeight
then the Tendermint update to 0 should be sent, all other situations it should not be sent (I think)
@rigelrozanski thanks for the review! To be quite honest, I don't really follow what you're suggesting in terms of how to change this, but I'll try to respond based on my understanding as is:
Mhmmm perhaps, but I'd have to respectfully disagree in the sense that it is "bizarre". While it might not be the optimal solution in terms of placement, I think there are a handful solutions that would get this job done and the one provided is one of them. I'm certainly open to more optimal, rather more clear and concise, solutions.
Actually, this was my first inclination and attempt -- I didn't even want to modify e.g. a validator going from zero to non-zero power would trigger this update procedure. Then when it goes back to zero, it's already in the update store, so having this logic there would do us no good. Please clarify if this is what you had in mind? Maybe I'm completely misunderstanding and I could very well be totally wrong here and a more concise solution exists.
Hmm, wording is a bit off I think and it's confusing me. Are you suggesting the following? When unbonding a validator, check if: |
you're right - the logic is not bizarre (bad use of language) - it's really good! (my bad!) However let me attempt to explain my idea clearer.
not quite, my pseudocode in the next comment should clear things up though
yes but there is only one occurrence where it should be updating for a zero validator (the one which I linked too)... It's possible that there are edge cases on the other 4 occurrences where the validator is also being updated as a zero validator (maybe not?) - either way what I think is a more optimal design is to create a special function which contains all the set logic for the tendermint store. so throughout the code rather than calling func SetTendermintStore(ctx sdk.Context, validator stake.Validator, abciVal stake.AbciVal) {
store := ctx.GetStore()
if abciVal.Amount.IsZero() &&
validator.BondHeight < UnbondingHeight{
// already unbonded
return
}
bz := marshal(abciVal)
store.Set(GetTendermintUpdatesKey(valAddr), bz)
return
} |
Per discussion with @jaekwon @cwgoes and @rigelrozanski, we've concluded it's best and safer to keep the new logic as is but change the naming of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
@alexanderbez Let's separate #2238 (comment) into a separate issue. |
Ref #2277 |
Should we merge this despite it failing the simulation? I guess not, but it does definitely fix a bug. Let's work on the simulation fix in a separate PR targeted to this one to separate concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work on the design :)
Will do @cwgoes |
* Add a unit test showing invalid TM validator updates with bonded vals * Re-add defensive check in UpdateBondedValidators for jailed validators * Cleanup and set bond height in keeper#bondValidator * Get pool after getting context with new block height in unit test * Fix broken unit tests * Update prevBonded value * Rename validator to oldValidator in bondIncrement
602e626
to
c16f33f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK
Ideas for future simplification: #2312
GetTendermintUpdates
to enforce the following invariants:closes: #2189, #2241
ref: #2188
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added entries in
PENDING.md
with issue #rereviewed
Files changed
in the github PR explorerFor Admin Use: