-
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: fix decoding for initial zero sid #4391
lnwire: fix decoding for initial zero sid #4391
Conversation
This fixes a decoding error when the list of short channel ids within a QueryShortChanIDs message started with a zero sid. BOLT-0007 specifies that lists of short channel ids should be sorted in ascending order. Previously, this was checked within lnwire by comparing two consecutive sids in the list, starting at the empty (zero) sid. This meant that a list that started with a zero sid couldn't be decoded since the first element would _not_ be greater than the last one (namely: also zero). Given that one can only check for ordering starting at the second element, we add a check to ensure the proper behavior. A unit test is also added to ensure no future regressions on this behavior.
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.
Thank you @matheusd! LGTM 🤘
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.
Changes LGTM as far as sorting goes, but zero channel IDs don't seem to make any sense given that our supported chains have predefined genesis blocks that only include a single coinbase transaction.
@@ -49,6 +49,7 @@ var ( | |||
// TestQueryShortChanIDsUnsorted tests that decoding a QueryShortChanID request | |||
// that contains duplicate or unsorted ids returns an ErrUnsortedSIDs failure. | |||
func TestQueryShortChanIDsUnsorted(t *testing.T) { | |||
|
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.
Nit: extra newline.
Yeah zero short channel ids don't make sense, but the question is where does that check/failure occur. This changes things so that we don't treat it as a decoding error (after all, the sender did encode 0 properly) and let's it fail further upstream in the gossiper when detecting that the genesis coinbase output is not a valid channel. Main difference is that one causes a disconnection while the other doesn't. It is still the case that you can't have an unsorted stream with one element, so the logic is flat out incorrect. |
Build is green, looking like this along with #4370 were indeed the root causes 🎉 |
This fixes a decoding error when the list of short channel ids within a
QueryShortChanIDs message started with a zero sid.
BOLT-0007 specifies that lists of short channel ids should be sorted in
ascending order. Previously, this was checked within lnwire by comparing
two consecutive sids in the list, starting at the empty (zero) sid.
This meant that a list that started with a zero sid couldn't be decoded
since the first element would not be greater than the last one
(namely: also zero).
Given that one can only check for ordering starting at the second
element, we add a check to ensure the proper behavior.
A unit test is also added to ensure no future regressions on this
behavior.
Extracted from #4346