-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
lnwire: encode channel_update type in onion errors #7665
lnwire: encode channel_update type in onion errors #7665
Conversation
For about a year [1], the spec has prescribed encoding channel_updates with their type prefix (0x0102) in onion failure messages. LND can decode correctly with or without the prefix but hasn't been writing the prefix during encoding. This commit starts writing the prefix. [1] lightning/bolts#979
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 the PR! I think we save the error message on disk, but when decoding it, because we use parseChannelUpdateCompatabilityMode
, so we can always decode the newer format right?
Yes. Onion errors are deserialized from the DB in |
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🙏
@guggero: review reminder |
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 🎉
Going to change the base to 0-18-staging
so we can get it merged.
For about a year, the spec has prescribed encoding
channel_updates
with their type prefix (0x0102
) in onion failure messages. LND can decode correctly with or without the prefix but hasn't been writing the prefix during encoding. This PR starts writing the prefix.I ran across this while running some new encoding/decoding fuzz tests for onion errors, which I intend to submit in a separate PR. Those fuzz tests offer additional regression testing for this change.
Resolves #6461.