From f4840bb1275b606569f59c0d0f83bd607b9d46a5 Mon Sep 17 00:00:00 2001 From: sampocs Date: Wed, 6 Dec 2023 16:00:12 -0600 Subject: [PATCH 01/10] sort unbondings by capacity first --- deps/osmosis | 2 +- x/stakeibc/keeper/unbonding_records.go | 8 +------- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/deps/osmosis b/deps/osmosis index 3e2c326301..30532bd83e 160000 --- a/deps/osmosis +++ b/deps/osmosis @@ -1 +1 @@ -Subproject commit 3e2c326301aff138214c1d25630edb360459c0fd +Subproject commit 30532bd83ebac4e8985fb9d4ead91bc2722810fb diff --git a/x/stakeibc/keeper/unbonding_records.go b/x/stakeibc/keeper/unbonding_records.go index 676f5fae77..47a5ed5d2b 100644 --- a/x/stakeibc/keeper/unbonding_records.go +++ b/x/stakeibc/keeper/unbonding_records.go @@ -160,13 +160,7 @@ func SortUnbondingCapacityByPriority(validatorUnbondCapacity []ValidatorUnbondCa validatorA := validatorUnbondCapacity[i] validatorB := validatorUnbondCapacity[j] - balanceRatioValA, _ := validatorA.GetBalanceRatio() - balanceRatioValB, _ := validatorB.GetBalanceRatio() - - // Sort by the balance ratio first - in ascending order - so the more unbalanced validators appear first - if !balanceRatioValA.Equal(balanceRatioValB) { - return balanceRatioValA.LT(balanceRatioValB) - } + // TODO: Once more than 32 validators are supported, change back to using balance ratio first // If the ratio's are equal, use the capacity as a tie breaker // where the larget capacity comes first From 4887ef550134afe1a3ef843fc67a04eb726e6008 Mon Sep 17 00:00:00 2001 From: sampocs Date: Wed, 6 Dec 2023 20:23:43 -0600 Subject: [PATCH 02/10] first pass at consolidate function --- x/stakeibc/keeper/unbonding_records.go | 113 ++++++++++++++++++++++--- 1 file changed, 100 insertions(+), 13 deletions(-) diff --git a/x/stakeibc/keeper/unbonding_records.go b/x/stakeibc/keeper/unbonding_records.go index 47a5ed5d2b..f0e2b7bca9 100644 --- a/x/stakeibc/keeper/unbonding_records.go +++ b/x/stakeibc/keeper/unbonding_records.go @@ -202,16 +202,7 @@ func (k Keeper) GetUnbondingICAMessages( } else { unbondAmount = remainingUnbondAmount } - remainingUnbondAmount = remainingUnbondAmount.Sub(unbondAmount) - unbondToken := sdk.NewCoin(hostZone.HostDenom, unbondAmount) - - // Build the undelegate ICA messages - msgs = append(msgs, &stakingtypes.MsgUndelegate{ - DelegatorAddress: hostZone.DelegationIcaAddress, - ValidatorAddress: validatorCapacity.ValidatorAddress, - Amount: unbondToken, - }) // Build the validator splits for the callback unbondings = append(unbondings, &types.SplitDelegation{ @@ -220,6 +211,30 @@ func (k Keeper) GetUnbondingICAMessages( }) } + // If the number of messages exceeds the batch size, shrink it down the the batch size + // by re-distributing the exceess + if len(unbondings) > UndelegateICABatchSize { + unbondings, err = k.ConsolidateUnbondingMessages(totalUnbondAmount, unbondings, prioritizedUnbondCapacity, UndelegateICABatchSize) + if err != nil { + return msgs, unbondings, errorsmod.Wrapf(err, "unable to consolidate unbonding messages") + } + + // Sanity check that the number of messages is now under the batch size + if len(unbondings) > UndelegateICABatchSize { + return msgs, unbondings, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, + fmt.Sprintf("too many undelegation messages (%d) for host zone %s", len(msgs), hostZone.ChainId)) + } + } + + // Build the undelegate ICA messages from the splits + for _, unbonding := range unbondings { + msgs = append(msgs, &stakingtypes.MsgUndelegate{ + DelegatorAddress: hostZone.DelegationIcaAddress, + ValidatorAddress: unbonding.Validator, + Amount: sdk.NewCoin(hostZone.HostDenom, unbonding.Amount), + }) + } + // Sanity check that we had enough capacity to unbond if !remainingUnbondAmount.IsZero() { return msgs, unbondings, @@ -229,6 +244,82 @@ func (k Keeper) GetUnbondingICAMessages( return msgs, unbondings, nil } +// In the event that the number of generated undelegate messages exceeds the batch size, +// reduce the number of messages by dividing any excess amongst proportionally based on +// the remaining delegation +func (k Keeper) ConsolidateUnbondingMessages( + totalUnbondAmount sdkmath.Int, + initialUnbondings []*types.SplitDelegation, + unbondCapacities []ValidatorUnbondCapacity, + batchSize int, +) (finalUnbondings []*types.SplitDelegation, err error) { + // Grab the first {batch_size} number of messages from the list + // This will consist of the validators with the most capacity + unbondingsCapped := initialUnbondings[:batchSize] + + // Calculate the amount that was initially meant to be unbonded from that batch, + // and determine the remainder that needs to be redistributed + unbondAmountFromBatch := sdkmath.ZeroInt() + initialUnbondingsMap := map[string]sdkmath.Int{} + for _, unbonding := range unbondingsCapped { + unbondAmountFromBatch = unbondAmountFromBatch.Add(unbonding.Amount) + initialUnbondingsMap[unbonding.Validator] = unbonding.Amount + } + excessAmount := totalUnbondAmount.Sub(unbondAmountFromBatch) + + // Store the delegation of each validator that was expected *after* the originally + // planned unbonding went through + // e.g. If the validator had 10 before unbonding, and in the first pass, 3 was + // supposed to be unbonded, their delegation after the first pass is 7 + totalDelegationsAfterFirstPass := sdkmath.ZeroInt() + delegationsAfterFirstPass := map[string]sdkmath.Int{} + for _, capacity := range unbondCapacities { + // Only add validators that were in the initial unbonding plan + // The delegation after the first pass is calculated by taking the "current delegation" + // (aka delegation before unbonding) and subtracting the unbond amount + if initialUnbondAmount, ok := initialUnbondingsMap[capacity.ValidatorAddress]; ok { + delegationAfterFirstPass := capacity.CurrentDelegation.Sub(initialUnbondAmount) + + delegationsAfterFirstPass[capacity.ValidatorAddress] = delegationAfterFirstPass + totalDelegationsAfterFirstPass = totalDelegationsAfterFirstPass.Add(delegationAfterFirstPass) + } + } + + // This should never happen, but it protects against a division by zero error below + if totalDelegationsAfterFirstPass.IsZero() { + return finalUnbondings, errors.New("no delegations to redistribute during consolidation") + } + + // Loop through the original unbonding messages and proportionally divide out + // the excess amongst the validators in the set + for i := range unbondingsCapped { + unbonding := unbondingsCapped[i] + + var validatorUnbondIncrease sdkmath.Int + if i != len(unbondingsCapped)-1 { + // For all but the last validator, calculate their unbonding increase by + // splitting the excess proportionally in line with their remaining delegation + delegationAfterFirstPass, ok := delegationsAfterFirstPass[unbonding.Validator] + if !ok { + return finalUnbondings, fmt.Errorf("validator not found in initial unbonding plan") + } + unbondIncreaseProportion := delegationAfterFirstPass.Quo(totalDelegationsAfterFirstPass) + validatorUnbondIncrease = excessAmount.Mul(unbondIncreaseProportion) + } else { + // The last validator in the set should get any remainder from int truction + validatorUnbondIncrease = excessAmount + } + + // Build the updated message with the new amount + finalUnbondings = append(finalUnbondings, &types.SplitDelegation{ + Validator: unbonding.Validator, + Amount: unbonding.Amount.Add(validatorUnbondIncrease), + }) + } + + return finalUnbondings, nil +} + // Submits undelegation ICA messages for a given host zone // // First, the total unbond amount is determined from the epoch unbonding records @@ -304,10 +395,6 @@ func (k Keeper) UnbondFromHostZone(ctx sdk.Context, hostZone types.HostZone) err return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "Target unbonded amount was 0 for each validator") } - if len(msgs) > UndelegateICABatchSize { - return errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, fmt.Sprintf("too many undelegation messages (%d) for host zone %s", len(msgs), hostZone.ChainId)) - } - // Send the messages in batches so the gas limit isn't exceedeed for start := 0; start < len(msgs); start += UndelegateICABatchSize { end := start + UndelegateICABatchSize From 40e63745b716891865f67fc7fe7e1e557fa28aba Mon Sep 17 00:00:00 2001 From: sampocs Date: Wed, 6 Dec 2023 20:38:54 -0600 Subject: [PATCH 03/10] added unit test and debugged --- x/stakeibc/keeper/unbonding_records.go | 23 +++- ...ords_get_host_zone_unbondings_msgs_test.go | 107 ++++++++++++++++++ 2 files changed, 124 insertions(+), 6 deletions(-) diff --git a/x/stakeibc/keeper/unbonding_records.go b/x/stakeibc/keeper/unbonding_records.go index f0e2b7bca9..c009f10e9d 100644 --- a/x/stakeibc/keeper/unbonding_records.go +++ b/x/stakeibc/keeper/unbonding_records.go @@ -247,6 +247,7 @@ func (k Keeper) GetUnbondingICAMessages( // In the event that the number of generated undelegate messages exceeds the batch size, // reduce the number of messages by dividing any excess amongst proportionally based on // the remaining delegation +// This will no longer be necessary after undelegations to 32+ validators is supported func (k Keeper) ConsolidateUnbondingMessages( totalUnbondAmount sdkmath.Int, initialUnbondings []*types.SplitDelegation, @@ -265,20 +266,20 @@ func (k Keeper) ConsolidateUnbondingMessages( unbondAmountFromBatch = unbondAmountFromBatch.Add(unbonding.Amount) initialUnbondingsMap[unbonding.Validator] = unbonding.Amount } - excessAmount := totalUnbondAmount.Sub(unbondAmountFromBatch) + totalExcessAmount := totalUnbondAmount.Sub(unbondAmountFromBatch) // Store the delegation of each validator that was expected *after* the originally // planned unbonding went through // e.g. If the validator had 10 before unbonding, and in the first pass, 3 was // supposed to be unbonded, their delegation after the first pass is 7 - totalDelegationsAfterFirstPass := sdkmath.ZeroInt() - delegationsAfterFirstPass := map[string]sdkmath.Int{} + totalDelegationsAfterFirstPass := sdk.ZeroDec() + delegationsAfterFirstPass := map[string]sdk.Dec{} for _, capacity := range unbondCapacities { // Only add validators that were in the initial unbonding plan // The delegation after the first pass is calculated by taking the "current delegation" // (aka delegation before unbonding) and subtracting the unbond amount if initialUnbondAmount, ok := initialUnbondingsMap[capacity.ValidatorAddress]; ok { - delegationAfterFirstPass := capacity.CurrentDelegation.Sub(initialUnbondAmount) + delegationAfterFirstPass := sdk.NewDecFromInt(capacity.CurrentDelegation.Sub(initialUnbondAmount)) delegationsAfterFirstPass[capacity.ValidatorAddress] = delegationAfterFirstPass totalDelegationsAfterFirstPass = totalDelegationsAfterFirstPass.Add(delegationAfterFirstPass) @@ -292,6 +293,7 @@ func (k Keeper) ConsolidateUnbondingMessages( // Loop through the original unbonding messages and proportionally divide out // the excess amongst the validators in the set + excessRemaining := totalExcessAmount for i := range unbondingsCapped { unbonding := unbondingsCapped[i] @@ -304,10 +306,13 @@ func (k Keeper) ConsolidateUnbondingMessages( return finalUnbondings, fmt.Errorf("validator not found in initial unbonding plan") } unbondIncreaseProportion := delegationAfterFirstPass.Quo(totalDelegationsAfterFirstPass) - validatorUnbondIncrease = excessAmount.Mul(unbondIncreaseProportion) + validatorUnbondIncrease = sdk.NewDecFromInt(totalExcessAmount).Mul(unbondIncreaseProportion).TruncateInt() + + // Decrement excess + excessRemaining = excessRemaining.Sub(validatorUnbondIncrease) } else { // The last validator in the set should get any remainder from int truction - validatorUnbondIncrease = excessAmount + validatorUnbondIncrease = excessRemaining } // Build the updated message with the new amount @@ -317,6 +322,12 @@ func (k Keeper) ConsolidateUnbondingMessages( }) } + // Sanity check that we've accounted for all the excess + if excessRemaining.IsZero() { + return finalUnbondings, fmt.Errorf("Unable to redistribute all excess - initial: %v, remaining: %v", + totalExcessAmount, excessRemaining) + } + return finalUnbondings, nil } diff --git a/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go b/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go index 4148b1ada8..cbca90642e 100644 --- a/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go +++ b/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go @@ -888,3 +888,110 @@ func (s *KeeperTestSuite) TestGetUnbondingICAMessages() { }) } } + +func (s *KeeperTestSuite) TestConsolidateUnbondingMessages() { + batchSize := 4 + totalUnbondAmount := int64(1501) + excessUnbondAmount := int64(101) + + validatorMetadata := []struct { + address string + initialUnbondAmount int64 + remainingDelegation int64 + expectedDelegationIncrease int64 + }{ + // Total Remaining Portion: 1000 + 500 + 250 + 250 = 2000 + // Excess Portion = Remaining Delegation / Total Remaining Portion + + // Excess Portion: 1000 / 2000 = 50% + // Delegation Increase: 50% * 101 = 50 + {address: "val1", initialUnbondAmount: 500, remainingDelegation: 1000, expectedDelegationIncrease: 50}, + // Excess Portion: 500 / 2000 = 25% + // Delegation Increase: 25% * 101 = 25 + {address: "val2", initialUnbondAmount: 400, remainingDelegation: 500, expectedDelegationIncrease: 25}, + // Excess Portion: 250 / 2000 = 12.5% + // Delegation Increase: 12.5% * 101 = 12 + {address: "val3", initialUnbondAmount: 300, remainingDelegation: 250, expectedDelegationIncrease: 12}, + // Excess Portion: 250 / 2000 = 12.5% (gets overflow) + // Delegation Increase (overflow): 101 - 25 - 12 = 14 + {address: "val4", initialUnbondAmount: 200, remainingDelegation: 250, expectedDelegationIncrease: 14}, + + // Total Excess: 51 + 50 = 101 + {address: "val5", initialUnbondAmount: 51}, // excess + {address: "val6", initialUnbondAmount: 50}, // excess + } + + // Validate test setup - amounts in the list should match the expected totals + totalCheck := int64(0) + excessCheckFromInitial := int64(0) + excessCheckFromExpected := int64(0) + for i, validator := range validatorMetadata { + totalCheck += validator.initialUnbondAmount + if i >= batchSize { + excessCheckFromInitial += validator.initialUnbondAmount + excessCheckFromExpected += validator.initialUnbondAmount + } + } + s.Require().Equal(totalUnbondAmount, totalCheck, + "mismatch in test case setup - sum of initial unbond amount does not match total") + s.Require().Equal(excessUnbondAmount, excessCheckFromInitial, + "mismatch in test case setup - sum of excess from initial unbond amount does not match total excess") + s.Require().Equal(excessUnbondAmount, excessCheckFromExpected, + "mismatch in test case setup - sum of excess from expected delegation increase does not match total excess") + + // Retroactively build validator capacities and messages + // Also build the expected unbondings after the consolidation + initialUnbondings := []*types.SplitDelegation{} + expectedUnbondings := []*types.SplitDelegation{} + validatorCapacities := []keeper.ValidatorUnbondCapacity{} + for i, validator := range validatorMetadata { + // The "unbondings" are the initial unbond amounts + initialUnbondings = append(initialUnbondings, &types.SplitDelegation{ + Validator: validator.address, + Amount: sdkmath.NewInt(validator.initialUnbondAmount), + }) + + // The "capacity" should also be the initial unbond amount + // (we're assuming all validators tried to unbond to capacity) + // The "current delegation" is their delegation before the unbonding, + // which equals the initial unbond amount + the remainder + validatorCapacities = append(validatorCapacities, keeper.ValidatorUnbondCapacity{ + ValidatorAddress: validator.address, + Capacity: sdkmath.NewInt(validator.initialUnbondAmount), + CurrentDelegation: sdkmath.NewInt(validator.initialUnbondAmount + validator.remainingDelegation), + }) + + // The expected unbondings is their initial unbond amount plus the increase + if i < batchSize { + expectedUnbondings = append(expectedUnbondings, &types.SplitDelegation{ + Validator: validator.address, + Amount: sdkmath.NewInt(validator.initialUnbondAmount + validator.expectedDelegationIncrease), + }) + } + } + + // Call the consolidation function + finalUnbondings, err := s.App.StakeibcKeeper.ConsolidateUnbondingMessages( + sdkmath.NewInt(totalUnbondAmount), + initialUnbondings, + validatorCapacities, + batchSize, + ) + s.Require().NoError(err, "no error expected when consolidating unbonding messages") + + // Validate the final messages matched expectations + s.Require().Equal(batchSize, len(finalUnbondings), "number of consolidated unbondings") + + for i := range finalUnbondings { + validator := validatorMetadata[i] + initialUnbonding := initialUnbondings[i] + expectedUnbonding := expectedUnbondings[i] + finalUnbonding := finalUnbondings[i] + + s.Require().Equal(expectedUnbonding.Validator, finalUnbonding.Validator, + "validator address of output message - %d", i) + s.Require().Equal(expectedUnbonding.Amount.Int64(), finalUnbonding.Amount.Int64(), + "%s - validator final unbond amount should have increased by %d from %d", + expectedUnbonding.Validator, validator.expectedDelegationIncrease, initialUnbonding.Amount.Int64()) + } +} From 5156bacca686825718082ba0bf095b8a74e8dc90 Mon Sep 17 00:00:00 2001 From: sampocs Date: Wed, 6 Dec 2023 20:54:25 -0600 Subject: [PATCH 04/10] added test case to GetUnbondingICAMessages --- x/stakeibc/keeper/unbonding_records.go | 14 +++++++--- ...ords_get_host_zone_unbondings_msgs_test.go | 27 ++++++++++++++++++- x/stakeibc/keeper/undelegate_host.go | 2 +- 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/x/stakeibc/keeper/unbonding_records.go b/x/stakeibc/keeper/unbonding_records.go index c009f10e9d..1b82b3c3cf 100644 --- a/x/stakeibc/keeper/unbonding_records.go +++ b/x/stakeibc/keeper/unbonding_records.go @@ -185,6 +185,7 @@ func (k Keeper) GetUnbondingICAMessages( hostZone types.HostZone, totalUnbondAmount sdkmath.Int, prioritizedUnbondCapacity []ValidatorUnbondCapacity, + batchSize int, ) (msgs []proto.Message, unbondings []*types.SplitDelegation, err error) { // Loop through each validator and unbond as much as possible remainingUnbondAmount := totalUnbondAmount @@ -213,14 +214,14 @@ func (k Keeper) GetUnbondingICAMessages( // If the number of messages exceeds the batch size, shrink it down the the batch size // by re-distributing the exceess - if len(unbondings) > UndelegateICABatchSize { - unbondings, err = k.ConsolidateUnbondingMessages(totalUnbondAmount, unbondings, prioritizedUnbondCapacity, UndelegateICABatchSize) + if len(unbondings) > batchSize { + unbondings, err = k.ConsolidateUnbondingMessages(totalUnbondAmount, unbondings, prioritizedUnbondCapacity, batchSize) if err != nil { return msgs, unbondings, errorsmod.Wrapf(err, "unable to consolidate unbonding messages") } // Sanity check that the number of messages is now under the batch size - if len(unbondings) > UndelegateICABatchSize { + if len(unbondings) > batchSize { return msgs, unbondings, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, fmt.Sprintf("too many undelegation messages (%d) for host zone %s", len(msgs), hostZone.ChainId)) } @@ -396,7 +397,12 @@ func (k Keeper) UnbondFromHostZone(ctx sdk.Context, hostZone types.HostZone) err } // Get the undelegation ICA messages and split delegations for the callback - msgs, unbondings, err := k.GetUnbondingICAMessages(hostZone, totalUnbondAmount, prioritizedUnbondCapacity) + msgs, unbondings, err := k.GetUnbondingICAMessages( + hostZone, + totalUnbondAmount, + prioritizedUnbondCapacity, + UndelegateICABatchSize, + ) if err != nil { return err } diff --git a/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go b/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go index cbca90642e..66e90b39db 100644 --- a/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go +++ b/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go @@ -770,11 +770,24 @@ func (s *KeeperTestSuite) TestGetUnbondingICAMessages() { DelegationIcaAddress: delegationAddress, } + batchSize := 4 validatorCapacities := []keeper.ValidatorUnbondCapacity{ {ValidatorAddress: "val1", Capacity: sdkmath.NewInt(100)}, {ValidatorAddress: "val2", Capacity: sdkmath.NewInt(200)}, {ValidatorAddress: "val3", Capacity: sdkmath.NewInt(300)}, {ValidatorAddress: "val4", Capacity: sdkmath.NewInt(400)}, + + // This validator will fall out of the batch and will be redistributed + {ValidatorAddress: "val5", Capacity: sdkmath.NewInt(1000)}, + } + + // Set the current delegation to 1000 + capacity so after their delegation + // after the first pass at unbonding is left at 1000 + // This is so that we can simulate consolidating messages after reaching + // the capacity of the first four validators + for i, capacity := range validatorCapacities[:batchSize] { + capacity.CurrentDelegation = sdkmath.NewInt(1000).Add(capacity.Capacity) + validatorCapacities[i] = capacity } testCases := []struct { @@ -841,9 +854,20 @@ func (s *KeeperTestSuite) TestGetUnbondingICAMessages() { {Validator: "val4", UnbondAmount: sdkmath.NewInt(400)}, }, }, + { + name: "unbonding requires message consolidation", + totalUnbondAmount: sdkmath.NewInt(2000), // excess of 1000 + expectedUnbondings: []ValidatorUnbonding{ + // Redistributed excess denoted after plus sign + {Validator: "val1", UnbondAmount: sdkmath.NewInt(100 + 250)}, + {Validator: "val2", UnbondAmount: sdkmath.NewInt(200 + 250)}, + {Validator: "val3", UnbondAmount: sdkmath.NewInt(300 + 250)}, + {Validator: "val4", UnbondAmount: sdkmath.NewInt(400 + 250)}, + }, + }, { name: "insufficient delegation", - totalUnbondAmount: sdkmath.NewInt(1001), + totalUnbondAmount: sdkmath.NewInt(2001), expectedError: "unable to unbond full amount", }, } @@ -855,6 +879,7 @@ func (s *KeeperTestSuite) TestGetUnbondingICAMessages() { hostZone, tc.totalUnbondAmount, validatorCapacities, + batchSize, ) // If this is an error test case, check the error message diff --git a/x/stakeibc/keeper/undelegate_host.go b/x/stakeibc/keeper/undelegate_host.go index 98b13ffb06..3ef023f30b 100644 --- a/x/stakeibc/keeper/undelegate_host.go +++ b/x/stakeibc/keeper/undelegate_host.go @@ -80,7 +80,7 @@ func (k Keeper) UndelegateHostEvmos(ctx sdk.Context, totalUnbondAmount math.Int) } // Get the undelegation ICA messages and split delegations for the callback - msgs, unbondings, err := k.GetUnbondingICAMessages(evmosHost, totalUnbondAmount, prioritizedUnbondCapacity) + msgs, unbondings, err := k.GetUnbondingICAMessages(evmosHost, totalUnbondAmount, prioritizedUnbondCapacity, UndelegateICABatchSize) if err != nil { return err } From c28539c1b1056747e2d1806e715b551864236c10 Mon Sep 17 00:00:00 2001 From: sampocs Date: Wed, 13 Dec 2023 16:50:45 -0600 Subject: [PATCH 05/10] addressed ethan pr comment --- x/stakeibc/keeper/unbonding_records.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/x/stakeibc/keeper/unbonding_records.go b/x/stakeibc/keeper/unbonding_records.go index 1b82b3c3cf..8a3c42507d 100644 --- a/x/stakeibc/keeper/unbonding_records.go +++ b/x/stakeibc/keeper/unbonding_records.go @@ -249,6 +249,7 @@ func (k Keeper) GetUnbondingICAMessages( // reduce the number of messages by dividing any excess amongst proportionally based on // the remaining delegation // This will no longer be necessary after undelegations to 32+ validators is supported +// NOTE: This assumes unbondCapacities are stored in order of capacity func (k Keeper) ConsolidateUnbondingMessages( totalUnbondAmount sdkmath.Int, initialUnbondings []*types.SplitDelegation, @@ -287,11 +288,17 @@ func (k Keeper) ConsolidateUnbondingMessages( } } - // This should never happen, but it protects against a division by zero error below + // This is to protect against a division by zero error, but this would technically be possible + // if the 32 validators with the most capacity were all 0 weight if totalDelegationsAfterFirstPass.IsZero() { return finalUnbondings, errors.New("no delegations to redistribute during consolidation") } + // Before we start dividing up the excess, make sure we have sufficient stake in the capped set to cover it + if sdk.NewDecFromInt(totalExcessAmount).GT(totalDelegationsAfterFirstPass) { + return finalUnbondings, errors.New("not enough exisiting delegation in the batch to cover the excess") + } + // Loop through the original unbonding messages and proportionally divide out // the excess amongst the validators in the set excessRemaining := totalExcessAmount From 1a591f4926ad636c5bf3497548a58966503225ff Mon Sep 17 00:00:00 2001 From: sampocs Date: Wed, 13 Dec 2023 17:11:42 -0600 Subject: [PATCH 06/10] added unit test for failure case --- ...ords_get_host_zone_unbondings_msgs_test.go | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go b/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go index 66e90b39db..5e45969403 100644 --- a/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go +++ b/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go @@ -914,7 +914,7 @@ func (s *KeeperTestSuite) TestGetUnbondingICAMessages() { } } -func (s *KeeperTestSuite) TestConsolidateUnbondingMessages() { +func (s *KeeperTestSuite) TestConsolidateUnbondingMessages_Success() { batchSize := 4 totalUnbondAmount := int64(1501) excessUnbondAmount := int64(101) @@ -1020,3 +1020,33 @@ func (s *KeeperTestSuite) TestConsolidateUnbondingMessages() { expectedUnbonding.Validator, validator.expectedDelegationIncrease, initialUnbonding.Amount.Int64()) } } + +func (s *KeeperTestSuite) TestConsolidateUnbondingMessages_Failure() { + batchSize := 4 + totalUnbondAmount := sdkmath.NewInt(1000) + + // Setup the capacities such that after the first pass, there is 1 token remaining amongst the batch + capacities := []keeper.ValidatorUnbondCapacity{ + {ValidatorAddress: "val1", Capacity: sdkmath.NewInt(100), CurrentDelegation: sdkmath.NewInt(100 + 1)}, // extra token + {ValidatorAddress: "val2", Capacity: sdkmath.NewInt(100), CurrentDelegation: sdkmath.NewInt(100)}, + {ValidatorAddress: "val3", Capacity: sdkmath.NewInt(100), CurrentDelegation: sdkmath.NewInt(100)}, + {ValidatorAddress: "val4", Capacity: sdkmath.NewInt(100), CurrentDelegation: sdkmath.NewInt(100)}, + + // Excess + {ValidatorAddress: "val5", Capacity: sdkmath.NewInt(600), CurrentDelegation: sdkmath.NewInt(600)}, + } + + // Create the unbondings such that they align with the above and each validtor unbonds their full amount + unbondings := []*types.SplitDelegation{} + for _, capacitiy := range capacities { + unbondings = append(unbondings, &types.SplitDelegation{ + Validator: capacitiy.ValidatorAddress, + Amount: capacitiy.Capacity, + }) + } + + // Call consolidate - it should fail because there is not enough remaining delegation + // on each validator to cover the excess + _, err := s.App.StakeibcKeeper.ConsolidateUnbondingMessages(totalUnbondAmount, unbondings, capacities, batchSize) + s.Require().ErrorContains(err, "not enough exisiting delegation in the batch to cover the excess") +} From 031afe18abd17e23b79c3240e6ec22051fe1d992 Mon Sep 17 00:00:00 2001 From: sampocs Date: Wed, 13 Dec 2023 17:20:03 -0600 Subject: [PATCH 07/10] second pass at naming --- x/stakeibc/keeper/unbonding_records.go | 40 +++++++++++++------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/x/stakeibc/keeper/unbonding_records.go b/x/stakeibc/keeper/unbonding_records.go index 8a3c42507d..06fecbd7a7 100644 --- a/x/stakeibc/keeper/unbonding_records.go +++ b/x/stakeibc/keeper/unbonding_records.go @@ -258,62 +258,62 @@ func (k Keeper) ConsolidateUnbondingMessages( ) (finalUnbondings []*types.SplitDelegation, err error) { // Grab the first {batch_size} number of messages from the list // This will consist of the validators with the most capacity - unbondingsCapped := initialUnbondings[:batchSize] + unbondingsBatch := initialUnbondings[:batchSize] // Calculate the amount that was initially meant to be unbonded from that batch, // and determine the remainder that needs to be redistributed - unbondAmountFromBatch := sdkmath.ZeroInt() - initialUnbondingsMap := map[string]sdkmath.Int{} - for _, unbonding := range unbondingsCapped { - unbondAmountFromBatch = unbondAmountFromBatch.Add(unbonding.Amount) - initialUnbondingsMap[unbonding.Validator] = unbonding.Amount + initialUnbondAmountFromBatch := sdkmath.ZeroInt() + initialUnbondAmountFromBatchByVal := map[string]sdkmath.Int{} + for _, unbonding := range unbondingsBatch { + initialUnbondAmountFromBatch = initialUnbondAmountFromBatch.Add(unbonding.Amount) + initialUnbondAmountFromBatchByVal[unbonding.Validator] = unbonding.Amount } - totalExcessAmount := totalUnbondAmount.Sub(unbondAmountFromBatch) + totalExcessAmount := totalUnbondAmount.Sub(initialUnbondAmountFromBatch) // Store the delegation of each validator that was expected *after* the originally // planned unbonding went through // e.g. If the validator had 10 before unbonding, and in the first pass, 3 was // supposed to be unbonded, their delegation after the first pass is 7 - totalDelegationsAfterFirstPass := sdk.ZeroDec() - delegationsAfterFirstPass := map[string]sdk.Dec{} + totalRemainingDelegationsAcrossBatch := sdk.ZeroDec() + remainingDelegationsInBatchByVal := map[string]sdk.Dec{} for _, capacity := range unbondCapacities { // Only add validators that were in the initial unbonding plan // The delegation after the first pass is calculated by taking the "current delegation" // (aka delegation before unbonding) and subtracting the unbond amount - if initialUnbondAmount, ok := initialUnbondingsMap[capacity.ValidatorAddress]; ok { - delegationAfterFirstPass := sdk.NewDecFromInt(capacity.CurrentDelegation.Sub(initialUnbondAmount)) + if initialUnbondAmount, ok := initialUnbondAmountFromBatchByVal[capacity.ValidatorAddress]; ok { + remainingDelegation := sdk.NewDecFromInt(capacity.CurrentDelegation.Sub(initialUnbondAmount)) - delegationsAfterFirstPass[capacity.ValidatorAddress] = delegationAfterFirstPass - totalDelegationsAfterFirstPass = totalDelegationsAfterFirstPass.Add(delegationAfterFirstPass) + remainingDelegationsInBatchByVal[capacity.ValidatorAddress] = remainingDelegation + totalRemainingDelegationsAcrossBatch = totalRemainingDelegationsAcrossBatch.Add(remainingDelegation) } } // This is to protect against a division by zero error, but this would technically be possible // if the 32 validators with the most capacity were all 0 weight - if totalDelegationsAfterFirstPass.IsZero() { + if totalRemainingDelegationsAcrossBatch.IsZero() { return finalUnbondings, errors.New("no delegations to redistribute during consolidation") } // Before we start dividing up the excess, make sure we have sufficient stake in the capped set to cover it - if sdk.NewDecFromInt(totalExcessAmount).GT(totalDelegationsAfterFirstPass) { + if sdk.NewDecFromInt(totalExcessAmount).GT(totalRemainingDelegationsAcrossBatch) { return finalUnbondings, errors.New("not enough exisiting delegation in the batch to cover the excess") } // Loop through the original unbonding messages and proportionally divide out // the excess amongst the validators in the set excessRemaining := totalExcessAmount - for i := range unbondingsCapped { - unbonding := unbondingsCapped[i] + for i := range unbondingsBatch { + unbonding := unbondingsBatch[i] var validatorUnbondIncrease sdkmath.Int - if i != len(unbondingsCapped)-1 { + if i != len(unbondingsBatch)-1 { // For all but the last validator, calculate their unbonding increase by // splitting the excess proportionally in line with their remaining delegation - delegationAfterFirstPass, ok := delegationsAfterFirstPass[unbonding.Validator] + remainingDelegation, ok := remainingDelegationsInBatchByVal[unbonding.Validator] if !ok { return finalUnbondings, fmt.Errorf("validator not found in initial unbonding plan") } - unbondIncreaseProportion := delegationAfterFirstPass.Quo(totalDelegationsAfterFirstPass) + unbondIncreaseProportion := remainingDelegation.Quo(totalRemainingDelegationsAcrossBatch) validatorUnbondIncrease = sdk.NewDecFromInt(totalExcessAmount).Mul(unbondIncreaseProportion).TruncateInt() // Decrement excess From e388120788759f76b792e4e1988a98f6623bc705 Mon Sep 17 00:00:00 2001 From: sampocs Date: Wed, 13 Dec 2023 17:30:54 -0600 Subject: [PATCH 08/10] added additional safety check --- x/stakeibc/keeper/unbonding_records.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/x/stakeibc/keeper/unbonding_records.go b/x/stakeibc/keeper/unbonding_records.go index 06fecbd7a7..04437bbc8f 100644 --- a/x/stakeibc/keeper/unbonding_records.go +++ b/x/stakeibc/keeper/unbonding_records.go @@ -311,7 +311,7 @@ func (k Keeper) ConsolidateUnbondingMessages( // splitting the excess proportionally in line with their remaining delegation remainingDelegation, ok := remainingDelegationsInBatchByVal[unbonding.Validator] if !ok { - return finalUnbondings, fmt.Errorf("validator not found in initial unbonding plan") + return finalUnbondings, fmt.Errorf("validator %s not found in initial unbonding plan", unbonding.Validator) } unbondIncreaseProportion := remainingDelegation.Quo(totalRemainingDelegationsAcrossBatch) validatorUnbondIncrease = sdk.NewDecFromInt(totalExcessAmount).Mul(unbondIncreaseProportion).TruncateInt() @@ -320,6 +320,17 @@ func (k Keeper) ConsolidateUnbondingMessages( excessRemaining = excessRemaining.Sub(validatorUnbondIncrease) } else { // The last validator in the set should get any remainder from int truction + // First confirm the validator has sufficient remaining delegation to cover this + remainingDelegation, ok := remainingDelegationsInBatchByVal[unbonding.Validator] + if !ok { + return finalUnbondings, fmt.Errorf("validator %s not found in initial unbonding plan", unbonding.Validator) + } + if sdk.NewDecFromInt(excessRemaining).GT(remainingDelegation) { + return finalUnbondings, + fmt.Errorf("validator %s does not have enough remaining delegation (%v) to cover the excess (%v)", + unbonding.Validator, remainingDelegation, excessRemaining) + } + validatorUnbondIncrease = excessRemaining } From 27f2f3c49902dec8887713562ed0302ce440d70f Mon Sep 17 00:00:00 2001 From: sampocs Date: Wed, 13 Dec 2023 17:59:51 -0600 Subject: [PATCH 09/10] fixed unit tests --- ...ords_get_host_zone_unbondings_msgs_test.go | 117 +++++++++--------- 1 file changed, 58 insertions(+), 59 deletions(-) diff --git a/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go b/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go index 5e45969403..9503c971b3 100644 --- a/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go +++ b/x/stakeibc/keeper/unbonding_records_get_host_zone_unbondings_msgs_test.go @@ -189,12 +189,10 @@ func (s *KeeperTestSuite) TestUnbondFromHostZone_Successful_UnbondOnlyZeroWeight {Address: "valG", Weight: 15, Delegation: sdkmath.NewInt(160)}, } + // TODO: Change back to two messages after 32+ validators are supported expectedUnbondings := []ValidatorUnbonding{ - // valC has #1 priority - unbond up to capacity at 40 - {Validator: "valC", UnbondAmount: sdkmath.NewInt(40)}, - // 50 - 40 = 10 unbond remaining - // valE has #2 priority - unbond up to remaining - {Validator: "valE", UnbondAmount: sdkmath.NewInt(10)}, + // valF has the most capacity (80) so it takes the full unbonding + {Validator: "valF", UnbondAmount: sdkmath.NewInt(50)}, } tc := s.SetupTestUnbondFromHostZone(totalWeight, totalStake, totalUnbondAmount, validators) @@ -274,15 +272,16 @@ func (s *KeeperTestSuite) TestUnbondFromHostZone_Successful_UnbondTotalLessThanT {Address: "valG", Weight: 15, Delegation: sdkmath.NewInt(160)}, } + // TODO: Change back to two messages after 32+ validators are supported expectedUnbondings := []ValidatorUnbonding{ - // valC has #1 priority - unbond up to capacity at 40 + // valF has highest capacity - 90 + {Validator: "valF", UnbondAmount: sdkmath.NewInt(90)}, + // 150 - 90 = 60 unbond remaining + // valC has next highest capacity - 40 {Validator: "valC", UnbondAmount: sdkmath.NewInt(40)}, - // 150 - 40 = 110 unbond remaining - // valE has #2 priority - unbond up to capacity at 30 - {Validator: "valE", UnbondAmount: sdkmath.NewInt(30)}, - // 150 - 40 - 30 = 80 unbond remaining - // valF has #3 priority - unbond up to remaining - {Validator: "valF", UnbondAmount: sdkmath.NewInt(80)}, + // 60 - 40 = 20 unbond remaining + // valB has next highest capacity - 35, unbond up to remainder of 20 + {Validator: "valB", UnbondAmount: sdkmath.NewInt(20)}, } tc := s.SetupTestUnbondFromHostZone(totalWeight, totalStake, totalUnbondAmount, validators) @@ -324,26 +323,27 @@ func (s *KeeperTestSuite) TestUnbondFromHostZone_Successful_UnbondTotalGreaterTh {Address: "valG", Weight: 15, Delegation: sdkmath.NewInt(160)}, } + // TODO: Change back to two messages after 32+ validators are supported expectedUnbondings := []ValidatorUnbonding{ - // valC has #1 priority - unbond up to capacity at 40 - {Validator: "valC", UnbondAmount: sdkmath.NewInt(40)}, - // 350 - 40 = 310 unbond remaining - // valE has #2 priority - unbond up to capacity at 30 - {Validator: "valE", UnbondAmount: sdkmath.NewInt(30)}, - // 310 - 30 = 280 unbond remaining - // valF has #3 priority - unbond up to capacity at 110 + // valF has highest capacity - 110 {Validator: "valF", UnbondAmount: sdkmath.NewInt(110)}, - // 280 - 110 = 170 unbond remaining - // valB has #4 priority - unbond up to capacity at 105 + // 350 - 110 = 240 unbond remaining + // valB has next highest capacity - 105 {Validator: "valB", UnbondAmount: sdkmath.NewInt(105)}, - // 170 - 105 = 65 unbond remaining - // valG has #5 priority - unbond up to capacity at 25 - {Validator: "valG", UnbondAmount: sdkmath.NewInt(25)}, - // 65 - 25 = 40 unbond remaining - // valD has #6 priority - unbond up to capacity at 30 + // 240 - 105 = 135 unbond remaining + // valC has next highest capacity - 40 + {Validator: "valC", UnbondAmount: sdkmath.NewInt(40)}, + // 135 - 40 = 95 unbond remaining + // valD has next highest capacity - 30 {Validator: "valD", UnbondAmount: sdkmath.NewInt(30)}, - // 40 - 30 = 10 unbond remaining - // valA has #7 priority - unbond up to remaining + // 95 - 30 = 65 unbond remaining + // valE has next highest capacity - 30 + {Validator: "valE", UnbondAmount: sdkmath.NewInt(30)}, + // 65 - 30 = 35 unbond remaining + // valG has next highest capacity - 25 + {Validator: "valG", UnbondAmount: sdkmath.NewInt(25)}, + // 35 - 25 = 10 unbond remaining + // valA covers the remainder up to it's capacity {Validator: "valA", UnbondAmount: sdkmath.NewInt(10)}, } @@ -636,38 +636,8 @@ func (s *KeeperTestSuite) TestGetValidatorUnbondCapacity() { func (s *KeeperTestSuite) TestSortUnbondingCapacityByPriority() { // First we define what the ideal list will look like after sorting + // TODO: Change back to sorting by unbond ratio after 32+ validators are supported expectedSortedCapacities := []keeper.ValidatorUnbondCapacity{ - // Zero-weight validator's - { - // (1) Ratio: 0, Capacity: 100 - ValidatorAddress: "valE", - BalancedDelegation: sdkmath.NewInt(0), - CurrentDelegation: sdkmath.NewInt(100), // ratio = 0/100 - Capacity: sdkmath.NewInt(100), - }, - { - // (2) Ratio: 0, Capacity: 25 - ValidatorAddress: "valC", - BalancedDelegation: sdkmath.NewInt(0), - CurrentDelegation: sdkmath.NewInt(25), // ratio = 0/25 - Capacity: sdkmath.NewInt(25), - }, - { - // (3) Ratio: 0, Capacity: 25 - // Same ratio and capacity as above but name is tie breaker - ValidatorAddress: "valD", - BalancedDelegation: sdkmath.NewInt(0), - CurrentDelegation: sdkmath.NewInt(25), // ratio = 0/25 - Capacity: sdkmath.NewInt(25), - }, - // Non-zero-weight validator's - { - // (4) Ratio: 0.1 - ValidatorAddress: "valB", - BalancedDelegation: sdkmath.NewInt(1), - CurrentDelegation: sdkmath.NewInt(10), // ratio = 1/10 - Capacity: sdkmath.NewInt(9), - }, { // (5) Ratio: 0.25 ValidatorAddress: "valH", @@ -675,6 +645,13 @@ func (s *KeeperTestSuite) TestSortUnbondingCapacityByPriority() { CurrentDelegation: sdkmath.NewInt(1000), // ratio = 250/1000 Capacity: sdkmath.NewInt(750), }, + { + // (1) Ratio: 0, Capacity: 100 + ValidatorAddress: "valE", + BalancedDelegation: sdkmath.NewInt(0), + CurrentDelegation: sdkmath.NewInt(100), // ratio = 0/100 + Capacity: sdkmath.NewInt(100), + }, { // (6) Ratio: 0.5, Capacity: 100 ValidatorAddress: "valF", @@ -698,6 +675,28 @@ func (s *KeeperTestSuite) TestSortUnbondingCapacityByPriority() { CurrentDelegation: sdkmath.NewInt(100), // ratio = 50/100 Capacity: sdkmath.NewInt(50), }, + { + // (2) Ratio: 0, Capacity: 25 + ValidatorAddress: "valC", + BalancedDelegation: sdkmath.NewInt(0), + CurrentDelegation: sdkmath.NewInt(25), // ratio = 0/25 + Capacity: sdkmath.NewInt(25), + }, + { + // (3) Ratio: 0, Capacity: 25 + // Same ratio and capacity as above but name is tie breaker + ValidatorAddress: "valD", + BalancedDelegation: sdkmath.NewInt(0), + CurrentDelegation: sdkmath.NewInt(25), // ratio = 0/25 + Capacity: sdkmath.NewInt(25), + }, + { + // (4) Ratio: 0.1 + ValidatorAddress: "valB", + BalancedDelegation: sdkmath.NewInt(1), + CurrentDelegation: sdkmath.NewInt(10), // ratio = 1/10 + Capacity: sdkmath.NewInt(9), + }, { // (9) Ratio: 0.6 ValidatorAddress: "valA", From 50ddeb73d963dc721d167b2c05eb687a618bef57 Mon Sep 17 00:00:00 2001 From: sampocs Date: Mon, 18 Dec 2023 12:49:56 -0600 Subject: [PATCH 10/10] addressed vishal PR comments --- x/stakeibc/keeper/unbonding_records.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/x/stakeibc/keeper/unbonding_records.go b/x/stakeibc/keeper/unbonding_records.go index 04437bbc8f..30866cd61a 100644 --- a/x/stakeibc/keeper/unbonding_records.go +++ b/x/stakeibc/keeper/unbonding_records.go @@ -289,7 +289,8 @@ func (k Keeper) ConsolidateUnbondingMessages( } // This is to protect against a division by zero error, but this would technically be possible - // if the 32 validators with the most capacity were all 0 weight + // if the 32 validators with the most capacity were all 0 weight and we wanted to unbond more + // than their combined delegation if totalRemainingDelegationsAcrossBatch.IsZero() { return finalUnbondings, errors.New("no delegations to redistribute during consolidation") } @@ -304,15 +305,15 @@ func (k Keeper) ConsolidateUnbondingMessages( excessRemaining := totalExcessAmount for i := range unbondingsBatch { unbonding := unbondingsBatch[i] + remainingDelegation, ok := remainingDelegationsInBatchByVal[unbonding.Validator] + if !ok { + return finalUnbondings, fmt.Errorf("validator %s not found in initial unbonding plan", unbonding.Validator) + } var validatorUnbondIncrease sdkmath.Int if i != len(unbondingsBatch)-1 { // For all but the last validator, calculate their unbonding increase by // splitting the excess proportionally in line with their remaining delegation - remainingDelegation, ok := remainingDelegationsInBatchByVal[unbonding.Validator] - if !ok { - return finalUnbondings, fmt.Errorf("validator %s not found in initial unbonding plan", unbonding.Validator) - } unbondIncreaseProportion := remainingDelegation.Quo(totalRemainingDelegationsAcrossBatch) validatorUnbondIncrease = sdk.NewDecFromInt(totalExcessAmount).Mul(unbondIncreaseProportion).TruncateInt() @@ -321,16 +322,11 @@ func (k Keeper) ConsolidateUnbondingMessages( } else { // The last validator in the set should get any remainder from int truction // First confirm the validator has sufficient remaining delegation to cover this - remainingDelegation, ok := remainingDelegationsInBatchByVal[unbonding.Validator] - if !ok { - return finalUnbondings, fmt.Errorf("validator %s not found in initial unbonding plan", unbonding.Validator) - } if sdk.NewDecFromInt(excessRemaining).GT(remainingDelegation) { return finalUnbondings, fmt.Errorf("validator %s does not have enough remaining delegation (%v) to cover the excess (%v)", unbonding.Validator, remainingDelegation, excessRemaining) } - validatorUnbondIncrease = excessRemaining }