From af8669c9778011a042fd0bc6332b9e563545fc82 Mon Sep 17 00:00:00 2001 From: George Tsagkarelis Date: Thu, 5 Dec 2024 16:12:57 +0100 Subject: [PATCH 1/3] tapchannel: aux invoice manager rejects asset payment on btc invoice Previously we had strict-forwarding protection placed in the AuxInvoiceManager, but that would only protect asset invoices against pure-btc HTLCs. This commit adds the inverse complementary check which prevents settling the invoice if btc ras requested but assets were offered. --- tapchannel/aux_invoice_manager.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tapchannel/aux_invoice_manager.go b/tapchannel/aux_invoice_manager.go index 51b212c59..2df17ebb5 100644 --- a/tapchannel/aux_invoice_manager.go +++ b/tapchannel/aux_invoice_manager.go @@ -164,6 +164,13 @@ func (s *AuxInvoiceManager) handleInvoiceAccept(_ context.Context, resp.CancelSet = true } + return resp, nil + } else if !isAssetInvoice(req.Invoice, s) && !req.Invoice.IsKeysend { + // If we do have custom records, but the invoice does not + // correspond to an asset invoice, we do not settle the invoice. + // Since we requested btc we should be receiving btc. + resp.CancelSet = true + return resp, nil } From 1e5d966400f29cb2b15d256607c90e651628ad83 Mon Sep 17 00:00:00 2001 From: George Tsagkarelis Date: Fri, 6 Dec 2024 12:59:56 +0100 Subject: [PATCH 2/3] tapchannel: add test case for btc invoice paid with assets In this commit we add an extra test case that checks if the AuxInvoiceManager will reject the asset HTLCs that pay to a btc invoice. We also do some housekeeping (commonly used vars, comments) across the other test cases. --- tapchannel/aux_invoice_manager_test.go | 52 ++++++++++++++++++++------ 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/tapchannel/aux_invoice_manager_test.go b/tapchannel/aux_invoice_manager_test.go index d40d0b580..779557753 100644 --- a/tapchannel/aux_invoice_manager_test.go +++ b/tapchannel/aux_invoice_manager_test.go @@ -24,9 +24,6 @@ import ( ) const ( - // The test channel ID to use across the test cases. - testChanID = 1234 - // maxRandomInvoiceValueMSat is the maximum invoice value in mSAT to be // generated by the property based tests. maxRandomInvoiceValueMSat = 100_000_000_000 @@ -52,12 +49,20 @@ var ( // The node ID to be used for the RFQ peer. testNodeID = route.Vertex{1, 2, 3} + // The asset rate value to use across tests. assetRate = big.NewInt(100_000) + // The asset rate struct based on the assetRate value. testAssetRate = rfqmath.FixedPoint[rfqmath.BigInt]{ Coefficient: rfqmath.NewBigInt(assetRate), Scale: 0, } + + // The test RFQ ID to use across tests. + testRfqID = dummyRfqID(31) + + // The test RFQ SCID that is derived from testRfqID. + testScid = testRfqID.Scid() ) // mockRfqManager mocks the interface of the rfq manager required by the aux @@ -293,7 +298,7 @@ func TestAuxInvoiceManager(t *testing.T) { }, }, buyQuotes: map[rfq.SerialisedScid]rfqmsg.BuyAccept{ - testChanID: { + testScid: { Peer: testNodeID, }, }, @@ -315,7 +320,7 @@ func TestAuxInvoiceManager(t *testing.T) { }, }, buyQuotes: map[rfq.SerialisedScid]rfqmsg.BuyAccept{ - testChanID: { + testScid: { Peer: testNodeID, }, }, @@ -335,7 +340,7 @@ func TestAuxInvoiceManager(t *testing.T) { dummyAssetID(1), 3, ), - }, fn.Some(dummyRfqID(31)), + }, fn.Some(testRfqID), ), }, }, @@ -345,7 +350,7 @@ func TestAuxInvoiceManager(t *testing.T) { }, }, buyQuotes: rfq.BuyAcceptMap{ - fn.Ptr(dummyRfqID(31)).Scid(): { + testScid: { Peer: testNodeID, AssetRate: rfqmsg.NewAssetRate( testAssetRate, time.Now(), @@ -368,7 +373,7 @@ func TestAuxInvoiceManager(t *testing.T) { dummyAssetID(1), 4, ), - }, fn.Some(dummyRfqID(31)), + }, fn.Some(testRfqID), ), ExitHtlcAmt: 1234, }, @@ -379,7 +384,7 @@ func TestAuxInvoiceManager(t *testing.T) { }, }, buyQuotes: rfq.BuyAcceptMap{ - fn.Ptr(dummyRfqID(31)).Scid(): { + testScid: { Peer: testNodeID, AssetRate: rfqmsg.NewAssetRate( testAssetRate, time.Now(), @@ -387,6 +392,31 @@ func TestAuxInvoiceManager(t *testing.T) { }, }, }, + { + name: "btc invoice, custom records", + requests: []lndclient.InvoiceHtlcModifyRequest{ + { + Invoice: &lnrpc.Invoice{ + ValueMsat: 10_000_000, + PaymentAddr: []byte{1, 1, 1}, + }, + WireCustomRecords: newWireCustomRecords( + t, []*rfqmsg.AssetBalance{ + rfqmsg.NewAssetBalance( + dummyAssetID(1), + 4, + ), + }, fn.Some(testRfqID), + ), + ExitHtlcAmt: 1234, + }, + }, + responses: []lndclient.InvoiceHtlcModifyResponse{ + { + CancelSet: true, + }, + }, + }, } for _, testCase := range testCases { @@ -761,8 +791,8 @@ func testRouteHints() []*lnrpc.RouteHint { NodeId: route.Vertex{1, 1, 1}.String(), }, { - ChanId: 1234, - NodeId: route.Vertex{1, 2, 3}.String(), + ChanId: uint64(testScid), + NodeId: testNodeID.String(), }, }, }, From ffc6e682eff3b1872339920a9f3bdcbe324ea1e2 Mon Sep 17 00:00:00 2001 From: George Tsagkarelis Date: Fri, 6 Dec 2024 13:02:23 +0100 Subject: [PATCH 3/3] tapchannel: add AuxInvoiceManager prop test btc invoice protection In this commit we extend the property based tests to account for the new piece of AuxInvoiceManager behavior. If the randomly generated invoice is not an asset invoice, but the HTLCs do carry assets, we should expect the manager to reject the HTLCs. --- tapchannel/aux_invoice_manager_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tapchannel/aux_invoice_manager_test.go b/tapchannel/aux_invoice_manager_test.go index 779557753..a6dadc3a5 100644 --- a/tapchannel/aux_invoice_manager_test.go +++ b/tapchannel/aux_invoice_manager_test.go @@ -189,6 +189,11 @@ func (m *mockHtlcModifierProperty) HtlcModifier(ctx context.Context, if r.ExitHtlcAmt != res.AmtPaid { m.t.Errorf("AmtPaid != ExitHtlcAmt") } + } else if !isAssetInvoice(r.Invoice, m) { + if !res.CancelSet { + m.t.Errorf("expected cancel set flag") + } + continue } htlcBlob, err := r.WireCustomRecords.Serialize()