From a8671c485f34c4e21c222c8c9c5987b2f64c5bbb Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Fri, 24 Mar 2017 16:20:05 -0700 Subject: [PATCH] lnwallet: properly observe dust limits during cooperative chan closure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes a lingering TODO within the wallet portion of the codebase by properly adhering to the set dust limits when closing a channel. With this new commit if a party’s current settled balance is below their current dust-limit, then it will be omitted from the commitment transaction. The prior test that asserted negative outputs are rejected has been removed as they’ll now be avoided by ensuring we omit dust outputs from the commitment transaction. --- lnwallet/channel.go | 12 ++-- lnwallet/channel_test.go | 143 ++++++++++++++++++++++++++++++--------- 2 files changed, 119 insertions(+), 36 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index a4ccc5a2..e60ed324 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -2415,6 +2415,7 @@ func (lc *LightningChannel) InitCooperativeClose() ([]byte, *chainhash.Hash, err } closeTx := CreateCooperativeCloseTx(lc.fundingTxIn, + lc.channelState.OurDustLimit, lc.channelState.TheirDustLimit, lc.channelState.OurBalance, lc.channelState.TheirBalance, lc.channelState.OurDeliveryScript, lc.channelState.TheirDeliveryScript, lc.channelState.IsInitiator) @@ -2467,6 +2468,7 @@ func (lc *LightningChannel) CompleteCooperativeClose(remoteSig []byte) (*wire.Ms // on this active channel back to both parties. In this current model, // the initiator pays full fees for the cooperative close transaction. closeTx := CreateCooperativeCloseTx(lc.fundingTxIn, + lc.channelState.OurDustLimit, lc.channelState.TheirDustLimit, lc.channelState.OurBalance, lc.channelState.TheirBalance, lc.channelState.OurDeliveryScript, lc.channelState.TheirDeliveryScript, lc.channelState.IsInitiator) @@ -2594,7 +2596,7 @@ func CreateCommitTx(fundingOutput *wire.TxIn, selfKey, theirKey *btcec.PublicKey // expected that the initiator pays the transaction fees for the closing // transaction in full. func CreateCooperativeCloseTx(fundingTxIn *wire.TxIn, - ourBalance, theirBalance btcutil.Amount, + localDust, remoteDust, ourBalance, theirBalance btcutil.Amount, ourDeliveryScript, theirDeliveryScript []byte, initiator bool) *wire.MsgTx { @@ -2614,15 +2616,15 @@ func CreateCooperativeCloseTx(fundingTxIn *wire.TxIn, theirBalance -= 5000 } - // TODO(roasbeef): dust check... - // * although upper layers should prevent - if ourBalance != 0 { + // Create both cooperative closure outputs, properly respecting the + // dust limits of both parties. + if ourBalance >= localDust { closeTx.AddTxOut(&wire.TxOut{ PkScript: ourDeliveryScript, Value: int64(ourBalance), }) } - if theirBalance != 0 { + if theirBalance >= remoteDust { closeTx.AddTxOut(&wire.TxOut{ PkScript: theirDeliveryScript, Value: int64(theirBalance), diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 56185877..04962f3e 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -1133,9 +1133,10 @@ func TestCheckDustLimit(t *testing.T) { t.Fatal("their balance wasn't updated") } - // Next we will test when a non-HTLC output in the commitment transaction is below the dust limit. - // We create an HTLC that will only leave a small enough amount to Alice such that Bob will consider - // it a dust output. + // Next we will test when a non-HTLC output in the commitment + // transaction is below the dust limit. We create an HTLC that will + // only leave a small enough amount to Alice such that Bob will + // consider it a dust output. aliceAmount = aliceChannel.channelState.OurBalance bobAmount = bobChannel.channelState.OurBalance htlcAmount2 := aliceAmount - htlcAmount @@ -1150,7 +1151,7 @@ func TestCheckDustLimit(t *testing.T) { t.Fatalf("Can't update the channel state: %v", err) } - // From Alices' point of view, her output is bigger than the dust limit + // From Alice's point of view, her output is bigger than the dust limit commitment = aliceChannel.localCommitChain.tip() if len(commitment.txn.TxOut) != 3 { t.Fatal("incorrect number of outputs in commitment transaction "+ @@ -1163,7 +1164,7 @@ func TestCheckDustLimit(t *testing.T) { t.Fatal("their balance was updated") } - // From Bobs' point of view, Alice's output is lower than the dust limit + // From Bob's point of view, Alice's output is lower than the dust limit commitment = bobChannel.localCommitChain.tip() if len(commitment.txn.TxOut) != 2 { t.Fatal("incorrect number of outputs in commitment transaction "+ @@ -1595,38 +1596,118 @@ func TestCancelHTLC(t *testing.T) { } } -func TestCloseTransactionSanityChecks(t *testing.T) { - // We'd like to ensure that transactions which aren't "sane" aren't - // accepted as valid coopertive channel closure transactions. - - // We'll first create two test channels for our test case below. +func TestCooperativeCloseDustAdherance(t *testing.T) { + // 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(5) if err != nil { t.Fatalf("unable to create test channels: %v", err) } defer cleanUp() - // In order to force the transaction to be "insane", we'll modify - // Alice's balance to be zero. With the current fee logic, this'll - // cause a negative output due to the hard coded fees. A transaction - // with a negative output is not "sane". - // - // TODO(roasbeef): modify test-case after dynamic fees - aliceChannel.channelState.OurBalance = 0 - bobChannel.channelState.TheirBalance = 0 - - // Both Alice and Bob should reject a close attempt at this point since - // it will lead to Alice having a negative output within the commitment - // transaction. - _, _, err = aliceChannel.InitCooperativeClose() - if err == nil { - t.Fatalf("alice's closure transaction should have been rejected, " + - "but wasn't!") + setDustLimit := func(dustVal btcutil.Amount) { + aliceChannel.channelState.OurDustLimit = dustVal + aliceChannel.channelState.TheirDustLimit = dustVal + aliceChannel.channelState.OurDustLimit = dustVal + aliceChannel.channelState.TheirDustLimit = dustVal } - var fakeSig []byte - _, err = bobChannel.CompleteCooperativeClose(fakeSig) - if err == nil { - t.Fatalf("bob's closure transaction should have been rejected, but " + - "wasn't!") + + resetChannelState := func() { + aliceChannel.status = channelOpen + bobChannel.status = channelOpen + } + + setBalances := func(aliceBalance, bobBalance btcutil.Amount) { + aliceChannel.channelState.OurBalance = aliceBalance + aliceChannel.channelState.TheirBalance = bobBalance + bobChannel.channelState.OurBalance = bobBalance + bobChannel.channelState.TheirBalance = aliceBalance + } + + // We'll start be initializing the limit of both Alice and Bob to 10k + // satoshis. + dustLimit := btcutil.Amount(10000) + setDustLimit(dustLimit) + + // Both sides currently have over 1 BTC settled as part of their + // balances. As a result, performing a cooperative closure now result + // in both sides having an output within the closure transaction. + closeSig, _, err := aliceChannel.InitCooperativeClose() + if err != nil { + t.Fatalf("unable to close channel: %v", err) + } + closeSig = append(closeSig, byte(txscript.SigHashAll)) + closeTx, err := bobChannel.CompleteCooperativeClose(closeSig) + if err != nil { + t.Fatalf("unable to accept channel close: %v", err) + } + + // The closure transaction should have exactly two outputs. + if len(closeTx.TxOut) != 2 { + t.Fatalf("close tx has wrong number of outputs: expected %v "+ + "got %v", 2, len(closeTx.TxOut)) + } + + // We'll reset the channel states before proceeding to our nest test. + resetChannelState() + + // Next we'll modify the current balances and dust limits such that + // Bob's current balance is above _below_ his dust limit. + aliceBal := btcutil.Amount(btcutil.SatoshiPerBitcoin) + bobBal := btcutil.Amount(500) + setBalances(aliceBal, bobBal) + + // Attempt another cooperative channel closure. It should succeed + // without any issues. + closeSig, _, err = aliceChannel.InitCooperativeClose() + if err != nil { + t.Fatalf("unable to close channel: %v", err) + } + closeSig = append(closeSig, byte(txscript.SigHashAll)) + closeTx, err = bobChannel.CompleteCooperativeClose(closeSig) + if err != nil { + t.Fatalf("unable to accept channel close: %v", err) + } + + // The closure transaction should only have a single output, and that + // output should be Alice's balance. + // TODO(roasbeef): remove -5000 after fees are no longer hard coded + if len(closeTx.TxOut) != 1 { + t.Fatalf("close tx has wrong number of outputs: expected %v "+ + "got %v", 1, len(closeTx.TxOut)) + } + if closeTx.TxOut[0].Value != int64(aliceBal-5000) { + t.Fatalf("alice's balance is incorrect: expected %v, got %v", + aliceBal-5000, 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. + setBalances(bobBal, aliceBal) + resetChannelState() + + // Our final attempt at another cooperative channel closure. It should + // succeed without any issues. + closeSig, _, err = aliceChannel.InitCooperativeClose() + if err != nil { + t.Fatalf("unable to close channel: %v", err) + } + closeSig = append(closeSig, byte(txscript.SigHashAll)) + closeTx, err = bobChannel.CompleteCooperativeClose(closeSig) + if err != nil { + t.Fatalf("unable to accept channel close: %v", err) + } + + // The closure transaction should only have a single output, and that + // output should be Bob's balance. + // TODO(roasbeef): remove -5000 after fees are no longer hard coded + if len(closeTx.TxOut) != 1 { + t.Fatalf("close tx has wrong number of outputs: expected %v "+ + "got %v", 1, len(closeTx.TxOut)) + } + if closeTx.TxOut[0].Value != int64(aliceBal) { + t.Fatalf("bob's balance is incorrect: expected %v, got %v", + aliceBal, closeTx.TxOut[0].Value) } }