From 17e37bd7c20fb3aca993378d178e08c54e0b7836 Mon Sep 17 00:00:00 2001 From: ziggie Date: Tue, 28 Jan 2025 19:05:46 +0100 Subject: [PATCH] multi: introduce new traffic shaper method. We introduce a new specific fail resolution error when the external HTLC interceptor denies the incoming HTLC. Moreover we introduce a new traffic shaper method which moves the implementation of asset HTLC to the external layers. Moreover itests are adopted to reflect this new change. --- htlcswitch/interfaces.go | 5 + htlcswitch/link.go | 25 ++- invoices/invoiceregistry.go | 12 +- invoices/modification_interceptor.go | 3 - invoices/resolution_result.go | 10 +- itest/list_on_test.go | 8 - itest/lnd_forward_interceptor_test.go | 218 -------------------------- routing/bandwidth_test.go | 4 + 8 files changed, 37 insertions(+), 248 deletions(-) diff --git a/htlcswitch/interfaces.go b/htlcswitch/interfaces.go index 78cb01989..1ea1318ff 100644 --- a/htlcswitch/interfaces.go +++ b/htlcswitch/interfaces.go @@ -508,4 +508,9 @@ type AuxTrafficShaper interface { PaymentBandwidth(htlcBlob, commitmentBlob fn.Option[tlv.Blob], linkBandwidth, htlcAmt lnwire.MilliSatoshi) (lnwire.MilliSatoshi, error) + + // IsCustomHTLC returns true if the HTLC carries the set of relevant + // custom records to put it under the purview of the traffic shaper, + // meaning that it's from a custom channel. + IsCustomHTLC(htlcRecords lnwire.CustomRecords) bool } diff --git a/htlcswitch/link.go b/htlcswitch/link.go index f0e958992..77c8f67e3 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -4164,21 +4164,20 @@ func (l *channelLink) processExitHop(add lnwire.UpdateAddHTLC, return nil } + // In case the traffic shaper is active, we'll check if the HTLC has + // custom records and skip the amount check in the onion payload below. + isCustomHTLC := fn.MapOptionZ( + l.cfg.AuxTrafficShaper, + func(ts AuxTrafficShaper) bool { + return ts.IsCustomHTLC(add.CustomRecords) + }, + ) + // 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 - // is compatible with the HTLC we were extended. - // - // For a special case, if the fwdInfo doesn't have any blinded path - // information, and the incoming HTLC had special extra data, then - // we'll skip this amount check. The invoice acceptor will make sure we - // reject the HTLC if it's not containing the correct amount after - // examining the custom data. - hasBlindedPath := fwdInfo.NextBlinding.IsSome() - customHTLC := len(add.CustomRecords) > 0 && !hasBlindedPath - log.Tracef("Exit hop has_blinded_path=%v custom_htlc_bypass=%v", - hasBlindedPath, customHTLC) - - if !customHTLC && add.Amount < fwdInfo.AmountToForward { + // is compatible with the HTLC we were extended. If an external + // validator is active we might bypass the amount check. + if !isCustomHTLC && add.Amount < fwdInfo.AmountToForward { l.log.Errorf("onion payload of incoming htlc(%x) has "+ "incompatible value: expected <=%v, got %v", add.PaymentHash, add.Amount, fwdInfo.AmountToForward) diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index 97e6085ef..49f0cb0f0 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -1117,13 +1117,15 @@ func (i *InvoiceRegistry) notifyExitHopHtlcLocked( return nil, nil } - // If a cancel signal was set for the htlc set, we set - // the resolution as a failure with an underpayment - // indication. Something was wrong with this htlc, so - // we probably can't settle the invoice at all. + // The error `ExternalValidationFailed` error + // information will be packed in the + // `FailIncorrectDetails` msg when sending the msg to + // the peer. Error codes are defined by the BOLT 04 + // specification. The error text can be arbitrary + // therefore we return a custom error msg. resolution = NewFailResolution( ctx.circuitKey, ctx.currentHeight, - ResultAmountTooLow, + ExternalValidationFailed, ) // We cancel all HTLCs which are in the accepted state. diff --git a/invoices/modification_interceptor.go b/invoices/modification_interceptor.go index 58f5b63d0..c8d6783f5 100644 --- a/invoices/modification_interceptor.go +++ b/invoices/modification_interceptor.go @@ -137,9 +137,6 @@ func (s *HtlcModificationInterceptor) Intercept(clientRequest HtlcModifyRequest, // Wait for the client to respond or an error to occur. select { case response := <-responseChan: - log.Debugf("Received invoice HTLC interceptor response: %v", - response) - responseCallback(*response) return nil diff --git a/invoices/resolution_result.go b/invoices/resolution_result.go index f66198f05..5a7d9984d 100644 --- a/invoices/resolution_result.go +++ b/invoices/resolution_result.go @@ -120,6 +120,10 @@ const ( // ResultAmpReconstruction is returned when the derived child // hash/preimage pairs were invalid for at least one HTLC in the set. ResultAmpReconstruction + + // ExternalValidationFailed is returned when the external validation + // failed. + ExternalValidationFailed ) // String returns a string representation of the result. @@ -189,6 +193,9 @@ func (f FailResolutionResult) FailureString() string { case ResultAmpReconstruction: return "amp reconstruction failed" + case ExternalValidationFailed: + return "external validation failed" + default: return "unknown failure resolution result" } @@ -202,7 +209,8 @@ func (f FailResolutionResult) IsSetFailure() bool { ResultAmpReconstruction, ResultHtlcSetTotalTooLow, ResultHtlcSetTotalMismatch, - ResultHtlcSetOverpayment: + ResultHtlcSetOverpayment, + ExternalValidationFailed: return true diff --git a/itest/list_on_test.go b/itest/list_on_test.go index 6de56605b..2cf71feec 100644 --- a/itest/list_on_test.go +++ b/itest/list_on_test.go @@ -406,14 +406,6 @@ var allTestCases = []*lntest.TestCase{ Name: "forward interceptor", TestFunc: testForwardInterceptorBasic, }, - { - Name: "forward interceptor modified htlc", - TestFunc: testForwardInterceptorModifiedHtlc, - }, - { - Name: "forward interceptor wire records", - TestFunc: testForwardInterceptorWireRecords, - }, { Name: "forward interceptor restart", TestFunc: testForwardInterceptorRestart, diff --git a/itest/lnd_forward_interceptor_test.go b/itest/lnd_forward_interceptor_test.go index 93012c98f..7a6888a57 100644 --- a/itest/lnd_forward_interceptor_test.go +++ b/itest/lnd_forward_interceptor_test.go @@ -10,7 +10,6 @@ import ( "github.com/btcsuite/btcd/btcutil" "github.com/lightningnetwork/lnd/chainreg" "github.com/lightningnetwork/lnd/lnrpc" - "github.com/lightningnetwork/lnd/lnrpc/invoicesrpc" "github.com/lightningnetwork/lnd/lnrpc/routerrpc" "github.com/lightningnetwork/lnd/lntest" "github.com/lightningnetwork/lnd/lntest/node" @@ -346,223 +345,6 @@ func testForwardInterceptorBasic(ht *lntest.HarnessTest) { } } -// testForwardInterceptorModifiedHtlc tests that the interceptor can modify the -// amount and custom records of an intercepted HTLC and resume it. -func testForwardInterceptorModifiedHtlc(ht *lntest.HarnessTest) { - const chanAmt = btcutil.Amount(300000) - p := lntest.OpenChannelParams{Amt: chanAmt} - - // Initialize the test context with 3 connected nodes. - cfgs := [][]string{nil, nil, nil} - - // Open and wait for channels. - _, nodes := ht.CreateSimpleNetwork(cfgs, p) - alice, bob, carol := nodes[0], nodes[1], nodes[2] - - // Init the scenario. - ts := &interceptorTestScenario{ - ht: ht, - alice: alice, - bob: bob, - carol: carol, - } - - // Connect an interceptor to Bob's node. - bobInterceptor, cancelBobInterceptor := bob.RPC.HtlcInterceptor() - - // We're going to modify the payment amount and want Carol to accept the - // payment, so we set up an invoice acceptor on Dave. - carolAcceptor, carolCancel := carol.RPC.InvoiceHtlcModifier() - defer carolCancel() - - // Prepare the test cases. - invoiceValueAmtMsat := int64(20_000_000) - req := &lnrpc.Invoice{ValueMsat: invoiceValueAmtMsat} - addResponse := carol.RPC.AddInvoice(req) - invoice := carol.RPC.LookupInvoice(addResponse.RHash) - tc := &interceptorTestCase{ - amountMsat: invoiceValueAmtMsat, - invoice: invoice, - payAddr: invoice.PaymentAddr, - } - - // We initiate a payment from Alice. - done := make(chan struct{}) - go func() { - // Signal that all the payments have been sent. - defer close(done) - - ts.sendPaymentAndAssertAction(tc) - }() - - // We start the htlc interceptor with a simple implementation that saves - // all intercepted packets. These packets are held to simulate a - // pending payment. - packet := ht.ReceiveHtlcInterceptor(bobInterceptor) - - // Resume the intercepted HTLC with a modified amount and custom - // records. - customRecords := make(map[uint64][]byte) - - // Add custom records entry. - crKey := uint64(65537) - crValue := []byte("custom-records-test-value") - customRecords[crKey] = crValue - - // Modify the amount of the HTLC, so we send out less than the original - // amount. - const modifyAmount = 5_000_000 - newOutAmountMsat := packet.OutgoingAmountMsat - modifyAmount - err := bobInterceptor.Send(&routerrpc.ForwardHtlcInterceptResponse{ - IncomingCircuitKey: packet.IncomingCircuitKey, - OutAmountMsat: newOutAmountMsat, - OutWireCustomRecords: customRecords, - Action: actionResumeModify, - }) - require.NoError(ht, err, "failed to send request") - - invoicePacket := ht.ReceiveInvoiceHtlcModification(carolAcceptor) - require.EqualValues( - ht, newOutAmountMsat, invoicePacket.ExitHtlcAmt, - ) - amtPaid := newOutAmountMsat + modifyAmount - err = carolAcceptor.Send(&invoicesrpc.HtlcModifyResponse{ - CircuitKey: invoicePacket.ExitHtlcCircuitKey, - AmtPaid: &amtPaid, - }) - require.NoError(ht, err, "carol acceptor response") - - // Cancel the context, which will disconnect Bob's interceptor. - cancelBobInterceptor() - - // Make sure all goroutines are finished. - select { - case <-done: - case <-time.After(defaultTimeout): - require.Fail(ht, "timeout waiting for sending payment") - } - - // Assert that the payment was successful. - var preimage lntypes.Preimage - copy(preimage[:], invoice.RPreimage) - ht.AssertPaymentStatus(alice, preimage, lnrpc.Payment_SUCCEEDED) -} - -// testForwardInterceptorWireRecords tests that the interceptor can read any -// wire custom records provided by the sender of a payment as part of the -// update_add_htlc message. -func testForwardInterceptorWireRecords(ht *lntest.HarnessTest) { - const chanAmt = btcutil.Amount(300000) - p := lntest.OpenChannelParams{Amt: chanAmt} - - // Initialize the test context with 4 connected nodes. - cfgs := [][]string{nil, nil, nil, nil} - - // Open and wait for channels. - _, nodes := ht.CreateSimpleNetwork(cfgs, p) - alice, bob, carol, dave := nodes[0], nodes[1], nodes[2], nodes[3] - - // Connect an interceptor to Bob's node. - bobInterceptor, cancelBobInterceptor := bob.RPC.HtlcInterceptor() - defer cancelBobInterceptor() - - // Also connect an interceptor on Carol's node to check whether we're - // relaying the TLVs send in update_add_htlc over Alice -> Bob on the - // Bob -> Carol link. - carolInterceptor, cancelCarolInterceptor := carol.RPC.HtlcInterceptor() - defer cancelCarolInterceptor() - - // We're going to modify the payment amount and want Dave to accept the - // payment, so we set up an invoice acceptor on Dave. - daveAcceptor, daveCancel := dave.RPC.InvoiceHtlcModifier() - defer daveCancel() - - req := &lnrpc.Invoice{ValueMsat: 20_000_000} - addResponse := dave.RPC.AddInvoice(req) - invoice := dave.RPC.LookupInvoice(addResponse.RHash) - - customRecords := map[uint64][]byte{ - 65537: []byte("test"), - } - sendReq := &routerrpc.SendPaymentRequest{ - PaymentRequest: invoice.PaymentRequest, - TimeoutSeconds: int32(wait.PaymentTimeout.Seconds()), - FeeLimitMsat: noFeeLimitMsat, - FirstHopCustomRecords: customRecords, - } - ht.SendPaymentAssertInflight(alice, sendReq) - - // We start the htlc interceptor with a simple implementation that saves - // all intercepted packets. These packets are held to simulate a - // pending payment. - packet := ht.ReceiveHtlcInterceptor(bobInterceptor) - require.Equal(ht, lntest.CustomRecordsWithUnendorsed( - customRecords, - ), packet.InWireCustomRecords) - - // Just resume the payment on Bob. - err := bobInterceptor.Send(&routerrpc.ForwardHtlcInterceptResponse{ - IncomingCircuitKey: packet.IncomingCircuitKey, - Action: actionResume, - }) - require.NoError(ht, err, "failed to send request") - - // Assert that the Alice -> Bob custom records in update_add_htlc are - // not propagated on the Bob -> Carol link, just an endorsement signal. - packet = ht.ReceiveHtlcInterceptor(carolInterceptor) - require.Equal(ht, lntest.CustomRecordsWithUnendorsed(nil), - packet.InWireCustomRecords) - - // We're going to tell Carol to forward 5k sats less to Dave. We need to - // set custom records on the HTLC as well, to make sure the HTLC isn't - // rejected outright and actually gets to the invoice acceptor. - const modifyAmount = 5_000_000 - newOutAmountMsat := packet.OutgoingAmountMsat - modifyAmount - err = carolInterceptor.Send(&routerrpc.ForwardHtlcInterceptResponse{ - IncomingCircuitKey: packet.IncomingCircuitKey, - OutAmountMsat: newOutAmountMsat, - OutWireCustomRecords: customRecords, - Action: actionResumeModify, - }) - require.NoError(ht, err, "carol interceptor response") - - // The payment should get to Dave, and we should be able to intercept - // and modify it, telling Dave to accept it. - invoicePacket := ht.ReceiveInvoiceHtlcModification(daveAcceptor) - require.EqualValues( - ht, newOutAmountMsat, invoicePacket.ExitHtlcAmt, - ) - amtPaid := newOutAmountMsat + modifyAmount - err = daveAcceptor.Send(&invoicesrpc.HtlcModifyResponse{ - CircuitKey: invoicePacket.ExitHtlcCircuitKey, - AmtPaid: &amtPaid, - }) - require.NoError(ht, err, "dave acceptor response") - - // Assert that the payment was successful. - var preimage lntypes.Preimage - copy(preimage[:], invoice.RPreimage) - ht.AssertPaymentStatus( - alice, preimage, lnrpc.Payment_SUCCEEDED, - func(p *lnrpc.Payment) error { - recordsEqual := reflect.DeepEqual( - p.FirstHopCustomRecords, - lntest.CustomRecordsWithUnendorsed( - customRecords, - ), - ) - if !recordsEqual { - return fmt.Errorf("expected custom records to "+ - "be equal, got %v expected %v", - p.FirstHopCustomRecords, - sendReq.FirstHopCustomRecords) - } - - return nil - }, - ) -} - // testForwardInterceptorRestart tests that the interceptor can read any wire // custom records provided by the sender of a payment as part of the // update_add_htlc message and that those records are persisted correctly and diff --git a/routing/bandwidth_test.go b/routing/bandwidth_test.go index b31d0095a..259f347ba 100644 --- a/routing/bandwidth_test.go +++ b/routing/bandwidth_test.go @@ -164,3 +164,7 @@ func (*mockTrafficShaper) ProduceHtlcExtraData(totalAmount lnwire.MilliSatoshi, return totalAmount, nil, nil } + +func (*mockTrafficShaper) IsCustomHTLC(_ lnwire.CustomRecords) bool { + return false +}