[LSM] Prevent ICAs to validator's with slash query in progress

[sampocs]

Context

A race condition can occur between a slash query and an ICA that modifies a validator's delegation. This manifests itself in two flavors:

//	Case 1)
//	         ICQ Lands on Host                                          ICQ Ack on Stride
//	                             ICA Lands on Host    ICA Ack on Stride
//	Case 2)
//	         ICA Lands on Host                                          ICA Ack on Stride
//	                             ICQ Lands on Host    ICQ Ack on Stride

We identify if the overlap was possible by checking the following in the ICQ Ack:

• check If the delegation amount in Stride's record keeping changed since the query was submitted (case #​1)
• check if there there is currently an active ICA in progress (case #​2)

However, as if this wasn't complex enough already, there is a small edge case where an undelegation and delegation could have simultaneously occurred with equal amounts in between the slash query. This would be an example of case #1, but where our first check is unable to identify the overlap because the net delegation did not actually change.

Ex:
ICQ submitted                  
      ICA Undelegate lands on host           [true delegation -10]
ICQ lands on host                            [looks like a slash]    
              ICA delegate lands on host     [true delegation +10]
      ICA undelegate Ack                     [record keeping delegation -10]
             ICA delegate Ack                [record keeping delegation +10]
ICQ ack on Stride                            [delegation in record keeping did not change 
                                              → query records delegation amount less than record keeping
                                              → incorrectly processed as slash]

While the likelihood of this edge case is low, the impact is severe. The solution in this PR is to exclude validator's with a slash query in progress when submitting ICAs.

Brief Changelog

• Updated GetTargetValAmts to exclude validators with a slash query in progress (this function is called during each ICA)
• Added checks each time we access the map from GetTargetValAmts to make sure the validator is present
• Removed GetTotalValidatorDelegations function since we have this value on the host zone
• Changed GetTotalValidatorWeight to take a list of validators instead of the host zone (so a subset of validators could be provided)
• Removes sorting of validators on host zone struct - I don't think there should be any ramifications of this
• Note: With this change, we should still unbond and delegate the full amount, the distribution will just be slightly off (but will be corrected during rebalancing).

Impact described in psuedo-code

Considering it's a bit tough to review this without diving deeper into the code, here's some more detail on the changes to each ICA type.

For context, this is all a consequence of the change to GetTargetValAmts which now (a) ignores validators with a slash query in progress and (b) does not sort the array on the host zone struct (it sorts a copy)

Delegation

Before:

targets = GetTargetValAmtsForHostZone(delegationAmount) # includes all validators
for val in hostZone.Validators: # note: this is sorted from the above function
    valAmount = targets[val]
    if valAmount > 0:
       msgs.append(MsgDelegate{...})

After:

targets := GetTargetValAmtsForHostZone(delegationAmount)  # only returns validator w/o slash query in progress
for val in hostZone.Validators: # no longer sorted from above function
   if val not in targets:   # added check since not all vals are in `targets`
     continue

   valAmount = targets[val]
   if valAmount > 0:
      msgs.append(MsgDelegate{...})

Undelegation

Before:

balancedDelegations = GetTargetValAmtsForHostZone(currentDelegation - unbondAmount) # includes all validators

capacities = []
for val in hostZone.Validators:
   balanced = balancedDelegations[val]
   capacities.append({...})

capacityPrioritized = SortCapacity(capacities) # overrides the sorting done by GetTargetValAmtsForHostZone

for val in capacityPrioritized:
   msgs.append(MsgUndelegate{...})

After:

balancedDelegations = GetTargetValAmtsForHostZone(currentDelegation - unbondAmount) # excludes slash query in progress vals

capacities = []
for val in hostZone.Validators:
   if val not in balancedDelegations: # added check since not all vals are in `balanceDelegations`
     continue

   balanced = balancedDelegations[val]
   capacities.append({...})

capacityPrioritized = SortCapacity(capacities) 

for val in capacityPrioritized:
   msgs.append(MsgUndelegate{...})

Redelegate

Before:

targets = GetTargetValAmtsForHostZone(totalStakedBalance) # includes all validators

rebalancings = []
for val in hostZone.Validators:
   targetDelegation = targets[val]
   rebalancings.append({DelegationChange: targetDelegation - val.CurrentDelegation})

for rebalancing in rebalancings:
   msgs.append(MsgRedelegate{...}) 

After:

targets = GetTargetValAmtsForHostZone(totalStakedBalance) # excludes vals with slash query in progress

rebalancings = []
for val in hostZone.Validators:
   if val not in targets:  # added check since not all vals are in `targets`
      continue

   targetDelegation = targets[val]
   rebalancings.append({DelegationChange: targetDelegation - val.CurrentDelegation})

for rebalancing in rebalancings:
   msgs.append(MsgRedelegate{...}) 

[github-actions]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

[ethan-stride]

Overall much cleaner, but I have questions about if this is actually going to change the expected target outcome of rebalances, unbondings, etc. from what would have happened because of the validators removed from the set being considered. Will this also affect non-lsm related ICAs by removing the validators in these helpers rather than checking at the time of ICA?

[ethan-stride]

LGTM! This will be much cleaner to think about in the future by splitting it up this way to filter at the validator level instead of carrying the ICQ status all the way until ICAs need to be performed.

[riley-stride]

My main question is whether we should exclude from the weight calc in GetTargetValAmtsForHostZone the weight of the val who has an ICQ in flight

[jstr1121]

My main question is whether we should exclude from the weight calc in GetTargetValAmtsForHostZone the weight of the val who has an ICQ in flight

How rebalancing work if a ICQ for the val in flight? I not rebalance that validator, wouldn't it break the weight state more than rebalancing with previous ratio?

[sampocs]

@jstr1121

How rebalancing work if a ICQ for the val in flight? I not rebalance that validator, wouldn't it break the weight state more than rebalancing with previous ratio?

The validator's delegations will just remain unchanged (i.e. it will be excluded from rebalancing)