staking msgServer.CancelUnbondingDelegation could possibly choose mismatching UnbondingDelegationEntry to cancel

[adu-crypto]

Summary of Bug

this is an issue found in cronos latest simulation: crypto-org-chain/cronos#645.
Generally speaking, two UnbondingDelegationEntry could have the same CreationHeight, and the current unbond entry matching logic of msgServer.CancelUnbondingDelegation is ambiguous when dealing with this case.

Version

main and v0.46

Steps to Reproduce

refer to crypto-org-chain/cronos#645

[anilcse]

This we discussed internally with @gsk967 and possible solution can be:

• Restrict user to cancel entire unbonding. The original implementation allowed user to cancel a part (can be full) of the unbonding. Like if there are 1000token unbonding, user can cancel 500tokens if they wish. If we have to match the amount, we should remove this flexibility

cc @gsk967

[anilcse]

We can update the docs now to clarify this behavior and introduce a breaking chage in next release which would allow delegator to either force cancel-ubd with exact amount or allow a partical cancellation of ubd.

[yihuang]

I think there are two issues here:

1. currently, our simulation tests fail because of this, that probably can be fixed at the test case generation logic, maybe don't generate multiple unbonding requests at the same block?
2. is current behavior expected or problematic?

[anilcse]

It is expected behavior as per the implementation. But best solution can be giving an option for the delegator to chose either to cancel-entire unbonding or partial. Which is not there atm. It's not that problematic afaik. Having multiple unbondings between same-validator and delegator combination sounds rare to me. imo, it's not critical, should address in next release.

1. currently, our simulation tests fail because of this, that probably can be fixed at the test case generation logic, maybe don't generate multiple unbonding requests at the same block?

Yes, that should fix your simulations. You can consider multiple unbondings from different delegators<>validator combo if required.

[adu-crypto]

I think the current behavior is misleading.
the root cause is that we don't have the unique identifier for UnbondingDelegationEntry, so the delegator could not correctly specify which unbonding entry to cancel.

[anilcse]

Yes, the fix for this can be either to consdier unbonding-entry input from user or match exact amount

[anilcse]

For better UX, I would advise for matching exact amount instead of asking user to input the ubd entry. If there are multiple entries of same amount, I don't see any problem in cancelling any one of them.

[alexanderbez]

For better UX, I would advise for matching exact amount instead of asking user to input the ubd entry. If there are multiple entries of same amount, I don't see any problem in cancelling any one of them.

agreed.

[gsk967]

@anilcse @marbar3778 @alexanderbez

When delegator broadcast the multiple unbonding delegations msgs on same height, we are generating the multiple records with same creation_height and completion_time

instead of this way, can't we just update the balance and initial_balance to the existing unbonding delegation

Sample Query

simd q staking unbonding-delegations cosmos1huplz9naax6qe0g2pdaupjcmv4y0fxvhhkte73
pagination:
  next_key: null
  total: "0"
unbonding_responses:
- delegator_address: cosmos1huplz9naax6qe0g2pdaupjcmv4y0fxvhhkte73
  entries:
  - balance: "100"
    completion_time: "2022-09-08T10:35:22.277275812Z"
    creation_height: "110"
    initial_balance: "100"
  - balance: "20"
    completion_time: "2022-09-08T10:35:22.277275812Z"
    creation_height: "110"
    initial_balance: "20"
  - balance: "30"
    completion_time: "2022-09-08T10:35:22.277275812Z"
    creation_height: "110"
    initial_balance: "30"
  validator_address: cosmosvaloper16q08najdx3gu3gk5zyh4p7sa94knz4ukj5kclz

Solution:

// AddEntry - append entry to the unbonding delegation
func (ubd *UnbondingDelegation) AddEntry(creationHeight int64, minTime time.Time, balance math.Int) {
    // Check the entries exists with creation_height and complete_time
    var entryIndex int64 = -1 
    for index, ubdEntry := ranage ubd.Entries {
        if ubdEntry.CreationHeight == creationHeight && ubdEntry.CompletionTime.Equal(minTime){
            entryIndex = index
            break
        }
    }
    // entryIndex exists 
    if entryIndex != -1 {
        ubdEntry = ubd.Entries[entryIndex]
        ubdEntry.Balance =  ubdEntry.Balance.Add(balance)
        ubdEntry.InitialBalance =  ubdEntry.InitialBalance.Add(balance)

        // update the entry 
        ubd.Entries[entryIndex] = ubdEntry
    }else{
        // append the new unbond delegation entry 
        entry := NewUnbondingDelegationEntry(creationHeight, minTime, balance)
	    ubd.Entries = append(ubd.Entries, entry)
    }
}

[anilcse]

This would save us some storage too. Agree with this

[alexanderbez]

@gsk967 I can't see why that wouldn't work. I think it makes sense. Let's do it!