From fc54c5d8d828891696bcd19dba0f2617452e41f9 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Mon, 27 Feb 2017 21:00:18 -0600 Subject: [PATCH] lnwallet: perform sanity check on cooperative closure transacitons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes a class of bug that currently exists within the cooperative closure methods for the channel state machine. As an example, due to the current hard coded fees, if one of the outputs generated within the generated closure transaction has a negative output, then the initiating node would gladly forward this to the remote node. The remote node would then reject the closure as the transaction is invalid. However, the act of completing the closure would cause the remote node’s state machine to shift into a “closed” state. As a result, any further closure attempts by the first node (force or regular) would go unnoticed by the remote node. We fix this issue by ensuring the transaction is “sane” before initiating of completing a cooperative channel closure. At test case has been added exercising the particular erroneous case reported by “moli” on IRC. --- lnwallet/channel.go | 19 ++++++++++++++++++- lnwallet/channel_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 5a36f44a..64e128d9 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -12,6 +12,7 @@ import ( "github.com/lightningnetwork/lnd/chainntnfs" "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/lnwire" + "github.com/roasbeef/btcd/blockchain" "github.com/roasbeef/btcd/chaincfg/chainhash" "github.com/roasbeef/btcd/btcec" @@ -2238,7 +2239,14 @@ func (lc *LightningChannel) InitCooperativeClose() ([]byte, *chainhash.Hash, err lc.channelState.OurBalance, lc.channelState.TheirBalance, lc.channelState.OurDeliveryScript, lc.channelState.TheirDeliveryScript, lc.channelState.IsInitiator) - closeTxSha := closeTx.TxHash() + + // Ensure that the transaction doesn't explicitly validate any + // consensus rules such as being too big, or having any value with a + // negative output. + tx := btcutil.NewTx(closeTx) + if err := blockchain.CheckTransactionSanity(tx); err != nil { + return nil, nil, err + } // Finally, sign the completed cooperative closure transaction. As the // initiator we'll simply send our signature over to the remote party, @@ -2254,6 +2262,7 @@ func (lc *LightningChannel) InitCooperativeClose() ([]byte, *chainhash.Hash, err // channel closure has been initiated. lc.status = channelClosing + closeTxSha := closeTx.TxHash() return closeSig, &closeTxSha, nil } @@ -2283,6 +2292,14 @@ func (lc *LightningChannel) CompleteCooperativeClose(remoteSig []byte) (*wire.Ms lc.channelState.OurDeliveryScript, lc.channelState.TheirDeliveryScript, lc.channelState.IsInitiator) + // Ensure that the transaction doesn't explicitly validate any + // consensus rules such as being too big, or having any value with a + // negative output. + tx := btcutil.NewTx(closeTx) + if err := blockchain.CheckTransactionSanity(tx); err != nil { + return nil, err + } + // With the transaction created, we can finally generate our half of // the 2-of-2 multi-sig needed to redeem the funding output. hashCache := txscript.NewTxSigHashes(closeTx) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 9febf345..1684f5b0 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -1287,3 +1287,39 @@ func TestCancelHTLC(t *testing.T) { bobChannel.channelState.TheirBalance, expectedBalance) } } + +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. + 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!") + } + var fakeSig []byte + _, err = bobChannel.CompleteCooperativeClose(fakeSig) + if err == nil { + t.Fatalf("bob's closure transaction should have been rejected, but " + + "wasn't!") + } +}