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.

Sign up for GitHub

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

Jump to bottom

Add payment secret feature to Bolt 11 test vectors #898

Merged
merged 4 commits into from
Sep 14, 2021
Merged

Add payment secret feature to Bolt 11 test vectors #898

merged 4 commits into from
Sep 14, 2021

Conversation

t-bast
Copy link
Collaborator

@t-bast t-bast commented Aug 23, 2021

Bolt 11 invoices must contain a payment_secret, which means that the features field must set the payment_secret feature (and its dependency, var_onion_optin).

Fixes #897

@t-bast
Add payment secret feature to Bolt 11 test vectors
89a5da4 
Bolt 11 invoices must contain a `payment_secret`, which means that the
`features` field must set the `payment_secret` feature (and its dependency,
`var_onion_optin`).

Fixes #897
t-bast added a commit to ACINQ/eclair that referenced this pull request Aug 23, 2021
@t-bast
Update test vectors
16dc917 
Add feature bits, as indicated by
lightning/bolts#898
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Aug 23, 2021
@t-bast
Update Bolt11 test vectors
c1ae78d 
We now match the spec once lightning/bolts#898
is merged.
@t-bast t-bast mentioned this pull request Aug 23, 2021
Merged
TheBlueMatt
TheBlueMatt reviewed Aug 24, 2021
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

At least the "Signature is not recoverable." and "Invalid sub-millisatoshi precision." failure cases also need a feature flag, likely others as well but those are just the two that our implementation complains about as missing feature.

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Aug 24, 2021
@TheBlueMatt
Use new BOLT 11 test vectors with payment_secrets and feature flags
b52195e 
This pulls the BOLT 11 test vectors from
lightning/bolts#898,
tweaking our tests to properly handle them.
TheBlueMatt
TheBlueMatt approved these changes Aug 24, 2021
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

The changes here LGTM, verified they pass our own tests, though the error cases could use the feature flag as well.

11-payment-encoding.md

> ### On mainnet, with fallback address 1RustyRX2oai4EYYDpQGWvEL62BBGqN9T with extra routing info to go via nodes 029e03a901b85534ff1e92c43c74431f7ce72046060fcf7a95c37e148f78c77255 then 039e03a901b85534ff1e92c43c74431f7ce72046060fcf7a95c37e148f78c77255
> lnbc20m1pvjluezsp5zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygspp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqhp58yjmdan79s6qqdhdzgynm4zwqd5d7xmw5fk98klysy043l2ahrqsfpp3qjmp7lwpagxun9pygexvgpjdc4jdj85fr9yq20q82gphp2nflc7jtzrcazrra7wwgzxqc8u7754cdlpfrmccae92qgzqvzq2ps8pqqqqqqpqqqqq9qqqvpeuqafqxu92d8lr6fvg0r5gv0heeeqgcrqlnm6jhphu9y00rrhy4grqszsvpcgpy9qqqqqqgqqqqq7qqzqg042ea62q6wnxh909rfuu4x2amxc59mj99mrhr32kvqhytrm04us8edg0syy9n0ukgam0ud20jcxwskphv3rzpnengjsf8m3w9u5w8cqzrhtg2
> lnbc20m1pvjluezsp5zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygspp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqhp58yjmdan79s6qqdhdzgynm4zwqd5d7xmw5fk98klysy043l2ahrqsfpp3qjmp7lwpagxun9pygexvgpjdc4jdj85fr9yq20q82gphp2nflc7jtzrcazrra7wwgzxqc8u7754cdlpfrmccae92qgzqvzq2ps8pqqqqqqpqqqqq9qqqvpeuqafqxu92d8lr6fvg0r5gv0heeeqgcrqlnm6jhphu9y00rrhy4grqszsvpcgpy9qqqqqqgqqqqq7qqzq9qrsgqdfjcdk6w3ak5pca9hwfwfh63zrrz06wwfya0ydlzpgzxkn5xagsqz7x9j4jwe7yj7vaf2k9lqsdk45kts2fd0fkr28am0u4w95tt2nsq76cqw0
Copy link
Collaborator

Choose a reason for hiding this comment

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

While you're tweaking this, can you add an "0x" prefix on the short_channel_id fields a few lines down?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed to the friendlier short_channel_id notation that uses blockheight in 45222fb

@t-bast
Copy link
Collaborator Author

t-bast commented Aug 25, 2021

At least the "Signature is not recoverable." and "Invalid sub-millisatoshi precision." failure cases also need a feature flag, likely others as well but those are just the two that our implementation complains about as missing feature.

Thanks, I wondered where that would be verified in each implementation, in eclair this is verified last, but there's no reason other implementations do the same. I'll update all failure cases that make sense.

@t-bast
Copy link
Collaborator Author

t-bast commented Aug 25, 2021

As you pointed out, only the last two failure test vectors need to be updated, the others have a completely invalid format which should always be checked before the actual contents. I updated them in c32d06a

t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Aug 25, 2021
@t-bast
Update Bolt11 test vectors
b8a2b15 
We now match the spec once lightning/bolts#898
is merged.
@TheBlueMatt
Copy link
Collaborator

LDK still reports this failure case as "NoPaymentSecret":

"Signature is not recoverable.
lnbc2500u1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5xysxxatsyp3k7enxv4jsxqzpuaxtrnwngzn3kdzw5hydlzf03qdgm2hdq27cqv3agm2awhz5se903vruatfhq77w3ls4evs3ch9zw97j25emudupq63nyw24cg27h2rspk28uwq"

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Aug 27, 2021
@TheBlueMatt
Use new BOLT 11 test vectors with payment_secrets and feature flags
52468bf 
This pulls the BOLT 11 test vectors from
lightning/bolts#898,
tweaking our tests to properly handle them.
@TheBlueMatt TheBlueMatt mentioned this pull request Aug 27, 2021
Merged
@t-bast
Update non-recoverable sig test vector
6435025 
This test vector was generated by creating a valid invoice, then flipping
the recoveryId to be 2 instead of 0 or 1.
@t-bast
Copy link
Collaborator Author

t-bast commented Aug 27, 2021

LDK still reports this failure case as "NoPaymentSecret":

Right, there's no reason to assume recovering the pubkey from the sig should be done before validating the invoice's contents.
I updated this test vector in 6435025 (I generated the correct signature then flipped the recovery ID to be 0x02, that does the trick with libsecp256k1, let me know if that works for LDK).

@sstone sstone mentioned this pull request Aug 27, 2021
Merged
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Aug 27, 2021
@TheBlueMatt
Use new BOLT 11 test vectors with payment_secrets and feature flags
2735cea 
This pulls the BOLT 11 test vectors from
lightning/bolts#898,
tweaking our tests to properly handle them.
@TheBlueMatt
Copy link
Collaborator

Thanks! Tested the latest and it all passes/fails with expected errors.

t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Aug 30, 2021
@t-bast
Update Bolt11 test vectors
3cbcd7a 
We now match the spec once lightning/bolts#898
is merged.
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Aug 31, 2021
@TheBlueMatt
Use new BOLT 11 test vectors with payment_secrets and feature flags
a901927 
This pulls the BOLT 11 test vectors from
lightning/bolts#898,
tweaking our tests to properly handle them.
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Aug 31, 2021
@TheBlueMatt
Use new BOLT 11 test vectors with payment_secrets and feature flags
a906c49 
This pulls the BOLT 11 test vectors from
lightning/bolts#898,
tweaking our tests to properly handle them.
@t-bast t-bast mentioned this pull request Sep 8, 2021
Closed
19 tasks
t-bast added a commit to ACINQ/eclair that referenced this pull request Sep 9, 2021
@t-bast
Update test vectors
db60ccd 
Add feature bits, as indicated by
lightning/bolts#898
@t-bast
Copy link
Collaborator Author

t-bast commented Sep 14, 2021

Merging as per yesterday's spec meeting.

@t-bast t-bast merged commit c876dac into master Sep 14, 2021
@t-bast t-bast deleted the bolt11-test-vectors-features branch September 14, 2021 07:11
t-bast added a commit to ACINQ/eclair that referenced this pull request Sep 14, 2021
@t-bast
Update Bolt 11 official test vectors (#1870)
fb0199c 
Add payment secrets (see lightning/bolts#887)
Add feature bits (see lightning/bolts#898)
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Sep 22, 2021
@t-bast
Update Bolt11 test vectors
07a8947 
We now match the spec once lightning/bolts#898
is merged.
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Sep 22, 2021
@t-bast
Update Bolt11 test vectors (#293)
da7b603 
We now match the spec once lightning/bolts#898
is merged.
joostjager pushed a commit to joostjager/lightning-rfc that referenced this pull request Dec 6, 2021
@t-bast @joostjager
Add payment secret feature to Bolt 11 test vectors (lightning#898)
a775d1f 
Bolt 11 invoices must contain a `payment_secret`, which means that the
`features` field must set the `payment_secret` feature (and its dependency,
`var_onion_optin`).

Fixes lightning#897
joostjager pushed a commit to joostjager/lightning-rfc that referenced this pull request Dec 6, 2021
@t-bast @joostjager
Add payment secret feature to Bolt 11 test vectors (lightning#898)
536a16d 
Bolt 11 invoices must contain a `payment_secret`, which means that the
`features` field must set the `payment_secret` feature (and its dependency,
`var_onion_optin`).

Fixes lightning#897
SomberNight added a commit to SomberNight/electrum that referenced this pull request Jun 29, 2023
@SomberNight
lnaddr: make payment_secret field mandatory, in both directions
fcd296f 
we now require payment_secret both for sending and for receiving
(previously was optional for both)

see
lightning/bolts#898
ACINQ/eclair#1810
ElementsProject/lightning#4646

note: payment_secret depends on var_onion_optin, so that becomes mandatory as well,
however this commit does not yet remove the ability of creating legacy onions
SomberNight added a commit to SomberNight/electrum that referenced this pull request Jun 29, 2023
@SomberNight
lnaddr: make payment_secret field mandatory, in both directions
fc6486e 
we now require payment_secret both for sending and for receiving
(previously was optional for both)

see
lightning/bolts#898
ACINQ/eclair#1810
ElementsProject/lightning#4646

note: payment_secret depends on var_onion_optin, so that becomes mandatory as well,
however this commit does not yet remove the ability of creating legacy onions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@rustyrussell rustyrussell Awaiting requested review from rustyrussell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New BOLT 11 Test cases are missing payment secret feature
2 participants
@t-bast @TheBlueMatt