-
Notifications
You must be signed in to change notification settings - Fork 417
offer: make the merkle tree signature public #3892
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
base: main
Are you sure you want to change the base?
offer: make the merkle tree signature public #3892
Conversation
👋 I see @TheBlueMatt was un-assigned. |
0ffed61
to
be23002
Compare
I'm not personally opposed to this and it may be nice to expose it for the use cases you mention. I'm also not sure about any API guarantees for these methods. I don't think it's too much of a hassle to treat them the same way we treat any other public API. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3892 +/- ##
==========================================
- Coverage 89.66% 88.80% -0.86%
==========================================
Files 164 166 +2
Lines 134661 119539 -15122
Branches 134661 119539 -15122
==========================================
- Hits 120743 106161 -14582
+ Misses 11237 11038 -199
+ Partials 2681 2340 -341 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lightning/src/offers/merkle.rs
Outdated
@@ -41,7 +41,7 @@ impl TaggedHash { | |||
/// Creates a tagged hash with the given parameters. | |||
/// | |||
/// Panics if `bytes` is not a well-formed TLV stream containing at least one TLV record. | |||
pub(super) fn from_valid_tlv_stream_bytes(tag: &'static str, bytes: &[u8]) -> Self { | |||
pub fn from_valid_tlv_stream_bytes(tag: &'static str, bytes: &[u8]) -> Self { |
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.
Do we have a user for this? Feels pretty low-level to be exposing. Echo dunxen's sentiments about improving the docs if we do want to expose it.
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.
Yea, if we're gonna expose this we should really add a validating wrapper version that checks the stream is valid and can Err
instead of panicking.
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.
Do we have a user for this? Feels pretty low-level to be exposing.
Yeah, there is. Everyone who wants to verify that an offer is linked to a bolt12 invoice, also if the big picture is that the bolt12 invoice will never be exposed, but for now that we do not have a better PoP a person should allow this kind of verification IMHO
A possible example of verification can be
/// Verify that an offer, invoice, and preimage are valid and consistent
pub fn verify_offer_payment(offer: &str, invoice: &str, preimage: &str) -> Result<()> {
log::info!("Starting verification process...");
log::info!("Offer: {}", offer);
log::info!("Invoice: {}", invoice);
log::info!("Preimage: {}", preimage);
let offer =
Offer::from_str(offer).map_err(|e| anyhow::anyhow!("Failed to parse offer: {:?}", e))?;
// Parse the bolt12 invoice preimage
let (hrp, data) = bech32::decode_without_checksum(invoice)?;
if hrp.as_str() != "lni" {
return Err(anyhow::anyhow!(
"Invalid HRP: expected 'lni', found '{}'",
hrp
));
}
let data = Vec::<u8>::from_base32(&data)?;
let invoice = Bolt12Invoice::try_from(data)
.map_err(|e| anyhow::anyhow!("Failed to parse BOLT12: {e:?}"))?;
let issuer_sign_pubkey = if let Some(issue_sign_pubkey) = offer.issuer_signing_pubkey() {
issue_sign_pubkey
} else {
let valid_signing_keys = offer
.paths()
.iter()
.filter_map(|path| path.blinded_hops().last())
.map(|hop| hop.blinded_node_id)
.collect::<Vec<_>>();
log::debug!("{:?}", valid_signing_keys);
let valid_signing_keys = valid_signing_keys
.first()
.ok_or(anyhow::anyhow!(
"Offer does not contain issuer signing pubkey"
))?
.clone();
valid_signing_keys
};
let inv_signing_pubkey = invoice.signing_pubkey();
if issuer_sign_pubkey != inv_signing_pubkey {
return Err(anyhow::anyhow!(
"Offer and invoice issuer signing pubkeys do not match"
));
}
let mut bytes = vec![];
invoice
.write(&mut bytes)
.map_err(|e| anyhow::anyhow!("Failed to serialize invoice: {:?}", e))?;
let tagged_hash = TaggedHash::from_valid_tlv_stream_bytes(SIGNATURE_TAG, &bytes);
if let Err(err) =
merkle::verify_signature(&invoice.signature(), &tagged_hash, issuer_sign_pubkey)
{
anyhow::bail!("Failed to verify invoice signature: {:?}", err);
}
let preimage =
hex::decode(preimage).map_err(|e| anyhow::anyhow!("Failed to decode preimage: {:?}", e))?;
let preimage: [u8; 32] = preimage
.try_into()
.map_err(|_| anyhow::anyhow!("Preimage must be 32 bytes"))?;
let preimage = PaymentPreimage(preimage);
// Validate the Proof of Payment for the invoice
let computed_hash = PaymentHash::from(preimage);
let payment_hash = invoice.payment_hash();
if computed_hash != payment_hash {
anyhow::bail!("Fail to perform PoP");
}
log::info!("Parsed offer: {:?}", offer);
log::info!("Parsed invoice: {:?}", invoice);
Ok(())
}
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.
Hmm... we verify the invoice's signature when parsing it. So it's just a matter of matching that the Offer
given is the same one that is part of the Bolt12Invoice
. This should be possible by comparing Offer::id
-- which is just the tagged hash of the offer's merkle root -- against one obtained from the Bolt12Invoice
. We could possibly add Bolt12Invoice::offer_id
for this.
Note that while OfferId
uses an LDK-specific tag, it is computed over the TLV stream bytes so it doesn't matter if the offer wasn't created with LDK.
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.
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 implemented it @jkczyz cfd6ad4 let me know what do you think.
Thanks, left some comments.
However, I still think that ldk should expose a way to verify the signature with the public key that is inside the offer, at least till we do not get a better PoP with bolt12
Given the signature is verified when parsing, shouldn't it be sufficient to check that Bolt12Invoice::signing_pubkey
matches the one in the offer? The parser is already doing this when calling check_invoice_signing_pubkey
, too, FWIW. Not sure I understand why we need to expose these for the user to piece together and call on their own.
At very least, there doesn't seem to be a need to expose TaggedHash::from_valid_tlv_stream_bytes
if we can just have a method on Bolt12Invoice
give the TaggedHash
for manual signature verification.
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.
Given the signature is verified when parsing, shouldn't it be sufficient to check that Bolt12Invoice::signing_pubkey matches the one in the offer?
Yes, good point, I think it is just fine for a proof of linking point of view.
FWIW. Not sure I understand why we need to expose these for the user to piece together and call on their own.
At this point is just for a learning tool purpose if some users want to do the versification as specified inside the spec. Probably, this is a separate discussion
At very least, there doesn't seem to be a need to expose TaggedHash::from_valid_tlv_stream_bytes if we can just have a method on Bolt12Invoice give the TaggedHash for manual signature verification.
This is a really good point, lets me experiment with it
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
🔔 1st Reminder Hey @TheBlueMatt @valentinewallace! This PR has been waiting for your review. |
1 similar comment
🔔 1st Reminder Hey @TheBlueMatt @valentinewallace! This PR has been waiting for your review. |
cc @jkczyz |
dd046d5
to
29e5bd6
Compare
lightning/src/offers/invoice.rs
Outdated
let offer_tlv_stream = TlvStream::new(&self.bytes).range(OFFER_TYPES); | ||
let experimental_offer_tlv_stream = TlvStream::new(&self.experimental_bytes).range(EXPERIMENTAL_OFFER_TYPES); | ||
let combined_tlv_stream = offer_tlv_stream.chain(experimental_offer_tlv_stream); | ||
let tagged_hash = TaggedHash::from_tlv_stream("LDK Offer ID", combined_tlv_stream); | ||
Some(OfferId(tagged_hash.to_bytes())) |
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.
You can reuse OfferId::from_valid_invreq_tlv_stream
(or something with a more appropriate name) rather than duplicating that code.
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.
Make sense thanks!
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.
Done created the from_invoice_bytes
that IMHO made clear that we look at the range of the offer TLV, please let me know if you have any other suggestion on this
lightning/src/offers/merkle.rs
Outdated
@@ -41,7 +41,7 @@ impl TaggedHash { | |||
/// Creates a tagged hash with the given parameters. | |||
/// | |||
/// Panics if `bytes` is not a well-formed TLV stream containing at least one TLV record. | |||
pub(super) fn from_valid_tlv_stream_bytes(tag: &'static str, bytes: &[u8]) -> Self { | |||
pub fn from_valid_tlv_stream_bytes(tag: &'static str, bytes: &[u8]) -> Self { |
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 implemented it @jkczyz cfd6ad4 let me know what do you think.
Thanks, left some comments.
However, I still think that ldk should expose a way to verify the signature with the public key that is inside the offer, at least till we do not get a better PoP with bolt12
Given the signature is verified when parsing, shouldn't it be sufficient to check that Bolt12Invoice::signing_pubkey
matches the one in the offer? The parser is already doing this when calling check_invoice_signing_pubkey
, too, FWIW. Not sure I understand why we need to expose these for the user to piece together and call on their own.
At very least, there doesn't seem to be a need to expose TaggedHash::from_valid_tlv_stream_bytes
if we can just have a method on Bolt12Invoice
give the TaggedHash
for manual signature verification.
Will take a look at this after @jkczyz's comments are addressed, assuming he thinks it needs a second reviewer at that point. |
0c7fe08
to
123e16f
Compare
🔔 2nd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
123e16f
to
7429b6a
Compare
This is helpfull for the users that want to use the merkle tree signature in their own code, for example to verify the signature of bolt12 invoices or recreate it. Very useful for people that are building command line tools for the bolt12 offers. Signed-off-by: Vincenzo Palazzo <[email protected]>
7429b6a
to
1b958e9
Compare
Ok, I pushed the change that @jkczyz requested, and now the PR is organised in two commits as follows:
Let me know if you are fine with the commit divided in this way, or you prefer e60b87c |
1b958e9
to
699f37f
Compare
- Add an Option<OfferId> field to Bolt12Invoice to track the originating offer. - Compute the offer_id for invoices created from offers by extracting the offer TLV records and hashing them with the correct tag. - Expose a public offer_id() accessor on Bolt12Invoice. - Add tests to ensure the offer_id in the invoice matches the originating Offer, and that refund invoices have no offer_id. - All existing and new tests pass. This enables linking invoices to their originating offers in a robust and spec-compliant way. Signed-off-by: Vincenzo Palazzo <[email protected]> Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
22ef851
to
e60b87c
Compare
@@ -967,6 +980,11 @@ impl Bolt12Invoice { | |||
self.tagged_hash.as_digest().as_ref().clone() | |||
} | |||
|
|||
/// Returns the [`OfferId`] if this invoice corresponds to an [`crate::offers::offer::Offer`]. |
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 docs would be cleaner as:
/// Returns the [`OfferId`] if this invoice corresponds to an [`Offer`].
///
/// [`Offer`]: crate::offers::offer::Offer
@@ -665,6 +665,14 @@ impl UnsignedBolt12Invoice { | |||
pub fn tagged_hash(&self) -> &TaggedHash { | |||
&self.tagged_hash | |||
} | |||
|
|||
/// Computes the [`OfferId`] if this invoice corresponds to an [`Offer`]. |
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.
Link will need to be fixed here as described in my other comment. Or just drop the docs since this is a private method and doesn't need much explanation.
let offer_tlv_stream = TlvStream::new(bytes).range(OFFER_TYPES); | ||
let experimental_offer_tlv_stream = TlvStream::new(bytes).range(EXPERIMENTAL_OFFER_TYPES); | ||
let combined_tlv_stream = offer_tlv_stream.chain(experimental_offer_tlv_stream); | ||
let tagged_hash = TaggedHash::from_tlv_stream(Self::ID_TAG, combined_tlv_stream); | ||
Self(tagged_hash.to_bytes()) |
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 should be the same code as from_valid_invreq_tlv_stream
.
pub fn sign_message<F, T>( | ||
f: F, message: &T, pubkey: PublicKey, | ||
) -> Result<Signature, SignError> | ||
pub fn sign_message<F, T>(f: F, message: &T, pubkey: PublicKey) -> Result<Signature, SignError> | ||
where |
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 belongs in the previous commit.
fn compute_offer_id(&self) -> Option<OfferId> { | ||
match &self.contents { | ||
InvoiceContents::ForOffer { .. } => Some(OfferId::from_invoice_bytes(&self.bytes)), | ||
InvoiceContents::ForRefund { .. } => None, | ||
} | ||
} |
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.
Maybe we should just inline this given it's not reused in the parsing code.
This is helpful for users who want to use the Merkle tree signature in their own code, for example, to verify the signature of bolt12 invoices or recreate it.
Very useful for people who are building command line tools for the bolt12 offers.
I am opening this, but I do not know if it is something that you want to do, but at the same time, IMHO, this is very useful to expose because it allows to use LDK in command line tools and in learning tools.
However, this is not strictly necessary because
Bolt12Invoice::try_from
already verifies the signature, but I would open this to know your point of view.