routing+channeldb: use a more minimal encoding for MC routes #8911

ellemouton · 2024-07-15T10:59:00Z

Before this commit, Mission Control would serialise the entire route.Route for each mission control entry. However, MC only uses a fraction of the info in route.Route. Continuing to persist the entire encoding may become more constly once we start sending to more blinded routes because then this encoding will also include the encrypted cipher text sent to each hop on the blinded path.
So this commit migrates the MC store to only persist the data that the MC actually uses.

coderabbitai · 2024-07-15T10:59:06Z

Important

Review skipped

Auto reviews are limited to specific labels.

🏷️ Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

bhandras · 2024-07-15T12:12:44Z

Concept ACK, great idea to remove unused data and simplify the encoding! 🚀

I think in the end the choice of encoding (whether to use TLV or not) is not super important as eventually we should fully migrate to SQL.

channeldb/migration32/mission_control_store.go

guggero

Did a first pass. Looks good to me, nice work on the commit structure (as always 😉 ). Might need some updates after recently merged PRs, but other than that this is pretty close.

guggero · 2024-09-09T08:33:49Z

channeldb/migration32/route.go

+// first hop in the path. A route is only selected as valid if all the channels
+// have sufficient capacity to carry the initial payment amount after fees are
+// accounted for.
+type Route struct {


Oh, I think I just made this a bit more complicated by merging #9049, which added a new field to this struct, including new encoding/decoding logic that probably needs to be copied (lnwire/custom_records.go). Although no existing records should ever have that data. But if this PR only goes in with 0.19, there could be such records from 0.18.4 to 0.19.

This is the relevant commit btw: 4804cbf

thanks for pointing this out 🙏

I think we should still be good here though if im not mistaken since the new records would never have been encoded in the mission control store right? since that uses SerialiseRoute directly which does not use the new fields.

but I'll add the new fields to the struct for completeness sake 👍

channeldb/migration32/mission_control_store.go

routing/result_interpretation.go

ellemouton

Thanks @guggero 🙏

Updated with the hasCustomRecords suggestion & also added the new fields to Route

ellemouton · 2024-09-09T09:27:59Z

channeldb/migration32/route.go

+// first hop in the path. A route is only selected as valid if all the channels
+// have sufficient capacity to carry the initial payment amount after fees are
+// accounted for.
+type Route struct {


thanks for pointing this out 🙏

I think we should still be good here though if im not mistaken since the new records would never have been encoded in the mission control store right? since that uses SerialiseRoute directly which does not use the new fields.

but I'll add the new fields to the struct for completeness sake 👍

channeldb/migration32/mission_control_store.go

guggero

LGTM and tACK (on a copy of my mainnet node's MC history) 🎉

channeldb/migration/lnwire21/onion_error.go

channeldb/migration32/route.go

ellemouton

shweeeeet - updated! thanks @guggero

channeldb/migration32/route.go

bitromortac

Nice work 🎉! I'm curious how much this saves us, I'll also run a test migration next.

If we would like to add more data again in the future, how would that look like? Would adding a tlv stream make sense for extensibility?

channeldb/migration/lnwire21/onion_error.go

routing/result_interpretation.go

routing/missioncontrol_store.go

routing/missioncontrol_store_test.go

channeldb/migration32/migration.go

lightninglabs-deploy · 2024-09-26T10:40:42Z

@ellemouton, remember to re-request review from reviewers when ready

ellemouton · 2024-09-30T06:51:37Z

I'm curious how much this saves us

It will save us a lot especially once route blinding is more frequently used since currently, that would mean storing the encrypted data for a blinded route for each mission control entry

If we would like to add more data again in the future, how would that look like? Would adding a tlv stream make sense for extensibility?

Yeah i had the same thought before & chatted to oli about this but we decided this is fine for now especially given the shift to SQL land. If we do need to add more fields here before we get to sql land, then we can just do a migration at the time to shift to TLV form.

To prevent the need to copy the entire onion_error.go file for a new Mission Control migration, this commit just updates the existing lnwire21/onion_error.go file with the new CodeInvalidBlinding code. The lnwire21 should not really ever be updated but adding a new code should be fine as it does not affect old migrations since this is a new code.

We add the new custom records encoding/decoding logic to the "frozen" lnwire21 package. We can do this because nothing uses this logic yet. If the custom records logic changes, the changes should _not_ be added to the lnwire21 version.

ellemouton

Thanks for the review @bitromortac 🙏
Ready for final round

routing/missioncontrol_store_test.go

ellemouton · 2024-09-30T07:08:26Z

The other thing I forgot to mention above re the question regarding "how much are we actually saving here":

The main reason we are doing this PR is so that we dont need all the currently needed Route info in order to construct an MC entry - this is so that we can create MC entries on Route blinding receives as is done in #8991
which would not be possible given the current structure :)

bitromortac

LGTM 🎉, I tested the migration as well

channeldb/migration32/hop.go

In preparation for the commit which will add the main logic for migration 32 (which will migrate the MC store to use a more minimal encoding), this commit just adds some of the code that the migration will need to the package.

Add a new mcRoute type that houses the data about a route that MC actually uses. Then add a migration (channeldb/migration32) that migrates the existing store from its current serialisation to the new, more minimal serialisation.

ellemouton marked this pull request as draft July 15, 2024 10:59

bitromortac self-requested a review July 19, 2024 10:21

ellemouton force-pushed the reduceMCRouteEncoding branch 5 times, most recently from 2268ccb to b37d014 Compare July 19, 2024 11:09

ellemouton force-pushed the reduceMCRouteEncoding branch from b37d014 to f30310c Compare August 6, 2024 12:36

ellemouton marked this pull request as ready for review August 6, 2024 12:36

ellemouton marked this pull request as draft August 6, 2024 17:12

ellemouton mentioned this pull request Aug 8, 2024

Route Blinding: Report successful receives to Mission Control #8991

Open

ellemouton force-pushed the reduceMCRouteEncoding branch 2 times, most recently from 1c44d76 to 43f787e Compare August 8, 2024 06:14

ellemouton marked this pull request as ready for review August 8, 2024 06:14

ellemouton force-pushed the reduceMCRouteEncoding branch 2 times, most recently from 4f0c2eb to 1c0bba2 Compare August 13, 2024 14:22

ellemouton mentioned this pull request Aug 13, 2024

routing: Namespaced Mission Control #9001

Merged

ellemouton commented Aug 14, 2024

View reviewed changes

channeldb/migration32/mission_control_store.go Show resolved Hide resolved

ellemouton mentioned this pull request Aug 14, 2024

route blinding [4/4]: update MC on receive from blinded path #9004

Open

ellemouton added mission control migration labels Aug 15, 2024

ellemouton added the size/kilo medium, proper context needed, less than 1000 lines label Aug 22, 2024

ellemouton force-pushed the reduceMCRouteEncoding branch from 1c0bba2 to 3cf0d5f Compare September 9, 2024 08:17

guggero reviewed Sep 9, 2024

View reviewed changes

ellemouton force-pushed the reduceMCRouteEncoding branch from 3cf0d5f to 9d45dc9 Compare September 9, 2024 09:37

ellemouton commented Sep 9, 2024

View reviewed changes

guggero self-requested a review September 9, 2024 09:54

guggero approved these changes Sep 16, 2024

View reviewed changes

channeldb/migration/lnwire21/onion_error.go Outdated Show resolved Hide resolved

channeldb/migration32/route.go Outdated Show resolved Hide resolved

ellemouton force-pushed the reduceMCRouteEncoding branch from 9d45dc9 to 74b6af9 Compare September 16, 2024 11:03

ellemouton commented Sep 16, 2024

View reviewed changes

channeldb/migration32/route.go Outdated Show resolved Hide resolved

saubyk assigned ellemouton Sep 17, 2024

bitromortac reviewed Sep 19, 2024

View reviewed changes

ellemouton added 2 commits September 30, 2024 08:58

lnwire21: add custom records parsing

82ae022

We add the new custom records encoding/decoding logic to the "frozen" lnwire21 package. We can do this because nothing uses this logic yet. If the custom records logic changes, the changes should _not_ be added to the lnwire21 version.

ellemouton force-pushed the reduceMCRouteEncoding branch from 74b6af9 to 5c8c5e0 Compare September 30, 2024 07:06

ellemouton commented Sep 30, 2024

View reviewed changes

routing/missioncontrol_store_test.go Show resolved Hide resolved

bitromortac approved these changes Sep 30, 2024

View reviewed changes

channeldb/migration32/hop.go Outdated Show resolved Hide resolved

ellemouton added 2 commits September 30, 2024 11:40

channeldb: boiler plate code for migration32

96445f9

In preparation for the commit which will add the main logic for migration 32 (which will migrate the MC store to use a more minimal encoding), this commit just adds some of the code that the migration will need to the package.

ellemouton force-pushed the reduceMCRouteEncoding branch from 5c8c5e0 to eb6c552 Compare September 30, 2024 09:40

docs: add release notes entry

34303e7

ellemouton force-pushed the reduceMCRouteEncoding branch from eb6c552 to 34303e7 Compare October 1, 2024 08:53

ellemouton merged commit 75eaaf7 into lightningnetwork:master Oct 1, 2024
27 of 33 checks passed

ellemouton deleted the reduceMCRouteEncoding branch October 1, 2024 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

routing+channeldb: use a more minimal encoding for MC routes #8911

routing+channeldb: use a more minimal encoding for MC routes #8911

ellemouton commented Jul 15, 2024 •

edited

Loading

coderabbitai bot commented Jul 15, 2024 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

bhandras commented Jul 15, 2024 •

edited

Loading

guggero left a comment

guggero Sep 9, 2024

guggero Sep 9, 2024

ellemouton Sep 9, 2024

ellemouton left a comment •

edited

Loading

ellemouton Sep 9, 2024

guggero left a comment

ellemouton left a comment

bitromortac left a comment •

edited

Loading

lightninglabs-deploy commented Sep 26, 2024

ellemouton commented Sep 30, 2024

ellemouton left a comment

ellemouton commented Sep 30, 2024

bitromortac left a comment

routing+channeldb: use a more minimal encoding for MC routes #8911

routing+channeldb: use a more minimal encoding for MC routes #8911

Conversation

ellemouton commented Jul 15, 2024 • edited Loading

coderabbitai bot commented Jul 15, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

bhandras commented Jul 15, 2024 • edited Loading

guggero left a comment

Choose a reason for hiding this comment

guggero Sep 9, 2024

Choose a reason for hiding this comment

guggero Sep 9, 2024

Choose a reason for hiding this comment

ellemouton Sep 9, 2024

Choose a reason for hiding this comment

ellemouton left a comment • edited Loading

Choose a reason for hiding this comment

ellemouton Sep 9, 2024

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

bitromortac left a comment • edited Loading

Choose a reason for hiding this comment

lightninglabs-deploy commented Sep 26, 2024

ellemouton commented Sep 30, 2024

ellemouton left a comment

Choose a reason for hiding this comment

ellemouton commented Sep 30, 2024

bitromortac left a comment

Choose a reason for hiding this comment

ellemouton commented Jul 15, 2024 •

edited

Loading

coderabbitai bot commented Jul 15, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

bhandras commented Jul 15, 2024 •

edited

Loading

ellemouton left a comment •

edited

Loading

bitromortac left a comment •

edited

Loading