-
Notifications
You must be signed in to change notification settings - Fork 492
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
BOLT11 additional and negative tests #736
BOLT11 additional and negative tests #736
Conversation
See-also: lightning/bolts#736 Signed-off-by: Rusty Russell <[email protected]>
Otherwise you can ask for a sub-millisatoshi amount, which is dumb and violates the spec. See-also: lightning/bolts#736 Signed-off-by: Rusty Russell <[email protected]>
Otherwise you can ask for a sub-millisatoshi amount, which is dumb and violates the spec. See-also: lightning/bolts#736 Signed-off-by: Rusty Russell <[email protected]> Changelog-Changed: We now reject invoices which ask for sub-millisatoshi amounts
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.
Thanks for doing this!
It made me realize that eclair is not currently skipping over p
, h
, s
or n
of invalid lengths but instead rejects such invoices. I'll fix that (I'm guessing it's a requirement to allow smooth upgrades of those fields right?).
Correctly implement skipping over hashed tags with invalid length. See lightning/bolts#736.
See-also: lightning/bolts#736 Signed-off-by: Rusty Russell <[email protected]>
Otherwise you can ask for a sub-millisatoshi amount, which is dumb and violates the spec. See-also: lightning/bolts#736 Signed-off-by: Rusty Russell <[email protected]> Changelog-Changed: We now reject invoices which ask for sub-millisatoshi amounts
* Ignore fields with invalid length As per the spec: > A reader: > * MUST skip over unknown fields, OR an f field with unknown version, OR p, h, s or n fields that do NOT have data_lengths of 52, 52, 52 or 53, respectively. * Add more Bolt 11 tests See lightning/bolts#699 and lightning/bolts#736 Co-authored-by: Bastien Teinturier <[email protected]>
@cfromknecht or @Roasbeef, do we have an ACK to merge this? |
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 ⛰
We're going to extend the final case with negative test cases. Signed-off-by: Rusty Russell <[email protected]>
One for uppercase, and one with should-be-ignored fields. The first of these addresses lightning#659 (lightning#677 directly changes the text to make it clear this is allowed, and should also be applied). Signed-off-by: Rusty Russell <[email protected]>
This tests various forms of malformed invoices (it's not exhaustive though). Signed-off-by: Rusty Russell <[email protected]>
We added a requirement on the writer, not the reader. We can't really add a test vector without a new requirement, though. Signed-off-by: Rusty Russell <[email protected]>
…trings. Signed-off-by: Rusty Russell <[email protected]>
1413c35
to
7dc4383
Compare
OK, rebased: had to hoist the new pico-btc test higher, so we could put the new variants underneath the existing ones. |
Merging as per meeting agreement: http://www.erisian.com.au/meetbot/lightning-dev/2020/lightning-dev.2020-05-11-20.08.html |
return error tuple instead of crashing when decoding a ln invoice with sub msat in amount new test vectors are copied from lightning/bolts#736
return error tuple instead of crashing when decoding a ln invoice with sub msat in amount new test vectors are copied from lightning/bolts#736
As per last meeting, this expands the BOLT11 test vectors. I've reproduced them with the c-lightning unit tests.