-
Notifications
You must be signed in to change notification settings - Fork 390
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
[RFC] Implement a way to do BOLT 12 Proof of Payment #3593
base: main
Are you sure you want to change the base?
[RFC] Implement a way to do BOLT 12 Proof of Payment #3593
Conversation
c7e9e61
to
cc714ae
Compare
a5c211c
to
9cdbb44
Compare
9cdbb44
to
8f5dc5e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3593 +/- ##
==========================================
+ Coverage 89.25% 89.30% +0.05%
==========================================
Files 155 156 +1
Lines 119708 121825 +2117
Branches 119708 121825 +2117
==========================================
+ Hits 106845 108796 +1951
- Misses 10250 10388 +138
- Partials 2613 2641 +28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for picking this up! Sorry about the delay in review, not sure why this slipped through the cracks.
7da68f6
to
0e7b503
Compare
Thanks for the review @TheBlueMatt, I applied the suggestion in the review. In the meanwhile, I rebased on top of |
0e7b503
to
a4897fb
Compare
a4897fb
to
2c65d6a
Compare
9b72246
to
2c65d6a
Compare
Ok @TheBlueMatt this PR is really funny to work! I worked to clean up the PR a bit and divide the code to support both Bolt12 and static invoices. However, I am currently working on supporting the async payment functionality. As far as I understand, the code involving async payments is a bit tricky to support for the following reasons (code available 85c3f73). The async workflow is initialized with The only way I have to access the static invoice when passing it down inside
I would prefer the 3rd point, but this requires a follow-up PR to rework a little bit how the async payment is handled and I do not know if there is any plan for that,t or there is other code that will be depending on this architecture in the pipeline. Probably @valentinewallace will be interested in this conversation |
Generally we just copy and paste the impl with different cfg flags and go back and clean it up after we drop the cfg. I'll let @valentinewallace comment on the rest. |
Hey sorry I missed this tag. In #3618 right now we actually un-cfg-gate |
Yes, but in this case, we need to duplicate all the |
734c955
to
525c01e
Compare
8471783
to
2ebd702
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok applied the review (thanks for that) and rebased on current main to include #3640
It looks like this works: diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
index cb0ce3598..5405de413 100644
--- a/lightning/src/events/mod.rs
+++ b/lightning/src/events/mod.rs
@@ -2460,8 +2460,7 @@ pub enum PaidInvoice {
StaticInvoice(crate::offers::static_invoice::StaticInvoice),
}
-impl_writeable_tlv_based_enum_legacy!(PaidInvoice,
- ;
- (0, Bolt12Invoice),
- (2, StaticInvoice)
+impl_writeable_tlv_based_enum!(PaidInvoice,
+ {0, Bolt12Invoice} => (),
+ {2, StaticInvoice} => (),
);
diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index 56779e24a..f092690ef 100644
--- a/lightning/src/util/ser_macros.rs
+++ b/lightning/src/util/ser_macros.rs
@@ -1200,7 +1200,7 @@ macro_rules! impl_writeable_tlv_based_enum {
$($tuple_variant_id => {
let length: $crate::util::ser::BigSize = $crate::util::ser::Readable::read(reader)?;
let mut s = $crate::util::ser::FixedLengthReader::new(reader, length.0);
- let res = $crate::util::ser::Readable::read(&mut s)?;
+ let res = $crate::util::ser::LengthReadable::read_from_fixed_length_buffer(&mut s)?;
if s.bytes_remain() {
s.eat_remaining()?; // Return ShortRead if there's actually not enough bytes
return Err($crate::ln::msgs::DecodeError::InvalidValue); Tried to get it to work with |
2ebd702
to
43f5f33
Compare
Thanks @valentinewallace I tried to apply it but probably the diff was made in an old version of the PR, in any case I imported the diff manually and applied it in a separate commit 2354e2c to be able to document why we need this change in details. Now pushed the change and waiting for the Ci, thanks for your time! |
2b6aa59
to
5e36ba6
Compare
4050a82
to
0cd70fa
Compare
/// The BOLT 12 invoice is useful for proof of payment because it contains the | ||
/// payment hash. A third party can verify that the payment was made by | ||
/// showing the invoice and confirming that the payment hash matches | ||
/// the hash of the payment preimage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't provide proof-of-payment if a static invoice is provided, could amend the docs for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this is the docs for the older version - now should be it, let me know how looks like now, and if you want to do some more iteration on it
@@ -1200,7 +1200,7 @@ macro_rules! impl_writeable_tlv_based_enum { | |||
$($tuple_variant_id => { | |||
let length: $crate::util::ser::BigSize = $crate::util::ser::Readable::read(reader)?; | |||
let mut s = $crate::util::ser::FixedLengthReader::new(reader, length.0); | |||
let res = $crate::util::ser::Readable::read(&mut s)?; | |||
let res = $crate::util::ser::LengthReadable::read_from_fixed_length_buffer(&mut s)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so regarding the commit message, this actually isn't a temporary workaround but something we'd want to do no matter what since the refactor was introduced. You could just say that this change is needed since that landed?
For context, the deeper refactor would be needed to make impl_writeable_tlv_based_enum_upgradable
work for enum tuple variants with payloads that are LengthReadable
, IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the commit message hasn't been updated?
@@ -2294,7 +2294,7 @@ macro_rules! expect_payment_claimed { | |||
pub fn expect_payment_sent<CM: AChannelManager, H: NodeHolder<CM=CM>>(node: &H, | |||
expected_payment_preimage: PaymentPreimage, expected_fee_msat_opt: Option<Option<u64>>, | |||
expect_per_path_claims: bool, expect_post_ev_mon_update: bool, | |||
) { | |||
) -> Option<PaidInvoice> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we don't currently check that this is set for any static invoices, could you add a check in one of the calls to claim_payment_along_route
in async_payments_tests.rs`?
@@ -379,7 +389,8 @@ impl StaticInvoice { | |||
self.signature | |||
} | |||
|
|||
pub(crate) fn from_same_offer(&self, invreq: &InvoiceRequest) -> bool { | |||
#[allow(unused)] // TODO: remove this once we remove the `async_payments` cfg flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I updated this locally and forgot you had an outdated commit, we should be able to just use this simpler commit I believe: valentinewallace@bb70ee6
924d003
to
e588f04
Compare
We need to include static invoices in the public API as part of the onion messages we're adding for static invoice server support. Utilities to create these static invoices and other parts of the async receive API will remain cfg-gated for now. Generally, we can't totally avoid exposing half baked async receive support in the public API because OnionMessenger is parameterized by an async payments message handler, which can't be cfg-gated easily. This contains the changes to the public API, to the original commit made by Val, to fix the following clippy warning: ``` error: methods called `from_*` usually take no `self` --> lightning/src/offers/static_invoice.rs:393:32 | 393 | pub(crate) fn from_same_offer(&self, invreq: &InvoiceRequest) -> bool { | ^^^^^ | = help: consider choosing a less ambiguous name = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention = note: `-D clippy::wrong-self-convention` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::wrong_self_convention)]` error: could not compile `lightning` (lib) due to 1 previous error ``` ``` error[E0599]: no method named `bytes` found for reference `&InvoiceRequest` in the current scope --> lightning/src/offers/static_invoice.rs:387:34 | 387 | Offer::tlv_stream_iter(invreq.bytes()).map(|tlv_record| tlv_record.record_bytes); | ^^^^^-- help: remove the arguments | | | field, not a method | = help: items from traits can only be used if the trait is implemented and in scope = note: the following trait defines an item `bytes`, perhaps you need to implement it: candidate #1: `std::io::Read` warning: variable does not need to be mutable --> lightning/src/offers/offer.rs:261:4 | 261 | mut $self: $self_type, metadata: Vec<u8>, | ----^^^^^ | | | help: remove this `mut` ... 534 | offer_explicit_metadata_builder_methods!(self, &mut Self, (), ()); | ----------------------------------------------------------------- in this macro invocation | = note: `#[warn(unused_mut)]` on by default = note: this warning originates in the macro `offer_explicit_metadata_builder_methods` (in Nightly builds, run with -Z macro-backtrace for more info) For more information about this error, try `rustc --explain E0599`. ``` Signed-off-by: Vincenzo Palazzo <[email protected]>
When introducing the LengthLimitedRead trait in [1], for some serialization macros we need some temporary workaround to avoid that we have the following compilation error error[E0277]: the trait bound `Bolt12Invoice: Readable` is not satisfied --> lightning/src/util/ser_macros.rs:1243:35 | 1243 | Ok($st::$tuple_variant_name($crate::util::ser::Readable::read(reader)?)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Readable` is not implemented for `Bolt12Invoice` | ::: lightning/src/events/mod.rs:2463:1 | 2463 | / impl_writeable_tlv_based_enum_legacy!(PaidInvoice, 2464 | | ; 2465 | | (0, Bolt12Invoice), 2466 | | (2, StaticInvoice) 2467 | | ); | |_- in this macro invocation | = help: the following other types implement trait `Readable`: () (A, B) (A, B, C) (A, B, C, D) (A, B, C, D, E) (A, B, C, D, E, F) (A, B, C, D, E, F, G) AnnouncementSigsState and 200 others = note: this error originates in the macro `impl_writeable_tlv_based_enum_legacy` (in Nightly builds, run with -Z macro-backtrace for more info) This is a temporary workaround to avoid introducing a big refactoring right now. [1] lightningdevkit#3640 Co-Developed-by: @valentinewallace Suggested-by: @valentinewallace Signed-off-by: Vincenzo Palazzo <[email protected]>
This commit make two things possible: 1. make persistent BOLT12 invoice through PendingOutboundPayment This commit prepares the code to pass down the BOLT12 invoice inside the `PaymentSent` event. To achieve this, the `bolt12` field has been added to the `PendingOutboundPayment::Retryable` enum, allowing it to be attached to the `PaymentSent` event when the payment is completed. 2. To enable proof of payment, we need to share the bolt12 invoice with the library user. This is already possible if we `manually_handle_bolt12_invoices`, but this approach requires a significant amount of work from the user. This commit adds the bolt12 invoice to the PaymentSend event when the payment is completed. This allows the user to always have the option to perform proof of payment. Link: lightningdevkit#3344 Signed-off-by: Vincenzo Palazzo <[email protected]>
…Received Inject the static invoice inside the PendingOutboundPayment::StaticInvoiceReceived to allow to use the static invoice inside the PendingOutboundPayment::PaymentReceived. Signed-off-by: Vincenzo Palazzo <[email protected]>
This commit is separate from the feature because probably the route code is the commit c0f7de3 where the `async_payments` cfg is not set. Fixing the following warning error: associated function `features_unchecked` is never used --> lightning/src/offers/invoice_macros.rs:91:2 | 91 | / pub(crate) fn features_unchecked( 92 | | $($self_mut)* $self: $self_type, features: Bolt12InvoiceFeatures 93 | | ) -> $return_type { 94 | | $invoice_fields.features = features; 95 | | $return_value 96 | | } | |_____^ | ::: lightning/src/offers/static_invoice.rs:186:5 | 186 | invoice_builder_methods_test_common!(self, Self, self.invoice, Self, self, mut); | ------------------------------------------------------------------------------- in this macro invocation | = note: `-D dead-code` implied by `-D warnings` = note: this error originates in the macro `invoice_builder_methods_test_common` (in Nightly builds, run with -Z macro-backtrace for more info) Signed-off-by: Vincenzo Palazzo <[email protected]>
e588f04
to
168bb32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized some previous comments are not addressed so may have jumped the gun on this review! Oh well
@@ -1200,7 +1200,7 @@ macro_rules! impl_writeable_tlv_based_enum { | |||
$($tuple_variant_id => { | |||
let length: $crate::util::ser::BigSize = $crate::util::ser::Readable::read(reader)?; | |||
let mut s = $crate::util::ser::FixedLengthReader::new(reader, length.0); | |||
let res = $crate::util::ser::Readable::read(&mut s)?; | |||
let res = $crate::util::ser::LengthReadable::read_from_fixed_length_buffer(&mut s)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the commit message hasn't been updated?
/// | ||
/// However, the [`PaidInvoice`] can also be of type [`StaticInvoice`], which | ||
/// is a special [`Bolt12Invoice`] where proof of payment is not possible. | ||
/// For more details, see the `async_payments` specification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The async payments spec isn't merged so since we can't easily link to anything it's probably best to remove this line to avoid confusion. Otherwise these docs look good!
@@ -155,6 +156,12 @@ impl_writeable_tlv_based!(RetryableInvoiceRequest, { | |||
}); | |||
|
|||
impl PendingOutboundPayment { | |||
fn bolt12_invoice(&self) -> Option<PaidInvoice> { | |||
match self { | |||
PendingOutboundPayment::Retryable { bolt12_invoice, .. } => bolt12_invoice.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we return a reference here and clone at the callsite to make it clearer what's going on?
/// showing the invoice and confirming that the payment hash matches | ||
/// the hash of the payment preimage. | ||
/// | ||
/// However, the [`PaidInvoice`] can also be of type [`StaticInvoice`], which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like CI wants you to add a link to StaticInvoice
. Alternately, you can import StaticInvoice
in this file + not use the full import path in the PaidInvoice
variant.
This is already possible if we
manually_handle_bolt12_invoices
, butthis approach requires a significant amount of work from the user.
This commit adds the bolt12 invoice to the PaymentSend event when
the payment is completed. This always gives the user the option to
provide proof of payment.
I am opening this up for an initial review, but I need to look inside the
DNSSECProof
to add this proof, butprobably be also a follow-up PR
Opening in draft for now to make the CI happyFixes: #3344