Merge pull request #1673 from lightninglabs/invoice-tolerance-tweaks

Invoice manager tolerance tweaks

Author: Roasbeef

docs/release-notes/release-notes-0.7.0.md

@@ -35,11 +35,18 @@
   optimized sending to V2 TAP addresses by removing the need for creating
   tombstone outputs on a full-value send (by using interactive transfers for V2
   addresses).
+
 - [Updated](https://github.com/lightninglabs/taproot-assets/pull/1774) 
   `BuyOrderRequest` and `SellOrderRequest` proto docs to mark `peer_pub_key` as
   required. Previously, the field was incorrectly documented as optional.
   This change corrects the documentation to match the current implementation.
 
+- [Invoice tolerance calculations were fixed to properly account for per-HTLC
+  conversion errors](https://github.com/lightninglabs/taproot-assets/pull/1673).
+  This improves the accuracy of asset payment acceptance by correctly modeling
+  rounding errors that accumulate when converting between asset units and
+  millisatoshis across multiple HTLCs.
+
 # New Features
 
 ## Functional Enhancements

tapchannel/aux_invoice_manager.go

@@ -290,12 +290,6 @@ func (s *AuxInvoiceManager) handleInvoiceAccept(ctx context.Context,
 	// is not yet in that list.
 	allowedMarginAssetUnits := uint64(len(req.Invoice.Htlcs) + 1)
 
-	// Because we do a round trip of units -> milli-sats, then split into
-	// shards, then back to units and then back to milli-sats again, we lose
-	// up to numHTLCs+1 asset units in the process. So we allow for an
-	// additional unit here.
-	allowedMarginAssetUnits++
-
 	// Convert the allowed margin asset units to milli-satoshis.
 	marginAssetUnits := rfqmath.NewBigIntFixedPoint(
 		allowedMarginAssetUnits, 0,
@@ -304,10 +298,17 @@ func (s *AuxInvoiceManager) handleInvoiceAccept(ctx context.Context,
 		marginAssetUnits, *assetRate,
 	)
 
+	// Every HTLC makes a roundtrip from msats to units and then back to
+	// msats right here. Above we account for the asset units that may have
+	// been lost by our peer when converting their incoming HTLC amount to
+	// asset units, now we need to account for our loss for converting each
+	// HTLC to msats.
+	allowedMarginMSat += lnwire.MilliSatoshi(len(req.Invoice.Htlcs) + 1)
+
 	// If the sum of the accepted HTLCs plus the current HTLC amount plus
 	// the error margin is greater than the invoice amount, we'll accept it.
 	totalInbound := acceptedHtlcSum + resp.AmtPaid
-	totalInboundWithMargin := totalInbound + allowedMarginMSat + 1
+	totalInboundWithMargin := totalInbound + allowedMarginMSat
 	invoiceValue := lnwire.MilliSatoshi(req.Invoice.ValueMsat)
 
 	iLog.Debugf("accepted HTLC sum: %v, current HTLC amount: %v, allowed "+
@@ -318,6 +319,12 @@ func (s *AuxInvoiceManager) handleInvoiceAccept(ctx context.Context,
 	// If we're within the error margin, we'll increase the current HTLCs
 	// amount to cover the error rate and make the total sum match the
 	// invoice amount exactly.
+	//
+	// Note: The total calculated margin should match what is implemented in
+	// our TestUnitConversionToleranceRapid test. That test serves as a
+	// sanity check for our tolerance model, which should allow all
+	// combinations of shards and asset rates to be captured by the total
+	// calculated tolerance.
 	if totalInboundWithMargin >= invoiceValue {
 		resp.AmtPaid = invoiceValue - acceptedHtlcSum
 	}

tapchannel/aux_traffic_shaper_test.go

@@ -19,7 +19,7 @@ func TestUnitConversionTolerance(t *testing.T) {
 	)
 	var (
 		rate = rfqmath.BigIntFixedPoint{
-			Coefficient: rfqmath.NewBigIntFromUint64(9852216748),
+			Coefficient: rfqmath.NewBigIntFromUint64(9_852_216_748),
 			Scale:       0,
 		}
 	)
@@ -61,8 +61,12 @@ func TestUnitConversionTolerance(t *testing.T) {
 		newMarginAssetUnits, rate,
 	)
 
+	proposedMargin := rfqmath.UnitsToMilliSatoshi(marginAssetUnits, rate) +
+		lnwire.MilliSatoshi(numHTLCs)
+
 	t.Logf("Old tolerance allowed in msat: %d", allowedMarginMSat)
 	t.Logf("New tolerance allowed in msat: %d", newAllowedMarginMSat)
+	t.Logf("Proposed tolerance allowed in msat: %d", proposedMargin)
 }
 
 // TestUnitConversionToleranceRapid uses rapid to randomly draw invoice amounts,
@@ -74,7 +78,7 @@ func TestUnitConversionToleranceRapid(t *testing.T) {
 			Draw(t, "invoiceAmtUnits")
 		numHTLCs := rapid.Uint64Range(1, 16).
 			Draw(t, "numHTLCs")
-		coefficient := rapid.Uint64Range(1, 20_000_000_000).
+		coefficient := rapid.Uint64Range(1, 300_000_000_000).
 			Draw(t, "coefficient")
 
 		rate := rfqmath.BigIntFixedPoint{
@@ -89,26 +93,46 @@ func TestUnitConversionToleranceRapid(t *testing.T) {
 
 		shardSizeMSat := invoiceAmtMsat / lnwire.MilliSatoshi(numHTLCs)
 		shardSizeFP := rfqmath.MilliSatoshiToUnits(shardSizeMSat, rate)
-		shardSizeUnit := shardSizeFP.ScaleTo(0).ToUint64()
 
-		shardSumFP := rfqmath.NewBigIntFixedPoint(
-			shardSizeUnit*numHTLCs, 0,
-		)
-		inboundAmountMSat := rfqmath.UnitsToMilliSatoshi(
-			shardSumFP, rate,
-		)
+		// In order to account for the full invoice amt we need to also
+		// somehow carry the remainder of invoiceAmt/numHTLCs. We do so
+		// by appending it to the last shard.
+		invoiceMod := invoiceAmtMsat % lnwire.MilliSatoshi(numHTLCs)
+		lastShardMsat := shardSizeMSat + invoiceMod
+		lastShardFP := rfqmath.MilliSatoshiToUnits(lastShardMsat, rate)
+
+		var inboundAmountMSat lnwire.MilliSatoshi
+
+		// Each shard calls individually into the rfqmath library. This
+		// allows for a total error that scales with the number of
+		// shards. We go up to numHTLCS-1 because we want to add any
+		// leftovers to the last shard.
+		for range numHTLCs - 1 {
+			shardPartialSum := rfqmath.UnitsToMilliSatoshi(
+				shardSizeFP, rate,
+			)
+
+			inboundAmountMSat += shardPartialSum
+		}
+
+		// Make a single pass over the last shard, which may also carry
+		// the remainder of the payment amt.
+		lastShardMsat = rfqmath.UnitsToMilliSatoshi(lastShardFP, rate)
+		inboundAmountMSat += lastShardMsat
 
-		newMarginAssetUnits := rfqmath.NewBigIntFixedPoint(
-			numHTLCs+1, 0,
+		allowedMarginUnits := rfqmath.NewBigIntFixedPoint(
+			numHTLCs, 0,
 		)
-		newAllowedMarginMSat := rfqmath.UnitsToMilliSatoshi(
-			newMarginAssetUnits, rate,
+		marginAmt := rfqmath.UnitsToMilliSatoshi(
+			allowedMarginUnits, rate,
 		)
 
+		marginAmt += lnwire.MilliSatoshi(numHTLCs)
+
 		// The difference should be within the newly allowed margin.
 		require.LessOrEqual(
 			t,
-			invoiceAmtMsat-inboundAmountMSat, newAllowedMarginMSat,
+			invoiceAmtMsat-inboundAmountMSat, marginAmt,
 		)
 	})
 }