Verify blinded keysend payments

[valentinewallace]

If we're receiving a keysend to a blinded path, then we created the payment
secret within. Using our inbound_payment_key, we can decrypt the payment secret
bytes to get the payment's min_cltv_expiry_delta and min amount, to verify the
payment is valid. However, if we're receiving an MPP keysend not to a blinded
path, then we did not create the payment secret and shouldn't verify it since
it's only used to correlate MPP parts.

We also take this opportunity to remove the ChannelManager::inbound_payments map, since it's long deprecated and can't have been added to since 0.0.116.

• Finish tests

[valentinewallace]

I'm seeking conceptual feedback on this PR before finishing the tests.

At a high level, verifying payment_secrets for keysends only allows us to verify that we're not accepting a payment to an expired static invoice, whereas with non-keysend payments we would also be verifying the payment hash and specific amount, as well as correlating MPP parts. payment_secrets we create for keysends also can't be used to correlate MPP parts if senders happen to use a duplicate payment preimage.

So, let me know if this approach seems acceptable or if another one is preferred. Since we're basically only using the payment secret to verify the static invoice isn't expired¹, there may be other options such as putting the expiry in the PaymentContext and using random bytes for the payment secret instead.

Footnotes

1. we also (I think redundantly but need to check) verify that the min amount from the offer is met, and I see something in the inbound_payment module about custom min final CLTVs that may need to be dealt with if we go with an alternate approach. ↩

[TheBlueMatt]

Left some code review, but at a high level I guess there's really two ways to verify this - we can verify at the payment level, like here, or we could verify at the blinded path level, deciding whether to accept or reject the payment by examining if the blinded path we receive the payment over meets requirements we put on the blinded path. ISTM the second may well be more flexible and may even already cover the restrictions we want.

Vaguely related to this PR, we should probably ensure we have whatever is required in the blinded path to make sure a normal BOLT 12 invoice payment blinded path isn't payable as a static invoice payment and vice-versa.

[valentinewallace]

there's really two ways to verify this - we can verify at the payment level, like here, or we could verify at the blinded path level, deciding whether to accept or reject the payment by examining if the blinded path we receive the payment over meets requirements we put on the blinded path. ISTM the second may well be more flexible and may even already cover the restrictions we want.

I spent some time looking into 2nd approach you mention, which basically amounts to verifying the payment using the PaymentContext in the blinded path rather than the PaymentSecret.

ISTM that in our static invoice's blinded payment paths, we'd end up with a PaymentContext variant something like this:

/// The context of a payment made for a static invoice requested from a BOLT 12 [`Offer`].
pub struct AsyncBolt12OfferContext {
	/// The [`Nonce`] used to verify that an inbound [`InvoiceRequest`] corresponds to this static
	/// invoice's offer.
	pub offer_nonce: Nonce,
	/// Used instead of the `PaymentSecret` to verify that the offer's min amount is met.
	pub offer_min_amt_msat: Option<u64>,
	/// Used instead of the `PaymentSecret` to verify that the invoice's expiry is met.
	pub invoice_absolute_expiry: core::time::Duration,
	/// Fields to verify that this blinded path was created by us below. 
	pub nonce: Nonce,
	pub hmac: Hmac<Sha256>,
}

This context would allow us to check that the invoice's min amount and expiry are satisfied in onion_payment::create_recv_pending_htlc_info, whereas we'd usually do these checks in inbound_payment using the PaymentSecret.

The main issue I ran into with this approach was that we wouldn't have a natural payment secret to put in ReceiveTlvs and PaymentPurpose::Bolt12OfferPayment, and it'd be pretty annoying to make those fields Optional/break out a new enum variant. @TheBlueMatt suggested that we could instead put an hmac in the PaymentSecret though, which could replace the AsyncBolt12OfferContext::hmac above.

I don't really have a strong feeling between the two approaches. It kind of feels like we have nice infrastructure built for creating/verifying PaymentSecrets and invoice fields that we're not taking advantage of, but at the same time it doesn't seem like too much extra code to do it this way. Let me know your thoughts!

[TheBlueMatt]

I'm definitely okay using the PaymentSecret logic to check the various constraints here, you're right that we already have all the machinery for it so its nice to reuse. However, independent of that, we need to have a way to differentiate between static-invoice-blinded-paths and regular-invoice-blinded-paths, and reject ones that are for the wrong type, IMO. Its not really a critical, attack, I think, but just in general we should have a way to ensure the blinded paths we hand out in one context isn't swapped for a different use. Oh nvm I guess this is already enforced via the secret in the ReceiveTlvs being encrypted back to us?

[valentinewallace]

I guess this is already enforced via the secret in the ReceiveTlvs being encrypted back to us?

Yeah, attempting to pay a non-static invoice with keysend should fail payment secret verification. Non-keysend to static invoice blinded path doesn't really make sense because we just won't know the payment preimage, IIUC.

[codecov]

Codecov Report

Attention: Patch coverage is 65.00000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 89.64%. Comparing base (2c1e828) to head (9cc6969).
Report is 403 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/functional_test_utils.rs	16.66%	9 Missing and 1 partial ⚠️
lightning/src/ln/inbound_payment.rs	23.07%	10 Missing ⚠️
lightning/src/ln/channelmanager.rs	96.42%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3383      +/-   ##
==========================================
+ Coverage   89.57%   89.64%   +0.06%     
==========================================
  Files         125      128       +3     
  Lines      101756   104810    +3054     
  Branches   101756   104810    +3054     
==========================================
+ Hits        91151    93958    +2807     
- Misses       7884     8145     +261     
+ Partials     2721     2707      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jkczyz]

LGTM modulo some minor comments. Feel free to squash, IMO.

[valentinewallace]

Squashed with the following diff from Jeff's feedback:

diff --git a/lightning/src/ln/blinded_payment_tests.rs b/lightning/src/ln/blinded_payment_tests.rs
index c0913d51d..dcfd71a86 100644
--- a/lightning/src/ln/blinded_payment_tests.rs
+++ b/lightning/src/ln/blinded_payment_tests.rs
@@ -1345,7 +1345,6 @@ fn invalid_keysend_payment_secret() {
        let args = PassAlongPathArgs::new(
                &nodes[0], &expected_route[0], amt_msat, payment_hash, ev.clone()
        )
-               .without_claimable_event()
                .with_payment_secret(invalid_payment_secret)
                .with_payment_preimage(keysend_preimage)
                .expect_failure(HTLCDestination::FailedPayment { payment_hash });
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index bb951d3c2..29920f191 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -5721,8 +5721,8 @@ where
                                                                        },
                                                                        PendingHTLCRouting::ReceiveKeysend {
                                                                                payment_data, payment_preimage, payment_metadata,
-                                                                               incoming_cltv_expiry, custom_tlvs, has_recipient_created_payment_secret,
-                                                                               requires_blinded_error: _,
+                                                                               incoming_cltv_expiry, custom_tlvs, requires_blinded_error: _,
+                                                                               has_recipient_created_payment_secret,
                                                                        } => {
                                                                                let onion_fields = RecipientOnionFields {
                                                                                        payment_secret: payment_data.as_ref().map(|data| data.payment_secret),
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index 8a5d8c20b..359e58688 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -2631,6 +2631,7 @@ impl<'a, 'b, 'c, 'd> PassAlongPathArgs<'a, 'b, 'c, 'd> {
                self
        }
        pub fn expect_failure(mut self, failure: HTLCDestination) -> Self {
+               self.payment_claimable_expected = false;
                self.expected_failure = Some(failure);
                self
        }

[vincenzopalazzo]

Suggested change

	} else { debug_assert!(false); None }
	} else { debug_assert!(false); None }

why we do this dabug assert here? and not just panic always or emit a log if we are not panicking in production? I feel this can be hard to track down if this happen in some wallet in production, is it?

[valentinewallace]

We tend to add debug_asserts for cases that should be unreachable, if that makes sense. So we should never hit this.

[vincenzopalazzo]

Suggested change

	// LDK versions prior to 0.0.104 wrote `pending_inbound_payments` here, with deprecated support
	// LDK versions prior to 0.0.104 wrote `pending_inbound_payments` here, with deprecated support

if the version number right? probably is 0.0.115 but I can miss understanding what you want to say

[valentinewallace]

Hmm, why do you think it's 115?

[valentinewallace]

Squashed in Matt's feedback with the following diff:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 29920f191..60b6b6f29 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -5913,7 +5913,7 @@ where
                                                                                        }
                                                                                }
                                                                                payment_preimage
-                                                                       } else { debug_assert!(false); None }
+                                                                       } else { fail_htlc!(claimable_htlc, payment_hash); }
                                                                } else { None };
                                                                match claimable_htlc.onion_payload {
                                                                        OnionPayload::Invoice { .. } => {