diff --git a/docs/release-notes/release-notes-0.7.0.md b/docs/release-notes/release-notes-0.7.0.md index 0ea50fe0e..fa18db7af 100644 --- a/docs/release-notes/release-notes-0.7.0.md +++ b/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 diff --git a/tapchannel/aux_invoice_manager.go b/tapchannel/aux_invoice_manager.go index b0b667be1..285fe2edb 100644 --- a/tapchannel/aux_invoice_manager.go +++ b/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 } diff --git a/tapchannel/aux_traffic_shaper_test.go b/tapchannel/aux_traffic_shaper_test.go index cb79e6ffb..bbabc977d 100644 --- a/tapchannel/aux_traffic_shaper_test.go +++ b/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, ) }) }