Prevent overslashing and negative stakes #55

brentstone · 2023-05-27T14:21:15Z

Closes #46.

The changes in this PR were initiated by the need to adjust the pseudocode to reflect changes in the Namada implementation that prevent the validator's deltas from going negative with the existence of more arbitrary slashing conditions.

angbrav

The only thing that's unclear to me is the comment about capping in unbonding. I am integrating these changes to the Quint spec to see what happens. BTW, what about the slash pool? I think it did not make it into any PR.. it is fine like that, but we should not forget about it.

angbrav · 2023-05-29T09:23:55Z

2022/Q4/artifacts/PoS-pseudocode/PoS-model.md

@@ -316,16 +322,24 @@ func withdraw(validator_address, delegator_address)

 ```go
 compute_amount_after_slashing(set_slashes, amount) {
+  // First, group the slashes by epoch and a total rate


Why do we need to do this?

Slashes within the same epoch will always be applied by summing their rates, so we can essntially turn all slashes into one effective slash per epoch. These lines also cap the slash rate per epoch at 1.0

angbrav · 2023-05-29T11:11:01Z

2022/Q4/artifacts/PoS-pseudocode/PoS-model.md

+
+      // Ensure that the validator's stake does not go negative due to the slashing
+      var pipeline_stake = read_epoched_field(validators[validator_address].total_deltas, cur_epoch + pipeline_length, 0)
+      var token_change = min{ amount_after_slashing, pipeline_stake }


Can you think in a scenario in which this is needed? Intuitively I'd say we do not need it. We are computing how much from the bond is left after slashing and we are concerned about overslashing because the validator misbehaved at least twice with the same tokens. In those cases, amount_after_slashing would be 0. But I do not know if I am missing some scenarios in which the validator is slashed all its stake at the end of an epoch, and then when applying the slashes to individual bonds here some of the bonds won't be fully slashed... that would be a case in which we would need to be careful at this point...

I think you may be right in thinking this may not be necessary. It's possible I threw it in there when I was initially trying to prevent negative stake in lots of places. I've just tested the removal of this logic with our native state machine test and the test passed.

I'll remove this for now and remove it from the Namada code in a new PR.

…replace `slash.epoch` with `infraction_epoch`

angbrav · 2023-05-30T07:55:08Z

2022/Q4/artifacts/PoS-pseudocode/PoS-model.md

+      var sum_post_bonds = sum{bonds[validator_address].deltas[start] s.t. start > infraction_epoch && start <= cur_epoch + offset>}
+      var validator_stake = read_epoched_field(validators[validator_address].total_deltas, cur_epoch + offset, 0)
+      var slashable_stake = validator_stake - sum_post_bonds
+      var change = min{-slashable_stake, diff_slashed_amount}


This part I am not sure about. I think we just need to compute how much from this_slash is left after applying any preceding slash. Could not figure out if what it is in here is equivalent, but we should avoid iterating over bonds at the end of an epoch.

I keep thinking about this. I believe that to implement this correctly we need to be able to answer the following question: given an epoch e and a validator val, how much stake is left at an epoch e' > e from the stake the validator had at e. I haven't been able to figure out how to answer this question with the state variables that we have and without iterating over bonds/unbonds.

My guess is that your code tries to do something like that, but I am not sure it succeeds in doing so. My concern is related to unbonds that remove bonds but are only effective at the pipeline_length offset.

I am proposing the following: keep a new variable total_bonded for each validator which is a map from Epoch to int.
We update the variable when bonding as follows:
validators[validator_address].total_bonded[cur_epoch+pipeline_length] += amount

Given a offset, we use this state variable as follows to substitute lines 564.

var recent_unbonds = 0 forall (let [bond_start, amount] of validators[validator_address].unbond_records[cur_epoch+offset] s.t. amount > 0 && bond_start > infraction_epoch) do recent_unbonds += amount sum_post_bonds = sum_post_bonds + (validators[validator_address].total_bonded[cur_epoch+offset] - recent_unbonds)

Note that sum_post_bonds is always >= 0 (we are only interested in the bonds that were added after the infraction epoch and we subtract how much of those have been unbonded) and that sum_post_bonds is computed iteratively within the for all loops. How does this look for you? It is similar to your code but it addresses to issues:

Does not iterate over bonds

I am not 100% sure, but I think your code has the issue of not accounting for unbonds properly.

Does the commit 01a5028 satisfy everything you had in mind here?

I think we should start computing sum_post_bonds in the previous forall: the one that goes from slash.epoch+1 to cur_epoch. Otherwise, we would be possibly missing a bunch of bonds.

Tried to put this in pseudocode quickly here: ac99310

Tried to put this in pseudocode quickly here: ac99310

That may not work. Let's say you bond x tokens at epoch e and unbond those tokens at epoch e+2 and you are computing sum_post_bonds at epoch e. Then the stake at e includes those x tokens but your global_bonds[validator_address].deltas[start] won't: you subtract them when unbonding. This looks incorrect to me.

Sorry, I think I need more context for your example here... if I'm computing sum_post_bonds at epoch e in my version of things, then there is no way that an unbond of tokens can remove stake at e+2 because the validator will be frozen already by epoch e when the unbond tx is submitted (only computing sum_post_bonds for future epochs relative to the current).

Assume that unbonding_length=4, pipeline_length=3 and cubic_offset=1

A validator misbehaves at e. The infraction is submitted at e+unbonding_length=e+4 and processed at the end of e+unbonding_length+cubic_offset=e+5

A user delegates x tokens to the validator at e+1 and unbonds them at e+unbonding_length=e+4 (before the infraction is submitted).

When processing the infraction at the end of e+5, we would compute sum_post_bonds for e+6 (cur_epoch+1). By that epoch, the unbond has not materialized (it materializes at e+4+pipeline_length=e+7), but global_bonds[e+1] accounts for it.

I know those are not typical values for unbonding_length and pipeline_length, but I'd rather not have the correctness of the design depend on the value of these parameters.

Thx for this example. I agree that your proposed changes do indeed solve a problem that currently exists. I wrote up a comparison of the two approaches that demonstrates where the current one fails and where the proposed one succeeds: https://hackmd.io/yRsLbn5IT8C2xkj8csSZMg?both

angbrav · 2023-06-08T14:11:21Z

Looks good, merged

brentstone mentioned this pull request May 27, 2023

PoS updates to ensure non-negative stakes + misc upgrades #50

Closed

brentstone force-pushed the brent/prevent-negative-stakes branch 2 times, most recently from 41ae010 to 164e548 Compare May 27, 2023 15:01

brentstone marked this pull request as ready for review May 27, 2023 15:01

brentstone requested a review from angbrav May 27, 2023 15:01

brentstone force-pushed the brent/prevent-negative-stakes branch 2 times, most recently from a19e8a3 to d9af2d0 Compare May 27, 2023 15:22

angbrav reviewed May 29, 2023

View reviewed changes

brentstone added 5 commits May 29, 2023 10:01

compute_amount_after_slashing: ensure the minimum is 0

e058fe6

unbond: ensure validator stake doesn't go negative

44df7e8

only loop over slashes in end_of_epoch to sum a total slash rate + …

9681102

…replace `slash.epoch` with `infraction_epoch`

compute_amount_after_slashing: cap the rates per epoch at 1

2263d97

prevent overslashing for bonds activated after an infraction

8b2b338

brentstone force-pushed the brent/prevent-negative-stakes branch from d9af2d0 to 8b2b338 Compare May 29, 2023 14:01

remove logic that is not needed to prevent negative stakes

1225087

angbrav reviewed May 30, 2023

View reviewed changes

brentstone force-pushed the brent/prevent-negative-stakes branch from 01a5028 to 65f7335 Compare June 5, 2023 22:06

WIP possible change to sum_post_bonds

16d4268

brentstone force-pushed the brent/prevent-negative-stakes branch from 65f7335 to 16d4268 Compare June 6, 2023 03:08

brentstone added 3 commits June 6, 2023 12:50

Namada currently looks like this

ac99310

revert previous implementation in favor of Manu's (will squash later)

a104c20

miscellaneous textual fixes

62a3d7d

brentstone mentioned this pull request Jun 8, 2023

Fix slashable stake in slash processing anoma/namada#1520

Merged

bug fix in change

85ff661

brentstone force-pushed the brent/prevent-negative-stakes branch from 3a0e1ff to 85ff661 Compare June 8, 2023 04:48

angbrav mentioned this pull request Jun 8, 2023

Integrate latests pseudocode-related PRs to Quint #57

Merged

angbrav merged commit 7e32384 into trunk Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent overslashing and negative stakes #55

Prevent overslashing and negative stakes #55

brentstone commented May 27, 2023 •

edited

Loading

angbrav left a comment

angbrav May 29, 2023

brentstone May 29, 2023

angbrav May 29, 2023

brentstone May 29, 2023

angbrav May 30, 2023 •

edited

Loading

angbrav May 30, 2023 •

edited

Loading

angbrav May 31, 2023

brentstone Jun 1, 2023

angbrav Jun 5, 2023

brentstone Jun 6, 2023 •

edited

Loading

angbrav Jun 6, 2023

brentstone Jun 6, 2023

angbrav Jun 7, 2023 •

edited

Loading

brentstone Jun 8, 2023

angbrav commented Jun 8, 2023

Prevent overslashing and negative stakes #55

Prevent overslashing and negative stakes #55

Conversation

brentstone commented May 27, 2023 • edited Loading

angbrav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angbrav May 30, 2023 • edited Loading

Choose a reason for hiding this comment

angbrav May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentstone Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angbrav Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angbrav commented Jun 8, 2023

brentstone commented May 27, 2023 •

edited

Loading

angbrav May 30, 2023 •

edited

Loading

angbrav May 30, 2023 •

edited

Loading

brentstone Jun 6, 2023 •

edited

Loading

angbrav Jun 7, 2023 •

edited

Loading