From b8b7ffedaafccf940a9b14ab07d3430d34bf42e6 Mon Sep 17 00:00:00 2001 From: George Tsagkarelis Date: Thu, 19 Sep 2024 16:21:05 +0200 Subject: [PATCH] lnwallet+lnrpc: use maxFeeRatio across coin selection tests --- lnrpc/walletrpc/walletkit_server_test.go | 71 +++++++++++++++++++++++- lnwallet/chanfunding/coin_select.go | 12 +--- lnwallet/chanfunding/coin_select_test.go | 50 +++++++++++++---- 3 files changed, 113 insertions(+), 20 deletions(-) diff --git a/lnrpc/walletrpc/walletkit_server_test.go b/lnrpc/walletrpc/walletkit_server_test.go index 5b848d156d..4973a0af47 100644 --- a/lnrpc/walletrpc/walletkit_server_test.go +++ b/lnrpc/walletrpc/walletkit_server_test.go @@ -162,10 +162,18 @@ func TestFundPsbtCoinSelect(t *testing.T) { // packet in bytes. expectedFee btcutil.Amount + // maxFeeRatio is the maximum fee to total output amount ratio + // that we consider valid. + maxFeeRatio float64 + // expectedErr is the expected concrete error. If not nil, then // the error must match exactly. expectedErr error + // expectedContainedErrStr is the expected string to be + // contained in the returned error. + expectedContainedErrStr string + // expectedErrType is the expected error type. If not nil, then // the error must be of this type. expectedErrType error @@ -178,6 +186,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: -1, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, expectedErrType: &chanfunding.ErrInsufficientFunds{}, }, { name: "1 p2wpkh utxo, add p2wkh change", @@ -193,6 +202,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: -1, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, expectedUtxoIndexes: []int{0}, expectChangeOutputIndex: 1, expectedFee: calcFee(0, 1, 1, 1, 0), @@ -210,6 +220,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: -1, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, changeType: chanfunding.P2TRChangeAddress, expectedUtxoIndexes: []int{0}, expectChangeOutputIndex: 1, @@ -228,6 +239,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: -1, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, expectedUtxoIndexes: []int{0}, expectChangeOutputIndex: -1, expectedFee: calcFee(0, 1, 1, 0, 0), @@ -247,6 +259,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: -1, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, changeType: chanfunding.P2WKHChangeAddress, expectedUtxoIndexes: []int{0}, expectChangeOutputIndex: -1, @@ -267,6 +280,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: -1, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, changeType: chanfunding.P2TRChangeAddress, expectedUtxoIndexes: []int{0}, expectChangeOutputIndex: -1, @@ -285,6 +299,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: 0, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, changeType: chanfunding.ExistingChangeAddress, expectedUtxoIndexes: []int{0}, expectChangeOutputIndex: 0, @@ -303,6 +318,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: 0, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, changeType: chanfunding.ExistingChangeAddress, expectedUtxoIndexes: []int{0}, expectChangeOutputIndex: 0, @@ -321,6 +337,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: 0, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, changeType: chanfunding.ExistingChangeAddress, expectedUtxoIndexes: []int{0}, expectChangeOutputIndex: 0, @@ -369,6 +386,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: 0, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, changeType: chanfunding.ExistingChangeAddress, expectedUtxoIndexes: []int{0, 1}, expectChangeOutputIndex: 0, @@ -417,6 +435,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: -1, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, changeType: chanfunding.P2TRChangeAddress, expectedUtxoIndexes: []int{0, 1}, expectChangeOutputIndex: 1, @@ -456,6 +475,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: -1, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, changeType: chanfunding.P2WKHChangeAddress, expectedUtxoIndexes: []int{}, expectChangeOutputIndex: 1, @@ -497,10 +517,51 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: -1, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, changeType: chanfunding.P2TRChangeAddress, expectedUtxoIndexes: []int{}, expectChangeOutputIndex: -1, expectedFee: calcFee(1, 0, 1, 0, 0), + }, { + name: "1 p2wpkh utxo, existing p2wkh change, invalid fee ratio", + utxos: []*lnwallet.Utxo{ + { + Value: 250, + PkScript: p2wkhScript, + }, + }, + packet: makePacket(&wire.TxOut{ + Value: 50, + PkScript: p2wkhScript, + }), + changeIndex: 0, + feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, + changeType: chanfunding.ExistingChangeAddress, + expectedUtxoIndexes: []int{0}, + expectChangeOutputIndex: 0, + expectedFee: calcFee(0, 1, 0, 1, 0), + + expectedContainedErrStr: "with max fee ratio", + }, { + name: "1 p2wpkh utxo, existing p2wkh change, big fee ratio", + utxos: []*lnwallet.Utxo{ + { + Value: 250, + PkScript: p2wkhScript, + }, + }, + packet: makePacket(&wire.TxOut{ + Value: 50, + PkScript: p2wkhScript, + }), + changeIndex: 0, + feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: 0.85, + changeType: chanfunding.ExistingChangeAddress, + expectedUtxoIndexes: []int{0}, + expectChangeOutputIndex: 0, + expectedFee: calcFee(0, 1, 0, 1, 0), }, { name: "large existing p2tr input, fee estimation existing " + "change output", @@ -536,6 +597,7 @@ func TestFundPsbtCoinSelect(t *testing.T) { }), changeIndex: 0, feeRate: chainfee.FeePerKwFloor, + maxFeeRatio: chanfunding.DefaultMaxFeeRatio, changeType: chanfunding.ExistingChangeAddress, expectedUtxoIndexes: []int{}, expectChangeOutputIndex: 0, @@ -574,7 +636,8 @@ func TestFundPsbtCoinSelect(t *testing.T) { resp, err := rpcServer.fundPsbtCoinSelect( "", tc.changeIndex, copiedPacket, 0, tc.changeType, tc.feeRate, - rpcServer.cfg.CoinSelectionStrategy, 0, + rpcServer.cfg.CoinSelectionStrategy, + tc.maxFeeRatio, ) switch { @@ -588,6 +651,12 @@ func TestFundPsbtCoinSelect(t *testing.T) { require.Error(tt, err) require.ErrorAs(tt, err, &tc.expectedErr) + return + case tc.expectedContainedErrStr != "": + require.ErrorContains( + tt, err, tc.expectedContainedErrStr, + ) + return } diff --git a/lnwallet/chanfunding/coin_select.go b/lnwallet/chanfunding/coin_select.go index 562096afdc..5b983c867f 100644 --- a/lnwallet/chanfunding/coin_select.go +++ b/lnwallet/chanfunding/coin_select.go @@ -280,9 +280,7 @@ func CalculateChangeAmount(totalInputAmt, requiredAmt, requiredFeeNoChange, // don't burn a great part to fees. totalOut := requiredAmt + changeAmt - err := sanityCheckFee( - totalOut, totalInputAmt-totalOut, maxFeeRatio, - ) + err := sanityCheckFee(totalOut, totalInputAmt-totalOut, maxFeeRatio) if err != nil { return 0, 0, err } @@ -347,9 +345,7 @@ func CoinSelectSubtractFees(feeRate chainfee.SatPerKWeight, amt, // Sanity check the resulting output values to make sure we // don't burn a great part to fees. totalOut := outputAmt + changeAmt - err = sanityCheckFee( - totalOut, totalSat-totalOut, maxFeeRatio, - ) + err = sanityCheckFee(totalOut, totalSat-totalOut, maxFeeRatio) if err != nil { return nil, 0, 0, err } @@ -448,9 +444,7 @@ func CoinSelectUpToAmount(feeRate chainfee.SatPerKWeight, minAmount, maxAmount, return sum } - err = sanityCheckFee( - totalOut, sum(selected)-totalOut, maxFeeRatio, - ) + err = sanityCheckFee(totalOut, sum(selected)-totalOut, maxFeeRatio) if err != nil { return nil, 0, 0, err } diff --git a/lnwallet/chanfunding/coin_select_test.go b/lnwallet/chanfunding/coin_select_test.go index 8242bc874e..1d77ca266a 100644 --- a/lnwallet/chanfunding/coin_select_test.go +++ b/lnwallet/chanfunding/coin_select_test.go @@ -316,7 +316,8 @@ func TestCoinSelect(t *testing.T) { selected, changeAmt, err := CoinSelect( feeRate, test.outputValue, dustLimit, test.coins, wallet.CoinSelectionLargest, - fundingOutputEstimate, test.changeType, 0, + fundingOutputEstimate, test.changeType, + DefaultMaxFeeRatio, ) if test.expectErr { @@ -356,6 +357,7 @@ func TestCalculateChangeAmount(t *testing.T) { feeWithChange btcutil.Amount dustLimit btcutil.Amount changeType ChangeAddressType + maxFeeRatio float64 expectErr string expectChangeAmt btcutil.Amount @@ -369,6 +371,7 @@ func TestCalculateChangeAmount(t *testing.T) { totalInputAmt: 500, requiredAmt: 490, feeNoChange: 12, + maxFeeRatio: DefaultMaxFeeRatio, expectNeedMore: 502, }, { @@ -383,6 +386,7 @@ func TestCalculateChangeAmount(t *testing.T) { feeWithChange: 10, dustLimit: 100, changeType: ExistingChangeAddress, + maxFeeRatio: DefaultMaxFeeRatio, expectChangeAmt: 90, }, { @@ -392,6 +396,7 @@ func TestCalculateChangeAmount(t *testing.T) { feeNoChange: 40, feeWithChange: 50, dustLimit: 100, + maxFeeRatio: DefaultMaxFeeRatio, expectChangeAmt: 150, }, { @@ -401,6 +406,7 @@ func TestCalculateChangeAmount(t *testing.T) { requiredAmt: 460, feeNoChange: 40, feeWithChange: 50, + maxFeeRatio: DefaultMaxFeeRatio, expectChangeAmt: 0, }, { @@ -410,14 +416,36 @@ func TestCalculateChangeAmount(t *testing.T) { feeNoChange: 10, feeWithChange: 45, dustLimit: 5, + maxFeeRatio: DefaultMaxFeeRatio, - expectErr: "fee (0.00000045 BTC) exceeds 20% of total output " + - "(0.00000055 BTC)", + expectErr: "fee 0.00000045 BTC on total output value " + + "0.00000055 BTC with max fee ratio of 0.20", + }, { + name: "fee percent ok", + totalInputAmt: 100, + requiredAmt: 50, + feeNoChange: 10, + feeWithChange: 45, + dustLimit: 5, + maxFeeRatio: 0.95, + + expectChangeAmt: 5, + }, { + name: "invalid max fee ratio", + totalInputAmt: 100, + requiredAmt: 50, + feeNoChange: 10, + feeWithChange: 45, + dustLimit: 5, + maxFeeRatio: 3.14, + + expectErr: "maxFeeRatio must be between 0.00 and 1.00", }, { name: "invalid usage of function", feeNoChange: 5, feeWithChange: 10, changeType: ExistingChangeAddress, + maxFeeRatio: DefaultMaxFeeRatio, expectErr: "fees for with or without change must be the same", }} @@ -428,7 +456,7 @@ func TestCalculateChangeAmount(t *testing.T) { changeAmt, needMore, err := CalculateChangeAmount( tc.totalInputAmt, tc.requiredAmt, tc.feeNoChange, tc.feeWithChange, tc.dustLimit, - tc.changeType, 0, + tc.changeType, tc.maxFeeRatio, ) if tc.expectErr != "" { @@ -606,8 +634,8 @@ func TestCoinSelectSubtractFees(t *testing.T) { }, spendValue: 5 * fundingFee(highFeeRate, 1, false), - expectErr: "fee ( BTC) exceeds % of total " + - "output ( BTC)", + expectErr: "fee BTC on total output value " + + " BTC with max fee ratio of ", }, } @@ -627,7 +655,8 @@ func TestCoinSelectSubtractFees(t *testing.T) { feeRate, test.spendValue, dustLimit, test.coins, wallet.CoinSelectionLargest, fundingOutputEstimate, - defaultChanFundingChangeType, 0, + defaultChanFundingChangeType, + DefaultMaxFeeRatio, ) if err != nil { switch { @@ -813,8 +842,8 @@ func TestCoinSelectUpToAmount(t *testing.T) { minValue: minValue, maxValue: 16 * fundingFee(feeRate, 1, false), - expectErr: "fee (0.00000192 BTC) exceeds 20% of total output " + - "(0.00000768 BTC)", + expectErr: "fee 0.00000192 BTC on total output value " + + "0.00000768 BTC with max fee ratio of 0.20", }, { // This test makes sure that the implementation detail of using // CoinSelect and CoinSelectSubtractFees is done correctly. @@ -872,7 +901,8 @@ func TestCoinSelectUpToAmount(t *testing.T) { test.reserved, dustLimit, test.coins, wallet.CoinSelectionLargest, fundingOutputEstimate, - defaultChanFundingChangeType, 0, + defaultChanFundingChangeType, + DefaultMaxFeeRatio, ) if len(test.expectErr) == 0 && err != nil { t.Fatal(err.Error())