From 452ee6aad4fc3038b62576f8ca00e0f3c9434749 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:35 +0200 Subject: [PATCH 01/17] fundingmanager+server: define MaxPendingChans. RejectPush in funding config This commit makes the funding manager access the MaxPendingChannels and RejectPush values from the fundingConfig instead of the global config struct. Done to avoid sharing state between tests. --- fundingmanager.go | 12 +++++-- fundingmanager_test.go | 79 ++++++++++++++++++++++++------------------ server.go | 2 ++ 3 files changed, 57 insertions(+), 36 deletions(-) diff --git a/fundingmanager.go b/fundingmanager.go index 3f67db0c..bfd3f017 100644 --- a/fundingmanager.go +++ b/fundingmanager.go @@ -337,6 +337,14 @@ type fundingConfig struct { // due to fees. MinChanSize btcutil.Amount + // MaxPendingChannels is the maximum number of pending channels we + // allow for each peer. + MaxPendingChannels int + + // RejectPush is set true if the fundingmanager should reject any + // incoming channels having a non-zero push amount. + RejectPush bool + // NotifyOpenChannelEvent informs the ChannelNotifier when channels // transition from pending open to open. NotifyOpenChannelEvent func(wire.OutPoint) @@ -1012,7 +1020,7 @@ func (f *fundingManager) handleFundingOpen(fmsg *fundingOpenMsg) { // TODO(roasbeef): modify to only accept a _single_ pending channel per // block unless white listed - if numPending >= cfg.MaxPendingChannels { + if numPending >= f.cfg.MaxPendingChannels { f.failFundingFlow( fmsg.peer, fmsg.msg.PendingChannelID, lnwire.ErrMaxPendingChannels, @@ -1057,7 +1065,7 @@ func (f *fundingManager) handleFundingOpen(fmsg *fundingOpenMsg) { // If request specifies non-zero push amount and 'rejectpush' is set, // signal an error. - if cfg.RejectPush && msg.PushAmount > 0 { + if f.cfg.RejectPush && msg.PushAmount > 0 { f.failFundingFlow( fmsg.peer, fmsg.msg.PendingChannelID, lnwallet.ErrNonZeroPushAmount()) diff --git a/fundingmanager_test.go b/fundingmanager_test.go index 891f5eab..5416304b 100644 --- a/fundingmanager_test.go +++ b/fundingmanager_test.go @@ -236,7 +236,8 @@ func createTestWallet(cdb *channeldb.DB, netParams *chaincfg.Params, } func createTestFundingManager(t *testing.T, privKey *btcec.PrivateKey, - addr *lnwire.NetAddress, tempTestDir string) (*testNode, error) { + addr *lnwire.NetAddress, tempTestDir string, + options ...cfgOption) (*testNode, error) { netParams := activeNetParams.Params estimator := lnwallet.NewStaticFeeEstimator(62500, 0) @@ -282,7 +283,7 @@ func createTestFundingManager(t *testing.T, privKey *btcec.PrivateKey, var chanIDSeed [32]byte - f, err := newFundingManager(fundingConfig{ + fundingCfg := fundingConfig{ IDKey: privKey.PubKey(), Wallet: lnw, Notifier: chainNotifier, @@ -363,8 +364,15 @@ func createTestFundingManager(t *testing.T, privKey *btcec.PrivateKey, }, ZombieSweeperInterval: 1 * time.Hour, ReservationTimeout: 1 * time.Nanosecond, + MaxPendingChannels: DefaultMaxPendingChannels, NotifyOpenChannelEvent: func(wire.OutPoint) {}, - }) + } + + for _, op := range options { + op(&fundingCfg) + } + + f, err := newFundingManager(fundingCfg) if err != nil { t.Fatalf("failed creating fundingManager: %v", err) } @@ -468,12 +476,10 @@ func recreateAliceFundingManager(t *testing.T, alice *testNode) { } } -func setupFundingManagers(t *testing.T, maxPendingChannels int) (*testNode, *testNode) { - // We need to set the global config, as fundingManager uses - // MaxPendingChannels, and it is usually set in lndMain(). - cfg = &config{ - MaxPendingChannels: maxPendingChannels, - } +type cfgOption func(*fundingConfig) + +func setupFundingManagers(t *testing.T, + options ...cfgOption) (*testNode, *testNode) { aliceTestDir, err := ioutil.TempDir("", "alicelnwallet") if err != nil { @@ -481,7 +487,7 @@ func setupFundingManagers(t *testing.T, maxPendingChannels int) (*testNode, *tes } alice, err := createTestFundingManager( - t, alicePrivKey, aliceAddr, aliceTestDir, + t, alicePrivKey, aliceAddr, aliceTestDir, options..., ) if err != nil { t.Fatalf("failed creating fundingManager: %v", err) @@ -492,7 +498,9 @@ func setupFundingManagers(t *testing.T, maxPendingChannels int) (*testNode, *tes t.Fatalf("unable to create temp directory: %v", err) } - bob, err := createTestFundingManager(t, bobPrivKey, bobAddr, bobTestDir) + bob, err := createTestFundingManager( + t, bobPrivKey, bobAddr, bobTestDir, options..., + ) if err != nil { t.Fatalf("failed creating fundingManager: %v", err) } @@ -1029,7 +1037,7 @@ func assertHandleFundingLocked(t *testing.T, alice, bob *testNode) { } func TestFundingManagerNormalWorkflow(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // We will consume the channel updates as we go, so no buffering is needed. @@ -1102,7 +1110,7 @@ func TestFundingManagerNormalWorkflow(t *testing.T) { } func TestFundingManagerRestartBehavior(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // Run through the process of opening the channel, up until the funding @@ -1240,7 +1248,7 @@ func TestFundingManagerRestartBehavior(t *testing.T) { // server to notify when the peer comes online, in case sending the // fundingLocked message fails the first time. func TestFundingManagerOfflinePeer(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // Run through the process of opening the channel, up until the funding @@ -1374,7 +1382,7 @@ func TestFundingManagerOfflinePeer(t *testing.T) { // will properly clean up a zombie reservation that times out after the // initFundingMsg has been handled. func TestFundingManagerPeerTimeoutAfterInitFunding(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // We will consume the channel updates as we go, so no buffering is needed. @@ -1434,7 +1442,7 @@ func TestFundingManagerPeerTimeoutAfterInitFunding(t *testing.T) { // will properly clean up a zombie reservation that times out after the // fundingOpenMsg has been handled. func TestFundingManagerPeerTimeoutAfterFundingOpen(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // We will consume the channel updates as we go, so no buffering is needed. @@ -1503,7 +1511,7 @@ func TestFundingManagerPeerTimeoutAfterFundingOpen(t *testing.T) { // will properly clean up a zombie reservation that times out after the // fundingAcceptMsg has been handled. func TestFundingManagerPeerTimeoutAfterFundingAccept(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // We will consume the channel updates as we go, so no buffering is needed. @@ -1577,7 +1585,7 @@ func TestFundingManagerPeerTimeoutAfterFundingAccept(t *testing.T) { } func TestFundingManagerFundingTimeout(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // We will consume the channel updates as we go, so no buffering is needed. @@ -1622,7 +1630,7 @@ func TestFundingManagerFundingTimeout(t *testing.T) { // the channel initiator, that it does not timeout when the lnd restarts. func TestFundingManagerFundingNotTimeoutInitiator(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // We will consume the channel updates as we go, so no buffering is needed. @@ -1689,7 +1697,7 @@ func TestFundingManagerFundingNotTimeoutInitiator(t *testing.T) { // continues to operate as expected in case we receive a duplicate fundingLocked // message. func TestFundingManagerReceiveFundingLockedTwice(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // We will consume the channel updates as we go, so no buffering is needed. @@ -1781,7 +1789,7 @@ func TestFundingManagerReceiveFundingLockedTwice(t *testing.T) { // handles receiving a fundingLocked after the its own fundingLocked and channel // announcement is sent and gets restarted. func TestFundingManagerRestartAfterChanAnn(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // We will consume the channel updates as we go, so no buffering is needed. @@ -1858,7 +1866,7 @@ func TestFundingManagerRestartAfterChanAnn(t *testing.T) { // fundingManager continues to operate as expected after it has received // fundingLocked and then gets restarted. func TestFundingManagerRestartAfterReceivingFundingLocked(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // We will consume the channel updates as we go, so no buffering is needed. @@ -1931,7 +1939,7 @@ func TestFundingManagerRestartAfterReceivingFundingLocked(t *testing.T) { // (a channel not supposed to be announced to the rest of the network), // the announcementSignatures nor the nodeAnnouncement messages are sent. func TestFundingManagerPrivateChannel(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // We will consume the channel updates as we go, so no buffering is needed. @@ -2033,7 +2041,7 @@ func TestFundingManagerPrivateChannel(t *testing.T) { // announcement signatures nor the node announcement messages are sent upon // restart. func TestFundingManagerPrivateRestart(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // We will consume the channel updates as we go, so no buffering is needed. @@ -2155,7 +2163,7 @@ func TestFundingManagerPrivateRestart(t *testing.T) { // TestFundingManagerCustomChannelParameters checks that custom requirements we // specify during the channel funding flow is preserved correcly on both sides. func TestFundingManagerCustomChannelParameters(t *testing.T) { - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // This is the custom parameters we'll use. @@ -2385,7 +2393,11 @@ func TestFundingManagerCustomChannelParameters(t *testing.T) { func TestFundingManagerMaxPendingChannels(t *testing.T) { const maxPending = 4 - alice, bob := setupFundingManagers(t, maxPending) + alice, bob := setupFundingManagers( + t, func(cfg *fundingConfig) { + cfg.MaxPendingChannels = maxPending + }, + ) defer tearDownFundingManagers(t, alice, bob) // Create openChanReqs for maxPending+1 channels. @@ -2545,13 +2557,12 @@ func TestFundingManagerMaxPendingChannels(t *testing.T) { // option, namely that non-zero incoming push amounts are disabled. func TestFundingManagerRejectPush(t *testing.T) { // Enable 'rejectpush' option and initialize funding managers. - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) - rejectPush := cfg.RejectPush - cfg.RejectPush = true - defer func() { - tearDownFundingManagers(t, alice, bob) - cfg.RejectPush = rejectPush - }() + alice, bob := setupFundingManagers( + t, func(cfg *fundingConfig) { + cfg.RejectPush = true + }, + ) + defer tearDownFundingManagers(t, alice, bob) // Create a funding request and start the workflow. updateChan := make(chan *lnrpc.OpenStatusUpdate) @@ -2607,7 +2618,7 @@ func TestFundingManagerRejectPush(t *testing.T) { func TestFundingManagerMaxConfs(t *testing.T) { t.Parallel() - alice, bob := setupFundingManagers(t, DefaultMaxPendingChannels) + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) // Create a funding request and start the workflow. diff --git a/server.go b/server.go index 64b37666..a5e97ea6 100644 --- a/server.go +++ b/server.go @@ -1051,6 +1051,8 @@ func newServer(listenAddrs []net.Addr, chanDB *channeldb.DB, ZombieSweeperInterval: 1 * time.Minute, ReservationTimeout: 10 * time.Minute, MinChanSize: btcutil.Amount(cfg.MinChanSize), + MaxPendingChannels: cfg.MaxPendingChannels, + RejectPush: cfg.RejectPush, NotifyOpenChannelEvent: s.channelNotifier.NotifyOpenChannelEvent, }) if err != nil { From 78da62c6f72b92a16dd7654fac1a749fe1d3bcc0 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:35 +0200 Subject: [PATCH 02/17] fundingmanager tests: run tests in parallel --- fundingmanager_test.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/fundingmanager_test.go b/fundingmanager_test.go index 5416304b..fd58fa72 100644 --- a/fundingmanager_test.go +++ b/fundingmanager_test.go @@ -682,6 +682,8 @@ func assertErrorSent(t *testing.T, msgChan chan lnwire.Message) { func assertFundingMsgSent(t *testing.T, msgChan chan lnwire.Message, msgType string) lnwire.Message { + t.Helper() + var msg lnwire.Message select { case msg = <-msgChan: @@ -1037,6 +1039,8 @@ func assertHandleFundingLocked(t *testing.T, alice, bob *testNode) { } func TestFundingManagerNormalWorkflow(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -1110,6 +1114,8 @@ func TestFundingManagerNormalWorkflow(t *testing.T) { } func TestFundingManagerRestartBehavior(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -1248,6 +1254,8 @@ func TestFundingManagerRestartBehavior(t *testing.T) { // server to notify when the peer comes online, in case sending the // fundingLocked message fails the first time. func TestFundingManagerOfflinePeer(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -1382,6 +1390,8 @@ func TestFundingManagerOfflinePeer(t *testing.T) { // will properly clean up a zombie reservation that times out after the // initFundingMsg has been handled. func TestFundingManagerPeerTimeoutAfterInitFunding(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -1442,6 +1452,8 @@ func TestFundingManagerPeerTimeoutAfterInitFunding(t *testing.T) { // will properly clean up a zombie reservation that times out after the // fundingOpenMsg has been handled. func TestFundingManagerPeerTimeoutAfterFundingOpen(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -1511,6 +1523,8 @@ func TestFundingManagerPeerTimeoutAfterFundingOpen(t *testing.T) { // will properly clean up a zombie reservation that times out after the // fundingAcceptMsg has been handled. func TestFundingManagerPeerTimeoutAfterFundingAccept(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -1585,6 +1599,8 @@ func TestFundingManagerPeerTimeoutAfterFundingAccept(t *testing.T) { } func TestFundingManagerFundingTimeout(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -1629,6 +1645,7 @@ func TestFundingManagerFundingTimeout(t *testing.T) { // TestFundingManagerFundingNotTimeoutInitiator checks that if the user was // the channel initiator, that it does not timeout when the lnd restarts. func TestFundingManagerFundingNotTimeoutInitiator(t *testing.T) { + t.Parallel() alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -1697,6 +1714,8 @@ func TestFundingManagerFundingNotTimeoutInitiator(t *testing.T) { // continues to operate as expected in case we receive a duplicate fundingLocked // message. func TestFundingManagerReceiveFundingLockedTwice(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -1789,6 +1808,8 @@ func TestFundingManagerReceiveFundingLockedTwice(t *testing.T) { // handles receiving a fundingLocked after the its own fundingLocked and channel // announcement is sent and gets restarted. func TestFundingManagerRestartAfterChanAnn(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -1866,6 +1887,8 @@ func TestFundingManagerRestartAfterChanAnn(t *testing.T) { // fundingManager continues to operate as expected after it has received // fundingLocked and then gets restarted. func TestFundingManagerRestartAfterReceivingFundingLocked(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -1939,6 +1962,8 @@ func TestFundingManagerRestartAfterReceivingFundingLocked(t *testing.T) { // (a channel not supposed to be announced to the rest of the network), // the announcementSignatures nor the nodeAnnouncement messages are sent. func TestFundingManagerPrivateChannel(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -2041,6 +2066,8 @@ func TestFundingManagerPrivateChannel(t *testing.T) { // announcement signatures nor the node announcement messages are sent upon // restart. func TestFundingManagerPrivateRestart(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -2163,6 +2190,8 @@ func TestFundingManagerPrivateRestart(t *testing.T) { // TestFundingManagerCustomChannelParameters checks that custom requirements we // specify during the channel funding flow is preserved correcly on both sides. func TestFundingManagerCustomChannelParameters(t *testing.T) { + t.Parallel() + alice, bob := setupFundingManagers(t) defer tearDownFundingManagers(t, alice, bob) @@ -2391,6 +2420,8 @@ func TestFundingManagerCustomChannelParameters(t *testing.T) { // TestFundingManagerMaxPendingChannels checks that trying to open another // channel with the same peer when MaxPending channels are pending fails. func TestFundingManagerMaxPendingChannels(t *testing.T) { + t.Parallel() + const maxPending = 4 alice, bob := setupFundingManagers( @@ -2556,6 +2587,8 @@ func TestFundingManagerMaxPendingChannels(t *testing.T) { // TestFundingManagerRejectPush checks behaviour of 'rejectpush' // option, namely that non-zero incoming push amounts are disabled. func TestFundingManagerRejectPush(t *testing.T) { + t.Parallel() + // Enable 'rejectpush' option and initialize funding managers. alice, bob := setupFundingManagers( t, func(cfg *fundingConfig) { From 2f5c1a69fbfab5bdcb44ca722956abea6ad4db15 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:36 +0200 Subject: [PATCH 03/17] lnwallet test: add TestCoinSelect --- lnwallet/wallet.go | 2 + lnwallet/wallet_test.go | 180 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+) create mode 100644 lnwallet/wallet_test.go diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 5d2646bf..a003628f 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -1428,6 +1428,8 @@ func coinSelect(feeRate SatPerKWeight, amt btcutil.Amount, // // TODO: Handle wallets that generate non-witness change // addresses. + // TODO(halseth): make coinSelect not estimate change output + // for dust change. weightEstimate.AddP2WKHOutput() // The difference between the selected amount and the amount diff --git a/lnwallet/wallet_test.go b/lnwallet/wallet_test.go new file mode 100644 index 00000000..2c2a21cc --- /dev/null +++ b/lnwallet/wallet_test.go @@ -0,0 +1,180 @@ +package lnwallet + +import ( + "testing" + + "github.com/btcsuite/btcutil" + "github.com/lightningnetwork/lnd/input" +) + +// fundingFee is a helper method that returns the fee estimate used for a tx +// with the given number of inputs and the optional change output. This matches +// the estimate done by the wallet. +func fundingFee(feeRate SatPerKWeight, numInput int, change bool) btcutil.Amount { + var weightEstimate input.TxWeightEstimator + + // All inputs. + for i := 0; i < numInput; i++ { + weightEstimate.AddP2WKHInput() + } + + // The multisig funding output. + weightEstimate.AddP2WSHOutput() + + // Optionally count a change output. + if change { + weightEstimate.AddP2WKHOutput() + } + + totalWeight := int64(weightEstimate.Weight()) + return feeRate.FeeForWeight(totalWeight) +} + +// TestCoinSelect tests that we pick coins adding up to the expected amount +// 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. +func TestCoinSelect(t *testing.T) { + t.Parallel() + + const feeRate = SatPerKWeight(100) + const dust = btcutil.Amount(100) + + type testCase struct { + name string + outputValue btcutil.Amount + coins []*Utxo + + expectedInput []btcutil.Amount + expectedChange btcutil.Amount + expectErr bool + } + + testCases := []testCase{ + { + // We have 1.0 BTC available, and wants to send 0.5. + // This will obviously lead to a change output of + // almost 0.5 BTC. + name: "big change", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: 1 * btcutil.SatoshiPerBitcoin, + }, + }, + outputValue: 0.5 * btcutil.SatoshiPerBitcoin, + + // The one and only input will be selected. + expectedInput: []btcutil.Amount{ + 1 * btcutil.SatoshiPerBitcoin, + }, + // Change will be what's left minus the fee. + expectedChange: 0.5*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, true), + }, + { + // We have 1 BTC available, and we want to send 1 BTC. + // This should lead to an error, as we don't have + // enough funds to pay the fee. + name: "nothing left for fees", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: 1 * btcutil.SatoshiPerBitcoin, + }, + }, + outputValue: 1 * btcutil.SatoshiPerBitcoin, + expectErr: true, + }, + { + // We have a 1 BTC input, and want to create an output + // as big as possible, such that the remaining change + // will be dust. + name: "dust change", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: 1 * btcutil.SatoshiPerBitcoin, + }, + }, + // We tune the output value by subtracting the expected + // fee and a small dust amount. + outputValue: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, true) - dust, + + expectedInput: []btcutil.Amount{ + 1 * btcutil.SatoshiPerBitcoin, + }, + + // Change will the dust. + expectedChange: dust, + }, + { + // We have a 1 BTC input, and want to create an output + // as big as possible, such that there is nothing left + // for change. + name: "no change", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: 1 * btcutil.SatoshiPerBitcoin, + }, + }, + // We tune the output value to be the maximum amount + // possible, leaving just enough for fees. + outputValue: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, true), + + expectedInput: []btcutil.Amount{ + 1 * btcutil.SatoshiPerBitcoin, + }, + // We have just enough left to pay the fee, so there is + // nothing left for change. + // TODO(halseth): currently coinselect estimates fees + // assuming a change output. + expectedChange: 0, + }, + } + + for _, test := range testCases { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + selected, changeAmt, err := coinSelect( + feeRate, test.outputValue, test.coins, + ) + 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 { + return + } + + // 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)) + } + + for i, coin := range selected { + if coin.Value != test.expectedInput[i] { + t.Fatalf("expected input %v to have value %v, "+ + "had %v", i, 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) + } + }) + } +} From e716251805f0481b68db854b7d9c038a0e4901ba Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:36 +0200 Subject: [PATCH 04/17] server+funding: remove unused remoteFundingAmt --- fundingmanager.go | 3 +-- server.go | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/fundingmanager.go b/fundingmanager.go index bfd3f017..71c5553c 100644 --- a/fundingmanager.go +++ b/fundingmanager.go @@ -2742,8 +2742,7 @@ func (f *fundingManager) handleInitFundingMsg(msg *initFundingMsg) { var ( peerKey = msg.peer.IdentityKey() localAmt = msg.localFundingAmt - remoteAmt = msg.remoteFundingAmt - capacity = localAmt + remoteAmt + capacity = localAmt minHtlc = msg.minHtlc remoteCsvDelay = msg.remoteCsvDelay ) diff --git a/server.go b/server.go index a5e97ea6..4e19ce14 100644 --- a/server.go +++ b/server.go @@ -2979,8 +2979,7 @@ type openChanReq struct { chainHash chainhash.Hash - localFundingAmt btcutil.Amount - remoteFundingAmt btcutil.Amount + localFundingAmt btcutil.Amount pushAmt lnwire.MilliSatoshi From fcf74debe6f6a1afd839e1574474fad6aacce9d8 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:36 +0200 Subject: [PATCH 05/17] lnwallet+funding: rename Capacity and FundingAmt Instead use LocalFundingAmt and RemoteFundingAmt to make it clear who is contributing funds. --- fundingmanager.go | 40 +++++------ lnwallet/interface_test.go | 144 ++++++++++++++++++------------------- lnwallet/reservation.go | 14 ++-- lnwallet/wallet.go | 22 +++--- 4 files changed, 113 insertions(+), 107 deletions(-) diff --git a/fundingmanager.go b/fundingmanager.go index 71c5553c..9df9da84 100644 --- a/fundingmanager.go +++ b/fundingmanager.go @@ -1084,16 +1084,16 @@ func (f *fundingManager) handleFundingOpen(fmsg *fundingOpenMsg) { // funds to the channel ourselves. chainHash := chainhash.Hash(msg.ChainHash) req := &lnwallet.InitFundingReserveMsg{ - ChainHash: &chainHash, - NodeID: fmsg.peer.IdentityKey(), - NodeAddr: fmsg.peer.Address(), - FundingAmount: 0, - Capacity: amt, - CommitFeePerKw: lnwallet.SatPerKWeight(msg.FeePerKiloWeight), - FundingFeePerKw: 0, - PushMSat: msg.PushAmount, - Flags: msg.ChannelFlags, - MinConfs: 1, + ChainHash: &chainHash, + NodeID: fmsg.peer.IdentityKey(), + NodeAddr: fmsg.peer.Address(), + LocalFundingAmt: 0, + RemoteFundingAmt: amt, + CommitFeePerKw: lnwallet.SatPerKWeight(msg.FeePerKiloWeight), + FundingFeePerKw: 0, + PushMSat: msg.PushAmount, + Flags: msg.ChannelFlags, + MinConfs: 1, } reservation, err := f.cfg.Wallet.InitChannelReservation(req) @@ -2783,16 +2783,16 @@ func (f *fundingManager) handleInitFundingMsg(msg *initFundingMsg) { // wallet doesn't have enough funds to commit to this channel, then the // request will fail, and be aborted. req := &lnwallet.InitFundingReserveMsg{ - ChainHash: &msg.chainHash, - NodeID: peerKey, - NodeAddr: msg.peer.Address(), - FundingAmount: localAmt, - Capacity: capacity, - CommitFeePerKw: commitFeePerKw, - FundingFeePerKw: msg.fundingFeePerKw, - PushMSat: msg.pushAmt, - Flags: channelFlags, - MinConfs: msg.minConfs, + ChainHash: &msg.chainHash, + NodeID: peerKey, + NodeAddr: msg.peer.Address(), + LocalFundingAmt: localAmt, + RemoteFundingAmt: 0, + CommitFeePerKw: commitFeePerKw, + FundingFeePerKw: msg.fundingFeePerKw, + PushMSat: msg.pushAmt, + Flags: channelFlags, + MinConfs: msg.minConfs, } reservation, err := f.cfg.Wallet.InitChannelReservation(req) diff --git a/lnwallet/interface_test.go b/lnwallet/interface_test.go index ca7d9a27..866e79e4 100644 --- a/lnwallet/interface_test.go +++ b/lnwallet/interface_test.go @@ -415,15 +415,15 @@ func testDualFundingReservationWorkflow(miner *rpctest.Harness, t.Fatalf("unable to query fee estimator: %v", err) } aliceReq := &lnwallet.InitFundingReserveMsg{ - ChainHash: chainHash, - NodeID: bobPub, - NodeAddr: bobAddr, - FundingAmount: fundingAmount, - Capacity: fundingAmount * 2, - CommitFeePerKw: feePerKw, - FundingFeePerKw: feePerKw, - PushMSat: 0, - Flags: lnwire.FFAnnounceChannel, + ChainHash: chainHash, + NodeID: bobPub, + NodeAddr: bobAddr, + LocalFundingAmt: fundingAmount, + RemoteFundingAmt: fundingAmount, + CommitFeePerKw: feePerKw, + FundingFeePerKw: feePerKw, + PushMSat: 0, + Flags: lnwire.FFAnnounceChannel, } aliceChanReservation, err := alice.InitChannelReservation(aliceReq) if err != nil { @@ -458,15 +458,15 @@ func testDualFundingReservationWorkflow(miner *rpctest.Harness, // receives' Alice's contribution, and consumes that so we can continue // the funding process. bobReq := &lnwallet.InitFundingReserveMsg{ - ChainHash: chainHash, - NodeID: alicePub, - NodeAddr: aliceAddr, - FundingAmount: fundingAmount, - Capacity: fundingAmount * 2, - CommitFeePerKw: feePerKw, - FundingFeePerKw: feePerKw, - PushMSat: 0, - Flags: lnwire.FFAnnounceChannel, + ChainHash: chainHash, + NodeID: alicePub, + NodeAddr: aliceAddr, + LocalFundingAmt: fundingAmount, + RemoteFundingAmt: fundingAmount, + CommitFeePerKw: feePerKw, + FundingFeePerKw: feePerKw, + PushMSat: 0, + Flags: lnwire.FFAnnounceChannel, } bobChanReservation, err := bob.InitChannelReservation(bobReq) if err != nil { @@ -618,15 +618,15 @@ func testFundingTransactionLockedOutputs(miner *rpctest.Harness, t.Fatalf("unable to query fee estimator: %v", err) } req := &lnwallet.InitFundingReserveMsg{ - ChainHash: chainHash, - NodeID: bobPub, - NodeAddr: bobAddr, - FundingAmount: fundingAmount, - Capacity: fundingAmount, - CommitFeePerKw: feePerKw, - FundingFeePerKw: feePerKw, - PushMSat: 0, - Flags: lnwire.FFAnnounceChannel, + ChainHash: chainHash, + NodeID: bobPub, + NodeAddr: bobAddr, + LocalFundingAmt: fundingAmount, + RemoteFundingAmt: 0, + CommitFeePerKw: feePerKw, + FundingFeePerKw: feePerKw, + PushMSat: 0, + Flags: lnwire.FFAnnounceChannel, } if _, err := alice.InitChannelReservation(req); err != nil { t.Fatalf("unable to initialize funding reservation 1: %v", err) @@ -640,15 +640,15 @@ func testFundingTransactionLockedOutputs(miner *rpctest.Harness, t.Fatalf("unable to create amt: %v", err) } failedReq := &lnwallet.InitFundingReserveMsg{ - ChainHash: chainHash, - NodeID: bobPub, - NodeAddr: bobAddr, - FundingAmount: amt, - Capacity: amt, - CommitFeePerKw: feePerKw, - FundingFeePerKw: feePerKw, - PushMSat: 0, - Flags: lnwire.FFAnnounceChannel, + ChainHash: chainHash, + NodeID: bobPub, + NodeAddr: bobAddr, + LocalFundingAmt: amt, + RemoteFundingAmt: 0, + CommitFeePerKw: feePerKw, + FundingFeePerKw: feePerKw, + PushMSat: 0, + Flags: lnwire.FFAnnounceChannel, } failedReservation, err := alice.InitChannelReservation(failedReq) if err == nil { @@ -676,15 +676,15 @@ func testFundingCancellationNotEnoughFunds(miner *rpctest.Harness, t.Fatalf("unable to create amt: %v", err) } req := &lnwallet.InitFundingReserveMsg{ - ChainHash: chainHash, - NodeID: bobPub, - NodeAddr: bobAddr, - FundingAmount: fundingAmount, - Capacity: fundingAmount, - CommitFeePerKw: feePerKw, - FundingFeePerKw: feePerKw, - PushMSat: 0, - Flags: lnwire.FFAnnounceChannel, + ChainHash: chainHash, + NodeID: bobPub, + NodeAddr: bobAddr, + LocalFundingAmt: fundingAmount, + RemoteFundingAmt: 0, + CommitFeePerKw: feePerKw, + FundingFeePerKw: feePerKw, + PushMSat: 0, + Flags: lnwire.FFAnnounceChannel, } chanReservation, err := alice.InitChannelReservation(req) if err != nil { @@ -766,15 +766,15 @@ func testReservationInitiatorBalanceBelowDustCancel(miner *rpctest.Harness, numBTC * numBTC * btcutil.SatoshiPerBitcoin, ) req := &lnwallet.InitFundingReserveMsg{ - ChainHash: chainHash, - NodeID: bobPub, - NodeAddr: bobAddr, - FundingAmount: fundingAmount, - Capacity: fundingAmount, - CommitFeePerKw: feePerKw, - FundingFeePerKw: feePerKw, - PushMSat: 0, - Flags: lnwire.FFAnnounceChannel, + ChainHash: chainHash, + NodeID: bobPub, + NodeAddr: bobAddr, + LocalFundingAmt: fundingAmount, + RemoteFundingAmt: 0, + CommitFeePerKw: feePerKw, + FundingFeePerKw: feePerKw, + PushMSat: 0, + Flags: lnwire.FFAnnounceChannel, } _, err = alice.InitChannelReservation(req) switch { @@ -847,15 +847,15 @@ func testSingleFunderReservationWorkflow(miner *rpctest.Harness, t.Fatalf("unable to query fee estimator: %v", err) } aliceReq := &lnwallet.InitFundingReserveMsg{ - ChainHash: chainHash, - NodeID: bobPub, - NodeAddr: bobAddr, - FundingAmount: fundingAmt, - Capacity: fundingAmt, - CommitFeePerKw: feePerKw, - FundingFeePerKw: feePerKw, - PushMSat: pushAmt, - Flags: lnwire.FFAnnounceChannel, + ChainHash: chainHash, + NodeID: bobPub, + NodeAddr: bobAddr, + LocalFundingAmt: fundingAmt, + RemoteFundingAmt: 0, + CommitFeePerKw: feePerKw, + FundingFeePerKw: feePerKw, + PushMSat: pushAmt, + Flags: lnwire.FFAnnounceChannel, } aliceChanReservation, err := alice.InitChannelReservation(aliceReq) if err != nil { @@ -890,15 +890,15 @@ func testSingleFunderReservationWorkflow(miner *rpctest.Harness, // Next, Bob receives the initial request, generates a corresponding // reservation initiation, then consume Alice's contribution. bobReq := &lnwallet.InitFundingReserveMsg{ - ChainHash: chainHash, - NodeID: alicePub, - NodeAddr: aliceAddr, - FundingAmount: 0, - Capacity: fundingAmt, - CommitFeePerKw: feePerKw, - FundingFeePerKw: feePerKw, - PushMSat: pushAmt, - Flags: lnwire.FFAnnounceChannel, + ChainHash: chainHash, + NodeID: alicePub, + NodeAddr: aliceAddr, + LocalFundingAmt: 0, + RemoteFundingAmt: fundingAmt, + CommitFeePerKw: feePerKw, + FundingFeePerKw: feePerKw, + PushMSat: pushAmt, + Flags: lnwire.FFAnnounceChannel, } bobChanReservation, err := bob.InitChannelReservation(bobReq) if err != nil { diff --git a/lnwallet/reservation.go b/lnwallet/reservation.go index 287eead8..02fab50d 100644 --- a/lnwallet/reservation.go +++ b/lnwallet/reservation.go @@ -127,7 +127,7 @@ type ChannelReservation struct { // used only internally by lnwallet. In order to concurrent safety, the // creation of all channel reservations should be carried out via the // lnwallet.InitChannelReservation interface. -func NewChannelReservation(capacity, fundingAmt btcutil.Amount, +func NewChannelReservation(capacity, localFundingAmt btcutil.Amount, commitFeePerKw SatPerKWeight, wallet *LightningWallet, id uint64, pushMSat lnwire.MilliSatoshi, chainHash *chainhash.Hash, flags lnwire.FundingFlag) (*ChannelReservation, error) { @@ -139,14 +139,16 @@ func NewChannelReservation(capacity, fundingAmt btcutil.Amount, ) commitFee := commitFeePerKw.FeeForWeight(input.CommitWeight) - fundingMSat := lnwire.NewMSatFromSatoshis(fundingAmt) + localFundingMSat := lnwire.NewMSatFromSatoshis(localFundingAmt) + // TODO(halseth): make method take remote funding amount direcly + // instead of inferring it from capacity and local amt. capacityMSat := lnwire.NewMSatFromSatoshis(capacity) feeMSat := lnwire.NewMSatFromSatoshis(commitFee) // If we're the responder to a single-funder reservation, then we have // no initial balance in the channel unless the remote party is pushing // some funds to us within the first commitment state. - if fundingAmt == 0 { + if localFundingAmt == 0 { ourBalance = pushMSat theirBalance = capacityMSat - feeMSat - pushMSat initiator = false @@ -163,7 +165,7 @@ func NewChannelReservation(capacity, fundingAmt btcutil.Amount, // TODO(roasbeef): need to rework fee structure in general and // also when we "unlock" dual funder within the daemon - if capacity == fundingAmt { + if capacity == localFundingAmt { // If we're initiating a single funder workflow, then // we pay all the initial fees within the commitment // transaction. We also deduct our balance by the @@ -174,8 +176,8 @@ func NewChannelReservation(capacity, fundingAmt btcutil.Amount, // Otherwise, this is a dual funder workflow where both // slides split the amount funded and the commitment // fee. - ourBalance = fundingMSat - (feeMSat / 2) - theirBalance = capacityMSat - fundingMSat - (feeMSat / 2) + pushMSat + ourBalance = localFundingMSat - (feeMSat / 2) + theirBalance = capacityMSat - localFundingMSat - (feeMSat / 2) + pushMSat } initiator = true diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index a003628f..8904b5d2 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -69,12 +69,13 @@ type InitFundingReserveMsg struct { // workflow. NodeAddr net.Addr - // FundingAmount is the amount of funds requested for this channel. - FundingAmount btcutil.Amount + // LocalFundingAmt is the amount of funds requested from us for this + // channel. + LocalFundingAmt btcutil.Amount - // Capacity is the total capacity of the channel which includes the - // amount of funds the remote party contributes (if any). - Capacity btcutil.Amount + // RemoteFundingAmnt is the amount of funds the remote will contribute + // to this channel. + RemoteFundingAmt btcutil.Amount // CommitFeePerKw is the starting accepted satoshis/Kw fee for the set // of initial commitment transactions. In order to ensure timely @@ -431,7 +432,7 @@ func (l *LightningWallet) InitChannelReservation( // validate a funding reservation request. func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg) { // It isn't possible to create a channel with zero funds committed. - if req.FundingAmount+req.Capacity == 0 { + if req.LocalFundingAmt+req.RemoteFundingAmt == 0 { err := ErrZeroCapacity() req.err <- err req.resp <- nil @@ -449,9 +450,12 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg return } + capacity := req.LocalFundingAmt + req.RemoteFundingAmt + localFundingAmt := req.LocalFundingAmt + id := atomic.AddUint64(&l.nextFundingID, 1) reservation, err := NewChannelReservation( - req.Capacity, req.FundingAmount, req.CommitFeePerKw, l, id, + capacity, localFundingAmt, req.CommitFeePerKw, l, id, req.PushMSat, l.Cfg.NetParams.GenesisHash, req.Flags, ) if err != nil { @@ -470,11 +474,11 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg // If we're on the receiving end of a single funder channel then we // don't need to perform any coin selection. Otherwise, attempt to // obtain enough coins to meet the required funding amount. - if req.FundingAmount != 0 { + if req.LocalFundingAmt != 0 { // Coin selection is done on the basis of sat/kw, so we'll use // the fee rate passed in to perform coin selection. err := l.selectCoinsAndChange( - req.FundingFeePerKw, req.FundingAmount, req.MinConfs, + req.FundingFeePerKw, req.LocalFundingAmt, req.MinConfs, reservation.ourContribution, ) if err != nil { From 98a3d04ba30e5c7d88c29b007d2ee903f23898d2 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:36 +0200 Subject: [PATCH 06/17] lnwallet: make selectCoinsAndChange return selected coins This makes the method independent of the ChannelContribution struct. We also add a function closure to the return of selectCoinsAndChange, that let is unlock the selected output in case of error. --- lnwallet/wallet.go | 82 ++++++++++++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 29 deletions(-) diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 8904b5d2..57fa5b21 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -477,15 +477,16 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg if req.LocalFundingAmt != 0 { // Coin selection is done on the basis of sat/kw, so we'll use // the fee rate passed in to perform coin selection. - err := l.selectCoinsAndChange( + selected, err := l.selectCoinsAndChange( req.FundingFeePerKw, req.LocalFundingAmt, req.MinConfs, - reservation.ourContribution, ) if err != nil { req.err <- err req.resp <- nil return } + reservation.ourContribution.Inputs = selected.coins + reservation.ourContribution.ChangeOutputs = selected.change } // Next, we'll grab a series of keys from the wallet which will be used @@ -1273,14 +1274,21 @@ func (l *LightningWallet) WithCoinSelectLock(f func() error) error { return f() } +// coinSelection holds the result from selectCoinsAndChange. +type coinSelection struct { + coins []*wire.TxIn + change []*wire.TxOut + unlockCoins func() +} + // selectCoinsAndChange performs coin selection in order to obtain witness -// outputs which sum to at least 'numCoins' amount of satoshis. If coin -// selection is successful/possible, then the selected coins are available -// within the passed contribution's inputs. If necessary, a change address will -// also be generated. +// outputs which sum to at least 'amt' amount of satoshis. If necessary, +// a change address will also be generated. If coin selection is +// successful/possible, then the selected coins and change outputs are +// returned. This method locks the selected outputs, and a function closure to +// unlock them in case of an error is returned. func (l *LightningWallet) selectCoinsAndChange(feeRate SatPerKWeight, - amt btcutil.Amount, minConfs int32, - contribution *ChannelContribution) error { + amt btcutil.Amount, minConfs int32) (*coinSelection, error) { // We hold the coin select mutex while querying for outputs, and // performing coin selection in order to avoid inadvertent double @@ -1295,7 +1303,7 @@ func (l *LightningWallet) selectCoinsAndChange(feeRate SatPerKWeight, // number of confirmations required. coins, err := l.ListUnspentWitness(minConfs, math.MaxInt32) if err != nil { - return err + return nil, err } // Perform coin selection over our available, unlocked unspent outputs @@ -1303,13 +1311,34 @@ func (l *LightningWallet) selectCoinsAndChange(feeRate SatPerKWeight, // requirements. selectedCoins, changeAmt, err := coinSelect(feeRate, amt, coins) if err != nil { - return err + return nil, err + } + + // Record any change output(s) generated as a result of the coin + // selection, but only if the addition of the output won't lead to the + // creation of dust. + var changeOutputs []*wire.TxOut + if changeAmt != 0 && changeAmt > DefaultDustLimit() { + changeAddr, err := l.NewAddress(WitnessPubKey, true) + if err != nil { + return nil, err + } + changeScript, err := txscript.PayToAddrScript(changeAddr) + if err != nil { + return nil, err + } + + changeOutputs = make([]*wire.TxOut, 1) + changeOutputs[0] = &wire.TxOut{ + Value: int64(changeAmt), + PkScript: changeScript, + } } // Lock the selected coins. These coins are now "reserved", this // prevents concurrent funding requests from referring to and this // double-spending the same set of coins. - contribution.Inputs = make([]*wire.TxIn, len(selectedCoins)) + inputs := make([]*wire.TxIn, len(selectedCoins)) for i, coin := range selectedCoins { outpoint := &coin.OutPoint l.lockedOutPoints[*outpoint] = struct{}{} @@ -1317,30 +1346,25 @@ func (l *LightningWallet) selectCoinsAndChange(feeRate SatPerKWeight, // Empty sig script, we'll actually sign if this reservation is // queued up to be completed (the other side accepts). - contribution.Inputs[i] = wire.NewTxIn(outpoint, nil, nil) + inputs[i] = wire.NewTxIn(outpoint, nil, nil) } - // Record any change output(s) generated as a result of the coin - // selection, but only if the addition of the output won't lead to the - // creation of dust. - if changeAmt != 0 && changeAmt > DefaultDustLimit() { - changeAddr, err := l.NewAddress(WitnessPubKey, true) - if err != nil { - return err - } - changeScript, err := txscript.PayToAddrScript(changeAddr) - if err != nil { - return err - } + unlock := func() { + l.coinSelectMtx.Lock() + defer l.coinSelectMtx.Unlock() - contribution.ChangeOutputs = make([]*wire.TxOut, 1) - contribution.ChangeOutputs[0] = &wire.TxOut{ - Value: int64(changeAmt), - PkScript: changeScript, + for _, coin := range selectedCoins { + outpoint := &coin.OutPoint + delete(l.lockedOutPoints, *outpoint) + l.UnlockOutpoint(*outpoint) } } - return nil + return &coinSelection{ + coins: inputs, + change: changeOutputs, + unlockCoins: unlock, + }, nil } // DeriveStateHintObfuscator derives the bytes to be used for obfuscating the From 44384a1b5be314f6a2b0275f2cb367e3cffaca9b Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:36 +0200 Subject: [PATCH 07/17] lnwallet: move coin selection before ChannelReservation Now that coin selection is independent of creating the channel reservation, we can move it first, in preparation for doing custom coin selection. --- lnwallet/interface_test.go | 8 +++---- lnwallet/wallet.go | 44 +++++++++++++++++++++++--------------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/lnwallet/interface_test.go b/lnwallet/interface_test.go index 866e79e4..ac650778 100644 --- a/lnwallet/interface_test.go +++ b/lnwallet/interface_test.go @@ -753,9 +753,9 @@ func testCancelNonExistentReservation(miner *rpctest.Harness, func testReservationInitiatorBalanceBelowDustCancel(miner *rpctest.Harness, alice, _ *lnwallet.LightningWallet, t *testing.T) { - // We'll attempt to create a new reservation with an extremely high fee - // rate. This should push our balance into the negative and result in a - // failure to create the reservation. + // We'll attempt to create a new reservation with an extremely high + // commitment fee rate. This should push our balance into the negative + // and result in a failure to create the reservation. const numBTC = 4 fundingAmount, err := btcutil.NewAmount(numBTC) if err != nil { @@ -772,7 +772,7 @@ func testReservationInitiatorBalanceBelowDustCancel(miner *rpctest.Harness, LocalFundingAmt: fundingAmount, RemoteFundingAmt: 0, CommitFeePerKw: feePerKw, - FundingFeePerKw: feePerKw, + FundingFeePerKw: 1000, PushMSat: 0, Flags: lnwire.FFAnnounceChannel, } diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 57fa5b21..122c7a2e 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -453,6 +453,29 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg capacity := req.LocalFundingAmt + req.RemoteFundingAmt localFundingAmt := req.LocalFundingAmt + var ( + selected *coinSelection + err error + ) + + // If we're on the receiving end of a single funder channel then we + // don't need to perform any coin selection, and the remote contributes + // all funds. Otherwise, attempt to obtain enough coins to meet the + // required funding amount. + if req.LocalFundingAmt != 0 { + // Coin selection is done on the basis of sat/kw, so we'll use + // the fee rate passed in to perform coin selection. + var err error + selected, err = l.selectCoinsAndChange( + req.FundingFeePerKw, req.LocalFundingAmt, req.MinConfs, + ) + if err != nil { + req.err <- err + req.resp <- nil + return + } + } + id := atomic.AddUint64(&l.nextFundingID, 1) reservation, err := NewChannelReservation( capacity, localFundingAmt, req.CommitFeePerKw, l, id, @@ -468,27 +491,14 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg reservation.Lock() defer reservation.Unlock() - reservation.nodeAddr = req.NodeAddr - reservation.partialState.IdentityPub = req.NodeID - - // If we're on the receiving end of a single funder channel then we - // don't need to perform any coin selection. Otherwise, attempt to - // obtain enough coins to meet the required funding amount. - if req.LocalFundingAmt != 0 { - // Coin selection is done on the basis of sat/kw, so we'll use - // the fee rate passed in to perform coin selection. - selected, err := l.selectCoinsAndChange( - req.FundingFeePerKw, req.LocalFundingAmt, req.MinConfs, - ) - if err != nil { - req.err <- err - req.resp <- nil - return - } + if selected != nil { reservation.ourContribution.Inputs = selected.coins reservation.ourContribution.ChangeOutputs = selected.change } + reservation.nodeAddr = req.NodeAddr + reservation.partialState.IdentityPub = req.NodeID + // Next, we'll grab a series of keys from the wallet which will be used // for the duration of the channel. The keys include: our multi-sig // key, the base revocation key, the base htlc key,the base payment From 1b2297c2b509fc6475f59a362cba7f06f20d4cec Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:37 +0200 Subject: [PATCH 08/17] lnwallet/wallet: extract contribution setup into initOurContribution This lets us easily call unlock() in case contribution setup fails. --- lnwallet/wallet.go | 187 +++++++++++++++++++++++---------------------- 1 file changed, 94 insertions(+), 93 deletions(-) diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 122c7a2e..025977f0 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -482,110 +482,21 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg req.PushMSat, l.Cfg.NetParams.GenesisHash, req.Flags, ) if err != nil { + selected.unlockCoins() req.err <- err req.resp <- nil return } - // Grab the mutex on the ChannelReservation to ensure thread-safety - reservation.Lock() - defer reservation.Unlock() - - if selected != nil { - reservation.ourContribution.Inputs = selected.coins - reservation.ourContribution.ChangeOutputs = selected.change - } - - reservation.nodeAddr = req.NodeAddr - reservation.partialState.IdentityPub = req.NodeID - - // Next, we'll grab a series of keys from the wallet which will be used - // for the duration of the channel. The keys include: our multi-sig - // key, the base revocation key, the base htlc key,the base payment - // key, and the delayed payment key. - // - // TODO(roasbeef): "salt" each key as well? - reservation.ourContribution.MultiSigKey, err = l.DeriveNextKey( - keychain.KeyFamilyMultiSig, + err = l.initOurContribution( + reservation, selected, req.NodeAddr, req.NodeID, ) if err != nil { + selected.unlockCoins() req.err <- err req.resp <- nil return } - reservation.ourContribution.RevocationBasePoint, err = l.DeriveNextKey( - keychain.KeyFamilyRevocationBase, - ) - if err != nil { - req.err <- err - req.resp <- nil - return - } - reservation.ourContribution.HtlcBasePoint, err = l.DeriveNextKey( - keychain.KeyFamilyHtlcBase, - ) - if err != nil { - req.err <- err - req.resp <- nil - return - } - reservation.ourContribution.PaymentBasePoint, err = l.DeriveNextKey( - keychain.KeyFamilyPaymentBase, - ) - if err != nil { - req.err <- err - req.resp <- nil - return - } - reservation.ourContribution.DelayBasePoint, err = l.DeriveNextKey( - keychain.KeyFamilyDelayBase, - ) - if err != nil { - req.err <- err - req.resp <- nil - return - } - - // With the above keys created, we'll also need to initialization our - // initial revocation tree state. - nextRevocationKeyDesc, err := l.DeriveNextKey( - keychain.KeyFamilyRevocationRoot, - ) - if err != nil { - req.err <- err - req.resp <- nil - return - } - revocationRoot, err := l.DerivePrivKey(nextRevocationKeyDesc) - if err != nil { - req.err <- err - req.resp <- nil - return - } - - // Once we have the root, we can then generate our shachain producer - // and from that generate the per-commitment point. - revRoot, err := chainhash.NewHash(revocationRoot.Serialize()) - if err != nil { - req.err <- err - req.resp <- nil - return - } - producer := shachain.NewRevocationProducer(*revRoot) - firstPreimage, err := producer.AtIndex(0) - if err != nil { - req.err <- err - req.resp <- nil - return - } - reservation.ourContribution.FirstCommitmentPoint = input.ComputeCommitmentPoint( - firstPreimage[:], - ) - - reservation.partialState.RevocationProducer = producer - reservation.ourContribution.ChannelConstraints = l.Cfg.DefaultConstraints - - // TODO(roasbeef): turn above into: initContribution() // Create a limbo and record entry for this newly pending funding // request. @@ -600,6 +511,96 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg req.err <- nil } +// initOurContribution initializes the given ChannelReservation with our coins +// and change reserved for the channel, and derives the keys to use for this +// channel. +func (l *LightningWallet) initOurContribution(reservation *ChannelReservation, + selected *coinSelection, nodeAddr net.Addr, nodeID *btcec.PublicKey) error { + + // Grab the mutex on the ChannelReservation to ensure thread-safety + reservation.Lock() + defer reservation.Unlock() + + if selected != nil { + reservation.ourContribution.Inputs = selected.coins + reservation.ourContribution.ChangeOutputs = selected.change + } + + reservation.nodeAddr = nodeAddr + reservation.partialState.IdentityPub = nodeID + + // Next, we'll grab a series of keys from the wallet which will be used + // for the duration of the channel. The keys include: our multi-sig + // key, the base revocation key, the base htlc key,the base payment + // key, and the delayed payment key. + // + // TODO(roasbeef): "salt" each key as well? + var err error + reservation.ourContribution.MultiSigKey, err = l.DeriveNextKey( + keychain.KeyFamilyMultiSig, + ) + if err != nil { + return err + } + reservation.ourContribution.RevocationBasePoint, err = l.DeriveNextKey( + keychain.KeyFamilyRevocationBase, + ) + if err != nil { + return err + } + reservation.ourContribution.HtlcBasePoint, err = l.DeriveNextKey( + keychain.KeyFamilyHtlcBase, + ) + if err != nil { + return err + } + reservation.ourContribution.PaymentBasePoint, err = l.DeriveNextKey( + keychain.KeyFamilyPaymentBase, + ) + if err != nil { + return err + } + reservation.ourContribution.DelayBasePoint, err = l.DeriveNextKey( + keychain.KeyFamilyDelayBase, + ) + if err != nil { + return err + } + + // With the above keys created, we'll also need to initialization our + // initial revocation tree state. + nextRevocationKeyDesc, err := l.DeriveNextKey( + keychain.KeyFamilyRevocationRoot, + ) + if err != nil { + return err + } + revocationRoot, err := l.DerivePrivKey(nextRevocationKeyDesc) + if err != nil { + return err + } + + // Once we have the root, we can then generate our shachain producer + // and from that generate the per-commitment point. + revRoot, err := chainhash.NewHash(revocationRoot.Serialize()) + if err != nil { + return err + } + producer := shachain.NewRevocationProducer(*revRoot) + firstPreimage, err := producer.AtIndex(0) + if err != nil { + return err + } + reservation.ourContribution.FirstCommitmentPoint = input.ComputeCommitmentPoint( + firstPreimage[:], + ) + + reservation.partialState.RevocationProducer = producer + reservation.ourContribution.ChannelConstraints = l.Cfg.DefaultConstraints + + return nil +} + // handleFundingReserveCancel cancels an existing channel reservation. As part // of the cancellation, outputs previously selected as inputs for the funding // transaction via coin selection are freed allowing future reservations to From 4239f7d6006e406fcad01559fb5e87929863639c Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:37 +0200 Subject: [PATCH 09/17] lnwallet/wallet: add coinSelectSubtractFees --- lnwallet/wallet.go | 94 +++++++++++++++++++++- lnwallet/wallet_test.go | 170 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 263 insertions(+), 1 deletion(-) diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 025977f0..bd9f279d 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -1455,7 +1455,7 @@ func coinSelect(feeRate SatPerKWeight, amt btcutil.Amount, case NestedWitnessPubKey: weightEstimate.AddNestedP2WKHInput() default: - return nil, 0, fmt.Errorf("Unsupported address type: %v", + return nil, 0, fmt.Errorf("unsupported address type: %v", utxo.AddressType) } } @@ -1494,3 +1494,95 @@ func coinSelect(feeRate SatPerKWeight, amt btcutil.Amount, return selectedUtxos, changeAmt, 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. +func coinSelectSubtractFees(feeRate SatPerKWeight, amt, + dustLimit btcutil.Amount, coins []*Utxo) ([]*Utxo, btcutil.Amount, + btcutil.Amount, error) { + + // First perform an initial round of coin selection to estimate + // the required fee. + totalSat, selectedUtxos, err := selectInputs(amt, coins) + if err != nil { + return nil, 0, 0, err + } + + var weightEstimate input.TxWeightEstimator + for _, utxo := range selectedUtxos { + switch utxo.AddressType { + case WitnessPubKey: + weightEstimate.AddP2WKHInput() + case NestedWitnessPubKey: + weightEstimate.AddNestedP2WKHInput() + default: + return nil, 0, 0, fmt.Errorf("unsupported "+ + "address type: %v", utxo.AddressType) + } + } + + // Channel funding multisig output is P2WSH. + weightEstimate.AddP2WSHOutput() + + // At this point we've got two possibilities, either create a + // change output, or not. We'll first try without creating a + // change output. + // + // Estimate the fee required for a transaction without a change + // output. + totalWeight := int64(weightEstimate.Weight()) + requiredFee := feeRate.FeeForWeight(totalWeight) + + // For a transaction without a change output, we'll let everything go + // to our multi-sig output after subtracting fees. + outputAmt := totalSat - requiredFee + changeAmt := btcutil.Amount(0) + + // If the the output is too small after subtracting the fee, the coin + // selection cannot be performed with an amount this small. + if outputAmt <= dustLimit { + return nil, 0, 0, fmt.Errorf("output amount(%v) after "+ + "subtracting fees(%v) below dust limit(%v)", outputAmt, + requiredFee, dustLimit) + } + + // We were able to create a transaction with no change from the + // selected inputs. We'll remember the resulting values for + // now, while we try to add a change output. Assume that change output + // is a P2WKH output. + weightEstimate.AddP2WKHOutput() + + // Now that we have added the change output, redo the fee + // estimate. + totalWeight = int64(weightEstimate.Weight()) + requiredFee = feeRate.FeeForWeight(totalWeight) + + // For a transaction with a change output, everything we don't spend + // will go to change. + newChange := totalSat - amt + newOutput := amt - requiredFee + + // If adding a change output leads to both outputs being above + // the dust limit, we'll add the change output. Otherwise we'll + // go with the no change tx we originally found. + if newChange > dustLimit && newOutput > dustLimit { + outputAmt = newOutput + changeAmt = newChange + } + + // Sanity check the resulting output values to make sure we + // don't burn a great part to fees. + totalOut := outputAmt + changeAmt + fee := totalSat - totalOut + + // Fail if more than 20% goes to fees. + // 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"+ + "value %v", fee, totalOut) + } + + return selectedUtxos, outputAmt, changeAmt, nil +} diff --git a/lnwallet/wallet_test.go b/lnwallet/wallet_test.go index 2c2a21cc..f9a6c0d3 100644 --- a/lnwallet/wallet_test.go +++ b/lnwallet/wallet_test.go @@ -178,3 +178,173 @@ func TestCoinSelect(t *testing.T) { }) } } + +// 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. +func TestCoinSelectSubtractFees(t *testing.T) { + t.Parallel() + + const feeRate = SatPerKWeight(100) + const dustLimit = btcutil.Amount(1000) + const dust = btcutil.Amount(100) + + type testCase struct { + name string + spendValue btcutil.Amount + coins []*Utxo + + expectedInput []btcutil.Amount + expectedFundingAmt btcutil.Amount + expectedChange btcutil.Amount + expectErr bool + } + + testCases := []testCase{ + { + // We have 1.0 BTC available, spend them all. This + // should lead to a funding TX with one output, the + // rest goes to fees. + name: "spend all", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: 1 * btcutil.SatoshiPerBitcoin, + }, + }, + spendValue: 1 * btcutil.SatoshiPerBitcoin, + + // The one and only input will be selected. + expectedInput: []btcutil.Amount{ + 1 * btcutil.SatoshiPerBitcoin, + }, + expectedFundingAmt: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, false), + expectedChange: 0, + }, + { + // The total funds available is below the dust limit + // after paying fees. + name: "dust output", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: fundingFee(feeRate, 1, false) + dust, + }, + }, + spendValue: fundingFee(feeRate, 1, false) + dust, + + expectErr: true, + }, + { + // After subtracting fees, the resulting change output + // is below the dust limit. The remainder should go + // towards the funding output. + name: "dust change", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: 1 * btcutil.SatoshiPerBitcoin, + }, + }, + spendValue: 1*btcutil.SatoshiPerBitcoin - dust, + + expectedInput: []btcutil.Amount{ + 1 * btcutil.SatoshiPerBitcoin, + }, + expectedFundingAmt: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, false), + expectedChange: 0, + }, + { + // We got just enough funds to create an output above the dust limit. + name: "output right above dustlimit", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: fundingFee(feeRate, 1, false) + dustLimit + 1, + }, + }, + spendValue: fundingFee(feeRate, 1, false) + dustLimit + 1, + + expectedInput: []btcutil.Amount{ + fundingFee(feeRate, 1, false) + dustLimit + 1, + }, + expectedFundingAmt: dustLimit + 1, + expectedChange: 0, + }, + { + // Amount left is below dust limit after paying fee for + // a change output, resulting in a no-change tx. + name: "no amount to pay fee for change", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: fundingFee(feeRate, 1, false) + 2*(dustLimit+1), + }, + }, + spendValue: fundingFee(feeRate, 1, false) + dustLimit + 1, + + expectedInput: []btcutil.Amount{ + fundingFee(feeRate, 1, false) + 2*(dustLimit+1), + }, + expectedFundingAmt: 2 * (dustLimit + 1), + expectedChange: 0, + }, + { + // If more than 20% of funds goes to fees, it should fail. + name: "high fee", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: 5 * fundingFee(feeRate, 1, false), + }, + }, + spendValue: 5 * fundingFee(feeRate, 1, false), + + expectErr: true, + }, + } + + for _, test := range testCases { + t.Run(test.name, func(t *testing.T) { + selected, localFundingAmt, changeAmt, err := coinSelectSubtractFees( + feeRate, test.spendValue, dustLimit, test.coins, + ) + 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 { + return + } + + // 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)) + } + + for i, coin := range selected { + if coin.Value != test.expectedInput[i] { + t.Fatalf("expected input %v to have value %v, "+ + "had %v", i, test.expectedInput[i], + coin.Value) + } + } + + // Assert we got the expected change amount. + if localFundingAmt != test.expectedFundingAmt { + t.Fatalf("expected %v local funding amt, got %v", + test.expectedFundingAmt, localFundingAmt) + } + if changeAmt != test.expectedChange { + t.Fatalf("expected %v change amt, got %v", + test.expectedChange, changeAmt) + } + }) + } +} From f15d81426cb4a1958cba35b4090897dfaca91fda Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:37 +0200 Subject: [PATCH 10/17] lnwallet/wallet: define SubtractFees for InitFundingReserveMsg This commit adds a SubtractFees option to the funding request, letting the caller specify that the fees should be deducted from the funding amount. This paves the way for letting the funding manager spend up to a given amount when creating a channel, like the rest of the funds in the wallet. --- lnwallet/wallet.go | 55 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 7 deletions(-) diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index bd9f279d..3a2a0160 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -69,6 +69,13 @@ type InitFundingReserveMsg struct { // workflow. NodeAddr net.Addr + // SubtractFees should be set if we intend to spend exactly + // LocalFundingAmt when opening the channel, subtracting the fees from + // the funding output. This can be used for instance to use all our + // remaining funds to open the channel, since it will take fees into + // account. + SubtractFees bool + // LocalFundingAmt is the amount of funds requested from us for this // channel. LocalFundingAmt btcutil.Amount @@ -450,7 +457,6 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg return } - capacity := req.LocalFundingAmt + req.RemoteFundingAmt localFundingAmt := req.LocalFundingAmt var ( @@ -468,14 +474,21 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg var err error selected, err = l.selectCoinsAndChange( req.FundingFeePerKw, req.LocalFundingAmt, req.MinConfs, + req.SubtractFees, ) if err != nil { req.err <- err req.resp <- nil return } + + localFundingAmt = selected.fundingAmt } + // The total channel capacity will be the size of the funding output we + // created plus the remote contribution. + capacity := localFundingAmt + req.RemoteFundingAmt + id := atomic.AddUint64(&l.nextFundingID, 1) reservation, err := NewChannelReservation( capacity, localFundingAmt, req.CommitFeePerKw, l, id, @@ -1289,6 +1302,7 @@ func (l *LightningWallet) WithCoinSelectLock(f func() error) error { type coinSelection struct { coins []*wire.TxIn change []*wire.TxOut + fundingAmt btcutil.Amount unlockCoins func() } @@ -1296,10 +1310,12 @@ type coinSelection struct { // outputs which sum to at least 'amt' amount of satoshis. If necessary, // a change address will also be generated. If coin selection is // successful/possible, then the selected coins and change outputs are -// returned. This method locks the selected outputs, and a function closure to -// unlock them in case of an error is returned. +// returned, and the value of the resulting funding output. This method locks +// the selected outputs, and a function closure to unlock them in case of an +// error is returned. func (l *LightningWallet) selectCoinsAndChange(feeRate SatPerKWeight, - amt btcutil.Amount, minConfs int32) (*coinSelection, error) { + amt btcutil.Amount, minConfs int32, subtractFees bool) ( + *coinSelection, error) { // We hold the coin select mutex while querying for outputs, and // performing coin selection in order to avoid inadvertent double @@ -1317,12 +1333,36 @@ func (l *LightningWallet) selectCoinsAndChange(feeRate SatPerKWeight, return nil, err } + var ( + selectedCoins []*Utxo + fundingAmt btcutil.Amount + changeAmt btcutil.Amount + ) + // Perform coin selection over our available, unlocked unspent outputs // in order to find enough coins to meet the funding amount // requirements. - selectedCoins, changeAmt, err := coinSelect(feeRate, amt, coins) - if err != nil { - return nil, err + switch { + // In case this request want the fees subtracted from the local amount, + // we'll call the specialized method for that. This ensures that we + // won't deduct more that the specified balance from our wallet. + case subtractFees: + dustLimit := l.Cfg.DefaultConstraints.DustLimit + selectedCoins, fundingAmt, changeAmt, err = coinSelectSubtractFees( + feeRate, amt, dustLimit, coins, + ) + if err != nil { + return nil, err + } + + // Ótherwise do a normal coin selection where we target a given funding + // amount. + default: + fundingAmt = amt + selectedCoins, changeAmt, err = coinSelect(feeRate, amt, coins) + if err != nil { + return nil, err + } } // Record any change output(s) generated as a result of the coin @@ -1374,6 +1414,7 @@ func (l *LightningWallet) selectCoinsAndChange(feeRate SatPerKWeight, return &coinSelection{ coins: inputs, change: changeOutputs, + fundingAmt: fundingAmt, unlockCoins: unlock, }, nil } From 2cd7d5a2a48899e02d9a78f5ef5b1edbf742d773 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:37 +0200 Subject: [PATCH 11/17] fundingmanager test: extract publishing logic into fundChannel, set funding fee during unit tests --- fundingmanager_test.go | 224 +++++++++++++++++++++++++++++------------ 1 file changed, 161 insertions(+), 63 deletions(-) diff --git a/fundingmanager_test.go b/fundingmanager_test.go index fd58fa72..a189fc9e 100644 --- a/fundingmanager_test.go +++ b/fundingmanager_test.go @@ -552,7 +552,26 @@ func tearDownFundingManagers(t *testing.T, a, b *testNode) { // transaction is confirmed on-chain. Returns the funding out point. func openChannel(t *testing.T, alice, bob *testNode, localFundingAmt, pushAmt btcutil.Amount, numConfs uint32, - updateChan chan *lnrpc.OpenStatusUpdate, announceChan bool) *wire.OutPoint { + updateChan chan *lnrpc.OpenStatusUpdate, announceChan bool) ( + *wire.OutPoint, *wire.MsgTx) { + + publ := fundChannel( + t, alice, bob, localFundingAmt, pushAmt, numConfs, updateChan, + announceChan, + ) + fundingOutPoint := &wire.OutPoint{ + Hash: publ.TxHash(), + Index: 0, + } + return fundingOutPoint, publ +} + +// fundChannel takes the funding process to the point where the funding +// transaction is confirmed on-chain. Returns the funding tx. +func fundChannel(t *testing.T, alice, bob *testNode, localFundingAmt, + pushAmt btcutil.Amount, numConfs uint32, + updateChan chan *lnrpc.OpenStatusUpdate, announceChan bool) *wire.MsgTx { + // Create a funding request and start the workflow. errChan := make(chan error, 1) initReq := &openChanReq{ @@ -560,6 +579,7 @@ func openChannel(t *testing.T, alice, bob *testNode, localFundingAmt, chainHash: *activeNetParams.GenesisHash, localFundingAmt: localFundingAmt, pushAmt: lnwire.NewMSatFromSatoshis(pushAmt), + fundingFeePerKw: 1000, private: !announceChan, updates: updateChan, err: errChan, @@ -644,17 +664,12 @@ func openChannel(t *testing.T, alice, bob *testNode, localFundingAmt, t.Fatalf("alice did not publish funding tx") } - fundingOutPoint := &wire.OutPoint{ - Hash: publ.TxHash(), - Index: 0, - } - // Finally, make sure neither have active reservation for the channel // now pending open in the database. assertNumPendingReservations(t, alice, bobPubKey, 0) assertNumPendingReservations(t, bob, alicePubKey, 0) - return fundingOutPoint + return publ } func assertErrorNotSent(t *testing.T, msgChan chan lnwire.Message) { @@ -1052,16 +1067,21 @@ func TestFundingManagerNormalWorkflow(t *testing.T) { localAmt := btcutil.Amount(500000) pushAmt := btcutil.Amount(0) capacity := localAmt + pushAmt - fundingOutPoint := openChannel(t, alice, bob, localAmt, pushAmt, 1, - updateChan, true) + fundingOutPoint, fundingTx := openChannel( + t, alice, bob, localAmt, pushAmt, 1, updateChan, true, + ) // Check that neither Alice nor Bob sent an error message. assertErrorNotSent(t, alice.msgChan) assertErrorNotSent(t, bob.msgChan) // Notify that transaction was mined. - alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // The funding transaction was mined, so assert that both funding // managers now have the state of this channel 'markedOpen' in their @@ -1102,8 +1122,12 @@ func TestFundingManagerNormalWorkflow(t *testing.T) { assertHandleFundingLocked(t, alice, bob) // Notify that six confirmations has been reached on funding transaction. - alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // Make sure the fundingManagers exchange announcement signatures. assertAnnouncementSignatures(t, alice, bob) @@ -1125,8 +1149,9 @@ func TestFundingManagerRestartBehavior(t *testing.T) { pushAmt := btcutil.Amount(0) capacity := localAmt + pushAmt updateChan := make(chan *lnrpc.OpenStatusUpdate) - fundingOutPoint := openChannel(t, alice, bob, localAmt, pushAmt, 1, - updateChan, true) + fundingOutPoint, fundingTx := openChannel( + t, alice, bob, localAmt, pushAmt, 1, updateChan, true, + ) // After the funding transaction gets mined, both nodes will send the // fundingLocked message to the other peer. If the funding node fails @@ -1147,8 +1172,12 @@ func TestFundingManagerRestartBehavior(t *testing.T) { } // Notify that transaction was mined - alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // The funding transaction was mined, so assert that both funding // managers now have the state of this channel 'markedOpen' in their @@ -1239,8 +1268,12 @@ func TestFundingManagerRestartBehavior(t *testing.T) { time.Sleep(300 * time.Millisecond) // Notify that six confirmations has been reached on funding transaction. - alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // Make sure the fundingManagers exchange announcement signatures. assertAnnouncementSignatures(t, alice, bob) @@ -1265,8 +1298,9 @@ func TestFundingManagerOfflinePeer(t *testing.T) { pushAmt := btcutil.Amount(0) capacity := localAmt + pushAmt updateChan := make(chan *lnrpc.OpenStatusUpdate) - fundingOutPoint := openChannel(t, alice, bob, localAmt, pushAmt, 1, - updateChan, true) + fundingOutPoint, fundingTx := openChannel( + t, alice, bob, localAmt, pushAmt, 1, updateChan, true, + ) // After the funding transaction gets mined, both nodes will send the // fundingLocked message to the other peer. If the funding node fails @@ -1288,8 +1322,12 @@ func TestFundingManagerOfflinePeer(t *testing.T) { } // Notify that transaction was mined - alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // The funding transaction was mined, so assert that both funding // managers now have the state of this channel 'markedOpen' in their @@ -1374,8 +1412,12 @@ func TestFundingManagerOfflinePeer(t *testing.T) { assertHandleFundingLocked(t, alice, bob) // Notify that six confirmations has been reached on funding transaction. - alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // Make sure both fundingManagers send the expected announcement // signatures. @@ -1609,7 +1651,7 @@ func TestFundingManagerFundingTimeout(t *testing.T) { // Run through the process of opening the channel, up until the funding // transaction is broadcasted. - _ = openChannel(t, alice, bob, 500000, 0, 1, updateChan, true) + _, _ = openChannel(t, alice, bob, 500000, 0, 1, updateChan, true) // Bob will at this point be waiting for the funding transaction to be // confirmed, so the channel should be considered pending. @@ -1655,7 +1697,7 @@ func TestFundingManagerFundingNotTimeoutInitiator(t *testing.T) { // Run through the process of opening the channel, up until the funding // transaction is broadcasted. - _ = openChannel(t, alice, bob, 500000, 0, 1, updateChan, true) + _, _ = openChannel(t, alice, bob, 500000, 0, 1, updateChan, true) // Alice will at this point be waiting for the funding transaction to be // confirmed, so the channel should be considered pending. @@ -1727,12 +1769,17 @@ func TestFundingManagerReceiveFundingLockedTwice(t *testing.T) { localAmt := btcutil.Amount(500000) pushAmt := btcutil.Amount(0) capacity := localAmt + pushAmt - fundingOutPoint := openChannel(t, alice, bob, localAmt, pushAmt, 1, - updateChan, true) + fundingOutPoint, fundingTx := openChannel( + t, alice, bob, localAmt, pushAmt, 1, updateChan, true, + ) // Notify that transaction was mined - alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // The funding transaction was mined, so assert that both funding // managers now have the state of this channel 'markedOpen' in their @@ -1793,8 +1840,12 @@ func TestFundingManagerReceiveFundingLockedTwice(t *testing.T) { } // Notify that six confirmations has been reached on funding transaction. - alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // Make sure the fundingManagers exchange announcement signatures. assertAnnouncementSignatures(t, alice, bob) @@ -1821,12 +1872,17 @@ func TestFundingManagerRestartAfterChanAnn(t *testing.T) { localAmt := btcutil.Amount(500000) pushAmt := btcutil.Amount(0) capacity := localAmt + pushAmt - fundingOutPoint := openChannel(t, alice, bob, localAmt, pushAmt, 1, - updateChan, true) + fundingOutPoint, fundingTx := openChannel( + t, alice, bob, localAmt, pushAmt, 1, updateChan, true, + ) // Notify that transaction was mined - alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // The funding transaction was mined, so assert that both funding // managers now have the state of this channel 'markedOpen' in their @@ -1872,8 +1928,12 @@ func TestFundingManagerRestartAfterChanAnn(t *testing.T) { assertHandleFundingLocked(t, alice, bob) // Notify that six confirmations has been reached on funding transaction. - alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // Make sure both fundingManagers send the expected channel announcements. assertAnnouncementSignatures(t, alice, bob) @@ -1900,12 +1960,17 @@ func TestFundingManagerRestartAfterReceivingFundingLocked(t *testing.T) { localAmt := btcutil.Amount(500000) pushAmt := btcutil.Amount(0) capacity := localAmt + pushAmt - fundingOutPoint := openChannel(t, alice, bob, localAmt, pushAmt, 1, - updateChan, true) + fundingOutPoint, fundingTx := openChannel( + t, alice, bob, localAmt, pushAmt, 1, updateChan, true, + ) // Notify that transaction was mined - alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // The funding transaction was mined, so assert that both funding // managers now have the state of this channel 'markedOpen' in their @@ -1947,8 +2012,12 @@ func TestFundingManagerRestartAfterReceivingFundingLocked(t *testing.T) { assertAddedToRouterGraph(t, alice, bob, fundingOutPoint) // Notify that six confirmations has been reached on funding transaction. - alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // Make sure both fundingManagers send the expected channel announcements. assertAnnouncementSignatures(t, alice, bob) @@ -1975,12 +2044,17 @@ func TestFundingManagerPrivateChannel(t *testing.T) { localAmt := btcutil.Amount(500000) pushAmt := btcutil.Amount(0) capacity := localAmt + pushAmt - fundingOutPoint := openChannel(t, alice, bob, localAmt, pushAmt, 1, - updateChan, false) + fundingOutPoint, fundingTx := openChannel( + t, alice, bob, localAmt, pushAmt, 1, updateChan, false, + ) // Notify that transaction was mined - alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // The funding transaction was mined, so assert that both funding // managers now have the state of this channel 'markedOpen' in their @@ -2018,8 +2092,12 @@ func TestFundingManagerPrivateChannel(t *testing.T) { assertHandleFundingLocked(t, alice, bob) // Notify that six confirmations has been reached on funding transaction. - alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // Since this is a private channel, we shouldn't receive the // announcement signatures. @@ -2079,12 +2157,17 @@ func TestFundingManagerPrivateRestart(t *testing.T) { localAmt := btcutil.Amount(500000) pushAmt := btcutil.Amount(0) capacity := localAmt + pushAmt - fundingOutPoint := openChannel(t, alice, bob, localAmt, pushAmt, 1, - updateChan, false) + fundingOutPoint, fundingTx := openChannel( + t, alice, bob, localAmt, pushAmt, 1, updateChan, false, + ) // Notify that transaction was mined - alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // The funding transaction was mined, so assert that both funding // managers now have the state of this channel 'markedOpen' in their @@ -2127,8 +2210,12 @@ func TestFundingManagerPrivateRestart(t *testing.T) { assertHandleFundingLocked(t, alice, bob) // Notify that six confirmations has been reached on funding transaction. - alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.sixConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // Since this is a private channel, we shouldn't receive the public // channel announcement messages. @@ -2385,15 +2472,20 @@ func TestFundingManagerCustomChannelParameters(t *testing.T) { } // Wait for Alice to published the funding tx to the network. + var fundingTx *wire.MsgTx select { - case <-alice.publTxChan: + case fundingTx = <-alice.publTxChan: case <-time.After(time.Second * 5): t.Fatalf("alice did not publish funding tx") } // Notify that transaction was mined. - alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } + bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: fundingTx, + } // After the funding transaction is mined, Alice will send // fundingLocked to Bob. @@ -2526,6 +2618,7 @@ func TestFundingManagerMaxPendingChannels(t *testing.T) { ).(*lnwire.Error) // Give the FundingSigned messages to Alice. + var txs []*wire.MsgTx for i, sign := range signs { alice.fundingMgr.processFundingSigned(sign, bob) @@ -2544,7 +2637,8 @@ func TestFundingManagerMaxPendingChannels(t *testing.T) { } select { - case <-alice.publTxChan: + case tx := <-alice.publTxChan: + txs = append(txs, tx) case <-time.After(time.Second * 5): t.Fatalf("alice did not publish funding tx") } @@ -2561,8 +2655,12 @@ func TestFundingManagerMaxPendingChannels(t *testing.T) { // Notify that the transactions were mined. for i := 0; i < maxPending; i++ { - alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} - bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{} + alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: txs[i], + } + bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ + Tx: txs[i], + } // Expect both to be sending FundingLocked. _ = assertFundingMsgSent( From 87c8165f01e79db04be6045889661824789f9b2c Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:37 +0200 Subject: [PATCH 12/17] mock: make it possible to set list of returned utxos --- mock.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/mock.go b/mock.go index 657069f4..cb4892ed 100644 --- a/mock.go +++ b/mock.go @@ -231,6 +231,7 @@ type mockWalletController struct { prevAddres btcutil.Address publishedTransactions chan *wire.MsgTx index uint32 + utxos []*lnwallet.Utxo } // BackEnd returns "mock" to signify a mock wallet controller. @@ -284,6 +285,13 @@ func (*mockWalletController) CreateSimpleTx(outputs []*wire.TxOut, // need one unspent for the funding transaction. func (m *mockWalletController) ListUnspentWitness(minconfirms, maxconfirms int32) ([]*lnwallet.Utxo, error) { + + // If the mock already has a list of utxos, return it. + if m.utxos != nil { + return m.utxos, nil + } + + // Otherwise create one to return. utxo := &lnwallet.Utxo{ AddressType: lnwallet.WitnessPubKey, Value: btcutil.Amount(10 * btcutil.SatoshiPerBitcoin), From b9816259cb520fc169cb2cd829edf07f1eb11e1b Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:38 +0200 Subject: [PATCH 13/17] fundingmanager+server: define subtractFees Let one initiate a funding request with the remaining funds in the wallet. --- fundingmanager.go | 52 ++++++++++++++++++++++++++++++----------- lnwallet/reservation.go | 7 ++++++ server.go | 1 + 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/fundingmanager.go b/fundingmanager.go index 9df9da84..0ede9720 100644 --- a/fundingmanager.go +++ b/fundingmanager.go @@ -1722,8 +1722,8 @@ func (f *fundingManager) handleFundingSigned(fmsg *fundingSignedMsg) { return case shortChanID, ok = <-confChan: if !ok { - fndgLog.Errorf("waiting for funding confirmation" + - " failed") + fndgLog.Errorf("waiting for funding " + + "confirmation failed") return } } @@ -1893,8 +1893,9 @@ func makeFundingScript(channel *channeldb.OpenChannel) ([]byte, error) { // when a channel has become active for lightning transactions. // The wait can be canceled by closing the cancelChan. In case of success, // a *lnwire.ShortChannelID will be passed to confChan. -func (f *fundingManager) waitForFundingConfirmation(completeChan *channeldb.OpenChannel, - cancelChan <-chan struct{}, confChan chan<- *lnwire.ShortChannelID) { +func (f *fundingManager) waitForFundingConfirmation( + completeChan *channeldb.OpenChannel, cancelChan <-chan struct{}, + confChan chan<- *lnwire.ShortChannelID) { defer close(confChan) @@ -1904,16 +1905,19 @@ func (f *fundingManager) waitForFundingConfirmation(completeChan *channeldb.Open fundingScript, err := makeFundingScript(completeChan) if err != nil { fndgLog.Errorf("unable to create funding script for "+ - "ChannelPoint(%v): %v", completeChan.FundingOutpoint, err) + "ChannelPoint(%v): %v", completeChan.FundingOutpoint, + err) return } numConfs := uint32(completeChan.NumConfsRequired) confNtfn, err := f.cfg.Notifier.RegisterConfirmationsNtfn( - &txid, fundingScript, numConfs, completeChan.FundingBroadcastHeight, + &txid, fundingScript, numConfs, + completeChan.FundingBroadcastHeight, ) if err != nil { fndgLog.Errorf("Unable to register for confirmation of "+ - "ChannelPoint(%v): %v", completeChan.FundingOutpoint, err) + "ChannelPoint(%v): %v", completeChan.FundingOutpoint, + err) return } @@ -1928,14 +1932,17 @@ func (f *fundingManager) waitForFundingConfirmation(completeChan *channeldb.Open select { case confDetails, ok = <-confNtfn.Confirmed: // fallthrough + case <-cancelChan: fndgLog.Warnf("canceled waiting for funding confirmation, "+ "stopping funding flow for ChannelPoint(%v)", completeChan.FundingOutpoint) return + case <-f.quit: fndgLog.Warnf("fundingManager shutting down, stopping funding "+ - "flow for ChannelPoint(%v)", completeChan.FundingOutpoint) + "flow for ChannelPoint(%v)", + completeChan.FundingOutpoint) return } @@ -1948,6 +1955,18 @@ func (f *fundingManager) waitForFundingConfirmation(completeChan *channeldb.Open fundingPoint := completeChan.FundingOutpoint chanID := lnwire.NewChanIDFromOutPoint(&fundingPoint) + if int(fundingPoint.Index) >= len(confDetails.Tx.TxOut) { + fndgLog.Warnf("Funding point index does not exist for "+ + "ChannelPoint(%v)", completeChan.FundingOutpoint) + return + } + + outputAmt := btcutil.Amount(confDetails.Tx.TxOut[fundingPoint.Index].Value) + if outputAmt != completeChan.Capacity { + fndgLog.Warnf("Invalid output value for ChannelPoint(%v)", + completeChan.FundingOutpoint) + return + } fndgLog.Infof("ChannelPoint(%v) is now active: ChannelID(%x)", fundingPoint, chanID[:]) @@ -2742,7 +2761,6 @@ func (f *fundingManager) handleInitFundingMsg(msg *initFundingMsg) { var ( peerKey = msg.peer.IdentityKey() localAmt = msg.localFundingAmt - capacity = localAmt minHtlc = msg.minHtlc remoteCsvDelay = msg.remoteCsvDelay ) @@ -2756,10 +2774,11 @@ func (f *fundingManager) handleInitFundingMsg(msg *initFundingMsg) { ourDustLimit = defaultLitecoinDustLimit } - fndgLog.Infof("Initiating fundingRequest(localAmt=%v, remoteAmt=%v, "+ - "capacity=%v, chainhash=%v, peer=%x, dustLimit=%v, min_confs=%v)", - localAmt, msg.pushAmt, capacity, msg.chainHash, - peerKey.SerializeCompressed(), ourDustLimit, msg.minConfs) + fndgLog.Infof("Initiating fundingRequest(local_amt=%v "+ + "(subtract_fees=%v), push_amt=%v, chain_hash=%v, peer=%x, "+ + "dust_limit=%v, min_confs=%v)", localAmt, msg.subtractFees, + msg.pushAmt, msg.chainHash, peerKey.SerializeCompressed(), + ourDustLimit, msg.minConfs) // First, we'll query the fee estimator for a fee that should get the // commitment transaction confirmed by the next few blocks (conf target @@ -2786,6 +2805,7 @@ func (f *fundingManager) handleInitFundingMsg(msg *initFundingMsg) { ChainHash: &msg.chainHash, NodeID: peerKey, NodeAddr: msg.peer.Address(), + SubtractFees: msg.subtractFees, LocalFundingAmt: localAmt, RemoteFundingAmt: 0, CommitFeePerKw: commitFeePerKw, @@ -2801,6 +2821,12 @@ func (f *fundingManager) handleInitFundingMsg(msg *initFundingMsg) { return } + // Now that we have successfully reserved funds for this channel in the + // wallet, we can fetch the final channel capacity. This is done at + // this point since the final capacity might change in case of + // SubtractFees=true. + capacity := reservation.Capacity() + // Obtain a new pending channel ID which is used to track this // reservation throughout its lifetime. chanID := f.nextPendingChanID() diff --git a/lnwallet/reservation.go b/lnwallet/reservation.go index 02fab50d..555d0ded 100644 --- a/lnwallet/reservation.go +++ b/lnwallet/reservation.go @@ -515,6 +515,13 @@ func (r *ChannelReservation) FundingOutpoint() *wire.OutPoint { return &r.partialState.FundingOutpoint } +// Capacity returns the channel capacity for this reservation. +func (r *ChannelReservation) Capacity() btcutil.Amount { + r.RLock() + defer r.RUnlock() + return r.partialState.Capacity +} + // Cancel abandons this channel reservation. This method should be called in // the scenario that communications with the counterparty break down. Upon // cancellation, all resources previously reserved for this pending payment diff --git a/server.go b/server.go index 4e19ce14..01c0079c 100644 --- a/server.go +++ b/server.go @@ -2979,6 +2979,7 @@ type openChanReq struct { chainHash chainhash.Hash + subtractFees bool localFundingAmt btcutil.Amount pushAmt lnwire.MilliSatoshi From 0405703019ee32ad99a9c6a354b1978a72fe3377 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:38 +0200 Subject: [PATCH 14/17] fundingmanager test: add TestFundingManagerFundAll TestFundingManagerFundAll tests that we can initiate a funding request to use the funds remaining in the wallet. This should produce a funding tx with no change output. --- fundingmanager_test.go | 106 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 103 insertions(+), 3 deletions(-) diff --git a/fundingmanager_test.go b/fundingmanager_test.go index a189fc9e..d90f691c 100644 --- a/fundingmanager_test.go +++ b/fundingmanager_test.go @@ -556,8 +556,8 @@ func openChannel(t *testing.T, alice, bob *testNode, localFundingAmt, *wire.OutPoint, *wire.MsgTx) { publ := fundChannel( - t, alice, bob, localFundingAmt, pushAmt, numConfs, updateChan, - announceChan, + t, alice, bob, localFundingAmt, pushAmt, false, numConfs, + updateChan, announceChan, ) fundingOutPoint := &wire.OutPoint{ Hash: publ.TxHash(), @@ -569,7 +569,7 @@ func openChannel(t *testing.T, alice, bob *testNode, localFundingAmt, // fundChannel takes the funding process to the point where the funding // transaction is confirmed on-chain. Returns the funding tx. func fundChannel(t *testing.T, alice, bob *testNode, localFundingAmt, - pushAmt btcutil.Amount, numConfs uint32, + pushAmt btcutil.Amount, subtractFees bool, numConfs uint32, updateChan chan *lnrpc.OpenStatusUpdate, announceChan bool) *wire.MsgTx { // Create a funding request and start the workflow. @@ -577,6 +577,7 @@ func fundChannel(t *testing.T, alice, bob *testNode, localFundingAmt, initReq := &openChanReq{ targetPubkey: bob.privKey.PubKey(), chainHash: *activeNetParams.GenesisHash, + subtractFees: subtractFees, localFundingAmt: localFundingAmt, pushAmt: lnwire.NewMSatFromSatoshis(pushAmt), fundingFeePerKw: 1000, @@ -2811,3 +2812,102 @@ func TestFundingManagerMaxConfs(t *testing.T) { string(err.Data)) } } + +// TestFundingManagerFundAll tests that we can initiate a funding request to +// use the funds remaining in the wallet. This should produce a funding tx with +// no change output. +func TestFundingManagerFundAll(t *testing.T) { + t.Parallel() + + // We set up our mock wallet to control a list of UTXOs that sum to + // less than the max channel size. + allCoins := []*lnwallet.Utxo{ + { + AddressType: lnwallet.WitnessPubKey, + Value: btcutil.Amount( + 0.05 * btcutil.SatoshiPerBitcoin, + ), + PkScript: make([]byte, 22), + OutPoint: wire.OutPoint{ + Hash: chainhash.Hash{}, + Index: 0, + }, + }, + { + AddressType: lnwallet.WitnessPubKey, + Value: btcutil.Amount( + 0.06 * btcutil.SatoshiPerBitcoin, + ), + PkScript: make([]byte, 22), + OutPoint: wire.OutPoint{ + Hash: chainhash.Hash{}, + Index: 1, + }, + }, + } + + tests := []struct { + spendAmt btcutil.Amount + change bool + }{ + { + // We will spend all the funds in the wallet, and + // expects no change output. + spendAmt: btcutil.Amount( + 0.11 * btcutil.SatoshiPerBitcoin, + ), + change: false, + }, + { + // We spend a little less than the funds in the wallet, + // so a change output should be created. + spendAmt: btcutil.Amount( + 0.10 * btcutil.SatoshiPerBitcoin, + ), + change: true, + }, + } + + for _, test := range tests { + alice, bob := setupFundingManagers(t) + defer tearDownFundingManagers(t, alice, bob) + + alice.fundingMgr.cfg.Wallet.WalletController.(*mockWalletController).utxos = allCoins + + // We will consume the channel updates as we go, so no + // buffering is needed. + updateChan := make(chan *lnrpc.OpenStatusUpdate) + + // Initiate a fund channel, and inspect the funding tx. + pushAmt := btcutil.Amount(0) + fundingTx := fundChannel( + t, alice, bob, test.spendAmt, pushAmt, true, 1, + updateChan, true, + ) + + // Check whether the expected change output is present. + if test.change && len(fundingTx.TxOut) != 2 { + t.Fatalf("expected 2 outputs, had %v", + len(fundingTx.TxOut)) + } + + if !test.change && len(fundingTx.TxOut) != 1 { + t.Fatalf("expected 1 output, had %v", + len(fundingTx.TxOut)) + } + + // Inputs should be all funds in the wallet. + if len(fundingTx.TxIn) != len(allCoins) { + t.Fatalf("Had %d inputs, expected %d", + len(fundingTx.TxIn), len(allCoins)) + } + + for i, txIn := range fundingTx.TxIn { + if txIn.PreviousOutPoint != allCoins[i].OutPoint { + t.Fatalf("expected outpoint to be %v, was %v", + allCoins[i].OutPoint, + txIn.PreviousOutPoint) + } + } + } +} From bf7f392c11159852cd74cf0ee23e0ac8d9418122 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:38 +0200 Subject: [PATCH 15/17] autopilot: use subtractFees when funding channel To make the autopilot able to account for fees, we let it use the subtractFees option when opening channels. This makes sure that each channel we attempt to open will eat at most Amt out of our budget. Previously fees would eat into our funds in addition, causing us to deplete our funds more than expected on each channel opening. --- autopilot/agent.go | 1 + autopilot/interface.go | 9 +++++---- pilot.go | 1 + 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/autopilot/agent.go b/autopilot/agent.go index 8e625c14..2fe123c9 100644 --- a/autopilot/agent.go +++ b/autopilot/agent.go @@ -584,6 +584,7 @@ func (a *Agent) openChans(availableFunds btcutil.Amount, numChans uint32, // Use the heuristic to calculate a score for each node in the // graph. + log.Debugf("Scoring %d nodes for chan_size=%v", len(nodes), chanSize) scores, err := a.cfg.Heuristic.NodeScores( a.cfg.Graph, totalChans, chanSize, nodes, ) diff --git a/autopilot/interface.go b/autopilot/interface.go index 34ef7597..b63661e3 100644 --- a/autopilot/interface.go +++ b/autopilot/interface.go @@ -188,10 +188,11 @@ func init() { // open a channel within the graph to a target peer, close targeted channels, // or add/remove funds from existing channels via a splice in/out mechanisms. type ChannelController interface { - // OpenChannel opens a channel to a target peer, with a capacity of the - // specified amount. This function should un-block immediately after - // the funding transaction that marks the channel open has been - // broadcast. + // OpenChannel opens a channel to a target peer, using at most amt + // funds. This means that the resulting channel capacity might be + // slightly less to account for fees. This function should un-block + // immediately after the funding transaction that marks the channel + // open has been broadcast. OpenChannel(target *btcec.PublicKey, amt btcutil.Amount) error // CloseChannel attempts to close out the target channel. diff --git a/pilot.go b/pilot.go index 5748cf97..9a0f402c 100644 --- a/pilot.go +++ b/pilot.go @@ -100,6 +100,7 @@ func (c *chanController) OpenChannel(target *btcec.PublicKey, req := &openChanReq{ targetPubkey: target, chainHash: *activeNetParams.GenesisHash, + subtractFees: true, localFundingAmt: amt, pushAmt: 0, minHtlc: minHtlc, From 68f7642564864db0bacece93a8aa9eb563f2fb5a Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:38 +0200 Subject: [PATCH 16/17] autopilot/agent_test: wait for expected num chans and balance Previously we waited only for the number of channels to become what we expected, but this wasn't enough. Sometimes the agent had't yet updated its internal balance, causing the test to fail with an unexpected balance. --- autopilot/agent_test.go | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/autopilot/agent_test.go b/autopilot/agent_test.go index 92b2b358..dff8f565 100644 --- a/autopilot/agent_test.go +++ b/autopilot/agent_test.go @@ -1259,34 +1259,40 @@ func TestAgentChannelSizeAllocation(t *testing.T) { waitForNumChans := func(expChans int) { t.Helper() + var ( + numChans int + balance btcutil.Amount + ) + Loop: for { select { case arg := <-testCtx.constraints.moreChanArgs: + numChans = len(arg.chans) + balance = arg.balance + // As long as the number of existing channels // is below our expected number of channels, - // we'll keep responding with "no more - // channels". - if len(arg.chans) != expChans { - select { - case testCtx.constraints.moreChansResps <- moreChansResp{0, 0}: - case <-time.After(time.Second * 3): - t.Fatalf("heuristic wasn't " + - "queried in time") - } - continue + // and the balance is not what we expect, we'll + // keep responding with "no more channels". + if numChans == expChans && + balance == testCtx.walletBalance { + break Loop } - if arg.balance != testCtx.walletBalance { - t.Fatalf("expectd agent to have %v "+ - "balance, had %v", - testCtx.walletBalance, - arg.balance) + select { + case testCtx.constraints.moreChansResps <- moreChansResp{0, 0}: + case <-time.After(time.Second * 3): + t.Fatalf("heuristic wasn't queried " + + "in time") } - break Loop case <-time.After(time.Second * 3): - t.Fatalf("heuristic wasn't queried in time") + t.Fatalf("did not receive expected "+ + "channels(%d) and balance(%d), "+ + "instead got %d and %d", expChans, + testCtx.walletBalance, numChans, + balance) } } } From 7838e6e1fa4d2483cfa1c3a4dc41da44032189ae Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 11 Jul 2019 13:14:39 +0200 Subject: [PATCH 17/17] autopilot/agent_test: add more agent channel allocation testcases Add tests for allocation when well within and well above channel budget. --- autopilot/agent_test.go | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/autopilot/agent_test.go b/autopilot/agent_test.go index dff8f565..bd703c9e 100644 --- a/autopilot/agent_test.go +++ b/autopilot/agent_test.go @@ -1196,7 +1196,7 @@ func TestAgentChannelSizeAllocation(t *testing.T) { t.Parallel() // Total number of nodes in our mock graph. - const numNodes = 10 + const numNodes = 20 testCtx, cleanup := setup(t, nil) defer cleanup() @@ -1240,6 +1240,8 @@ func TestAgentChannelSizeAllocation(t *testing.T) { numNewChannels, nodeScores, ) + // We expect the autopilot to have allocated all funds towards + // channels. expectedAllocation := testCtx.constraints.MaxChanSize() * btcutil.Amount(numNewChannels) nodes := checkChannelOpens( t, testCtx, expectedAllocation, numNewChannels, @@ -1314,5 +1316,30 @@ func TestAgentChannelSizeAllocation(t *testing.T) { // To stay within the budget, we expect the autopilot to open 2 // channels. - checkChannelOpens(t, testCtx, channelBudget, 2) + expectedAllocation = channelBudget + nodes = checkChannelOpens(t, testCtx, expectedAllocation, 2) + numExistingChannels = 7 + + for _, node := range nodes { + delete(nodeScores, node) + } + + waitForNumChans(numExistingChannels) + + // Finally check that we make maximum channels if we are well within + // our budget. + channelBudget = btcutil.SatoshiPerBitcoin * 5 + numNewChannels = 2 + respondWithScores( + t, testCtx, channelBudget, numExistingChannels, + numNewChannels, nodeScores, + ) + + // We now expect the autopilot to open 2 channels, and since it has + // more than enough balance within the budget, they should both be of + // maximum size. + expectedAllocation = testCtx.constraints.MaxChanSize() * + btcutil.Amount(numNewChannels) + + checkChannelOpens(t, testCtx, expectedAllocation, numNewChannels) }