diff --git a/docs/release-notes/release-notes-0.15.0.md b/docs/release-notes/release-notes-0.15.0.md index 301f41e02..e9f6df6a0 100644 --- a/docs/release-notes/release-notes-0.15.0.md +++ b/docs/release-notes/release-notes-0.15.0.md @@ -18,6 +18,8 @@ ## Bug Fixes +* [Pipelining an UpdateFulfillHTLC message now only happens when the related UpdateAddHTLC is locked-in.](https://github.com/lightningnetwork/lnd/pull/6246) + * [Fixed an inactive invoice subscription not removed from invoice registry](https://github.com/lightningnetwork/lnd/pull/6053). When an invoice subscription is created and canceled immediately, it could be left uncleaned diff --git a/htlcswitch/link.go b/htlcswitch/link.go index f4714803d..57b523318 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -1672,6 +1672,28 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) { case *lnwire.UpdateFulfillHTLC: pre := msg.PaymentPreimage idx := msg.ID + + // Before we pipeline the settle, we'll check the set of active + // htlc's to see if the related UpdateAddHTLC has been fully + // locked-in. + var lockedin bool + htlcs := l.channel.ActiveHtlcs() + for _, add := range htlcs { + // The HTLC will be outgoing and match idx. + if !add.Incoming && add.HtlcIndex == idx { + lockedin = true + break + } + } + + if !lockedin { + l.fail( + LinkFailureError{code: ErrInvalidUpdate}, + "unable to handle upstream settle", + ) + return + } + if err := l.channel.ReceiveHTLCSettle(pre, idx); err != nil { l.fail( LinkFailureError{ diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 39d215005..0d6f9eb7a 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -5500,8 +5500,8 @@ func TestChannelLinkFail(t *testing.T) { false, }, { - // Test that we force close the channel if we receive - // an invalid Settle message. + // Test that we don't force close the channel if we + // receive an invalid Settle message. func(c *channelLink) { }, func(t *testing.T, c *channelLink, _ *lnwallet.LightningChannel) { @@ -5513,7 +5513,7 @@ func TestChannelLinkFail(t *testing.T) { } c.HandleChannelUpdate(htlcSettle) }, - true, + false, false, }, { @@ -6684,6 +6684,179 @@ func TestShutdownIfChannelClean(t *testing.T) { } } +// TestPipelineSettle tests that a link should only pipeline a settle if the +// related add is fully locked-in meaning it is on both sides' commitment txns. +func TestPipelineSettle(t *testing.T) { + t.Parallel() + + const chanAmt = btcutil.SatoshiPerBitcoin * 5 + const chanReserve = btcutil.SatoshiPerBitcoin * 1 + aliceLink, bobChannel, _, start, cleanUp, restore, err := + newSingleLinkTestHarness(chanAmt, chanReserve) + require.NoError(t, err) + defer cleanUp() + + alice := newPersistentLinkHarness( + t, aliceLink, nil, restore, + ) + + linkErrors := make(chan LinkFailureError, 1) + + // Modify OnChannelFailure so we are notified when the link is failed. + alice.coreLink.cfg.OnChannelFailure = func(_ lnwire.ChannelID, + _ lnwire.ShortChannelID, linkErr LinkFailureError) { + + linkErrors <- linkErr + } + + // Modify ForwardPackets so we are notified if a settle packet is + // erroneously forwarded. If the forwardChan is closed before the last + // step, then the test will fail. + forwardChan := make(chan struct{}) + fwdPkts := func(c chan struct{}, hp ...*htlcPacket) error { + close(forwardChan) + return nil + } + alice.coreLink.cfg.ForwardPackets = fwdPkts + + // Put Alice in ExitSettle mode, so we can simulate a multi-hop route + // without actually doing so. This allows us to test the locked-in add + // logic without having the add being removed by Alice sending a + // settle. + alice.coreLink.cfg.HodlMask = hodl.Mask(hodl.ExitSettle) + + err = start() + require.NoError(t, err) + + ctx := linkTestContext{ + t: t, + aliceLink: alice.link, + bobChannel: bobChannel, + aliceMsgs: alice.msgs, + } + + // First lock in an HTLC from Bob to Alice. + htlc1, invoice1 := generateHtlcAndInvoice(t, 0) + preimage1 := invoice1.Terms.PaymentPreimage + + // Add the invoice to Alice's registry so she expects it. + aliceReg := alice.coreLink.cfg.Registry.(*mockInvoiceRegistry) + err = aliceReg.AddInvoice(*invoice1, htlc1.PaymentHash) + require.NoError(t, err) + + // <---add----- + ctx.sendHtlcBobToAlice(htlc1) + // <---sig----- + ctx.sendCommitSigBobToAlice(1) + // ----rev----> + ctx.receiveRevAndAckAliceToBob() + // ----sig----> + ctx.receiveCommitSigAliceToBob(1) + // <---rev----- + ctx.sendRevAndAckBobToAlice() + + // Bob will send the preimage for the HTLC he just sent. This will test + // the check that the HTLC is locked-in. The channel should not be + // force closed if everything is working correctly. + settle1 := &lnwire.UpdateFulfillHTLC{ + ID: 0, + PaymentPreimage: *preimage1, + } + ctx.aliceLink.HandleChannelUpdate(settle1) + + // ForceClose should be false. + select { + case linkErr := <-linkErrors: + require.False(t, linkErr.ForceClose) + case <-forwardChan: + t.Fatal("packet was erroneously forwarded") + } + + // Restart Alice's link with the hodl.ExitSettle and hodl.Commit flags. + alice.restart(false, false, hodl.ExitSettle, hodl.Commit) + ctx.aliceLink = alice.link + ctx.aliceMsgs = alice.msgs + + alice.coreLink.cfg.OnChannelFailure = func(_ lnwire.ChannelID, + _ lnwire.ShortChannelID, linkErr LinkFailureError) { + + linkErrors <- linkErr + } + alice.coreLink.cfg.ForwardPackets = fwdPkts + + // Alice will now send an HTLC to Bob, but won't sign a commitment for + // it. This HTLC will have the same payment hash as the one above. + htlc2 := htlc1 + + // ----add---> + ctx.sendHtlcAliceToBob(0, htlc2) + ctx.receiveHtlcAliceToBob() + + // Now Bob will send a settle backwards before the HTLC is locked in + // and the link should be failed again. + settle2 := &lnwire.UpdateFulfillHTLC{ + ID: 0, + PaymentPreimage: *preimage1, + } + ctx.aliceLink.HandleChannelUpdate(settle2) + + // ForceClose should be false. + select { + case linkErr := <-linkErrors: + require.False(t, linkErr.ForceClose) + case <-forwardChan: + t.Fatal("packet was erroneously forwarded") + } + + // Restart Alice's link without the hodl.Commit flag. + alice.restart(false, false, hodl.ExitSettle) + ctx.aliceLink = alice.link + ctx.aliceMsgs = alice.msgs + + alice.coreLink.cfg.OnChannelFailure = func(_ lnwire.ChannelID, + _ lnwire.ShortChannelID, linkErr LinkFailureError) { + + linkErrors <- linkErr + } + alice.coreLink.cfg.ForwardPackets = fwdPkts + + // Alice's mailbox should give the link the HTLC to send again. + select { + case msg := <-ctx.aliceMsgs: + _, ok := msg.(*lnwire.UpdateAddHTLC) + require.True(t, ok) + case <-time.After(5 * time.Second): + t.Fatal("did not receive htlc from alice") + } + + // Trigger the BatchTicker. + select { + case alice.batchTicker <- time.Now(): + case <-time.After(5 * time.Second): + t.Fatalf("could not force commit sig") + } + + // ----sig---> + ctx.receiveCommitSigAliceToBob(2) + // <---rev---- + ctx.sendRevAndAckBobToAlice() + // <---sig---- + ctx.sendCommitSigBobToAlice(2) + // ----rev---> + ctx.receiveRevAndAckAliceToBob() + + // Bob should now be able to send the settle to Alice without making + // the link fail. + ctx.aliceLink.HandleChannelUpdate(settle2) + + select { + case <-linkErrors: + t.Fatal("should not have received a link error") + case <-forwardChan: + // success + } +} + // assertFailureCode asserts that an error is of type ClearTextError and that // the failure code is as expected. func assertFailureCode(t *testing.T, err error, code lnwire.FailCode) {