Immediately set bond height/intratxcounter for new validators #3051
Conversation
jaekwon
requested review from
cwgoes,
ebuchman and
rigelrozanski
as code owners
| @@ -112,6 +112,8 @@ func handleMsgCreateValidator(ctx sdk.Context, msg types.MsgCreateValidator, k k | |||
| } | |||
|
|
|||
| validator := NewValidator(msg.ValidatorAddr, msg.PubKey, msg.Description) | |||
| k.BumpValidatorBondHeightAndCounter(ctx, &validator) | |||
There was a problem hiding this comment.
Do you think we can already set the BondHeight here?
If the maxValidators is already reached and the new validator does not have enough bonded tokens to be included in the Validator set can we still call him "Bonded". I'd say no because bond() is never called because of the EndBlocker code:
So in the end we would have a BondHeight even though the validator was never bonded / added to the voting set.
There was a problem hiding this comment.
It's OK, but semantically wrong. And, as per #2909 (comment) I will implement a solution w/ addresses for GoS.
|
Hmm, I'm not sure this is really the way we want to fix this problem. See further discussion at #2909 (comment). |
| counter := k.GetIntraTxCounter(ctx) | ||
| validator.BondIntraTxCounter = counter | ||
| k.SetIntraTxCounter(ctx, counter+1) | ||
| k.BumpValidatorBondHeightAndCounter(ctx, &validator) |
There was a problem hiding this comment.
Not sure this makes sense - In the old code, the bond height remains the same (it stays old) whereas not we are also updating the bond height, which means that even if you're an old validator, your "precedence" that you get from having an old bond height is effectively erased every time anyone bonds to you... As I understand it the bond height should only ever get updated when you're actually bonding (aka in the master update function ApplyAndReturnValidatorSetUpdates - which is currently happening)
There was a problem hiding this comment.
Semantically yes, and that's the problem with this... I'm more concerned about not wiping out a validator (not even in the cliff) by ending up with the same stake amount, which is a grief'ing factor, so wanted to ensure that keys were unique. Solving this now.
| // Set validator bond height/intratxcounter. In case of a conflict, the | ||
| // validator which least recently changed power takes precedence. | ||
| // NOTE: Mutates the validator. | ||
| func (k Keeper) BumpValidatorBondHeightAndCounter(ctx sdk.Context, validator *types.Validator) { |
There was a problem hiding this comment.
Passing in pointers and modifying those objects is the type of programming pattern which I'd like to avoid unless there is an important performance consideration. In this situation, it's not so bad and pretty straightforward, however I've seen some seriously confusing spaghetti code which we've produced in the past using this pattern. So yeah I've got a reasonably strong preference for always passing back modified inputs instead of silently modifying them via pointer inputs.
There was a problem hiding this comment.
It's only because we aren't already using pointers for validators... We should discuss this in the future when we have more time, but passing non-pointer structs has worse issues in terms of code readability and analysis of correctness... and it's semantically uglier as well. I'd rather we use pointers more, but something to discuss in the future.
There was a problem hiding this comment.
k we can discuss more in the future - maybe there is a balance that can be found, in this particular case I actually don't find it difficult to understand whatsoever... however I've seen some near-incomprehensibly code in the past due to this pattern... thus my preference has been in keep things more verbose but also quite obvious.
I disagree that this makes analysis of correctness more difficult... I actually think it makes it more clear, because you see at exactly which points things are modified.
There was a problem hiding this comment.
Lets agree to disagree for now ;)
Addresses #3049
NOTE: Need tests and refactors.
TODO: create issue to refactor NewValidator() to require certain arguments including bond height/intertxcounter. Unify the two into a single struct. Refactor all around.
Many thanks to @SLAMPER for identifying the issue and finding the root cause.