From e91dff44e675efd751118521921dac700bd90344 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 4 Apr 2018 17:02:56 -0700 Subject: [PATCH 1/5] test: extend testRevokedCloseRetributionRemoteHodl to have HTLCs in both directions In this commit, we extend the testRevokedCloseRetributionRemoteHodl so that the final broadcast revoked transaction has incoming *and* outgoing HTLC's. As is, this test fails as there's a lingering bug in the way we generate htlc resolutions. A follow up commit will remedy this issue. --- lnd_test.go | 158 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 105 insertions(+), 53 deletions(-) diff --git a/lnd_test.go b/lnd_test.go index bdbac159..d3fe6e74 100644 --- a/lnd_test.go +++ b/lnd_test.go @@ -3824,7 +3824,7 @@ func testRevokedCloseRetributionZeroValueRemoteOutput(net *lntest.NetworkHarness assertNodeNumChannels(t, ctxb, net.Alice, 0) } -// testRevokedCloseRetributionRemoteHodl tests that Alice properly responds to a +// testRevokedCloseRetributionRemoteHodl tests that Dave properly responds to a // channel breach made by the remote party, specifically in the case that the // remote party breaches before settling extended HTLCs. func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, @@ -3834,33 +3834,50 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, const ( timeout = time.Duration(time.Second * 10) chanAmt = maxFundingAmount - pushAmt = 20000 + pushAmt = 200000 paymentAmt = 10000 numInvoices = 6 ) - // Since this test will result in the counterparty being left in a weird - // state, we will introduce another node into our test network: Carol. + // Since this test will result in the counterparty being left in a + // weird state, we will introduce another node into our test network: + // Carol. carol, err := net.NewNode([]string{"--debughtlc", "--hodlhtlc"}) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } - // We must let Alice communicate with Carol before they are able to - // open channel, so we connect Alice and Carol, - if err := net.ConnectNodes(ctxb, net.Alice, carol); err != nil { - t.Fatalf("unable to connect alice to carol: %v", err) + // We'll also create a new node Dave, who will have a channel with + // Carol, and also use similar settings so we can broadcast a commit + // with active HTLCs. + dave, err := net.NewNode([]string{"--debughtlc", "--hodlhtlc"}) + if err != nil { + t.Fatalf("unable to create new dave node: %v", err) } - // In order to test Alice's response to an uncooperative channel - // closure by Carol, we'll first open up a channel between them with a + // We must let Dave communicate with Carol before they are able to open + // channel, so we connect Dave and Carol, + if err := net.ConnectNodes(ctxb, dave, carol); err != nil { + t.Fatalf("unable to connect dave to carol: %v", err) + } + + // Before we make a channel, we'll load up Dave with some coins sent + // directly from the miner. + err = net.SendCoins(ctxb, btcutil.SatoshiPerBitcoin, dave) + if err != nil { + t.Fatalf("unable to send coins to dave: %v", err) + } + + // In order to test Dave's response to an uncooperative channel closure + // by Carol, we'll first open up a channel between them with a // maxFundingAmount (2^24) satoshis value. ctxt, _ := context.WithTimeout(ctxb, timeout) - chanPoint := openChannelAndAssert(ctxt, t, net, net.Alice, carol, - chanAmt, pushAmt) + chanPoint := openChannelAndAssert( + ctxt, t, net, dave, carol, chanAmt, pushAmt, + ) // With the channel open, we'll create a few invoices for Carol that - // Alice will pay to in order to advance the state of the channel. + // Dave will pay to in order to advance the state of the channel. carolPayReqs := make([]string, numInvoices) for i := 0; i < numInvoices; i++ { preimage := bytes.Repeat([]byte{byte(192 - i)}, 32) @@ -3892,6 +3909,7 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, return carolChannelInfo.Channels[0], nil } + // We'll introduce a closure to validate that Carol's current balance // matches the given expected amount. checkCarolBalance := func(expectedAmt int64) { @@ -3905,6 +3923,7 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, expectedAmt) } } + // We'll introduce another closure to validate that Carol's current // number of updates is at least as large as the provided minimum // number. @@ -3920,21 +3939,50 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, } } - // Wait for Alice to receive the channel edge from the funding manager. + // Wait for Dave to receive the channel edge from the funding manager. ctxt, _ = context.WithTimeout(ctxb, timeout) - err = net.Alice.WaitForNetworkChannelOpen(ctxt, chanPoint) + err = dave.WaitForNetworkChannelOpen(ctxt, chanPoint) if err != nil { - t.Fatalf("alice didn't see the alice->carol channel before "+ + t.Fatalf("dave didn't see the dave->carol channel before "+ "timeout: %v", err) } // Ensure that carol's balance starts with the amount we pushed to her. checkCarolBalance(pushAmt) - // Send payments from Alice to Carol using 3 of Carol's payment hashes + // Send payments from Dave to Carol using 3 of Carol's payment hashes // generated above. - err = completePaymentRequests(ctxb, net.Alice, carolPayReqs[:numInvoices/2], - false) + err = completePaymentRequests( + ctxb, dave, carolPayReqs[:numInvoices/2], false, + ) + if err != nil { + t.Fatalf("unable to send payments: %v", err) + } + + // At this point, we'll also send over a set of HTLC's from Carol to + // Dave. This ensures that the final revoked transaction has HTLC's in + // both directions. + davePayReqs := make([]string, numInvoices) + for i := 0; i < numInvoices; i++ { + preimage := bytes.Repeat([]byte{byte(199 - i)}, 32) + invoice := &lnrpc.Invoice{ + Memo: "testing", + RPreimage: preimage, + Value: paymentAmt, + } + resp, err := dave.AddInvoice(ctxb, invoice) + if err != nil { + t.Fatalf("unable to add invoice: %v", err) + } + + davePayReqs[i] = resp.PaymentRequest + } + + // Send payments from Carol to Dave using 3 of Dave's payment hashes + // generated above. + err = completePaymentRequests( + ctxb, carol, davePayReqs[:numInvoices/2], false, + ) if err != nil { t.Fatalf("unable to send payments: %v", err) } @@ -3946,14 +3994,16 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, if err != nil { t.Fatalf("unable to get carol's channel info: %v", err) } + // Grab Carol's current commitment height (update number), we'll later // revert her to this state after additional updates to force her to // broadcast this soon to be revoked state. carolStateNumPreCopy := carolChan.NumUpdates // Ensure that carol's balance still reflects the original amount we - // pushed to her. - checkCarolBalance(pushAmt) + // pushed to her, minus the HTLCs she just sent to Dave. + checkCarolBalance(pushAmt - 3*paymentAmt) + // Since Carol has not settled, she should only see at least one update // to her channel. checkCarolNumUpdatesAtLeast(1) @@ -3974,18 +4024,20 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, t.Fatalf("unable to copy database files: %v", err) } - // Finally, send payments from Alice to Carol, consuming Carol's remaining - // payment hashes. - err = completePaymentRequests(ctxb, net.Alice, carolPayReqs[numInvoices/2:], - false) + // Finally, send payments from Dave to Carol, consuming Carol's + // remaining payment hashes. + err = completePaymentRequests( + ctxb, dave, carolPayReqs[numInvoices/2:], false, + ) if err != nil { t.Fatalf("unable to send payments: %v", err) } // Ensure that carol's balance still shows the amount we originally - // pushed to her, and that at least one more update has occurred. + // pushed to her (minus the HTLCs she sent to Bob), and that at least + // one more update has occurred. time.Sleep(500 * time.Millisecond) - checkCarolBalance(pushAmt) + checkCarolBalance(pushAmt - 3*paymentAmt) checkCarolNumUpdatesAtLeast(carolStateNumPreCopy + 1) // Now we shutdown Carol, copying over the her temporary database state @@ -4000,9 +4052,9 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, time.Sleep(200 * time.Millisecond) - // Ensure that Carol's view of the channel is consistent with the - // state of the channel just before it was snapshotted. - checkCarolBalance(pushAmt) + // Ensure that Carol's view of the channel is consistent with the state + // of the channel just before it was snapshotted. + checkCarolBalance(pushAmt - 3*paymentAmt) checkCarolNumUpdatesAtLeast(1) // Now query for Carol's channel state, it should show that she's at a @@ -4018,69 +4070,69 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, // Now force Carol to execute a *force* channel closure by unilaterally // broadcasting her current channel state. This is actually the // commitment transaction of a prior *revoked* state, so she'll soon - // feel the wrath of Alice's retribution. + // feel the wrath of Dave's retribution. force := true closeUpdates, _, err := net.CloseChannel(ctxb, carol, chanPoint, force) if err != nil { t.Fatalf("unable to close channel: %v", err) } - // Query the mempool for Alice's justice transaction, this should be + // Query the mempool for Dave's justice transaction, this should be // broadcast as Carol's contract breaching transaction gets confirmed // above. _, err = waitForTxInMempool(net.Miner.Node, 5*time.Second) if err != nil { - t.Fatalf("unable to find Alice's justice tx in mempool: %v", err) + t.Fatalf("unable to find Dave's justice tx in mempool: %v", err) } time.Sleep(200 * time.Millisecond) // Generate a single block to mine the breach transaction. block := mineBlocks(t, net, 1)[0] - // Wait so Alice receives a confirmation of Carol's breach transaction. + // Wait so Dave receives a confirmation of Carol's breach transaction. time.Sleep(200 * time.Millisecond) - // We restart Alice to ensure that she is persisting her retribution + // We restart Dave to ensure that he is persisting his retribution // state and continues exacting justice after her node restarts. - if err := net.RestartNode(net.Alice, nil); err != nil { - t.Fatalf("unable to stop Alice's node: %v", err) + if err := net.RestartNode(dave, nil); err != nil { + t.Fatalf("unable to stop Dave's node: %v", err) } - // Finally, Wait for the final close status update, then ensure that the - // closing transaction was included in the block. + // Finally, wait for the final close status update, then ensure that + // the closing transaction was included in the block. breachTXID, err := net.WaitForChannelClose(ctxb, closeUpdates) if err != nil { t.Fatalf("error while waiting for channel close: %v", err) } assertTxInBlock(t, block, breachTXID) - // Query the mempool for Alice's justice transaction, this should be + // Query the mempool for Dave's justice transaction, this should be // broadcast as Carol's contract breaching transaction gets confirmed // above. justiceTXID, err := waitForTxInMempool(net.Miner.Node, 5*time.Second) if err != nil { - t.Fatalf("unable to find Alice's justice tx in mempool: %v", err) + t.Fatalf("unable to find Dave's justice tx in mempool: %v", err) } time.Sleep(100 * time.Millisecond) - // We restart Alice here to ensure that she persists her retribution state + // We restart Dave here to ensure that he persists he retribution state // and successfully continues exacting retribution after restarting. At - // this point, Alice has broadcast the justice transaction, but it hasn't - // been confirmed yet; when Alice restarts, she should start waiting for - // the justice transaction to confirm again. - if err := net.RestartNode(net.Alice, nil); err != nil { - t.Fatalf("unable to restart Alice's node: %v", err) + // this point, Dave has broadcast the justice transaction, but it + // hasn't been confirmed yet; when Dave restarts, he should start + // waiting for the justice transaction to confirm again. + if err := net.RestartNode(dave, nil); err != nil { + t.Fatalf("unable to restart Dave's node: %v", err) } // Query for the mempool transaction found above. Then assert that (1) - // the justice tx has the appropriate number of inputs, and (2) all - // the inputs of this transaction are spending outputs generated by - // Carol's breach transaction above. + // the justice tx has the appropriate number of inputs, and (2) all the + // inputs of this transaction are spending outputs generated by Carol's + // breach transaction above, and also the HTLCs from Carol to Dave. justiceTx, err := net.Miner.Node.GetRawTransaction(justiceTXID) if err != nil { t.Fatalf("unable to query for justice tx: %v", err) } - exNumInputs := 2 + numInvoices/2 + exNumInputs := 2 + numInvoices if len(justiceTx.MsgTx().TxIn) != exNumInputs { t.Fatalf("justice tx should have exactly 2 commitment inputs"+ "and %v htlc inputs, expected %v in total, got %v", @@ -4088,7 +4140,7 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, len(justiceTx.MsgTx().TxIn)) } - // Now mine a block, this transaction should include Alice's justice + // Now mine a block, this transaction should include Dave's justice // transaction which was just accepted into the mempool. block = mineBlocks(t, net, 1)[0] @@ -4102,7 +4154,7 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, t.Fatalf("justice tx wasn't mined") } - assertNodeNumChannels(t, ctxb, net.Alice, 0) + assertNodeNumChannels(t, ctxb, dave, 0) } // assertNodeNumChannels polls the provided node's list channels rpc until it From 4e44528bffc2f4b4840dec0e24d3f24f3ff640b0 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 4 Apr 2018 17:07:07 -0700 Subject: [PATCH 2/5] lnwallet: fix ordering of keys when we generate the sender script during a breach In this commit, we fix an existing within lnd. Before this commit, within NewBreachRetribution the order of the keys when generating the sender HTLC script was incorrect. As in this case, the remote party is the sender, their key should be first. However, the order was swapped, meaning that at breach time, our transaction would be rejected as it had the incorrect witness script. The fix is simple: swap the ordering of the keys. After this commit, the test extension added in the prior commit now passes. --- lnwallet/channel.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 22717eda..2581c165 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1992,7 +1992,7 @@ func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64, // re-generate the sender HTLC script. if htlc.Incoming { htlcScript, err = senderHTLCScript( - keyRing.LocalHtlcKey, keyRing.RemoteHtlcKey, + keyRing.RemoteHtlcKey, keyRing.LocalHtlcKey, keyRing.RevocationKey, htlc.RHash[:], ) if err != nil { From c393475d39348557194f55178d1bb7571d7e614d Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 4 Apr 2018 17:07:45 -0700 Subject: [PATCH 3/5] lnwallet: ensure that we skip dust HTLC's in NewBreachRetribution --- lnwallet/channel.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 2581c165..71fe56e3 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1976,6 +1976,16 @@ func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64, err error ) + // If the HTLC is dust, then we'll skip it as it doesn't have + // an output on the commitment transaction. + if htlcIsDust( + htlc.Incoming, false, + SatPerKWeight(revokedSnapshot.FeePerKw), + htlc.Amt.ToSatoshis(), chanState.RemoteChanCfg.DustLimit, + ) { + continue + } + // We'll generate the original second level witness script now, // as we'll need it if we're revoking an HTLC output on the // remote commitment transaction, and *they* go to the second From f79af461d32feaa53881c761cabb05972d6b6148 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 5 Apr 2018 16:19:44 -0700 Subject: [PATCH 4/5] lnwallet: in NewBreachRetribution create the htlcRetributions slice to capacity, not length In this commit, we fix an existing bug in the NewBreachRetribution method. Rather than creating the slice to the proper length, we instead now create it to the proper _capacity_. As we'll now properly filter out any dust HTLCs, before this commit, even if no HTLCs were added, then the slice would still have a full length, meaning callers could actually interact with _blank_ HtlcRetribution structs. The fix is simple: create the slice with the proper capacity, and append to the end of it. --- lnwallet/channel.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 71fe56e3..0aa81435 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1969,8 +1969,8 @@ func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64, // With the commitment outputs located, we'll now generate all the // retribution structs for each of the HTLC transactions active on the // remote commitment transaction. - htlcRetributions := make([]HtlcRetribution, len(revokedSnapshot.Htlcs)) - for i, htlc := range revokedSnapshot.Htlcs { + htlcRetributions := make([]HtlcRetribution, 0, len(revokedSnapshot.Htlcs)) + for _, htlc := range revokedSnapshot.Htlcs { var ( htlcScript []byte err error @@ -2023,7 +2023,7 @@ func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64, } } - htlcRetributions[i] = HtlcRetribution{ + htlcRetributions = append(htlcRetributions, HtlcRetribution{ SignDesc: SignDescriptor{ KeyDesc: chanState.LocalChanCfg.RevocationBasePoint, DoubleTweak: commitmentSecret, @@ -2039,7 +2039,7 @@ func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64, }, SecondLevelWitnessScript: secondLevelWitnessScript, IsIncoming: htlc.Incoming, - } + }) } // Finally, with all the necessary data constructed, we can create the From 7da8cb29101627991f385fb15668474ad7977ace Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 5 Apr 2018 16:20:16 -0700 Subject: [PATCH 5/5] lnwallet: add new TestNewBreachRetributionSkipsDustHtlcs test --- lnwallet/channel_test.go | 95 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index ac4ba1e8..70da04fc 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -5012,3 +5012,98 @@ func TestMinHTLC(t *testing.T) { t.Fatalf("expected ErrBelowMinHTLC, instead received: %v", err) } } + +// TestNewBreachRetributionSkipsDustHtlcs ensures that in the case of a +// contract breach, all dust HTLCs are ignored and not reflected in the +// produced BreachRetribution struct. We ignore these HTLCs as they aren't +// actually manifested on the commitment transaction, as a result we can't +// actually revoked them. +func TestNewBreachRetributionSkipsDustHtlcs(t *testing.T) { + t.Parallel() + + // We'll kick off the test by creating our channels which both are + // loaded with 5 BTC each. + aliceChannel, bobChannel, cleanUp, err := createTestChannels(1) + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + defer cleanUp() + + var fakeOnionBlob [lnwire.OnionPacketSize]byte + copy(fakeOnionBlob[:], bytes.Repeat([]byte{0x05}, lnwire.OnionPacketSize)) + + // We'll modify the dust settings on both channels to be a predictable + // value for the prurpose of the test. + dustValue := btcutil.Amount(200) + aliceChannel.channelState.LocalChanCfg.DustLimit = dustValue + aliceChannel.channelState.RemoteChanCfg.DustLimit = dustValue + bobChannel.channelState.LocalChanCfg.DustLimit = dustValue + bobChannel.channelState.RemoteChanCfg.DustLimit = dustValue + + // We'll now create a series of dust HTLC's, and send then from Alice + // to Bob, finally locking both of them in. + var bobPreimage [32]byte + copy(bobPreimage[:], bytes.Repeat([]byte{0xbb}, 32)) + for i := 0; i < 3; i++ { + rHash := sha256.Sum256(bobPreimage[:]) + h := &lnwire.UpdateAddHTLC{ + PaymentHash: rHash, + Amount: lnwire.NewMSatFromSatoshis(dustValue), + Expiry: uint32(10), + OnionBlob: fakeOnionBlob, + } + + htlcIndex, err := aliceChannel.AddHTLC(h, nil) + if err != nil { + t.Fatalf("unable to add bob's htlc: %v", err) + } + + h.ID = htlcIndex + if _, err := bobChannel.ReceiveHTLC(h); err != nil { + t.Fatalf("unable to recv bob's htlc: %v", err) + } + } + + // With the HTLC's applied to both update logs, we'll initiate a state + // transition from Alice. + if err := forceStateTransition(bobChannel, aliceChannel); err != nil { + t.Fatalf("unable to complete bob's state transition: %v", err) + } + + // At this point, we'll capture the current state number, as well as + // the current commitment. + revokedCommit := bobChannel.channelState.LocalCommitment.CommitTx + revokedStateNum := aliceChannel.channelState.LocalCommitment.CommitHeight + + // We'll now have Bob settle those HTLC's to Alice and then advance + // forward to a new state. + for i := 0; i < 3; i++ { + err := bobChannel.SettleHTLC(bobPreimage, uint64(i), nil, nil, nil) + if err != nil { + t.Fatalf("unable to settle htlc: %v", err) + } + err = aliceChannel.ReceiveHTLCSettle(bobPreimage, uint64(i)) + if err != nil { + t.Fatalf("unable to settle htlc: %v", err) + } + } + if err := forceStateTransition(bobChannel, aliceChannel); err != nil { + t.Fatalf("unable to complete bob's state transition: %v", err) + } + + // At this point, we'll now simulate a contract breach by Bob using the + // NewBreachRetribution method. + breachRet, err := NewBreachRetribution( + aliceChannel.channelState, revokedStateNum, revokedCommit, 100, + ) + if err != nil { + t.Fatalf("unable to create breach retribution: %v", err) + } + + // The retribution shouldn't have any HTLCs set as they were all below + // dust for both parties. + if len(breachRet.HtlcRetributions) != 0 { + t.Fatalf("zero HTLC retributions should have been created, "+ + "instead %v were", len(breachRet.HtlcRetributions)) + } +}