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

Fix payment onion payload decode #1659

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented Aug 9, 2022

Prior to this change, we could have failed to decode a valid payload of size >253. This is because we were decoding the length (a BigSize, big-endian) as a VarInt (little-endian).

Plus some minor cleanups. Pair-debugged with @wpaulino.

TheBlueMatt
TheBlueMatt previously approved these changes Aug 9, 2022
@TheBlueMatt
Copy link
Collaborator

(New) CI failures are due to rust-lang/libc#2866

dunxen
dunxen previously approved these changes Aug 15, 2022
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

ACK 15074bd

@tnull
Copy link
Contributor

tnull commented Aug 15, 2022

LGTM!

Out of curiosity: should this VarInt also be a BigSize? Or am I looking at the wrong place of the spec?

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved
@valentinewallace
Copy link
Contributor Author

LGTM!

Out of curiosity: should this VarInt also be a BigSize? Or am I looking at the wrong place of the spec?

Yup, exactly! That fix was needed for #1652 (and covered by the tests added) so I stuck it in there.

Prior to this change, we could have failed to decode a valid payload of size
>253. This is because we were decoding the length (a BigSize, big-endian) as a
VarInt (little-endian).

Found in lightningdevkit#1652.
As observed by @wpaulino, this struct encodes its bytes as big-endian,
therefore it's a BigSize, not a VarInt.
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

reACK dfbebbf

@tnull
Copy link
Contributor

tnull commented Aug 15, 2022

Yup, exactly! That fix was needed for #1652 (and covered by the tests added) so I stuck it in there.

Ah, thought so!

@valentinewallace valentinewallace merged commit ca4e31d into lightningdevkit:main Aug 15, 2022
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