Skip to content

Asynchronous Bolt12Invoice payment #3078

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jun 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions fuzz/src/full_stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,7 @@ mod tests {
// our network key
ext_from_hex("0100000000000000000000000000000000000000000000000000000000000000", &mut test);
// config
ext_from_hex("0000000000900000000000000000640001000000000001ffff0000000000000000ffffffffffffffffffffffffffffffff0000000000000000ffffffffffffffff000000ffffffff00ffff1a000400010000020400000000040200000a08ffffffffffffffff0001000000", &mut test);
ext_from_hex("0000000000900000000000000000640001000000000001ffff0000000000000000ffffffffffffffffffffffffffffffff0000000000000000ffffffffffffffff000000ffffffff00ffff1a000400010000020400000000040200000a08ffffffffffffffff000100000000", &mut test);

// new outbound connection with id 0
ext_from_hex("00", &mut test);
Expand Down Expand Up @@ -1383,7 +1383,7 @@ mod tests {
// our network key
ext_from_hex("0100000000000000000000000000000000000000000000000000000000000000", &mut test);
// config
ext_from_hex("0000000000900000000000000000640001000000000001ffff0000000000000000ffffffffffffffffffffffffffffffff0000000000000000ffffffffffffffff000000ffffffff00ffff1a000400010000020400000000040200000a08ffffffffffffffff0001000000", &mut test);
ext_from_hex("0000000000900000000000000000640001000000000001ffff0000000000000000ffffffffffffffffffffffffffffffff0000000000000000ffffffffffffffff000000ffffffff00ffff1a000400010000020400000000040200000a08ffffffffffffffff000100000000", &mut test);

// new outbound connection with id 0
ext_from_hex("00", &mut test);
Expand Down
58 changes: 54 additions & 4 deletions lightning/src/events/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,20 @@ pub mod bump_transaction;
pub use bump_transaction::BumpTransactionEvent;

use crate::blinded_path::payment::{Bolt12OfferContext, Bolt12RefundContext, PaymentContext, PaymentContextRef};
use crate::sign::SpendableOutputDescriptor;
use crate::chain::transaction;
use crate::ln::channelmanager::{InterceptId, PaymentId, RecipientOnionFields};
use crate::ln::channel::FUNDING_CONF_DEADLINE_BLOCKS;
use crate::ln::features::ChannelTypeFeatures;
use crate::ln::msgs;
use crate::ln::types::{ChannelId, PaymentPreimage, PaymentHash, PaymentSecret};
use crate::chain::transaction;
use crate::offers::invoice::Bolt12Invoice;
use crate::onion_message::messenger::Responder;
use crate::routing::gossip::NetworkUpdate;
use crate::routing::router::{BlindedTail, Path, RouteHop, RouteParameters};
use crate::sign::SpendableOutputDescriptor;
use crate::util::errors::APIError;
use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, UpgradableRequired, WithoutLength};
use crate::util::string::UntrustedString;
use crate::routing::router::{BlindedTail, Path, RouteHop, RouteParameters};

use bitcoin::{Transaction, OutPoint};
use bitcoin::blockdata::locktime::absolute::LockTime;
Expand Down Expand Up @@ -715,6 +717,31 @@ pub enum Event {
/// The `payment_id` to have been associated with payment for the requested invoice.
payment_id: PaymentId,
},
/// Indicates a [`Bolt12Invoice`] in response to an [`InvoiceRequest`] or a [`Refund`] was
/// received.
///
/// This event will only be generated if [`UserConfig::manually_handle_bolt12_invoices`] is set.
/// Use [`ChannelManager::send_payment_for_bolt12_invoice`] to pay the invoice or
/// [`ChannelManager::abandon_payment`] to abandon the associated payment. See those docs for
/// further details.
///
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention how long the user has to pay the invoice before it'll time out and they'll have to start again (and is that gonna be an issue for fedi)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, don't we expire the payment based on StaleExpiration too? As if we'd never received the invoice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. I forgot that that's checked at time of payment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up reverting those changes and re-writing the send_payment_for_bolt12_invoice accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you forget to push this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be there: f2721af

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I interpreted "re-writing the send_payment_for_bolt12_invoice accordingly" comment as you changed it so we don't time out, rather than rewriting the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm...send_payment_for_bolt12_invoice docs were changed to clarify when a Bolt12PaymentError::UnexpectedInvoice can occur, including the timeout scenarios. StaleExpiration is not explicitly mentioned because it is pub(crate). I did remove the invoice-level expiration part since that's really a detail surfaced by Event::PaymentFailed, which is linked in the send_payment_for_bolt12_invoice docs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I just read the comment without looking at the code, I had expected a behavior change based on the comment, rather than doc changes. But doc changes are fine too :)

/// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest
/// [`Refund`]: crate::offers::refund::Refund
/// [`UserConfig::manually_handle_bolt12_invoices`]: crate::util::config::UserConfig::manually_handle_bolt12_invoices
/// [`ChannelManager::send_payment_for_bolt12_invoice`]: crate::ln::channelmanager::ChannelManager::send_payment_for_bolt12_invoice
/// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment
InvoiceReceived {
/// The `payment_id` associated with payment for the invoice.
payment_id: PaymentId,
/// The invoice to pay.
invoice: Bolt12Invoice,
/// A responder for replying with an [`InvoiceError`] if needed.
///
/// `None` if the invoice wasn't sent with a reply path.
///
/// [`InvoiceError`]: crate::offers::invoice_error::InvoiceError
responder: Option<Responder>,
},
/// Indicates an outbound payment we made succeeded (i.e. it made it all the way to its target
/// and we got back the payment preimage for it).
///
Expand Down Expand Up @@ -1471,7 +1498,15 @@ impl Writeable for Event {
write_tlv_fields!(writer, {
(0, peer_node_id, required),
});
}
},
&Event::InvoiceReceived { ref payment_id, ref invoice, ref responder } => {
41u8.write(writer)?;
write_tlv_fields!(writer, {
(0, payment_id, required),
(2, invoice, required),
(4, responder, option),
})
},
// Note that, going forward, all new events must only write data inside of
// `write_tlv_fields`. Versions 0.0.101+ will ignore odd-numbered events that write
// data via `write_tlv_fields`.
Expand Down Expand Up @@ -1908,6 +1943,21 @@ impl MaybeReadable for Event {
};
f()
},
41u8 => {
let mut f = || {
_init_and_read_len_prefixed_tlv_fields!(reader, {
(0, payment_id, required),
(2, invoice, required),
(4, responder, option),
});
Ok(Some(Event::InvoiceReceived {
payment_id: payment_id.0.unwrap(),
invoice: invoice.0.unwrap(),
responder,
}))
};
f()
},
// Versions prior to 0.0.100 did not ignore odd types, instead returning InvalidValue.
// Version 0.0.100 failed to properly ignore odd types, possibly resulting in corrupt
// reads.
Expand Down
76 changes: 54 additions & 22 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ use crate::ln::onion_utils::{HTLCFailReason, INVALID_ONION_BLINDING};
use crate::ln::msgs::{ChannelMessageHandler, DecodeError, LightningError};
#[cfg(test)]
use crate::ln::outbound_payment;
use crate::ln::outbound_payment::{Bolt12PaymentError, OutboundPayments, PaymentAttempts, PendingOutboundPayment, SendAlongPathArgs, StaleExpiration};
use crate::ln::outbound_payment::{OutboundPayments, PaymentAttempts, PendingOutboundPayment, SendAlongPathArgs, StaleExpiration};
use crate::ln::wire::Encode;
use crate::offers::invoice::{BlindedPayInfo, Bolt12Invoice, DEFAULT_RELATIVE_EXPIRY, DerivedSigningPubkey, ExplicitSigningPubkey, InvoiceBuilder, UnsignedBolt12Invoice};
use crate::offers::invoice_error::InvoiceError;
Expand Down Expand Up @@ -105,7 +105,7 @@ use core::time::Duration;
use core::ops::Deref;

// Re-export this for use in the public API.
pub use crate::ln::outbound_payment::{PaymentSendFailure, ProbeSendFailure, Retry, RetryableSendFailure, RecipientOnionFields};
pub use crate::ln::outbound_payment::{Bolt12PaymentError, PaymentSendFailure, ProbeSendFailure, Retry, RetryableSendFailure, RecipientOnionFields};
use crate::ln::script::ShutdownScript;

// We hold various information about HTLC relay in the HTLC objects in Channel itself:
Expand Down Expand Up @@ -3996,7 +3996,36 @@ where
self.pending_outbound_payments.test_set_payment_metadata(payment_id, new_payment_metadata);
}

pub(super) fn send_payment_for_bolt12_invoice(&self, invoice: &Bolt12Invoice, payment_id: PaymentId) -> Result<(), Bolt12PaymentError> {
/// Pays the [`Bolt12Invoice`] associated with the `payment_id` encoded in its `payer_metadata`.
///
/// The invoice's `payer_metadata` is used to authenticate that the invoice was indeed requested
/// before attempting a payment. [`Bolt12PaymentError::UnexpectedInvoice`] is returned if this
/// fails or if the encoded `payment_id` is not recognized. The latter may happen once the
/// payment is no longer tracked because the payment was attempted after:
/// - an invoice for the `payment_id` was already paid,
/// - one full [timer tick] has elapsed since initially requesting the invoice when paying an
/// offer, or
/// - the refund corresponding to the invoice has already expired.
///
/// To retry the payment, request another invoice using a new `payment_id`.
///
/// Attempting to pay the same invoice twice while the first payment is still pending will
/// result in a [`Bolt12PaymentError::DuplicateInvoice`].
///
/// Otherwise, either [`Event::PaymentSent`] or [`Event::PaymentFailed`] are used to indicate
/// whether or not the payment was successful.
///
/// [timer tick]: Self::timer_tick_occurred
pub fn send_payment_for_bolt12_invoice(&self, invoice: &Bolt12Invoice) -> Result<(), Bolt12PaymentError> {
let secp_ctx = &self.secp_ctx;
let expanded_key = &self.inbound_payment_key;
match invoice.verify(expanded_key, secp_ctx) {
Ok(payment_id) => self.send_payment_for_verified_bolt12_invoice(invoice, payment_id),
Err(()) => Err(Bolt12PaymentError::UnexpectedInvoice),
}
}

fn send_payment_for_verified_bolt12_invoice(&self, invoice: &Bolt12Invoice, payment_id: PaymentId) -> Result<(), Bolt12PaymentError> {
let best_block_height = self.best_block.read().unwrap().height;
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
self.pending_outbound_payments
Expand Down Expand Up @@ -10272,42 +10301,45 @@ where
};

match response {
Ok(invoice) => return responder.respond(OffersMessage::Invoice(invoice)),
Err(error) => return responder.respond(OffersMessage::InvoiceError(error.into())),
Ok(invoice) => responder.respond(OffersMessage::Invoice(invoice)),
Err(error) => responder.respond(OffersMessage::InvoiceError(error.into())),
}
},
OffersMessage::Invoice(invoice) => {
let response = invoice
.verify(expanded_key, secp_ctx)
.map_err(|()| InvoiceError::from_string("Unrecognized invoice".to_owned()))
.and_then(|payment_id| {
let result = match invoice.verify(expanded_key, secp_ctx) {
Ok(payment_id) => {
let features = self.bolt12_invoice_features();
if invoice.invoice_features().requires_unknown_bits_from(&features) {
Err(InvoiceError::from(Bolt12SemanticError::UnknownRequiredFeatures))
} else if self.default_configuration.manually_handle_bolt12_invoices {
let event = Event::InvoiceReceived { payment_id, invoice, responder };
self.pending_events.lock().unwrap().push_back((event, None));
return ResponseInstruction::NoResponse;
} else {
self.send_payment_for_bolt12_invoice(&invoice, payment_id)
self.send_payment_for_verified_bolt12_invoice(&invoice, payment_id)
.map_err(|e| {
log_trace!(self.logger, "Failed paying invoice: {:?}", e);
InvoiceError::from_string(format!("{:?}", e))
})
}
});
},
Err(()) => Err(InvoiceError::from_string("Unrecognized invoice".to_owned())),
};

match (responder, response) {
(Some(responder), Err(e)) => responder.respond(OffersMessage::InvoiceError(e)),
(None, Err(_)) => {
log_trace!(
self.logger,
"A response was generated, but there is no reply_path specified for sending the response."
);
return ResponseInstruction::NoResponse;
}
_ => return ResponseInstruction::NoResponse,
match result {
Ok(()) => ResponseInstruction::NoResponse,
Err(e) => match responder {
Some(responder) => responder.respond(OffersMessage::InvoiceError(e)),
None => {
log_trace!(self.logger, "No reply path for sending invoice error: {:?}", e);
ResponseInstruction::NoResponse
},
},
}
},
OffersMessage::InvoiceError(invoice_error) => {
log_trace!(self.logger, "Received invoice_error: {}", invoice_error);
return ResponseInstruction::NoResponse;
ResponseInstruction::NoResponse
},
}
}
Expand Down
87 changes: 86 additions & 1 deletion lightning/src/ln/offers_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,10 @@ use core::time::Duration;
use crate::blinded_path::{BlindedPath, IntroductionNode};
use crate::blinded_path::payment::{Bolt12OfferContext, Bolt12RefundContext, PaymentContext};
use crate::events::{Event, MessageSendEventsProvider, PaymentPurpose};
use crate::ln::channelmanager::{MAX_SHORT_LIVED_RELATIVE_EXPIRY, PaymentId, RecentPaymentDetails, Retry, self};
use crate::ln::channelmanager::{Bolt12PaymentError, MAX_SHORT_LIVED_RELATIVE_EXPIRY, PaymentId, RecentPaymentDetails, Retry, self};
use crate::ln::functional_test_utils::*;
use crate::ln::msgs::{ChannelMessageHandler, Init, NodeAnnouncement, OnionMessage, OnionMessageHandler, RoutingMessageHandler, SocketAddress, UnsignedGossipMessage, UnsignedNodeAnnouncement};
use crate::ln::outbound_payment::IDEMPOTENCY_TIMEOUT_TICKS;
use crate::offers::invoice::Bolt12Invoice;
use crate::offers::invoice_error::InvoiceError;
use crate::offers::invoice_request::{InvoiceRequest, InvoiceRequestFields};
Expand Down Expand Up @@ -865,6 +866,90 @@ fn pays_for_refund_without_blinded_paths() {
expect_recent_payment!(bob, RecentPaymentDetails::Fulfilled, payment_id);
}

/// Checks that a deferred invoice can be paid asynchronously from an Event::InvoiceReceived.
#[test]
fn pays_bolt12_invoice_asynchronously() {
let mut manually_pay_cfg = test_default_channel_config();
manually_pay_cfg.manually_handle_bolt12_invoices = true;

let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(manually_pay_cfg)]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 1_000_000_000);

let alice = &nodes[0];
let alice_id = alice.node.get_our_node_id();
let bob = &nodes[1];
let bob_id = bob.node.get_our_node_id();

let offer = alice.node
.create_offer_builder(None).unwrap()
.amount_msats(10_000_000)
.build().unwrap();

let payment_id = PaymentId([1; 32]);
bob.node.pay_for_offer(&offer, None, None, None, payment_id, Retry::Attempts(0), None).unwrap();
expect_recent_payment!(bob, RecentPaymentDetails::AwaitingInvoice, payment_id);

let onion_message = bob.onion_messenger.next_onion_message_for_peer(alice_id).unwrap();
alice.onion_messenger.handle_onion_message(&bob_id, &onion_message);

let (invoice_request, _) = extract_invoice_request(alice, &onion_message);
let payment_context = PaymentContext::Bolt12Offer(Bolt12OfferContext {
offer_id: offer.id(),
invoice_request: InvoiceRequestFields {
payer_id: invoice_request.payer_id(),
quantity: None,
payer_note_truncated: None,
},
});

let onion_message = alice.onion_messenger.next_onion_message_for_peer(bob_id).unwrap();
bob.onion_messenger.handle_onion_message(&alice_id, &onion_message);

let invoice = match get_event!(bob, Event::InvoiceReceived) {
Event::InvoiceReceived { payment_id: actual_payment_id, invoice, .. } => {
assert_eq!(actual_payment_id, payment_id);
invoice
},
_ => panic!("No Event::InvoiceReceived"),
};
assert_eq!(invoice.amount_msats(), 10_000_000);
assert_ne!(invoice.signing_pubkey(), alice_id);
assert!(!invoice.payment_paths().is_empty());
for (_, path) in invoice.payment_paths() {
assert_eq!(path.introduction_node, IntroductionNode::NodeId(alice_id));
}

assert!(bob.node.send_payment_for_bolt12_invoice(&invoice).is_ok());
assert_eq!(
bob.node.send_payment_for_bolt12_invoice(&invoice),
Err(Bolt12PaymentError::DuplicateInvoice),
);

route_bolt12_payment(bob, &[alice], &invoice);
expect_recent_payment!(bob, RecentPaymentDetails::Pending, payment_id);

claim_bolt12_payment(bob, &[alice], payment_context);
expect_recent_payment!(bob, RecentPaymentDetails::Fulfilled, payment_id);

assert_eq!(
bob.node.send_payment_for_bolt12_invoice(&invoice),
Err(Bolt12PaymentError::DuplicateInvoice),
);

for _ in 0..=IDEMPOTENCY_TIMEOUT_TICKS {
bob.node.timer_tick_occurred();
}

assert_eq!(
bob.node.send_payment_for_bolt12_invoice(&invoice),
Err(Bolt12PaymentError::UnexpectedInvoice),
);
}

/// Fails creating an offer when a blinded path cannot be created without exposing the node's id.
#[test]
fn fails_creating_offer_without_blinded_paths() {
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/outbound_payment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -501,9 +501,9 @@ pub enum PaymentSendFailure {
},
}

/// An error when attempting to pay a BOLT 12 invoice.
/// An error when attempting to pay a [`Bolt12Invoice`].
#[derive(Clone, Debug, PartialEq, Eq)]
pub(super) enum Bolt12PaymentError {
pub enum Bolt12PaymentError {
/// The invoice was not requested.
UnexpectedInvoice,
/// Payment for an invoice with the corresponding [`PaymentId`] was already initiated.
Expand Down
9 changes: 8 additions & 1 deletion lightning/src/offers/invoice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ use crate::offers::parse::{Bolt12ParseError, Bolt12SemanticError, ParsedMessage}
use crate::offers::payer::{PAYER_METADATA_TYPE, PayerTlvStream, PayerTlvStreamRef};
use crate::offers::refund::{IV_BYTES as REFUND_IV_BYTES, Refund, RefundContents};
use crate::offers::signer;
use crate::util::ser::{HighZeroBytesDroppedBigSize, Iterable, SeekReadable, WithoutLength, Writeable, Writer};
use crate::util::ser::{HighZeroBytesDroppedBigSize, Iterable, Readable, SeekReadable, WithoutLength, Writeable, Writer};
use crate::util::string::PrintableString;

#[allow(unused_imports)]
Expand Down Expand Up @@ -1205,6 +1205,13 @@ impl Writeable for Bolt12Invoice {
}
}

impl Readable for Bolt12Invoice {
fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
let bytes: WithoutLength<Vec<u8>> = Readable::read(reader)?;
Self::try_from(bytes.0).map_err(|_| DecodeError::InvalidValue)
}
}

impl Writeable for InvoiceContents {
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
self.as_tlv_stream().write(writer)
Expand Down
6 changes: 6 additions & 0 deletions lightning/src/onion_message/messenger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,12 +325,18 @@ impl OnionMessageRecipient {

/// The `Responder` struct creates an appropriate [`ResponseInstruction`]
/// for responding to a message.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct Responder {
/// The path along which a response can be sent.
reply_path: BlindedPath,
path_id: Option<[u8; 32]>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably missing something, but isn't path_id a TLV field in the encrypted_data_tlv stream? Are we positive it will always fit in 32 bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the path_id from the BlindedPath that we created for receiving the message. So, currently yes though we never set it. It's only in the Responder for logging purposes, which given the previous sentence means it's not very useful at the moment. 😛

Though that is a good point considering we are likely going to change it to a Vec and start using it as part of #3085. I'm wondering how useful logging it really is unless we also log it when creating the blinded path, too.

Copy link
Contributor

@tnull tnull Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I was wondering if it indeed is variable-length whether we should switch the type now before we actually start to serialize objects? I guess the field is TLV anyways and from a brief look it even might work out, but not entirely sure if WithoutLength is byte identical with the [u8; 32] serialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt Any thoughts on where we should go with this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't set it maybe we drop it from our logging (even if we intend to set it later)? Its not really clear to me what users have useful to do with a path_id?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If we want to do that we can do that in a followup PR as long as it comes before the next release, I imagine)

Copy link
Contributor Author

@jkczyz jkczyz Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's useful for correlating an onion message to the blinded path it was sent over -- but only if the path_id of the latter is actually logged, too -- and with any response to that onion message. But seeing that data encoded in the path_id will be given to the handler in #3085, then any logging responsibility could be moved to the handler instead, if desired, and use handler-specific concepts rather than opaque bytes.

I'm not sure where that leaves Responder. It would just a wrapper around BlindedPath with methods for converting to a ResponseInstruction. Those methods could be moved to BlindedPath, but that would add an onion_message module dependency to blinded_path, which doesn't feel right. Maybe the wrapper is fine to keep?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then any logging responsibility could be moved to the handler instead, if desired, and use handler-specific concepts rather than opaque bytes.

Right, as a user I really want this info to come to me in the form of an offer_id or a payment_id. path_id is yet another token I have to correlate with something I know.

I'm not sure where that leaves Responder. It would just a wrapper around BlindedPath with methods for converting to a ResponseInstruction. Those methods could be moved to BlindedPath, but that would add an onion_message module dependency to blinded_path, which doesn't feel right. Maybe the wrapper is fine to keep?

I'd say its fine to keep the wrapper, its a separate concept. What it has internally doesn't matter too much.

}

impl_writeable_tlv_based!(Responder, {
(0, reply_path, required),
(2, path_id, option),
});

impl Responder {
/// Creates a new [`Responder`] instance with the provided reply path.
pub(super) fn new(reply_path: BlindedPath, path_id: Option<[u8; 32]>) -> Self {
Expand Down
Loading
Loading