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 diff --git a/lnwallet/channel.go b/lnwallet/channel.go index c7804c0f..962bc867 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1971,13 +1971,23 @@ 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 ) + // 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 @@ -1994,7 +2004,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 { @@ -2015,7 +2025,7 @@ func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64, } } - htlcRetributions[i] = HtlcRetribution{ + htlcRetributions = append(htlcRetributions, HtlcRetribution{ SignDesc: SignDescriptor{ KeyDesc: chanState.LocalChanCfg.RevocationBasePoint, DoubleTweak: commitmentSecret, @@ -2031,7 +2041,7 @@ func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64, }, SecondLevelWitnessScript: secondLevelWitnessScript, IsIncoming: htlc.Incoming, - } + }) } // Finally, with all the necessary data constructed, we can create the 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)) + } +}