[custom channels]: enforce minimum amounts

[guggero]

Fixes #1240.

This PR introduces new minimum transportable amounts (asset units for invoice creation/buy quotes and milli-satoshi for sending payments/sell quotes) and then enforces them in the corresponding Taproot Asset Channel RPCs.

Because attempting to create an invoice for a too low amount would result in a payment that cannot be forwarded by the edge node (FeeInsufficient error on forwarding, see Godoc comment on rfqmath.MinTransportableUnits function), creating such invoices is prevented.
On the other side, when attempting to pay a small invoice with assets, the payment is rejected. But because it can succeed, the user has the option to override and send the payment anyway, even if that would be uneconomical.

For reference, here's the Godoc (and code) of rfqmath.MinTransportableUnits that explains the FeeInsufficient problem:

// MinTransportableUnits computes the minimum number of transportable units
// of an asset given its asset rate and the constant HTLC dust limit. This
// function can be used to enforce a minimum invoice amount to prevent
// forwarding failures due to invalid fees.
//
// Given a wallet end user A, an edge node B, an asset rate of 100 milli-
// satoshi per asset unit and a flat 0.1% routing fee (to simplify the
// scenario), the following invoice based receive events can occur:
//  1. Success case: User A creates an invoice over 5,000 units (500,000 milli-
//     satoshis) that is paid by the network. An HTLC over 500,500 milli-satoshis
//     arrives at B. B converts the HTLC to 5,000 units and sends 354,000 milli-
//     satoshis to A.
//     A receives a total "worth" of 854,000 milli-satoshis, which is already
//     more than the invoice amount. But at least the forwarding rule in `lnd`
//     for B is not violated (outgoing amount mSat < incoming amount mSat).
//  2. Failure case: User A creates an invoice over 3,530 units (353,000 milli-
//     satoshis) that is paid by the network. An HTLC over 353,530 milli-satoshis
//     arrives at B. B converts the HTLC to 3,530 units and sends 354,000 milli-
//     satoshis to A.
//     This fails in the `lnd` forwarding logic, because the outgoing amount
//     (354,000 milli-satoshis) is greater than the incoming amount (353,530
//     milli-satoshis).
func MinTransportableUnits(dustLimit lnwire.MilliSatoshi,
	rate BigIntFixedPoint) BigIntFixedPoint {

	// We can only transport an asset unit equivalent amount that's greater
	// than the dust limit for an HTLC, since we'll always want an HTLC that
	// carries an HTLC to be reflected in an on-chain output.
	units := MilliSatoshiToUnits(dustLimit, rate)

	// If the asset's rate is such that a single unit represents more than
	// the dust limit in satoshi, then the above calculation will come out
	// as 0. But we can't transport zero units, so we'll set the minimum to
	// one unit.
	if units.ScaleTo(0).ToUint64() == 0 {
		units = NewBigIntFixedPoint(1, 0)
	}

	return units
}

Just for clarity: This enforcement will no longer be necessary once we address #888 with the symmetric HTLC construction.

[coveralls]

Pull Request Test Coverage Report for Build 12377264530

Details

• 24 of 134 (17.91%) changed or added relevant lines in 4 files are covered.
• 27 unchanged lines in 7 files lost coverage.
• Overall coverage decreased (-0.04%) to 40.677%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rfq/order.go	0	1	0.0%
tapchannel/aux_traffic_shaper.go	0	2	0.0%
rpcserver.go	0	107	0.0%

Files with Coverage Reduction	New Missed Lines	%
tappsbt/create.go	2	53.22%
commitment/tap.go	2	84.7%
asset/mock.go	3	92.18%
tapgarden/caretaker.go	4	68.5%
asset/asset.go	4	81.43%
tapchannel/aux_invoice_manager.go	6	83.25%
tapdb/multiverse.go	6	68.21%

Totals
Change from base Build 12315504138:	-0.04%
Covered Lines:	25840
Relevant Lines:	63525

---

💛 - Coveralls

[guggero]

Taking this out of draft, since litd itests pass in the updated branch (PR to follow shortly).
The litd itests here are expected to fail, since we need some compile time fixes as well as new assertions.

[GeorgeTsagk]

Looks good

Don't we also want to enforce a min-amt when creating an invoice?

Current commits seem to only protect the payer, would be useful to know that my invoice is uneconomical when I create it too

[guggero]

Looks good

Don't we also want to enforce a min-amt when creating an invoice?

Current commits seem to only protect the payer, would be useful to know that my invoice is uneconomical when I create it too

Thanks for the review. Isn't that what 9abedab does?
Or do you mean that the user should be able to override there as well? That won't be possible since the payment will never make it through the network.

[guggero]

litd itest is here: lightninglabs/lightning-terminal#921.

[GeorgeTsagk]

Thanks for the review. Isn't that what 9abedab does?

Oh, probs missed it. 👍

[GeorgeTsagk]

🏌️ ⛳

[ZZiigguurraatt]

Generally, I'm wondering if the root cause of this should be fixed instead. Pretty soon, 354 sat is going to buy a soda or candy bar. How can we not make small payments? Seems like a real limitation. There is discussion of symmetric/bidirectional HTLC in #888 . Why introduce this this additional code complexity when it isn't going to even be needed when #888 is fixed?

[ffranr]

Generally, I'm wondering if the root cause of this should be fixed instead. Pretty soon, 354 sat is going to buy a soda or candy bar. How can we not make small payments? Seems like a real limitation. There is discussion of symmetric/bidirectional HTLC in #888 . Why introduce this this additional code complexity when it isn't going to even be needed when #888 is fixed?

@ZZiigguurraatt From what I understand, I think the idea is to work on both solutions? So that we can get RC3 out and then a long term solution also.

[ffranr]

Looks good so far. I still have a little more to review...

[Roasbeef]

Nice work tracking down this issue!

Thought about it a bit, and one issue I see with this solution is that even once we remove the behavior where we push the 354 sat HTLC across each time, this issue (trying to pay an invoice < 354 sats) still exists. Due to the NgU technology, 354 sats == ~35 cents, meaning that users won't be able to send amounts below that value with this change. Assuming NgU continues, the smallest invoice a user can pay will only grow.

I think an alternative way to resolve this, would be to modify the link.CheckHtlcForward method. IIUC, we're failing here: https://github.com/lightningnetwork/lnd/blob/bb9c680a48cd1075d793cfc97b85f3676b2812c2/htlcswitch/link.go#L3267-L3288. Referring to @guggero's example above, we fail as the incoming amount is less than the outgoing amt, once we swap in the carrier HTLC amount of 354 sats.

This fails as we're still expecting the normal BTC routing fee to be paid on the last hop. If we relax this for HTLCs w/ the proper set of custom records (adding stricter checks/parsing), then we'd enable smaller invoices to be paid. This isn't without a tradeoff though, as naively the last hop will still lose money (eg: 1 sat incoming, but send 354 out, a loss of 353 sats). One way to side step this would be to also pay that last forwarding node a portion of the routing fee in the asset unit. Unfortunately, this doesn't work when the receiver doesn't yet have any asset units to give to the other side.

TL;DR: NgU makes dust actually worth something, which hinders the smallest invoice that can be paid until the dust policy is abolished...

[Roasbeef]

Don't we actually need this value in the sell quote? In that we shouldn't even attempt to send if based on the rate returned by the outgoing edge peer, the transit HTLC amt would be below dust

Thought we needed this instead on the sell side, but once we remove the need to push 354 sats each time, users will be able to pay sat amts smaller than the dust limit since there'll be no HTLC amt swapping at the last hop.

[Roasbeef]

Why do we have to send out 354 sats? Or does this only apply while we push 354 sats each time?

Assuming that's lifted, then we only push an asset value, allowing the routing node to send any amount outgoing, since we don't need to anchor any assets to that outgoing HTLC.

[guggero]

Yeah, this calculation only applies until we add the symmetric HTLC trick. Then we can delete both of these functions (I think).

[guggero]

There is discussion of symmetric/bidirectional HTLC in #888 . Why introduce this this additional code complexity when it isn't going to even be needed when #888 is fixed?

Because without #888 things will fail in unexpected ways that are hard to understand for the user when small amounts are involved. So instead of running into weird timeouts or no_route errors, we disallow payments that we know are going to fail with the current situation and warn the user about situations where their expectations might be invalidated (e.g. sending more sats out than the invoice wanted).

Waiting for #888 isn't really an option for RC3 since that is quite complicated and involved to implement.