-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add Support for P2TR Fallback Addresses in BOLT-11 #9975
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
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
109c703 to
069f6d1
Compare
|
/gemini review |
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.
Code Review
This pull request adds support for Pay-to-Taproot (P2TR) fallback addresses in BOLT-11 invoices. The changes include updating the invoice decoding and encoding logic to handle P2TR addresses, along with corresponding unit tests. I recommend adding a full integration test case to TestDecodeEncode to ensure the end-to-end encoding and decoding of invoices with P2TR fallback addresses works as expected. This will improve test coverage and help prevent future regressions.
GustavoStingelin
left a comment
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.
💯
|
@saubyk this PR could be included in the lnd v0.20.0 milestone. |
|
#9591 also mentions a spec update for all implementation to behave the same is this also planed ? This should then include test vectors and those test vectors should be tested here. |
Hello @ziggie1984, I just reached out to @erickcestari (who opened the issue). Our understanding is that the spec already states that a P2TR address must be accepted: Handling cases where more than one fallback address is provided may be left to each implementation. I’ll open a PR in BOLT to add the test vector I created for this PR. |
|
Hello @ziggie1984 the PR#1276 adding the test vector for a p2tr fallback address in BOLT 11 has been submitted. Just to clarify — the test vector in this PR doesn’t include the s (payment secret) and 9 (features) tagged fields, following the same pattern as the other test vectors already presented here. |
Hello @ziggie1984, the BOLT PR just received its first approval. |
ziggie1984
left a comment
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.
Had some minor comments, other than that it is good to go
|
@MPins, remember to re-request review from reviewers when ready |
4f34854 to
0f84100
Compare
|
@ziggie1984 now it is using the P2TR test vector. Ready for your review. |
ziggie1984
left a comment
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.
LGTM, I wonder how you created the invoice for the spec in the first place, was thinking you were using LND but doesn't look like it ?
|
Manual testing of 0f84100 in signet: Works as expected! |
starius
left a comment
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.
LGTM! 🏆
The code looks great, pending comments.
I tested it manually in signet - works perfectly!
Thanks! Since I also needed to create an invalid invoice, I used lightning-payencode and modified it to fit my needs. I also submitted some PRs to make it compatible with the recent specs, but it seems Rusty is no longer maintaining the project. |
Thanks! I’ve addressed all the comments, except for the one about encoding back the invoice — I left a reply on that thread with my thoughts. 😉 |
Change Description
Fixes #9591
Add Support for P2TR Fallback Addresses in BOLT-11 invoices
Steps to Test
lncli addinvoice --fallback_addr=bcrt1ps89hq0hxen5y6yxqqk88ssdyhe7hdr92fzudtcskk37p460ldazsl0sfsw