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

That time Pieter Wuille tested us all and we failed him #2061

Merged
merged 5 commits into from Nov 2, 2018

Conversation

Projects
None yet
3 participants
@rustyrussell
Copy link
Contributor

rustyrussell commented Oct 31, 2018

Fixes #2063 as a side-effect.

rustyrussell added some commits Oct 31, 2018

bech32: fix overflow bug.
Copied from upstream:

 sipa/bech32@2b0aac6

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
common/bolt11: test for invoices without hrp separator.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Revert "bolt11: better message when you try to 'lightning-cli pay' an…
… onchain addr."

This reverts commit 6af8f29.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
bolt11: make sure we handle uppercase invoices.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

hrp = tal_arr(tmpctx, char, strlen(str));
data = tal_arr(tmpctx, u5, strlen(str));
hrp = tal_arr(tmpctx, char, strlen(str) - 6);

This comment has been minimized.

@niftynei

niftynei Oct 31, 2018

Collaborator

i realize this is a revert, so maybe this is already taken care of, but if not can you add some context about why these offsets are necessary, maybe as a code comment?

This comment has been minimized.

@rustyrussell

rustyrussell Nov 2, 2018

Contributor

It's actually documented in the bech32 reference code header to have these requirements.

The reason: bech32 includes a 30-bit (6char) checksum, and a 1 between hrp and data, meaning hrp must be 7 chars shorter. Plus one for NUL terminator, so -6.

The data is not NUL-terminated, and the code insists that hrp be at least one letter long, so that's -8.

But you made me re-read the code and write some test cases to be sure!

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 1, 2018

ACK 7e73ea7

CHANGELOG: note that we fixed uppercase invoices.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell merged commit 0c545d0 into ElementsProject:master Nov 2, 2018

1 of 2 checks passed

ackbot Need at least 1 ACKs
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment