From 2619c03d7d16a29ef44ed159be599eb79b5cac5c Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Tue, 6 Feb 2024 12:25:50 +0100 Subject: [PATCH] chanfunding: allow using existing change output We'll want to be able to tell the coin selection algorithm that we intend to add any change to an existing output instead of assuming that a change output is always created. If we add any left over change to an existing output, we can skip the dust amount check as we assume the selected existing output already has a non-dust amount requested (responsibility of the caller to assert). --- lnwallet/chanfunding/coin_select.go | 17 ++- lnwallet/chanfunding/coin_select_test.go | 125 ++++++++++++++--------- 2 files changed, 92 insertions(+), 50 deletions(-) diff --git a/lnwallet/chanfunding/coin_select.go b/lnwallet/chanfunding/coin_select.go index 88c2f7419..8151bd6ed 100644 --- a/lnwallet/chanfunding/coin_select.go +++ b/lnwallet/chanfunding/coin_select.go @@ -50,6 +50,14 @@ const ( // P2TRChangeAddress indicates that the change output should be a // P2TR output. P2TRChangeAddress ChangeAddressType = 1 + + // ExistingChangeAddress indicates that the coin selection algorithm + // should assume an existing output will be used for any change, meaning + // that the change amount calculated will be added to an existing output + // and no weight for a new change output should be assumed. The caller + // must assert that the output value of the selected existing output + // already is above dust when using this change address type. + ExistingChangeAddress ChangeAddressType = 2 ) // selectInputs selects a slice of inputs necessary to meet the specified @@ -119,6 +127,9 @@ func calculateFees(utxos []wallet.Coin, feeRate chainfee.SatPerKWeight, case P2TRChangeAddress: weightEstimate.AddP2TROutput() + case ExistingChangeAddress: + // Don't add an extra output. + default: return 0, 0, fmt.Errorf("unknown change address type: %v", changeType) @@ -203,7 +214,11 @@ func CoinSelect(feeRate chainfee.SatPerKWeight, amt, dustLimit btcutil.Amount, changeAmt = 0 } - if changeAmt < dustLimit { + // 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 } diff --git a/lnwallet/chanfunding/coin_select_test.go b/lnwallet/chanfunding/coin_select_test.go index 468acd78e..8c1010651 100644 --- a/lnwallet/chanfunding/coin_select_test.go +++ b/lnwallet/chanfunding/coin_select_test.go @@ -144,18 +144,23 @@ func TestCalculateFees(t *testing.T) { // when creating a funding transaction, and that the calculated change is the // expected amount. // -// NOTE: coinSelect will always attempt to add a change output, so we must -// account for this in the tests. +// NOTE: coinSelect will always attempt to add a change output (unless the +// ExistingChangeAddress change address type is selected), so we must account +// for this in the tests. func TestCoinSelect(t *testing.T) { t.Parallel() - const feeRate = chainfee.SatPerKWeight(100) - const dustLimit = btcutil.Amount(1000) + const ( + feeRate = chainfee.SatPerKWeight(100) + dustLimit = btcutil.Amount(1000) + fullCoin = btcutil.SatoshiPerBitcoin + ) type testCase struct { name string outputValue btcutil.Amount coins []wallet.Coin + changeType ChangeAddressType expectedInput []btcutil.Amount expectedChange btcutil.Amount @@ -172,18 +177,20 @@ func TestCoinSelect(t *testing.T) { { TxOut: wire.TxOut{ PkScript: p2wkhScript, - Value: 1 * btcutil.SatoshiPerBitcoin, + Value: 1 * fullCoin, }, }, }, - outputValue: 0.5 * btcutil.SatoshiPerBitcoin, + outputValue: 0.5 * fullCoin, + changeType: defaultChanFundingChangeType, // The one and only input will be selected. expectedInput: []btcutil.Amount{ - 1 * btcutil.SatoshiPerBitcoin, + 1 * fullCoin, }, // Change will be what's left minus the fee. - expectedChange: 0.5*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, true), + expectedChange: 0.5*fullCoin - + fundingFee(feeRate, 1, true), }, { // We have 1 BTC available, and we want to send 1 BTC. @@ -194,12 +201,14 @@ func TestCoinSelect(t *testing.T) { { TxOut: wire.TxOut{ PkScript: p2wkhScript, - Value: 1 * btcutil.SatoshiPerBitcoin, + Value: 1 * fullCoin, }, }, }, - outputValue: 1 * btcutil.SatoshiPerBitcoin, - expectErr: true, + outputValue: 1 * fullCoin, + changeType: defaultChanFundingChangeType, + + expectErr: true, }, { // We have a 1 BTC input, and want to create an output @@ -210,59 +219,90 @@ func TestCoinSelect(t *testing.T) { { TxOut: wire.TxOut{ PkScript: p2wkhScript, - Value: 1 * btcutil.SatoshiPerBitcoin, + Value: 1 * fullCoin, }, }, }, // We tune the output value by subtracting the expected - // fee and the dustlimit. - outputValue: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, false) - dustLimit, + // fee and the dust limit. + outputValue: 1*fullCoin - + fundingFee(feeRate, 1, false) - dustLimit, + changeType: defaultChanFundingChangeType, expectedInput: []btcutil.Amount{ - 1 * btcutil.SatoshiPerBitcoin, + 1 * fullCoin, }, // Change must be zero. expectedChange: 0, }, { - // We got just enough funds to create a change output above the - // dust limit. - name: "change right above dustlimit", + // We got just enough funds to create a change output + // above the dust limit. + name: "change right above dust limit", coins: []wallet.Coin{ { TxOut: wire.TxOut{ PkScript: p2wkhScript, - Value: int64(fundingFee(feeRate, 1, true) + 2*(dustLimit+1)), + Value: int64(fundingFee( + feeRate, 1, true, + ) + 2*(dustLimit+1)), }, }, }, - // We tune the output value to be just above the dust limit. + // We tune the output value to be just above the dust + // limit. outputValue: dustLimit + 1, + changeType: defaultChanFundingChangeType, expectedInput: []btcutil.Amount{ fundingFee(feeRate, 1, true) + 2*(dustLimit+1), }, - // After paying for the fee the change output should be just above - // the dust limit. + // After paying for the fee the change output should be + // just above the dust limit. expectedChange: dustLimit + 1, }, { - // If more than 20% of funds goes to fees, it should fail. + // If more than 20% of funds goes to fees, it should + // fail. name: "high fee", coins: []wallet.Coin{ { TxOut: wire.TxOut{ PkScript: p2wkhScript, - Value: int64(5 * fundingFee(feeRate, 1, false)), + Value: int64(5 * fundingFee( + feeRate, 1, false, + )), }, }, }, outputValue: 4 * fundingFee(feeRate, 1, false), + changeType: defaultChanFundingChangeType, expectErr: true, }, + { + // Fees go to an existing change output. + name: "existing change output", + coins: []wallet.Coin{ + { + TxOut: wire.TxOut{ + PkScript: p2wkhScript, + Value: 1000 + int64(fundingFee( + feeRate, 1, false, + )) + 1, + }, + }, + }, + outputValue: 1000, + changeType: ExistingChangeAddress, + + expectedInput: []btcutil.Amount{ + 1000 + fundingFee(feeRate, 1, false) + 1, + }, + expectedChange: 1, + }, } fundingOutputEstimate := input.TxWeightEstimator{} @@ -274,43 +314,30 @@ func TestCoinSelect(t *testing.T) { t.Parallel() selected, changeAmt, err := CoinSelect( - feeRate, test.outputValue, dustLimit, test.coins, - wallet.CoinSelectionLargest, - fundingOutputEstimate, - defaultChanFundingChangeType, + feeRate, test.outputValue, dustLimit, + test.coins, wallet.CoinSelectionLargest, + fundingOutputEstimate, test.changeType, ) - if !test.expectErr && err != nil { - t.Fatalf(err.Error()) - } - if test.expectErr && err == nil { - t.Fatalf("expected error") - } - - // If we got an expected error, there is nothing more to test. if test.expectErr { + require.Error(t, err) + return } + require.NoError(t, err) + // Check that the selected inputs match what we expect. - if len(selected) != len(test.expectedInput) { - t.Fatalf("expected %v inputs, got %v", - len(test.expectedInput), len(selected)) - } + require.Len(t, selected, len(test.expectedInput)) for i, coin := range selected { - if coin.Value != int64(test.expectedInput[i]) { - t.Fatalf("expected input %v to have value %v, "+ - "had %v", i, test.expectedInput[i], - coin.Value) - } + require.EqualValues( + t, test.expectedInput[i], coin.Value, + ) } // Assert we got the expected change amount. - if changeAmt != test.expectedChange { - t.Fatalf("expected %v change amt, got %v", - test.expectedChange, changeAmt) - } + require.EqualValues(t, test.expectedChange, changeAmt) }) } }