From a52d405998ade5de0425e9e0e7a4a0e7aa88be58 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 30 Aug 2017 15:25:01 -0700 Subject: [PATCH] lnwallet: ensure HTLC values are properly converted to SAT in commit tx This commit fixes a bug within the HTLC construction and commitment transaction construction that would result in HTLC _values_ within the commitment transaction being off by a factor of 1000. This was due to the fact that we failed to convert the amount of an HTLC, in mSAT, to SAT before placing it as an output within the commitment transaction. When attempt to locate the output index of a particular half, we use the unconverted amount, meaning it was unnoticed. This commit adds a new assertion within the TestSimpleAddSettleWorkflow test to ensure that the HTLC is found within the commitment transaction with the proper value in satoshi. --- lnwallet/channel.go | 4 ++-- lnwallet/channel_test.go | 31 ++++++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 7456b335..d5f0ed1b 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -321,7 +321,7 @@ func locateOutputIndex(p *PaymentDescriptor, tx *wire.MsgTx, ourCommit bool, for i, txOut := range tx.TxOut { if bytes.Equal(txOut.PkScript, pkScript) && - txOut.Value == int64(p.Amount) { + txOut.Value == int64(p.Amount.ToSatoshis()) { // If this payment hash and index has already been // found, then we'll continue in order to avoid any @@ -3274,7 +3274,7 @@ func (lc *LightningChannel) addHTLC(commitTx *wire.MsgTx, ourCommit bool, } // Add the new HTLC outputs to the respective commitment transactions. - amountPending := int64(paymentDesc.Amount) + amountPending := int64(paymentDesc.Amount.ToSatoshis()) commitTx.AddTxOut(wire.NewTxOut(amountPending, p2wsh)) // Store the pkScript of this particular PaymentDescriptor so we can diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index d41dee7e..9ae2699f 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -392,6 +392,19 @@ func createHTLC(data int, amount lnwire.MilliSatoshi) (*lnwire.UpdateAddHTLC, [3 }, returnPreimage } +func assertOutputExistsByValue(t *testing.T, commitTx *wire.MsgTx, + value btcutil.Amount) { + + for _, txOut := range commitTx.TxOut { + if txOut.Value == int64(value) { + return + } + } + + t.Fatalf("unable to find output of value %v within tx %v", value, + spew.Sdump(commitTx)) +} + // TestSimpleAddSettleWorkflow tests a simple channel scenario wherein the // local node (Alice in this case) creates a new outgoing HTLC to bob, commits // this change, then bob immediately commits a settlement of the HTLC after the @@ -415,9 +428,10 @@ func TestSimpleAddSettleWorkflow(t *testing.T) { paymentPreimage := bytes.Repeat([]byte{1}, 32) paymentHash := sha256.Sum256(paymentPreimage) + htlcAmt := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin) htlc := &lnwire.UpdateAddHTLC{ PaymentHash: paymentHash, - Amount: lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin), + Amount: htlcAmt, Expiry: uint32(5), } @@ -529,6 +543,21 @@ func TestSimpleAddSettleWorkflow(t *testing.T) { aliceChannel.currentHeight, 1) } + // Both commitment transactions should have three outputs, and one of + // them should be exactly the amount of the HTLC. + if len(aliceChannel.channelState.CommitTx.TxOut) != 3 { + t.Fatalf("alice should have three commitment outputs, instead "+ + "have %v", len(aliceChannel.channelState.CommitTx.TxOut)) + } + if len(bobChannel.channelState.CommitTx.TxOut) != 3 { + t.Fatalf("bob should have three commitment outputs, instead "+ + "have %v", len(bobChannel.channelState.CommitTx.TxOut)) + } + assertOutputExistsByValue(t, &aliceChannel.channelState.CommitTx, + htlcAmt.ToSatoshis()) + assertOutputExistsByValue(t, &bobChannel.channelState.CommitTx, + htlcAmt.ToSatoshis()) + // Now we'll repeat a similar exchange, this time with Bob settling the // HTLC once he learns of the preimage. var preimage [32]byte