diff --git a/docs/release-notes/release-notes-0.17.0.md b/docs/release-notes/release-notes-0.17.0.md index fab4919d4..5d177dede 100644 --- a/docs/release-notes/release-notes-0.17.0.md +++ b/docs/release-notes/release-notes-0.17.0.md @@ -162,6 +162,8 @@ unlock or create. * [Updated bbolt to v1.3.7](https://github.com/lightningnetwork/lnd/pull/7796) in order to address mmap issues affecting certain older iPhone devices. +* [Stop rejecting payments that overpay or over-timelock the final hop](https://github.com/lightningnetwork/lnd/pull/7768) + ### Tooling and documentation * Add support for [custom `RPCHOST` and @@ -187,6 +189,7 @@ unlock or create. * Hampus Sjöberg * hieblmi * Jordi Montes +* Keagan McClelland * Lele Calo * Matt Morehouse * Maxwell Sayles diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 7ef6d9a00..4fbe90554 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -1929,9 +1929,9 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) { // Instead of truncating the slice to conserve memory // allocations, we simply set the uncommitted preimage slice to // nil so that a new one will be initialized if any more - // witnesses are discovered. We do this maximum size of the - // slice can occupy 15KB, and want to ensure we release that - // memory back to the runtime. + // witnesses are discovered. We do this because the maximum size + // that the slice can occupy is 15KB, and we want to ensure we + // release that memory back to the runtime. l.uncommittedPreimages = nil // We just received a new updates to our local commitment @@ -3237,11 +3237,10 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor, // As we're the exit hop, we'll double check the hop-payload included in // the HTLC to ensure that it was crafted correctly by the sender and - // matches the HTLC we were extended. - if pd.Amount != fwdInfo.AmountToForward { - - l.log.Errorf("onion payload of incoming htlc(%x) has incorrect "+ - "value: expected %v, got %v", pd.RHash, + // is compatible with the HTLC we were extended. + if pd.Amount < fwdInfo.AmountToForward { + l.log.Errorf("onion payload of incoming htlc(%x) has "+ + "incompatible value: expected <=%v, got %v", pd.RHash, pd.Amount, fwdInfo.AmountToForward) failure := NewLinkError( @@ -3254,9 +3253,9 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor, // We'll also ensure that our time-lock value has been computed // correctly. - if pd.Timeout != fwdInfo.OutgoingCTLV { - l.log.Errorf("onion payload of incoming htlc(%x) has incorrect "+ - "time-lock: expected %v, got %v", + if pd.Timeout < fwdInfo.OutgoingCTLV { + l.log.Errorf("onion payload of incoming htlc(%x) has "+ + "incompatible time-lock: expected <=%v, got %v", pd.RHash[:], pd.Timeout, fwdInfo.OutgoingCTLV) failure := NewLinkError( diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index e744c75fc..6817ff930 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -12,6 +12,7 @@ import ( "runtime" "sync" "testing" + "testing/quick" "time" "github.com/btcsuite/btcd/btcec/v2" @@ -19,7 +20,6 @@ import ( "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/wire" "github.com/davecgh/go-spew/spew" - "github.com/go-errors/errors" sphinx "github.com/lightningnetwork/lightning-onion" "github.com/lightningnetwork/lnd/build" "github.com/lightningnetwork/lnd/channeldb" @@ -125,7 +125,7 @@ func createInterceptorFunc(prefix, receiver string, messages []expectedMessage, if messageChanID == chanID { if len(expectToReceive) == 0 { - return false, errors.Errorf("%v received "+ + return false, fmt.Errorf("%v received "+ "unexpected message out of range: %v", receiver, m.MsgType()) } @@ -134,9 +134,13 @@ func createInterceptorFunc(prefix, receiver string, messages []expectedMessage, expectToReceive = expectToReceive[1:] if expectedMessage.message.MsgType() != m.MsgType() { - return false, errors.Errorf("%v received wrong message: \n"+ - "real: %v\nexpected: %v", receiver, m.MsgType(), - expectedMessage.message.MsgType()) + return false, fmt.Errorf( + "%v received wrong message: \n"+ + "real: %v\nexpected: %v", + receiver, + m.MsgType(), + expectedMessage.message.MsgType(), + ) } if debug { @@ -721,11 +725,10 @@ func TestChannelLinkCancelFullCommitment(t *testing.T) { } } -// TestExitNodeTimelockPayloadMismatch tests that when an exit node receives an -// incoming HTLC, if the time lock encoded in the payload of the forwarded HTLC -// doesn't match the expected payment value, then the HTLC will be rejected -// with the appropriate error. -func TestExitNodeTimelockPayloadMismatch(t *testing.T) { +// TestExitNodeHLTCTimelockExceedsPayload tests that when an exit node receives +// an incoming HTLC, if the timelock of the incoming HTLC is greater than or +// equal to the timelock encoded in the payload, then the HTLC will be accepted. +func TestExitNodeHTLCTimelockExceedsPayload(t *testing.T) { t.Parallel() channels, _, err := createClusterChannels( @@ -733,35 +736,75 @@ func TestExitNodeTimelockPayloadMismatch(t *testing.T) { ) require.NoError(t, err, "unable to create channel") - n := newThreeHopNetwork(t, channels.aliceToBob, channels.bobToAlice, - channels.bobToCarol, channels.carolToBob, testStartingHeight) - if err := n.start(); err != nil { - t.Fatal(err) - } + n := newThreeHopNetwork( + t, channels.aliceToBob, channels.bobToAlice, + channels.bobToCarol, channels.carolToBob, testStartingHeight, + ) + require.NoError(t, n.start()) t.Cleanup(n.stop) const amount = btcutil.SatoshiPerBitcoin - htlcAmt, htlcExpiry, hops := generateHops(amount, - testStartingHeight, n.firstBobChannelLink) + htlcAmt, htlcExpiry, hops := generateHops( + amount, testStartingHeight, n.firstBobChannelLink, + ) // In order to exercise this case, we'll now _manually_ modify the - // per-hop payload for outgoing time lock to be the incorrect value. + // per-hop payload for outgoing time lock to be a compatible value that + // differs from the specified expiry. // The proper value of the outgoing CLTV should be the policy set by - // the receiving node, instead we set it to be a random value. - hops[0].FwdInfo.OutgoingCTLV = 500 + // the receiving node, instead we set it to be a value less than the + // incoming HTLC timelock. + hops[0].FwdInfo.OutgoingCTLV = htlcExpiry - 1 firstHop := n.firstBobChannelLink.ShortChanID() _, err = makePayment( n.aliceServer, n.bobServer, firstHop, hops, amount, htlcAmt, htlcExpiry, ).Wait(30 * time.Second) - if err == nil { - t.Fatalf("payment should have failed but didn't") - } + require.NoError(t, err, "payment should have succeeded but didn't") +} - rtErr, ok := err.(ClearTextError) - if !ok { - t.Fatalf("expected a ClearTextError, instead got: %T", err) - } +// TestExitNodeTimelockPayloadExceedsHTLC tests that when an exit node receives +// an incoming HTLC, if the timelock encoded in the payload of the forwarded +// HTLC exceeds the timelock on the incoming HTLC, then the HTLC will be +// rejected with the appropriate error. +func TestExitNodeTimelockPayloadExceedsHTLC(t *testing.T) { + t.Parallel() + + channels, _, err := createClusterChannels( + t, btcutil.SatoshiPerBitcoin*5, btcutil.SatoshiPerBitcoin*5, + ) + require.NoError(t, err, "unable to create channel") + + n := newThreeHopNetwork( + t, channels.aliceToBob, channels.bobToAlice, + channels.bobToCarol, channels.carolToBob, testStartingHeight, + ) + require.NoError(t, n.start()) + t.Cleanup(n.stop) + + const amount = btcutil.SatoshiPerBitcoin + htlcAmt, htlcExpiry, hops := generateHops( + amount, testStartingHeight, n.firstBobChannelLink, + ) + + // In order to exercise this case, we'll now _manually_ modify the + // per-hop payload for outgoing time lock to be the incorrect value. + // The proper value of the outgoing CLTV should be the policy set by + // the receiving node, instead we set it to be a value greater than the + // incoming HTLC timelock. + hops[0].FwdInfo.OutgoingCTLV = htlcExpiry + 1 + firstHop := n.firstBobChannelLink.ShortChanID() + _, err = makePayment( + n.aliceServer, n.bobServer, firstHop, hops, amount, htlcAmt, + htlcExpiry, + ).Wait(30 * time.Second) + require.NotNil(t, err, "payment should have failed but didn't") + + rtErr := &ForwardingError{} + require.ErrorAs( + t, err, &rtErr, "expected a ClearTextError, instead got: %T", + err, + ) switch rtErr.WireMessage().(type) { case *lnwire.FailFinalIncorrectCltvExpiry: @@ -771,43 +814,97 @@ func TestExitNodeTimelockPayloadMismatch(t *testing.T) { } } -// TestExitNodeAmountPayloadMismatch tests that when an exit node receives an -// incoming HTLC, if the amount encoded in the onion payload of the forwarded -// HTLC doesn't match the expected payment value, then the HTLC will be -// rejected. -func TestExitNodeAmountPayloadMismatch(t *testing.T) { +// TestExitNodeHTLCUnderpaysPayloadAmount tests that when an exit node receives +// an incoming HTLC, if the amount offered in the HTLC is less than the amount +// encoded in the onion payload then the HTLC will be rejected with the +// appropriate error. +func TestExitNodeHTLCUnderpaysPayloadAmount(t *testing.T) { t.Parallel() channels, _, err := createClusterChannels( - t, btcutil.SatoshiPerBitcoin*5, btcutil.SatoshiPerBitcoin*5, + t, btcutil.SatoshiPerBitcoin*5, + btcutil.SatoshiPerBitcoin*5, ) require.NoError(t, err, "unable to create channel") - n := newThreeHopNetwork(t, channels.aliceToBob, channels.bobToAlice, - channels.bobToCarol, channels.carolToBob, testStartingHeight) - if err := n.start(); err != nil { - t.Fatal(err) - } + n := newThreeHopNetwork( + t, channels.aliceToBob, channels.bobToAlice, + channels.bobToCarol, channels.carolToBob, + testStartingHeight, + ) + require.NoError(t, n.start()) t.Cleanup(n.stop) - const amount = btcutil.SatoshiPerBitcoin - htlcAmt, htlcExpiry, hops := generateHops(amount, testStartingHeight, - n.firstBobChannelLink) + f := func(underpaymentRand uint64) bool { + const amount = btcutil.SatoshiPerBitcoin / 100 + underpayment := lnwire.MilliSatoshi( + underpaymentRand%(amount-1) + 1, + ) - // In order to exercise this case, we'll now _manually_ modify the - // per-hop payload for amount to be the incorrect value. The proper - // value of the amount to forward should be the amount that the - // receiving node expects to receive. - hops[0].FwdInfo.AmountToForward = 1 - firstHop := n.firstBobChannelLink.ShortChanID() - _, err = makePayment( - n.aliceServer, n.bobServer, firstHop, hops, amount, htlcAmt, - htlcExpiry, - ).Wait(30 * time.Second) - if err == nil { - t.Fatalf("payment should have failed but didn't") + htlcAmt, htlcExpiry, hops := generateHops( + amount, testStartingHeight, n.firstBobChannelLink, + ) + + // In order to exercise this case, we'll now _manually_ modify + // the per-hop payload for amount to be the incorrect value. + // The acceptable values of the amount to forward should be less + // than the incoming HTLC value. + hops[0].FwdInfo.AmountToForward = amount + underpayment + firstHop := n.firstBobChannelLink.ShortChanID() + _, err = makePayment( + n.aliceServer, n.bobServer, firstHop, hops, amount, + htlcAmt, htlcExpiry, + ).Wait(30 * time.Second) + assertFailureCode(t, err, lnwire.CodeFinalIncorrectHtlcAmount) + + return err != nil } - assertFailureCode(t, err, lnwire.CodeFinalIncorrectHtlcAmount) + err = quick.Check(f, nil) + require.NoError(t, err, "payment should have failed but didn't") +} + +// TestExitNodeHTLCExceedsAmountPayload tests that when an exit node receives an +// incoming HTLC, if the amount encoded in the onion payload of the forwarded +// HTLC is lower than the incoming HTLC value, then the HTLC will be accepted. +func TestExitNodeHTLCExceedsAmountPayload(t *testing.T) { + t.Parallel() + + channels, _, err := createClusterChannels( + t, btcutil.SatoshiPerBitcoin*5, + btcutil.SatoshiPerBitcoin*5, + ) + require.NoError(t, err, "unable to create channel") + + n := newThreeHopNetwork(t, channels.aliceToBob, + channels.bobToAlice, channels.bobToCarol, + channels.carolToBob, testStartingHeight) + require.NoError(t, n.start()) + t.Cleanup(n.stop) + + f := func(overpaymentRand uint64) bool { + const amount = btcutil.SatoshiPerBitcoin / 100 + overpayment := lnwire.MilliSatoshi( + overpaymentRand%(amount-1) + 1, + ) + + htlcAmt, htlcExpiry, hops := generateHops(amount, + testStartingHeight, n.firstBobChannelLink) + + // In order to exercise this case, we'll now _manually_ modify + // the per-hop payload for amount to be the incorrect value. + // The acceptable values of the amount to forward should be + // lower than the incoming HTLC value. + hops[0].FwdInfo.AmountToForward = amount - overpayment + firstHop := n.firstBobChannelLink.ShortChanID() + _, err = makePayment( + n.aliceServer, n.bobServer, firstHop, hops, amount, + htlcAmt, htlcExpiry, + ).Wait(30 * time.Second) + + return err == nil + } + err = quick.Check(f, nil) + require.NoError(t, err, "payment should have succeeded but didn't") } // TestLinkForwardTimelockPolicyMismatch tests that if a node is an @@ -3512,25 +3609,37 @@ func TestChannelRetransmission(t *testing.T) { // bandwidth of htlc links hasn't been changed. invoice, err = receiver.registry.LookupInvoice(rhash) if err != nil { - err = errors.Errorf("unable to get invoice: %v", err) + err = fmt.Errorf( + "unable to get invoice: %w", err, + ) continue } if invoice.State != invpkg.ContractSettled { - err = errors.Errorf("alice invoice haven't been settled") + err = fmt.Errorf( + "alice invoice haven't been settled", + ) continue } aliceExpectedBandwidth := aliceBandwidthBefore - htlcAmt if aliceExpectedBandwidth != n.aliceChannelLink.Bandwidth() { - err = errors.Errorf("expected alice to have %v, instead has %v", - aliceExpectedBandwidth, n.aliceChannelLink.Bandwidth()) + err = fmt.Errorf( + "expected alice to have %v,"+ + " instead has %v", + aliceExpectedBandwidth, + n.aliceChannelLink.Bandwidth(), + ) continue } bobExpectedBandwidth := bobBandwidthBefore + htlcAmt if bobExpectedBandwidth != n.firstBobChannelLink.Bandwidth() { - err = errors.Errorf("expected bob to have %v, instead has %v", - bobExpectedBandwidth, n.firstBobChannelLink.Bandwidth()) + err = fmt.Errorf( + "expected bob to have %v,"+ + " instead has %v", + bobExpectedBandwidth, + n.firstBobChannelLink.Bandwidth(), + ) continue } @@ -5517,8 +5626,10 @@ func TestExpectedFee(t *testing.T) { } fee := ExpectedFee(f, test.htlcAmt) if fee != test.expected { - t.Errorf("expected fee to be (%v), instead got (%v)", test.expected, - fee) + t.Errorf( + "expected fee to be (%v), instead got (%v)", + test.expected, fee, + ) } } } diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index 866ad540f..586ceb240 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -4,6 +4,7 @@ import ( "crypto/rand" "math" "testing" + "testing/quick" "time" "github.com/lightningnetwork/lnd/amp" @@ -925,6 +926,83 @@ func TestMppPayment(t *testing.T) { } } +// TestMppPaymentWithOverpayment tests settling of an invoice with multiple +// partial payments. It covers the case where the mpp overpays what is in the +// invoice. +func TestMppPaymentWithOverpayment(t *testing.T) { + t.Parallel() + + f := func(overpaymentRand uint64) bool { + ctx := newTestContext(t, nil) + + // Add the invoice. + _, err := ctx.registry.AddInvoice( + testInvoice, testInvoicePaymentHash, + ) + if err != nil { + t.Fatal(err) + } + + mppPayload := &mockPayload{ + mpp: record.NewMPP(testInvoiceAmt, [32]byte{}), + } + + // We constrain overpayment amount to be [1,1000]. + overpayment := lnwire.MilliSatoshi((overpaymentRand % 999) + 1) + + // Send htlc 1. + hodlChan1 := make(chan interface{}, 1) + resolution, err := ctx.registry.NotifyExitHopHtlc( + testInvoicePaymentHash, testInvoice.Terms.Value/2, + testHtlcExpiry, testCurrentHeight, getCircuitKey(11), + hodlChan1, mppPayload, + ) + if err != nil { + t.Fatal(err) + } + if resolution != nil { + t.Fatal("expected no direct resolution") + } + + // Send htlc 2. + hodlChan2 := make(chan interface{}, 1) + resolution, err = ctx.registry.NotifyExitHopHtlc( + testInvoicePaymentHash, + testInvoice.Terms.Value/2+overpayment, testHtlcExpiry, + testCurrentHeight, getCircuitKey(12), hodlChan2, + mppPayload, + ) + if err != nil { + t.Fatal(err) + } + settleResolution, ok := + resolution.(*invpkg.HtlcSettleResolution) + if !ok { + t.Fatalf("expected settle resolution, got: %T", + resolution) + } + if settleResolution.Outcome != invpkg.ResultSettled { + t.Fatalf("expected result settled, got: %v", + settleResolution.Outcome) + } + + // Check that settled amount is equal to the sum of values of + // the htlcs 1 and 2. + inv, err := ctx.registry.LookupInvoice(testInvoicePaymentHash) + if err != nil { + t.Fatal(err) + } + if inv.State != invpkg.ContractSettled { + t.Fatal("expected invoice to be settled") + } + + return inv.AmtPaid == testInvoice.Terms.Value+overpayment + } + if err := quick.Check(f, nil); err != nil { + t.Fatalf("amount incorrect: %v", err) + } +} + // Tests that invoices are canceled after expiration. func TestInvoiceExpiryWithRegistry(t *testing.T) { t.Parallel() diff --git a/invoices/update.go b/invoices/update.go index 390f325a7..ed60c278c 100644 --- a/invoices/update.go +++ b/invoices/update.go @@ -214,11 +214,6 @@ func updateMpp(ctx *invoiceUpdateCtx, inv *Invoice) (*InvoiceUpdateDesc, // Add amount of new htlc. newSetTotal += ctx.amtPaid - // Make sure the communicated set total isn't overpaid. - if newSetTotal > ctx.mpp.TotalMsat() { - return nil, ctx.failRes(ResultHtlcSetOverpayment), nil - } - // The invoice is still open. Check the expiry. if ctx.expiry < uint32(ctx.currentHeight+ctx.finalCltvRejectDelta) { return nil, ctx.failRes(ResultExpiryTooSoon), nil @@ -243,7 +238,7 @@ func updateMpp(ctx *invoiceUpdateCtx, inv *Invoice) (*InvoiceUpdateDesc, } // If the invoice cannot be settled yet, only record the htlc. - setComplete := newSetTotal == ctx.mpp.TotalMsat() + setComplete := newSetTotal >= ctx.mpp.TotalMsat() if !setComplete { return &update, ctx.acceptRes(resultPartialAccepted), nil }