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.

• [x] 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[^1], there may be other options such as putting the expiry in the PaymentContext and using random bytes for the payment secret instead.

[^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.

[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[bot]]

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 :warning:
lightning/src/ln/inbound_payment.rs	23.07%	10 Missing :warning:
lightning/src/ln/channelmanager.rs	96.42%	1 Missing :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[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
        }

[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 { .. } => {