Skip to content
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

Expose more granular data in TaggedHash struct #2687

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

orbitalturtle
Copy link
Contributor

@orbitalturtle orbitalturtle commented Oct 26, 2023

This PR exposes the tag + merkle root data in the TaggedHash struct so that they can be used directly the sign closures in UnsignedInvoiceRequest (etc), if/when needed.

Have talked to @jkczyz a bit about this. For context, the motivation for this is that for LNDK's, we'd like to be able to be able to pull out the tag and merkle root hash directly so that we can pass these into the LND api for signing invoice requests etc. :)

@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (b3e7aac) 88.75% compared to head (caafced) 88.76%.
Report is 25 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2687      +/-   ##
==========================================
+ Coverage   88.75%   88.76%   +0.01%     
==========================================
  Files         112      113       +1     
  Lines       88504    89153     +649     
  Branches    88504    89153     +649     
==========================================
+ Hits        78548    79133     +585     
- Misses       7717     7769      +52     
- Partials     2239     2251      +12     
Files Coverage Δ
lightning/src/offers/invoice.rs 94.27% <100.00%> (+<0.01%) ⬆️
lightning/src/offers/invoice_request.rs 93.37% <100.00%> (ø)
lightning/src/offers/merkle.rs 98.97% <97.14%> (+0.10%) ⬆️

... and 22 files with indirect coverage changes

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

@jkczyz jkczyz self-requested a review October 26, 2023 13:18
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! The more I look at it, the more I think we can avoid the enum and the different branches (and panics!) it introduces. Left some comments to that end.

lightning/src/offers/invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved
lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved
lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved
@orbitalturtle
Copy link
Contributor Author

@jkczyz Thanks for taking a look! Indeed what you suggested simplified things quite a bit :)

One note that I kept the precomputed digest field because it looks like the digest is needed in either case anyway -- when verifying the signature after the sig is taken

let digest = message.as_ref().as_digest();

@orbitalturtle orbitalturtle changed the title SignatureData enum for invoice request signing Expose more granular data in TaggedHash struct Oct 27, 2023
lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved
lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved
lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved
lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved
lightning/src/offers/invoice_request.rs Outdated Show resolved Hide resolved
@TheBlueMatt
Copy link
Collaborator

I mean, sure, but, like, why does the LND API Want to see the tag? Isn't this a BOLT12-specific construction anyway?

@orbitalturtle
Copy link
Contributor Author

@TheBlueMatt So inititally we were were hoping to just pass the taggedhash into LND's signmessage API... But the API call requires that signmessage also hashes whatever is passed in -- apparently to restrict the input to disallow signing transactions etc. But with this LND PR we'll be able to pass in the tag and take a tagged hash instead of the default sha256. lightningnetwork/lnd#8106

@TheBlueMatt
Copy link
Collaborator

Why not just let the RPC sign arbitrary message digests? Given its BIP 340 tagging, can this not be used to sign a transaction anyway?

@orbitalturtle
Copy link
Contributor Author

@TheBlueMatt Oh yeah so the above LND PR limits it so BIP 340 tags can't be used. @guggero would be able to answer better than me on the question of allowing signing arbitrary message digests :)

@orbitalturtle orbitalturtle force-pushed the signature-data-enum branch 3 times, most recently from 74b35eb to 2108e95 Compare October 30, 2023 01:18
lightning/src/offers/invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/invoice_request.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Just some minor comments but otherwise looks good. Could you update the commit messages to contain motivation where it's not obvious?

lightning/src/offers/invoice.rs Outdated Show resolved Hide resolved
lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved
lightning/src/offers/merkle.rs Outdated Show resolved Hide resolved
We change the Bolt12Invoice struct to carry a tagged hash. Because
message_digest is then only used in one place, we can inline it in
the TaggedHash constructor.
Expose tag and merkle root fields in the TaggedHash struct.
@orbitalturtle
Copy link
Contributor Author

Made those changes, thanks for all the guidance @jkczyz !

let merkle_root = root_hash(tlv_stream);
let digest = Message::from_slice(&tagged_hash(tag_hash, merkle_root)).unwrap();
Self {
tag: tag.to_owned(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bleh. This allocation feels incredibly unnecessary given there's only two possible values here and they're both constants. We could instead trivially make tag an &'static str and avoid it entirely. Probably a reasonable followup.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@TheBlueMatt TheBlueMatt merged commit 96e7d7a into lightningdevkit:main Nov 7, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants