-
Notifications
You must be signed in to change notification settings - Fork 2.2k
graph+lnwire: start validating that extra lnwire msg bytes are valid TLV #9787
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
graph+lnwire: start validating that extra lnwire msg bytes are valid TLV #9787
Conversation
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Pull Request Overview
This PR introduces TLV blob validation during decoding and fixes the MilliSatoshi TLV record type size calculation. Key changes include:
- Updating the TLV size parameter for MilliSatoshi in lnwire/msat.go and its migration counterpart.
- Adding TLV stream validation to ExtraOpaqueData decoding in lnwire/extra_bytes.go.
- Adjusting test inputs in graph/db/graph_test.go to provide valid TLV byte sequences.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
lnwire/msat.go | Uses an explicit uint64 conversion for correct TLV size calculation. |
lnwire/extra_bytes.go | Adds TLV stream decoding and validation to ensure extra data integrity. |
graph/db/graph_test.go | Updates byte literal values for extra opaque data to simulate valid TLV data. |
channeldb/migration/lnwire21/msat.go | Mirrors the TLV size fix applied in lnwire/msat.go. |
Comments suppressed due to low confidence (2)
lnwire/extra_bytes.go:71
- [nitpick] The formatted error message now includes the invalid TLV bytes, which is helpful; consider adding further context or logging in cases where malformed TLV data is encountered to aid troubleshooting.
return fmt.Errorf("invalid TLV stream: %w: %v", err, *e)
graph/db/graph_test.go:113
- [nitpick] The test now uses a byte literal for ExtraOpaqueData that simulates a valid TLV stream; ensure that additional tests are added to cover both well-formed and malformed TLV cases to fully validate the new decoding logic.
ExtraOpaqueData: []byte{1, 1, 1, 2, 2, 2, 2},
bce1d82
to
2897f6e
Compare
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.
I think we should de-risk the TLV validation a bit, see inline comment. Other than that looks good to me.
4e9f7a9
to
3caa1f9
Compare
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 🎉
3caa1f9
to
dd5bddc
Compare
with the change in #9789, we can just merge that in 19 and hold off on the rest of the diff here until 20 |
dd5bddc
to
aa754b2
Compare
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-pending dependencies.
Later when we introduce our SQL version of the graph store, we will normalise the persistence of the ExtraOpaqueData using the fact that it is always made up of TLV entries. So we update our tests here to ensure that they use valid TLV streams as examples.
In this commit, we check that the extra bytes appended to gossip messages contain valid TLV streams. We do this here for: - channel_announcement - channel_announcement_2 - channel_update - channel_update_2 - node_announcement This is in preparation for the SQL version of the graph store which will normalise TLV streams at persistence time.
aa754b2
to
e6e6d72
Compare
8dec9cb
into
lightningnetwork:elle-graph
In this PR, we start validating that any gossip message we decode has a valid TLV stream.
This is in preparation for creating a SQL version of the graph store in which we will store the TLV
records in a normalised fashion instead of in a flat structure as is done today.
part of #9795