From 9e825e6c0fab499ce77f1504db1e324bacc12fd7 Mon Sep 17 00:00:00 2001 From: Bjarne Magnussen Date: Wed, 19 Aug 2020 11:54:19 +0200 Subject: [PATCH 1/5] chanfunding: assure logic for dust output Without this change the fee for the test case is above 20% and hence the error could be caught by the high-fee logic instead. --- lnwallet/chanfunding/coin_select_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lnwallet/chanfunding/coin_select_test.go b/lnwallet/chanfunding/coin_select_test.go index 67b24976..38a1a7e1 100644 --- a/lnwallet/chanfunding/coin_select_test.go +++ b/lnwallet/chanfunding/coin_select_test.go @@ -250,7 +250,7 @@ func TestCoinSelectSubtractFees(t *testing.T) { { TxOut: wire.TxOut{ PkScript: p2wkhScript, - Value: int64(fundingFee(feeRate, 1, false) + dust), + Value: int64(fundingFee(feeRate, 1, false) + dustLimit), }, }, }, From 07549d50ff4209265f4f421530b44875fae5af90 Mon Sep 17 00:00:00 2001 From: Bjarne Magnussen Date: Wed, 19 Aug 2020 12:00:05 +0200 Subject: [PATCH 2/5] chanfunding: assure logic for high-fee Without this change the high-fee logic is never tested as it is instead caught by the dust-output logic. This change uses a higher fee rate to ensure an output value above the dust limit, while still spending 20% on fees. --- lnwallet/chanfunding/coin_select_test.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lnwallet/chanfunding/coin_select_test.go b/lnwallet/chanfunding/coin_select_test.go index 38a1a7e1..36cc64fd 100644 --- a/lnwallet/chanfunding/coin_select_test.go +++ b/lnwallet/chanfunding/coin_select_test.go @@ -205,11 +205,13 @@ func TestCoinSelectSubtractFees(t *testing.T) { t.Parallel() const feeRate = chainfee.SatPerKWeight(100) + const highFeeRate = chainfee.SatPerKWeight(1000) const dustLimit = btcutil.Amount(1000) const dust = btcutil.Amount(100) type testCase struct { name string + highFee bool spendValue btcutil.Amount coins []Coin @@ -320,16 +322,17 @@ func TestCoinSelectSubtractFees(t *testing.T) { }, { // If more than 20% of funds goes to fees, it should fail. - name: "high fee", + name: "high fee", + highFee: true, coins: []Coin{ { TxOut: wire.TxOut{ PkScript: p2wkhScript, - Value: int64(5 * fundingFee(feeRate, 1, false)), + Value: int64(5 * fundingFee(highFeeRate, 1, false)), }, }, }, - spendValue: 5 * fundingFee(feeRate, 1, false), + spendValue: 5 * fundingFee(highFeeRate, 1, false), expectErr: true, }, @@ -339,6 +342,11 @@ func TestCoinSelectSubtractFees(t *testing.T) { test := test t.Run(test.name, func(t *testing.T) { + feeRate := feeRate + if test.highFee { + feeRate = highFeeRate + } + selected, localFundingAmt, changeAmt, err := CoinSelectSubtractFees( feeRate, test.spendValue, dustLimit, test.coins, ) From c56457c869042daf04b0746c8c8fc61ca4b0f61a Mon Sep 17 00:00:00 2001 From: Bjarne Magnussen Date: Wed, 19 Aug 2020 12:09:35 +0200 Subject: [PATCH 3/5] chanfunding: add test for a non-zero change --- lnwallet/chanfunding/coin_select_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lnwallet/chanfunding/coin_select_test.go b/lnwallet/chanfunding/coin_select_test.go index 36cc64fd..3b02c34b 100644 --- a/lnwallet/chanfunding/coin_select_test.go +++ b/lnwallet/chanfunding/coin_select_test.go @@ -244,6 +244,27 @@ func TestCoinSelectSubtractFees(t *testing.T) { expectedFundingAmt: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, false), expectedChange: 0, }, + { + // We have 1.0 BTC available and spend half of it. This + // should lead to a funding TX with a change output. + name: "spend with change", + coins: []Coin{ + { + TxOut: wire.TxOut{ + PkScript: p2wkhScript, + Value: 1 * btcutil.SatoshiPerBitcoin, + }, + }, + }, + spendValue: 0.5 * btcutil.SatoshiPerBitcoin, + + // The one and only input will be selected. + expectedInput: []btcutil.Amount{ + 1 * btcutil.SatoshiPerBitcoin, + }, + expectedFundingAmt: 0.5*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, true), + expectedChange: 0.5 * btcutil.SatoshiPerBitcoin, + }, { // The total funds available is below the dust limit // after paying fees. From 493bc27ec28c57e7d9a4b6b0245e8362c80327fd Mon Sep 17 00:00:00 2001 From: Bjarne Magnussen Date: Thu, 27 Aug 2020 11:27:03 +0200 Subject: [PATCH 4/5] chanfunding: fix tiny typo --- lnwallet/chanfunding/coin_select.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lnwallet/chanfunding/coin_select.go b/lnwallet/chanfunding/coin_select.go index f1ce008d..9a95b74e 100644 --- a/lnwallet/chanfunding/coin_select.go +++ b/lnwallet/chanfunding/coin_select.go @@ -208,7 +208,7 @@ func CoinSelectSubtractFees(feeRate chainfee.SatPerKWeight, amt, // TODO(halseth): smarter fee limit. Make configurable or dynamic wrt // total funding size? if fee > totalOut/5 { - return nil, 0, 0, fmt.Errorf("fee %v on total output"+ + return nil, 0, 0, fmt.Errorf("fee %v on total output "+ "value %v", fee, totalOut) } From 0e2d6dc0a9ecb6758b6ec5353bfc4cd23ca1e5cd Mon Sep 17 00:00:00 2001 From: Bjarne Magnussen Date: Fri, 28 Aug 2020 11:50:57 +0200 Subject: [PATCH 5/5] chanfunding: match error string when testing `CoinSelectSubtractFees` --- lnwallet/chanfunding/coin_select_test.go | 39 +++++++++++++++++------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/lnwallet/chanfunding/coin_select_test.go b/lnwallet/chanfunding/coin_select_test.go index 3b02c34b..c26dd756 100644 --- a/lnwallet/chanfunding/coin_select_test.go +++ b/lnwallet/chanfunding/coin_select_test.go @@ -2,6 +2,7 @@ package chanfunding import ( "encoding/hex" + "regexp" "testing" "github.com/btcsuite/btcd/wire" @@ -209,6 +210,12 @@ func TestCoinSelectSubtractFees(t *testing.T) { const dustLimit = btcutil.Amount(1000) const dust = btcutil.Amount(100) + // removeAmounts replaces any amounts in string with "". + removeAmounts := func(s string) string { + re := regexp.MustCompile(`[[:digit:]]+\.?[[:digit:]]*`) + return re.ReplaceAllString(s, "") + } + type testCase struct { name string highFee bool @@ -218,7 +225,7 @@ func TestCoinSelectSubtractFees(t *testing.T) { expectedInput []btcutil.Amount expectedFundingAmt btcutil.Amount expectedChange btcutil.Amount - expectErr bool + expectErr string } testCases := []testCase{ @@ -279,7 +286,8 @@ func TestCoinSelectSubtractFees(t *testing.T) { }, spendValue: fundingFee(feeRate, 1, false) + dust, - expectErr: true, + expectErr: "output amount( BTC) after subtracting " + + "fees( BTC) below dust limit( BTC)", }, { // After subtracting fees, the resulting change output @@ -355,7 +363,7 @@ func TestCoinSelectSubtractFees(t *testing.T) { }, spendValue: 5 * fundingFee(highFeeRate, 1, false), - expectErr: true, + expectErr: "fee BTC on total output value BTC", }, } @@ -371,19 +379,28 @@ func TestCoinSelectSubtractFees(t *testing.T) { selected, localFundingAmt, changeAmt, err := CoinSelectSubtractFees( feeRate, test.spendValue, dustLimit, test.coins, ) - if !test.expectErr && err != nil { - t.Fatalf(err.Error()) + if err != nil { + switch { + case test.expectErr == "": + t.Fatalf(err.Error()) + + case test.expectErr != removeAmounts(err.Error()): + t.Fatalf("expected error '%v', got '%v'", + test.expectErr, + removeAmounts(err.Error())) + + // If we got an expected error, there is + // nothing more to test. + default: + return + } } - if test.expectErr && err == nil { + // Check that there was no expected error we missed. + if test.expectErr != "" { t.Fatalf("expected error") } - // If we got an expected error, there is nothing more to test. - if test.expectErr { - return - } - // Check that the selected inputs match what we expect. if len(selected) != len(test.expectedInput) { t.Fatalf("expected %v inputs, got %v",