fix(oracle)!: count vote omission as abstain for less slashing + more stability #1565
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1565 +/- ##
==========================================
+ Coverage 74.49% 74.52% +0.02%
==========================================
Files 175 177 +2
Lines 14567 14651 +84
==========================================
+ Hits 10852 10918 +66
- Misses 3097 3112 +15
- Partials 618 621 +3
|
|
Overall I don't think we should make this change because it changes the economic incentives for validators to participate in the decentralized oracle. It allows validators to be lazy and not participate at all, without any slashing risk. I think we want validators to run the pricefeeder infrastructure always. In regular PoS, if you don't vote for a block, it's counted as a miss that counts towards your slashing. If the concern is about network stability, I think we should analyze what went wrong with the smaller slashing window and higher min_valid_pct instead of immediately making this change. |
Unique-Divine
force-pushed
the
realu/oracle-miss
branch
from
875cb43 to
fad86ca
Compare
x/oracle/keeper/slash_test.go
Outdated
| allowedMissPct := sdk.OneDec().Sub(minValidPerWindow) | ||
| allowedMissPeriods := allowedMissPct.MulInt64(votePeriodsPerWindow). | ||
| TruncateInt64() | ||
| t.Logf("For %v blocks, valoper0 does not vote, while 1 and 2 do.", allowedMissPeriods) |
There was a problem hiding this comment.
I think you're conflating blocks with vote periods here. One vote period is many blocks (by default 30 on testnet), so if the goal is to vote once every vote period, we have to skip the block number by 30 in each iteration.
However, this test doesn't really test validator slashing, only that the MissCounter doesn't increment. The slashing happens in the abci EndBlocker which is in another test.
There was a problem hiding this comment.
Yes, the name is wrong because it used to be a test about slashing that had abstain votes. Since that doesn't cause slashing, the test is no longer about that either. Will need to repurpose it
| // consensus power. | ||
| RewardWeight int64 | ||
| WinCount int64 // Number of valid votes for which the validator will be rewarded | ||
| AbstainCount int64 // Number of abstained votes for which there will be no reward or punishment |
There was a problem hiding this comment.
I don't think we're using AbstainCount usefully anywhere. Let's just get rid of it?
There was a problem hiding this comment.
There's a ticket open (from Ruslan, I think) to add an event for this and emit more info about the validator stats. The goal after some initial review of the changes in this PR is to add that in as well
|
|
||
| k.MissCounters.Insert(ctx, validatorPerformance.ValAddress, k.MissCounters.GetOr(ctx, validatorPerformance.ValAddress, 0)+1) | ||
| k.Logger(ctx).Info("vote miss", "validator", validatorPerformance.ValAddress.String()) | ||
| func (k Keeper) registerAbstainsByOmission( |
There was a problem hiding this comment.
I don't think we need this function. The return type of UpdateExchangeRates isn't used anywhere so we don't need to do anything after rewarding validators and updating miss counters.
x/oracle/keeper/ballot.go
Outdated
| ballots types.ExchangeRateBallots, | ||
| rewardBand sdk.Dec, | ||
| validatorPerformances types.ValidatorPerformances, | ||
| ) (sdk.Dec, types.ValidatorPerformances) { |
There was a problem hiding this comment.
This is a bad idea, to have a return only used for tests. The function already mentions that it mutates the validator perfomances, returning it is confusing.
Description
What does this PR do?
Purpose
Why is this PR important?