tapchannel: enforce strict forwarding for asset invoices

[GeorgeTsagk]

This PR adds a few extra steps to the AuxInvoiceManager before it settles an invoice. Previously we wouldn't check that we received an asset HTLC for an asset invoice, leading to some undesired behavior.

With these changes we enforce that if the invoice corresponds to an asset invoice then we only settle it if we received assets.

Todo:

• Tests

Closes #1008

[coveralls]

Pull Request Test Coverage Report for Build 11614783416

Details

• 46 of 50 (92.0%) changed or added relevant lines in 2 files are covered.
• 25 unchanged lines in 7 files lost coverage.
• Overall coverage increased (+0.3%) to 40.658%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rfq/manager.go	0	4	0.0%

Files with Coverage Reduction	New Missed Lines	%
commitment/tap.go	1	84.17%
rfq/manager.go	2	0.0%
asset/asset.go	2	81.8%
tapdb/universe.go	4	80.91%
tapgarden/caretaker.go	4	68.5%
tapchannel/aux_leaf_signer.go	5	35.92%
tapdb/multiverse.go	7	60.32%

Totals
Change from base Build 11599236597:	0.3%
Covered Lines:	24595
Relevant Lines:	60493

---

💛 - Coveralls

[dstadulis]

Before assigning reviewers, tests will need to be added.

[guggero]

Nice approach! And great to have a unit test. Though I wonder what would actually happen on the sender and receiver side with this change? My assumption is that things would just time out (rather than rejecting the HTLC).
So I think we should also have an integration test for this.

[guggero]

I wonder: Shouldn't an invoice always relate to a buy order only? Even in a direct peer payment, when I create an invoice, there should be a buy quote for it.

[guggero]

Still unaddressed. If there's no good reason why we'd eve want to check a sell order, I think we should change this to only look at buy orders.

[guggero]

Turns out I was wrong here! In case of a direct-peer payment through an asset invoice, it can actually be a sell order SCID. The itest failed here, so will update it.

[GeorgeTsagk]

An itest that covers this behavior has been added in a litd PR

[GeorgeTsagk]

Will enhance the HtlcModify interface to explicitly allow cancelling of HTLCs

[jharveyb]

A failing case (with the PR as-is, I think this seed is invalid if we change any of the generators):

go test -run TestAuxInvoiceManagerProperty -rapid.seed 11951275822997898772

Which fails here:

https://github.com/GeorgeTsagk/taproot-assets/blob/a1e4157e0b0905bbb76595485cee268402428fd2/tapchannel/aux_invoice_manager_test.go#L161

Discovered after running the test for 100_000 iterations. Given how fast each case is, I would recommend running the prop tests for at least a few CPU-minutes. For me that's at least a few million checks. The default is 100, which won't find much given the complexity of these tests / state space for generation.

To run for more iterations:

cd tapchannel
go test -run TestAuxInvoiceManagerProperty -rapid.checks=100_000

[jharveyb]

Solid start with the property tests! Definitely headed in the right direction.

The actual implementation changes look reasonable, left some notes on test improvements.

[guggero]

Looks very close! Will do final review once dependencies are in.

[guggero]

Still unaddressed. If there's no good reason why we'd eve want to check a sell order, I think we should change this to only look at buy orders.

[guggero]

LGTM, just a couple of last nits (and one CI failure).

[jharveyb]

Resetting my reviewer state.

[guggero]

Okay, I rolled back the isBuy/isSell change since that made the itest fail.

I should've just read the Godoc of the method, it was always right there 😅

// priceFromQuote retrieves the price from the accepted quote for the given RFQ
// ID. We allow the quote to either be a buy or a sell quote, since we don't
// know if this is a direct peer payment or a payment that is routed through the
// multiple hops. If it's a direct peer payment, then the quote will be a sell
// quote, since that's what the peer created to find out how many units to send
// for an invoice denominated in BTC.

[jharveyb]

LGTM! Generators look a lot better.

I think we can still simplify them more with other rapid built-ins but this definitely works for now.

[jharveyb]

I think rapid tests are not thread safe? Not sure if that meant between multiple rapid tests though, or just any other test.

[guggero]

Yeah, that's possible. But the t.Parallel() only means other tests in this file can run in parallel to the rapid test. And since we don't have any other rapid tests running there, this should be fine.

[jharveyb]

Tested for 2,000,000 iterations, looks good!

[Roasbeef]

Wanted to provide a bit of drive by review here, re the usage of property based tests.

I think the best way to approach property bsaed tests is from a black-box angle. So you come up with some invariants a given function/method should hold, then generate random inputs, prune that space to be relevant, and assert the output based on input.

A trivial example of this is encoding, we want decode(encode(x)) == x. That just asserts that the functions should do, now how they do it (which internal fields they update, if they use a buffer or not, etc, etc).

Glancing at this PR, I think the property tests are too white box, and too coarse. They use state in between each test to execute assertions, and re-compute a lot of intermediate values to attempt to assert correct behavior. In short, they're written in "normal" white box manner.

If we take a step back and think about what we wanted here, we want an invariant that goes something like:

• Given an incoming HTLC, and an invoice that may correspond to that HTLC, we only accept the HTLC if the payload in the HTLC matches the asset ID expectation in the invoice.

This is the invariant that would prevent a known payment hash from being accepted with a normal HTLC, rather than an asset HTLC.

If we look at the commit that added this logic, we see this section was changed: ad05718#diff-5761d8e06dd213a5ff31aca2abe70153485d5d7832da64b35d371c0303e42bbdR156-R167. isAssetInvoice is what's meant to enforce that invariant. So that's the smallest minimal unit we need to test here. Instead of trying to mock/test the much alrge system of the invoice manager, we could just focus on proving the input needed for that routine, and assert the output.

Then we can go another step up to also test the htlc wire custom records recognition. To do this, we'd need to break up the handleInvoiceAccept function into smaller components. Then we can write property tests on each of the components, along with the relevant composition of each component.

If we wanted to test the bit that handles summing up the set of HTLCs, then rather than re-implement the logic to do a white box test, we can use another invariant. This might be somewhere along the lines of:

• Only once a set of HTLCs that sum up to the invoice amount are extended do we accept the HTLC.

[Roasbeef]

The package also has some support for testing more stateful machines which may be useful in the future, haven't tried it out myself though: https://pkg.go.dev/pgregory.net/rapid#T.Repeat