Async payments message encoding and prefactor

[valentinewallace]

Support encoding async payments messages HeldHtlcAvailable and ReleaseHeldHtlc and add the necessary traits and some fields for handling these messages. The actual flow of the protocol is not yet supported.

Helps address #2298.

[valentinewallace]

Note: this commit is no longer required but it still seemed like a nice fix.

[valentinewallace]

Let me know if we want to hold off on this or parts of it if we want more of the follow-on work up for review first!

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 44.00000% with 42 lines in your changes missing coverage. Please review.

Project coverage is 90.33%. Comparing base (07f3380) to head (1a3dfdb).
Report is 4 commits behind head on main.

Files	Patch %	Lines
lightning/src/onion_message/async_payments.rs	0.00%	41 Missing ⚠️
lightning/src/ln/channelmanager.rs	87.50%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3125      +/-   ##
==========================================
+ Coverage   89.92%   90.33%   +0.41%     
==========================================
  Files         121      121              
  Lines       99172   101244    +2072     
  Branches    99172   101244    +2072     
==========================================
+ Hits        89180    91461    +2281     
+ Misses       7391     7222     -169     
+ Partials     2601     2561      -40     

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

[valentinewallace]

Because we probably don't want async payments variants in public enums OffersMessage and ParsedOnionMessageContents yet, I added a commit cfg-gating the module and the new variants.

[TheBlueMatt]

LGTM, I think.

[TheBlueMatt]

We normally break the messages out into separate methods rather than passing an enum. Not that it matters much but this is somewhat inconsistent with the remainder of our API.

[valentinewallace]

This is consistent with OffersMessageHandler. I don't feel strongly whether we break this into separate methods, but we may want to do it for both traits if we decide to.

[jkczyz]

On a related note, for message authentication it may make sense to move towards separate methods if each would need different data to authenticate (e.g, handling an invoice request may need a nonce from an offer's blinded path whereas as handling an invoice may need a payment id from the invoice request's reply path). See #3085.

But we may still want an enum for the return type if a method could return different types (e.g., Botl12Invoice or InvoiceError). There may need to be different enums depending on how strict we want to make the compiler checks. Currently, we allow any OffersMessage variant to be returned even though some aren't really allowed.

[valentinewallace]

Updated to have per-message methods in the trait. Also added default implementations which reduced the diff a bit.

[jkczyz]

LGTM module one small fix for CI. Feel free to squash.

[valentinewallace]

I squashed in re-adding the default implementations and docs updates, hopefully that's okay.

[valentinewallace]

Oops, just noticed a CI issue. Will push momentarily.

[valentinewallace]

Diff since @TheBlueMatt's last ack is docs tweaks and removing default method implementations, so going to land this and happy to address anything else in follow-up.

[valentinewallace]

Diff since Matt's ack for reference:

diff --git a/fuzz/src/onion_message.rs b/fuzz/src/onion_message.rs
index ccd17f42f..ba76815af 100644
--- a/fuzz/src/onion_message.rs
+++ b/fuzz/src/onion_message.rs
@@ -12,7 +12,9 @@ use lightning::ln::msgs::{self, DecodeError, OnionMessageHandler};
 use lightning::ln::script::ShutdownScript;
 use lightning::offers::invoice::UnsignedBolt12Invoice;
 use lightning::offers::invoice_request::UnsignedInvoiceRequest;
-use lightning::onion_message::async_payments::{AsyncPaymentsMessage, AsyncPaymentsMessageHandler};
+use lightning::onion_message::async_payments::{
+	AsyncPaymentsMessage, AsyncPaymentsMessageHandler, HeldHtlcAvailable, ReleaseHeldHtlc,
+};
 use lightning::onion_message::messenger::{
 	CustomOnionMessageHandler, Destination, MessageRouter, OnionMessagePath, OnionMessenger,
 	PendingOnionMessage, Responder, ResponseInstruction,
@@ -110,7 +112,14 @@ impl OffersMessageHandler for TestOffersMessageHandler {
 
 struct TestAsyncPaymentsMessageHandler {}
 
-impl AsyncPaymentsMessageHandler for TestAsyncPaymentsMessageHandler {}
+impl AsyncPaymentsMessageHandler for TestAsyncPaymentsMessageHandler {
+	fn held_htlc_available(
+		&self, _message: HeldHtlcAvailable, _responder: Option<Responder>,
+	) -> ResponseInstruction<ReleaseHeldHtlc> {
+		ResponseInstruction::NoResponse
+	}
+	fn release_held_htlc(&self, _message: ReleaseHeldHtlc) {}
+}
 
 #[derive(Debug)]
 struct TestCustomMessage {}
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index a11748bc0..fe3320a13 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -66,7 +66,6 @@ use crate::offers::invoice_request::{DerivedPayerId, InvoiceRequestBuilder};
 use crate::offers::offer::{Offer, OfferBuilder};
 use crate::offers::parse::Bolt12SemanticError;
 use crate::offers::refund::{Refund, RefundBuilder};
-use crate::onion_message::async_payments::AsyncPaymentsMessage;
 use crate::onion_message::messenger::{new_pending_onion_message, Destination, MessageRouter, PendingOnionMessage, Responder, ResponseInstruction};
 use crate::onion_message::offers::{OffersMessage, OffersMessageHandler};
 use crate::sign::{EntropySource, NodeSigner, Recipient, SignerProvider};
@@ -1848,8 +1847,6 @@ where
 //
 // `pending_offers_messages`
 //
-// `pending_async_payments_messages`
-//
 // `total_consistency_lock`
 //  |
 //  |__`forward_htlcs`
@@ -2102,8 +2099,6 @@ where
 	needs_persist_flag: AtomicBool,
 
 	pending_offers_messages: Mutex<Vec<PendingOnionMessage<OffersMessage>>>,
-	#[allow(unused)]
-	pending_async_payments_messages: Mutex<Vec<PendingOnionMessage<AsyncPaymentsMessage>>>,
 
 	/// Tracks the message events that are to be broadcasted when we are connected to some peer.
 	pending_broadcast_messages: Mutex<Vec<MessageSendEvent>>,
@@ -2898,7 +2893,6 @@ where
 			funding_batch_states: Mutex::new(BTreeMap::new()),
 
 			pending_offers_messages: Mutex::new(Vec::new()),
-			pending_async_payments_messages: Mutex::new(Vec::new()),
 			pending_broadcast_messages: Mutex::new(Vec::new()),
 
 			last_days_feerates: Mutex::new(VecDeque::new()),
@@ -12087,7 +12081,6 @@ where
 			funding_batch_states: Mutex::new(BTreeMap::new()),
 
 			pending_offers_messages: Mutex::new(Vec::new()),
-			pending_async_payments_messages: Mutex::new(Vec::new()),
 
 			pending_broadcast_messages: Mutex::new(Vec::new()),
 
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 4dfcd0e77..9a026d709 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -28,7 +28,7 @@ use crate::util::ser::{VecWriter, Writeable, Writer};
 use crate::ln::peer_channel_encryptor::{PeerChannelEncryptor, NextNoiseStep, MessageBuf, MSG_BUF_ALLOC_SIZE};
 use crate::ln::wire;
 use crate::ln::wire::{Encode, Type};
-use crate::onion_message::async_payments::AsyncPaymentsMessageHandler;
+use crate::onion_message::async_payments::{AsyncPaymentsMessageHandler, HeldHtlcAvailable, ReleaseHeldHtlc};
 use crate::onion_message::messenger::{CustomOnionMessageHandler, PendingOnionMessage, Responder, ResponseInstruction};
 use crate::onion_message::offers::{OffersMessage, OffersMessageHandler};
 use crate::onion_message::packet::OnionMessageContents;
@@ -149,7 +149,14 @@ impl OffersMessageHandler for IgnoringMessageHandler {
 		ResponseInstruction::NoResponse
 	}
 }
-impl AsyncPaymentsMessageHandler for IgnoringMessageHandler {}
+impl AsyncPaymentsMessageHandler for IgnoringMessageHandler {
+	fn held_htlc_available(
+		&self, _message: HeldHtlcAvailable, _responder: Option<Responder>,
+	) -> ResponseInstruction<ReleaseHeldHtlc> {
+		ResponseInstruction::NoResponse
+	}
+	fn release_held_htlc(&self, _message: ReleaseHeldHtlc) {}
+}
 impl CustomOnionMessageHandler for IgnoringMessageHandler {
 	type CustomMessage = Infallible;
 	fn handle_custom_message(&self, _message: Self::CustomMessage, _responder: Option<Responder>) -> ResponseInstruction<Self::CustomMessage> {
diff --git a/lightning/src/onion_message/async_payments.rs b/lightning/src/onion_message/async_payments.rs
index c7688ec2a..f5953c636 100644
--- a/lightning/src/onion_message/async_payments.rs
+++ b/lightning/src/onion_message/async_payments.rs
@@ -25,21 +25,17 @@ const RELEASE_HELD_HTLC_TLV_TYPE: u64 = 74;
 ///
 /// [`OnionMessage`]: crate::ln::msgs::OnionMessage
 pub trait AsyncPaymentsMessageHandler {
-	/// Handles a [`HeldHtlcAvailable`] message. The [`ReleaseHeldHtlc`] response, if any, is enqueued
-	/// to be sent by [`OnionMessenger`].
-	///
-	/// [`OnionMessenger`]: crate::onion_message::messenger::OnionMessenger
+	/// Handle a [`HeldHtlcAvailable`] message. A [`ReleaseHeldHtlc`] should be returned to release
+	/// the held funds.
 	fn held_htlc_available(
-		&self, _message: HeldHtlcAvailable, _responder: Option<Responder>,
-	) -> ResponseInstruction<ReleaseHeldHtlc> {
-		ResponseInstruction::NoResponse
-	}
+		&self, message: HeldHtlcAvailable, responder: Option<Responder>,
+	) -> ResponseInstruction<ReleaseHeldHtlc>;
 
-	/// Handles a [`ReleaseHeldHtlc`] message. If authentication of the message succeeds, an HTLC will
-	/// be released to the corresponding payee.
-	fn release_held_htlc(&self, _message: ReleaseHeldHtlc) {}
+	/// Handle a [`ReleaseHeldHtlc`] message. If authentication of the message succeeds, an HTLC
+	/// should be released to the corresponding payee.
+	fn release_held_htlc(&self, message: ReleaseHeldHtlc);
 
-	/// Releases any [`AsyncPaymentsMessage`]s that need to be sent.
+	/// Release any [`AsyncPaymentsMessage`]s that need to be sent.
 	///
 	/// Typically, this is used for messages initiating an async payment flow rather than in response
 	/// to another message.
@@ -48,7 +44,7 @@ pub trait AsyncPaymentsMessageHandler {
 		vec![]
 	}
 
-	/// Releases any [`AsyncPaymentsMessage`]s that need to be sent.
+	/// Release any [`AsyncPaymentsMessage`]s that need to be sent.
 	///
 	/// Typically, this is used for messages initiating a payment flow rather than in response to
 	/// another message.
@@ -89,7 +85,8 @@ pub struct HeldHtlcAvailable {
 /// Releases the HTLC corresponding to an inbound [`HeldHtlcAvailable`] message.
 #[derive(Clone, Debug)]
 pub struct ReleaseHeldHtlc {
-	/// Used to release the HTLC held upstream.
+	/// Used to release the HTLC held upstream if it matches the corresponding
+	/// [`HeldHtlcAvailable::payment_release_secret`].
 	pub payment_release_secret: [u8; 32],
 }
 
diff --git a/lightning/src/onion_message/functional_tests.rs b/lightning/src/onion_message/functional_tests.rs
index 07a1d1773..9a2dc36f8 100644
--- a/lightning/src/onion_message/functional_tests.rs
+++ b/lightning/src/onion_message/functional_tests.rs
@@ -19,7 +19,7 @@ use crate::routing::test_utils::{add_channel, add_or_update_node};
 use crate::sign::{NodeSigner, Recipient};
 use crate::util::ser::{FixedLengthReader, LengthReadable, Writeable, Writer};
 use crate::util::test_utils;
-use super::async_payments::AsyncPaymentsMessageHandler;
+use super::async_payments::{AsyncPaymentsMessageHandler, HeldHtlcAvailable, ReleaseHeldHtlc};
 use super::messenger::{CustomOnionMessageHandler, DefaultMessageRouter, Destination, OnionMessagePath, OnionMessenger, PendingOnionMessage, Responder, ResponseInstruction, SendError, SendSuccess};
 use super::offers::{OffersMessage, OffersMessageHandler};
 use super::packet::{OnionMessageContents, Packet};
@@ -83,7 +83,14 @@ impl OffersMessageHandler for TestOffersMessageHandler {
 
 struct TestAsyncPaymentsMessageHandler {}
 
-impl AsyncPaymentsMessageHandler for TestAsyncPaymentsMessageHandler {}
+impl AsyncPaymentsMessageHandler for TestAsyncPaymentsMessageHandler {
+	fn held_htlc_available(
+		&self, _message: HeldHtlcAvailable, _responder: Option<Responder>,
+	) -> ResponseInstruction<ReleaseHeldHtlc> {
+		ResponseInstruction::NoResponse
+	}
+	fn release_held_htlc(&self, _message: ReleaseHeldHtlc) {}
+}
 
 #[derive(Clone, Debug, PartialEq)]
 enum TestCustomMessage {

[TheBlueMatt]

Fuzzers should always do more, not less :)

[valentinewallace]

Addressed this and below in #3145.

[TheBlueMatt]

Since we're going ahead and exposing the message handler trait publicly, lets go ahead and impl it for ChannelManager and default to using that everywhere. That way the type signatures don't have to change again when we implement the trait for ChannelManager with a real impl.