informalsystems / partnership-heliax

1 stars 0 forks source link

Add redelegation to the pseudocode model #45

Closed angbrav closed 11 months ago

angbrav commented 1 year ago

This PR adds redelegation to the pseudocode model and English text to describe the main logic and some definitions.

Note that the text below may be a bit outdatet.

Definitions

Desired Properties

The solution aim to guarantee the following desired properties:

Data Structures

The solution introduces the following data structures to track redelegations:

How it roughly works

cwgoes commented 1 year ago

(ref some discussions in https://github.com/anoma/namada/issues/34)

We need to make sure not to iterate over delegations or redelegations (DoS vector). In this case I think we can treat all redelegations from X to Y in epoch e the same (so e.g. when slashing we can just store the total amount from X to Y in e and slash based on this, we don't have to iterate over the redelegations).

tzemanovic commented 1 year ago

(ref some discussions in anoma/namada#34)

We need to make sure not to iterate over delegations or redelegations (DoS vector). In this case I think we can treat all redelegations from X to Y in epoch e the same (so e.g. when slashing we can just store the total amount from X to Y in e and slash based on this, we don't have to iterate over the redelegations).

Thanks, that's a very important point! I think we unwittingly introduced iteration in https://github.com/informalsystems/partnership-heliax/pull/38 that's being carried over to here too, where in end_of_epoch we started iterating over validator.set_unbonds.

For the redelegations themselves, we suggested grouping these by a triple of src validator, dest validator and redelegation epoch, and then iteration over the groups is acceptable.

I think we could solve the iteration over the set_unbonds the same way - i.e. group unbonds by a pair of bond start epoch and unbond epoch.

tzemanovic commented 1 year ago

I tried to check for these properties:

  1. bonding, unbonding, withdrawal and slashing from a validator that has no outgoing or incoming re-delegations is unaffected by the changes
  2. when a re-delegation src validator is slashed before re-delegation epoch + pipeline and the src bond was in slashable epoch range, the slash for the re-delegated amount is subtracted for the slash applied on the src validator and applied on the dest validator instead
  3. when a re-delegation src validator is slashed at or after re-delegation epoch + pipeline, the dest validator is unaffected
  4. when a re-delegation dest validator is slashed before re-delegation epoch + pipeline, the re-delegated amount is unaffected
  5. when a re-delegation dest validator is slashed at or after re-delegation epoch + pipeline, the re-delegated amount is slashed
  6. when a re-delegation is unbonded after a slash on the src validator that's applicable to the re-delegation is discovered, the unbond gets slashed and the final amount that can be withdrawn is not slashed again
  7. when a re-delegation is unbonded before a slash on the src validator that's applicable to the re-delegation is discovered, the unbond isn't slashed and the final amount that can be withdrawn gets slashed
  8. when a re-delegation is unbonded and a slash on the dest validator that's applicable to the re-delegation is discovered, the unbond isn't slashed and the final amount that can be withdrawn gets slashed (like a regular bond unbond)

I haven't found any issues with the logic besides the comments, most of which are minor.

As per https://github.com/informalsystems/partnership-heliax/pull/45#discussion_r1174870619 I think for 2., 6. and 7. we're ignoring the start epochs of the src bond deltas, which I think is ok as we never skip a slash on something that should be slashed, only potentially slashing a re-delegation that might not need to be slashed if we were to track the start epochs of re-delegations in detail from the source deltas. I think this is applied consistently across the different paths (unbond/withdraw/end of epoch).

angbrav commented 1 year ago

I tried to check for these properties:

  1. bonding, unbonding, withdrawal and slashing from a validator that has no outgoing or incoming re-delegations is unaffected by the changes
  2. when a re-delegation src validator is slashed before re-delegation epoch + pipeline and the src bond was in slashable epoch range, the slash for the re-delegated amount is subtracted for the slash applied on the src validator and applied on the dest validator instead
  3. when a re-delegation src validator is slashed at or after re-delegation epoch + pipeline, the dest validator is unaffected
  4. when a re-delegation dest validator is slashed before re-delegation epoch + pipeline, the re-delegated amount is unaffected
  5. when a re-delegation dest validator is slashed at or after re-delegation epoch + pipeline, the re-delegated amount is slashed
  6. when a re-delegation is unbonded after a slash on the src validator that's applicable to the re-delegation is discovered, the unbond gets slashed and the final amount that can be withdrawn is not slashed again
  7. when a re-delegation is unbonded before a slash on the src validator that's applicable to the re-delegation is discovered, the unbond isn't slashed and the final amount that can be withdrawn gets slashed
  8. when a re-delegation is unbonded and a slash on the dest validator that's applicable to the re-delegation is discovered, the unbond isn't slashed and the final amount that can be withdrawn gets slashed (like a regular bond unbond)

I haven't found any issues with the logic besides the comments, most of which are minor.

As per #45 (comment) I think for 2., 6. and 7. we're ignoring the start epochs of the src bond deltas, which I think is ok as we never skip a slash on something that should be slashed, only potentially slashing a re-delegation that might not need to be slashed if we were to track the start epochs of re-delegations in detail from the source deltas. I think this is applied consistently across the different paths (unbond/withdraw/end of epoch).

Thanks for the detailed analysis @tzemanovic . I am transforming the spec into a Quint executable spec, so we will be able to test those scenarios much more easily. Regarding your last comment, see my response.

angbrav commented 1 year ago

@tzemanovic @cwgoes I've gotten rid of the expensive iterations, in case you want to take a look!

cwgoes commented 1 year ago

@angbrav should we merge this?

angbrav commented 1 year ago

We could, but it is not in sync with the Quint spec. Do we want to spend some time correcting it?

angbrav commented 11 months ago

Closed as agreed to stop maintaining pseudocode specs in favor of Quint specs.