diff --git a/lnwallet/chanfunding/coin_select.go b/lnwallet/chanfunding/coin_select.go index 8151bd6ed..5594d26b1 100644 --- a/lnwallet/chanfunding/coin_select.go +++ b/lnwallet/chanfunding/coin_select.go @@ -185,55 +185,95 @@ func CoinSelect(feeRate chainfee.SatPerKWeight, amt, dustLimit btcutil.Amount, return nil, 0, err } - // The difference between the selected amount and the amount - // requested will be used to pay fees, and generate a change - // output with the remaining. - overShootAmt := totalSat - amt - - var changeAmt btcutil.Amount - - switch { - // If the excess amount isn't enough to pay for fees based on - // fee rate and estimated size without using a change output, - // then increase the requested coin amount by the estimate - // required fee without using change, performing another round - // of coin selection. - case overShootAmt < requiredFeeNoChange: - amtNeeded = amt + requiredFeeNoChange - continue - - // If sufficient funds were selected to cover the fee required - // to include a change output, the remainder will be our change - // amount. - case overShootAmt > requiredFeeWithChange: - changeAmt = overShootAmt - requiredFeeWithChange - - // Otherwise we have selected enough to pay for a tx without a - // change output. - default: - changeAmt = 0 - } - - // In case we would end up with a dust output if we created a - // change output, we instead just let the dust amount go to - // fees. Unless we want the change to go to an existing output, - // in that case we can increase that output value by any amount. - if changeAmt < dustLimit && changeType != ExistingChangeAddress { - changeAmt = 0 - } - - // Sanity check the resulting output values to make sure we - // don't burn a great part to fees. - totalOut := amt + changeAmt - err = sanityCheckFee(totalOut, totalSat-totalOut) + changeAmount, newAmtNeeded, err := CalculateChangeAmount( + totalSat, amt, requiredFeeNoChange, + requiredFeeWithChange, dustLimit, changeType, + ) if err != nil { return nil, 0, err } - return selectedUtxos, changeAmt, nil + // Need another round, the selected coins aren't enough to pay + // for the fees. + if newAmtNeeded != 0 { + amtNeeded = newAmtNeeded + + continue + } + + // Coin selection was successful. + return selectedUtxos, changeAmount, nil } } +// CalculateChangeAmount calculates the change amount being left over when the +// given total amount of sats is provided as inputs for the required output +// amount. The calculation takes into account that we might not want to add a +// change output if the change amount is below the dust limit. The first amount +// returned is the change amount. If that is non-zero, change is left over and +// should be dealt with. The second amount, if non-zero, indicates that the +// total input amount was just not enough to pay for the required amount and +// fees and that more coins need to be selected. +func CalculateChangeAmount(totalInputAmt, requiredAmt, requiredFeeNoChange, + requiredFeeWithChange, dustLimit btcutil.Amount, + changeType ChangeAddressType) (btcutil.Amount, btcutil.Amount, error) { + + // This is just a sanity check to make sure the function is used + // correctly. + if changeType == ExistingChangeAddress && + requiredFeeNoChange != requiredFeeWithChange { + + return 0, 0, fmt.Errorf("when using existing change address, " + + "the fees for with or without change must be the same") + } + + // The difference between the selected amount and the amount + // requested will be used to pay fees, and generate a change + // output with the remaining. + overShootAmt := totalInputAmt - requiredAmt + + var changeAmt btcutil.Amount + + switch { + // If the excess amount isn't enough to pay for fees based on + // fee rate and estimated size without using a change output, + // then increase the requested coin amount by the estimate + // required fee without using change, performing another round + // of coin selection. + case overShootAmt < requiredFeeNoChange: + return 0, requiredAmt + requiredFeeNoChange, nil + + // If sufficient funds were selected to cover the fee required + // to include a change output, the remainder will be our change + // amount. + case overShootAmt > requiredFeeWithChange: + changeAmt = overShootAmt - requiredFeeWithChange + + // Otherwise we have selected enough to pay for a tx without a + // change output. + default: + changeAmt = 0 + } + + // In case we would end up with a dust output if we created a + // change output, we instead just let the dust amount go to + // fees. Unless we want the change to go to an existing output, + // in that case we can increase that output value by any amount. + if changeAmt < dustLimit && changeType != ExistingChangeAddress { + changeAmt = 0 + } + + // Sanity check the resulting output values to make sure we + // don't burn a great part to fees. + totalOut := requiredAmt + changeAmt + err := sanityCheckFee(totalOut, totalInputAmt-totalOut) + if err != nil { + return 0, 0, err + } + + return changeAmt, 0, nil +} + // CoinSelectSubtractFees attempts to select coins such that we'll spend up to // amt in total after fees, adhering to the specified fee rate. The selected // coins, the final output and change values are returned. diff --git a/lnwallet/chanfunding/coin_select_test.go b/lnwallet/chanfunding/coin_select_test.go index 8c1010651..43027f32f 100644 --- a/lnwallet/chanfunding/coin_select_test.go +++ b/lnwallet/chanfunding/coin_select_test.go @@ -342,6 +342,106 @@ func TestCoinSelect(t *testing.T) { } } +// TestCalculateChangeAmount tests that the change amount calculation performs +// correctly, taking into account the type of change output and whether we want +// to create a change output in the first place. +func TestCalculateChangeAmount(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + totalInputAmt btcutil.Amount + requiredAmt btcutil.Amount + feeNoChange btcutil.Amount + feeWithChange btcutil.Amount + dustLimit btcutil.Amount + changeType ChangeAddressType + + expectErr string + expectChangeAmt btcutil.Amount + expectNeedMore btcutil.Amount + }{{ + // Coin selection returned a coin larger than the required + // amount, but still not enough to pay for the fees. This should + // trigger another round of coin selection with a larger + // required amount. + name: "need to select more", + totalInputAmt: 500, + requiredAmt: 490, + feeNoChange: 12, + + expectNeedMore: 502, + }, { + // We are using an existing change output, so we'll only want + // to make sure to select enough for a TX _without_ a change + // output added. Because we're using an existing output, the + // dust limit calculation should also be skipped. + name: "sufficiently large for existing change output", + totalInputAmt: 500, + requiredAmt: 400, + feeNoChange: 10, + feeWithChange: 10, + dustLimit: 100, + changeType: ExistingChangeAddress, + + expectChangeAmt: 90, + }, { + name: "sufficiently large for adding a change output", + totalInputAmt: 500, + requiredAmt: 300, + feeNoChange: 40, + feeWithChange: 50, + dustLimit: 100, + + expectChangeAmt: 150, + }, { + name: "sufficiently large for tx without change " + + "amount", + totalInputAmt: 500, + requiredAmt: 460, + feeNoChange: 40, + feeWithChange: 50, + + expectChangeAmt: 0, + }, { + name: "fee percent too large", + totalInputAmt: 100, + requiredAmt: 50, + feeNoChange: 10, + feeWithChange: 45, + dustLimit: 5, + + expectErr: "fee 0.00000045 BTC on total output value " + + "0.00000055", + }, { + name: "invalid usage of function", + feeNoChange: 5, + feeWithChange: 10, + changeType: ExistingChangeAddress, + + expectErr: "fees for with or without change must be the same", + }} + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(tt *testing.T) { + changeAmt, needMore, err := CalculateChangeAmount( + tc.totalInputAmt, tc.requiredAmt, + tc.feeNoChange, tc.feeWithChange, tc.dustLimit, + tc.changeType, + ) + + if tc.expectErr != "" { + require.ErrorContains(tt, err, tc.expectErr) + return + } + + require.EqualValues(tt, tc.expectChangeAmt, changeAmt) + require.EqualValues(tt, tc.expectNeedMore, needMore) + }) + } +} + // TestCoinSelectSubtractFees tests that we pick coins adding up to the // expected amount when creating a funding transaction, and that a change // output is created only when necessary.