From 517fcd691ab4daa5efce7abe7b00a690d864f7bb Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 21 Aug 2020 13:38:36 +0200 Subject: [PATCH 1/6] channeldb: correct anchor comment --- channeldb/channel.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/channeldb/channel.go b/channeldb/channel.go index 0a5e47b2c..35a0700d4 100644 --- a/channeldb/channel.go +++ b/channeldb/channel.go @@ -206,8 +206,7 @@ const ( // AnchorOutputsBit indicates that the channel makes use of anchor // outputs to bump the commitment transaction's effective feerate. This - // channel type also uses a delayed to_remote output script. If bit is - // set, we'll find the size of the anchor outputs in the database. + // channel type also uses a delayed to_remote output script. AnchorOutputsBit ChannelType = 1 << 3 // FrozenBit indicates that the channel is a frozen channel, meaning From 724f439b0bd8f2aafdf8d3390c4fc04603534a69 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 21 Aug 2020 13:38:36 +0200 Subject: [PATCH 2/6] lnwire: update anchor bit to spec --- lnwire/features.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lnwire/features.go b/lnwire/features.go index 6cc899463..5584191b0 100644 --- a/lnwire/features.go +++ b/lnwire/features.go @@ -112,12 +112,12 @@ const ( // AnchorsRequired is a required feature bit that signals that the node // requires channels to be made using commitments having anchor // outputs. - AnchorsRequired FeatureBit = 1336 + AnchorsRequired FeatureBit = 20 // AnchorsRequired is an optional feature bit that signals that the // node supports channels to be made using commitments having anchor // outputs. - AnchorsOptional FeatureBit = 1337 + AnchorsOptional FeatureBit = 21 // maxAllowedSize is a maximum allowed size of feature vector. // From 07a57ae778fbe4c838c9b5b853a729985949481e Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 24 Aug 2020 15:26:06 +0200 Subject: [PATCH 3/6] lnwallet: extract coop close balance calc into method --- lnwallet/channel.go | 40 +++++++++++----------------------------- lnwallet/commitment.go | 22 ++++++++++++++++++++++ 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 94e268054..30deb5a24 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -6179,21 +6179,13 @@ func (lc *LightningChannel) CreateCloseProposal(proposedFee btcutil.Amount, return nil, nil, 0, ErrChanClosing } - // Subtract the proposed fee from the appropriate balance, taking care - // not to persist the adjusted balance, as the feeRate may change + // Get the final balances after subtracting the proposed fee, taking + // care not to persist the adjusted balance, as the feeRate may change // during the channel closing process. - localCommit := lc.channelState.LocalCommitment - ourBalance := localCommit.LocalBalance.ToSatoshis() - theirBalance := localCommit.RemoteBalance.ToSatoshis() - - // We'll make sure we account for the complete balance by adding the - // current dangling commitment fee to the balance of the initiator. - commitFee := localCommit.CommitFee - if lc.channelState.IsInitiator { - ourBalance = ourBalance - proposedFee + commitFee - } else { - theirBalance = theirBalance - proposedFee + commitFee - } + ourBalance, theirBalance := CoopCloseBalance( + lc.channelState.ChanType, lc.channelState.IsInitiator, + proposedFee, lc.channelState.LocalCommitment, + ) closeTx := CreateCooperativeCloseTx( fundingTxIn(lc.channelState), lc.channelState.LocalChanCfg.DustLimit, @@ -6248,21 +6240,11 @@ func (lc *LightningChannel) CompleteCooperativeClose( return nil, 0, ErrChanClosing } - // Subtract the proposed fee from the appropriate balance, taking care - // not to persist the adjusted balance, as the feeRate may change - // during the channel closing process. - localCommit := lc.channelState.LocalCommitment - ourBalance := localCommit.LocalBalance.ToSatoshis() - theirBalance := localCommit.RemoteBalance.ToSatoshis() - - // We'll make sure we account for the complete balance by adding the - // current dangling commitment fee to the balance of the initiator. - commitFee := localCommit.CommitFee - if lc.channelState.IsInitiator { - ourBalance = ourBalance - proposedFee + commitFee - } else { - theirBalance = theirBalance - proposedFee + commitFee - } + // Get the final balances after subtracting the proposed fee. + ourBalance, theirBalance := CoopCloseBalance( + lc.channelState.ChanType, lc.channelState.IsInitiator, + proposedFee, lc.channelState.LocalCommitment, + ) // Create the transaction used to return the current settled balance // on this active channel back to both parties. In this current model, diff --git a/lnwallet/commitment.go b/lnwallet/commitment.go index d70f3ca0e..ed7121d55 100644 --- a/lnwallet/commitment.go +++ b/lnwallet/commitment.go @@ -663,6 +663,28 @@ func CreateCommitTx(chanType channeldb.ChannelType, return commitTx, nil } +// CoopCloseBalance returns the final balances that should be used to create +// the cooperative close tx, given the channel type and transaction fee. +func CoopCloseBalance(chanType channeldb.ChannelType, isInitiator bool, + coopCloseFee btcutil.Amount, localCommit channeldb.ChannelCommitment) ( + btcutil.Amount, btcutil.Amount) { + + // Get both parties' balances from the latest commitment. + ourBalance := localCommit.LocalBalance.ToSatoshis() + theirBalance := localCommit.RemoteBalance.ToSatoshis() + + // We'll make sure we account for the complete balance by adding the + // current dangling commitment fee to the balance of the initiator. + commitFee := localCommit.CommitFee + if isInitiator { + ourBalance = ourBalance - coopCloseFee + commitFee + } else { + theirBalance = theirBalance - coopCloseFee + commitFee + } + + return ourBalance, theirBalance +} + // genHtlcScript generates the proper P2WSH public key scripts for the HTLC // output modified by two-bits denoting if this is an incoming HTLC, and if the // HTLC is being applied to their commitment transaction or ours. From 09a126b29f394b3f463139ab993c5d59820c2f38 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 21 Aug 2020 13:38:36 +0200 Subject: [PATCH 4/6] lnwallet: add anchor size back to balance on coop close To be spec compliant, we require the initiator to not pay the anchor values into fees on coop close. We extract the balance calculation into commitment.go, and add back the value of the anchors to the initiator's balance. --- lnwallet/channel_test.go | 57 +++++++++++++++++++++++++++++++++++++--- lnwallet/commitment.go | 17 +++++++++--- 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index f076ae3a0..5347e132d 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -683,14 +683,37 @@ func testCommitHTLCSigTieBreak(t *testing.T, restart bool) { } } +// TestCooperativeChannelClosure checks that the coop close process finishes +// with an agreement from both parties, and that the final balances of the +// close tx check out. func TestCooperativeChannelClosure(t *testing.T) { + t.Run("tweakless", func(t *testing.T) { + testCoopClose(t, &coopCloseTestCase{ + chanType: channeldb.SingleFunderTweaklessBit, + }) + }) + t.Run("anchors", func(t *testing.T) { + testCoopClose(t, &coopCloseTestCase{ + chanType: channeldb.SingleFunderTweaklessBit | + channeldb.AnchorOutputsBit, + anchorAmt: anchorSize * 2, + }) + }) +} + +type coopCloseTestCase struct { + chanType channeldb.ChannelType + anchorAmt btcutil.Amount +} + +func testCoopClose(t *testing.T, testCase *coopCloseTestCase) { t.Parallel() // Create a test channel which will be used for the duration of this // unittest. The channel will be funded evenly with Alice having 5 BTC, // and Bob having 5 BTC. aliceChannel, bobChannel, cleanUp, err := CreateTestChannels( - channeldb.SingleFunderTweaklessBit, + testCase.chanType, ) if err != nil { t.Fatalf("unable to create test channels: %v", err) @@ -707,7 +730,7 @@ func TestCooperativeChannelClosure(t *testing.T) { bobChannel.channelState.LocalCommitment.FeePerKw, ) - // We'll store with both Alice and Bob creating a new close proposal + // We'll start with both Alice and Bob creating a new close proposal // with the same fee. aliceFee := aliceChannel.CalcFee(aliceFeeRate) aliceSig, _, _, err := aliceChannel.CreateCloseProposal( @@ -728,7 +751,7 @@ func TestCooperativeChannelClosure(t *testing.T) { // With the proposals created, both sides should be able to properly // process the other party's signature. This indicates that the // transaction is well formed, and the signatures verify. - aliceCloseTx, _, err := bobChannel.CompleteCooperativeClose( + aliceCloseTx, bobTxBalance, err := bobChannel.CompleteCooperativeClose( bobSig, aliceSig, bobDeliveryScript, aliceDeliveryScript, bobFee, ) @@ -737,7 +760,7 @@ func TestCooperativeChannelClosure(t *testing.T) { } bobCloseSha := aliceCloseTx.TxHash() - bobCloseTx, _, err := aliceChannel.CompleteCooperativeClose( + bobCloseTx, aliceTxBalance, err := aliceChannel.CompleteCooperativeClose( aliceSig, bobSig, aliceDeliveryScript, bobDeliveryScript, aliceFee, ) @@ -749,6 +772,32 @@ func TestCooperativeChannelClosure(t *testing.T) { if bobCloseSha != aliceCloseSha { t.Fatalf("alice and bob close transactions don't match: %v", err) } + + // Finally, make sure the final balances are correct from both's + // perspective. + aliceBalance := aliceChannel.channelState.LocalCommitment. + LocalBalance.ToSatoshis() + + // The commit balance have had the initiator's (Alice) commitfee and + // any anchors subtracted, so add that back to the final expected + // balance. Alice also pays the coop close fee, so that must be + // subtracted. + commitFee := aliceChannel.channelState.LocalCommitment.CommitFee + expBalanceAlice := aliceBalance + commitFee + + testCase.anchorAmt - bobFee + if aliceTxBalance != expBalanceAlice { + t.Fatalf("expected balance %v got %v", expBalanceAlice, + aliceTxBalance) + } + + // Bob is not the initiator, so his final balance should simply be + // equal to the latest commitment balance. + expBalanceBob := bobChannel.channelState.LocalCommitment. + LocalBalance.ToSatoshis() + if bobTxBalance != expBalanceBob { + t.Fatalf("expected bob's balance to be %v got %v", + expBalanceBob, bobTxBalance) + } } // TestForceClose checks that the resulting ForceCloseSummary is correct when a diff --git a/lnwallet/commitment.go b/lnwallet/commitment.go index ed7121d55..1039bbfc6 100644 --- a/lnwallet/commitment.go +++ b/lnwallet/commitment.go @@ -675,11 +675,22 @@ func CoopCloseBalance(chanType channeldb.ChannelType, isInitiator bool, // We'll make sure we account for the complete balance by adding the // current dangling commitment fee to the balance of the initiator. - commitFee := localCommit.CommitFee + initiatorDelta := localCommit.CommitFee + + // Since the initiator's balance also is stored after subtracting the + // anchor values, add that back in case this was an anchor commitment. + if chanType.HasAnchors() { + initiatorDelta += 2 * anchorSize + } + + // The initiator will pay the full coop close fee, subtract that value + // from their balance. + initiatorDelta -= coopCloseFee + if isInitiator { - ourBalance = ourBalance - coopCloseFee + commitFee + ourBalance += initiatorDelta } else { - theirBalance = theirBalance - coopCloseFee + commitFee + theirBalance += initiatorDelta } return ourBalance, theirBalance From a48c369250831d8ca5ed3312091237c35acc56bc Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 24 Aug 2020 15:44:13 +0200 Subject: [PATCH 5/6] lnwallet: check coop close fee negative balance Also modify the test to check for this condition. --- lnwallet/channel.go | 10 ++++++++-- lnwallet/channel_test.go | 23 +++++++++++++++++++---- lnwallet/commitment.go | 12 ++++++++++-- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 30deb5a24..ccdf55308 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -6182,10 +6182,13 @@ func (lc *LightningChannel) CreateCloseProposal(proposedFee btcutil.Amount, // Get the final balances after subtracting the proposed fee, taking // care not to persist the adjusted balance, as the feeRate may change // during the channel closing process. - ourBalance, theirBalance := CoopCloseBalance( + ourBalance, theirBalance, err := CoopCloseBalance( lc.channelState.ChanType, lc.channelState.IsInitiator, proposedFee, lc.channelState.LocalCommitment, ) + if err != nil { + return nil, nil, 0, err + } closeTx := CreateCooperativeCloseTx( fundingTxIn(lc.channelState), lc.channelState.LocalChanCfg.DustLimit, @@ -6241,10 +6244,13 @@ func (lc *LightningChannel) CompleteCooperativeClose( } // Get the final balances after subtracting the proposed fee. - ourBalance, theirBalance := CoopCloseBalance( + ourBalance, theirBalance, err := CoopCloseBalance( lc.channelState.ChanType, lc.channelState.IsInitiator, proposedFee, lc.channelState.LocalCommitment, ) + if err != nil { + return nil, 0, err + } // Create the transaction used to return the current settled balance // on this active channel back to both parties. In this current model, diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 5347e132d..190e31c12 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -2230,11 +2230,11 @@ func TestCooperativeCloseDustAdherence(t *testing.T) { "got %v", 2, len(closeTx.TxOut)) } - // We'll reset the channel states before proceeding to our nest test. + // We'll reset the channel states before proceeding to our next test. resetChannelState() // Next we'll modify the current balances and dust limits such that - // Bob's current balance is above _below_ his dust limit. + // Bob's current balance is _below_ his dust limit. aliceBal := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin) bobBal := lnwire.NewMSatFromSatoshis(250) setBalances(aliceBal, bobBal) @@ -2277,11 +2277,26 @@ func TestCooperativeCloseDustAdherence(t *testing.T) { int64(closeTx.TxOut[0].Value)) } - // Finally, we'll modify the current balances and dust limits such that - // Alice's current balance is _below_ his her limit. + // We'll modify the current balances and dust limits such that + // Alice's current balance is too low to pay the proposed fee. setBalances(bobBal, aliceBal) resetChannelState() + // Attempting to close with this fee now should fail, since Alice + // cannot afford it. + _, _, _, err = aliceChannel.CreateCloseProposal( + aliceFee, aliceDeliveryScript, bobDeliveryScript, + ) + if err == nil { + t.Fatalf("expected error") + } + + // Finally, we'll modify the current balances and dust limits such that + // Alice's balance after paying the coop fee is _below_ her dust limit. + lowBalance := lnwire.NewMSatFromSatoshis(aliceFee) + 1000 + setBalances(lowBalance, aliceBal) + resetChannelState() + // Our final attempt at another cooperative channel closure. It should // succeed without any issues. aliceSig, _, _, err = aliceChannel.CreateCloseProposal( diff --git a/lnwallet/commitment.go b/lnwallet/commitment.go index 1039bbfc6..44b3c81c4 100644 --- a/lnwallet/commitment.go +++ b/lnwallet/commitment.go @@ -667,7 +667,7 @@ func CreateCommitTx(chanType channeldb.ChannelType, // the cooperative close tx, given the channel type and transaction fee. func CoopCloseBalance(chanType channeldb.ChannelType, isInitiator bool, coopCloseFee btcutil.Amount, localCommit channeldb.ChannelCommitment) ( - btcutil.Amount, btcutil.Amount) { + btcutil.Amount, btcutil.Amount, error) { // Get both parties' balances from the latest commitment. ourBalance := localCommit.LocalBalance.ToSatoshis() @@ -693,7 +693,15 @@ func CoopCloseBalance(chanType channeldb.ChannelType, isInitiator bool, theirBalance += initiatorDelta } - return ourBalance, theirBalance + // During fee negotiation it should always be verified that the + // initiator can pay the proposed fee, but we do a sanity check just to + // be sure here. + if ourBalance < 0 || theirBalance < 0 { + return 0, 0, fmt.Errorf("initiator cannot afford proposed " + + "coop close fee") + } + + return ourBalance, theirBalance, nil } // genHtlcScript generates the proper P2WSH public key scripts for the HTLC From bf18929f0ec8771402b871f86f8424c6d856fc21 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 24 Aug 2020 15:48:08 +0200 Subject: [PATCH 6/6] peer/brontide: fix pubkey log --- peer/brontide.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/peer/brontide.go b/peer/brontide.go index cc637207c..11e04928b 100644 --- a/peer/brontide.go +++ b/peer/brontide.go @@ -2785,11 +2785,11 @@ func (p *Brontide) handleCloseMsg(msg *closeMsg) { func (p *Brontide) HandleLocalCloseChanReqs(req *htlcswitch.ChanClose) { select { case p.localCloseChanReqs <- req: - peerLog.Infof("Local close channel request delivered to peer: %v", - p.PubKey()) + peerLog.Infof("Local close channel request delivered to "+ + "peer: %x", p.PubKey()) case <-p.quit: - peerLog.Infof("Unable to deliver local close channel request to peer "+ - "%x", p.PubKey()) + peerLog.Infof("Unable to deliver local close channel request "+ + "to peer %x", p.PubKey()) } }