routing+migration32: update migration 32 to use pure TLV encoding for mission control results by ellemouton · Pull Request #9167 · lightningnetwork/lnd

ellemouton · 2024-10-08T11:47:54Z

In this commit, we update an existing migration which at the time of
writing has not been included in a release. We update it so that it
converts the format used for MissionControl result encoding to use pure
TLV instead. The 3 structs that have been updated are: mcHop,
mcRoute and paymentResult.

coderabbitai · 2024-10-08T11:48: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.

guggero

tACK, LGTM 🎉

Just a couple of nits and suggestions.

bitromortac

Looks great ⚡, will do some more tests.

bitromortac · 2024-10-23T08:17:58Z

+}
+
+// Record returns a TLV record that can be used to encode/decode a list of
+// mcRoute to/from a TLV stream.


s/list of mcRoute/failureMessage/

nit: a list of is still present, same for Record of mcRoute (in the migration as well)

ellemouton

Thanks yall, updated

ellemouton · 2024-10-23T10:12:40Z

+}
+
+// Record returns a TLV record that can be used to encode/decode a list of
+// mcRoute to/from a TLV stream.


bitromortac

Very close 🚀, just a question if we want to also remove the success bool, which may be worth it because we do a migration.

I tested the migration as well and tested what happens if one runs with the old migration in master and then updates to the new migration, which will probably lead to an error like:

[ERR] LTND lnd.go:159: Shutting down because error in main method: unable to create server: can't create mission control manager: tlv stream is not canonical. This would require a manual deletion of the results bucket or the insertion of a temporary purge call at in initMissionControl.

bitromortac · 2024-10-25T07:20:14Z

-		sourcePubKey: route.SourcePubKey,
-		totalAmount:  route.TotalAmount,
-		hops:         extractMCHops(route.Hops),
+		sourcePubKey: tlv.NewRecordT[tlv.TlvType0, route.Vertex](r.SourcePubKey),


nit: there are still a few places left in this commit where some parameters can be removed, here and extractMCHop as well as in migration32/mission_control_store.go, migration_test.go

thanks - struggling to get my editor to do the thing....

bitromortac · 2024-10-25T07:44:26Z

 // contains information required to update mission control.
-func interpretResult(rt *mcRoute, success bool, failureSrcIdx *int,
-	failure lnwire.FailureMessage) *interpretedResult {
+func interpretResult(rt *mcRoute, success bool,


do you think it makes sense to get rid of the success bool as well? We could interpret the non-existence of a failure (index) as a success. Since we can remove unknownFailureSourceIdx it seems like that one was never used as well

hmmm I dont think they are duplicates/redundancies. There are times when we want to report a failure but we don't know the index of the failure. See this call where we are reporting a failure but we set the source index and failure messages to nil

right, but in the end we only have successes or failures, so we could combine the failure index and failure message into an optional TLV sub-struct whose presence encodes if it was a failure? but non-blocking, as it's not a huge gain in savings, but it would enforce that we can't have a success and failures set in the same result

ah! good point - cool will update 👍

bitromortac · 2024-10-25T08:46:19Z

+}
+
+// Record returns a TLV record that can be used to encode/decode a list of
+// mcRoute to/from a TLV stream.


nit: a list of is still present, same for Record of mcRoute (in the migration as well)

bitromortac · 2024-10-25T08:52:00Z

+
+func encodeMCRoute(w io.Writer, val interface{}, _ *[8]byte) error {
+	if v, ok := val.(*mcRoute); ok {
+		return serializeRoute(w, v)


nit: serializeRoute and deserializeRoute could be inlined, as those are not reused and small, to be in a single place (please ignore if too nitty)

cool yeah think im gonna leave this one just cause of the mismatch in signatures. So would need to put the full serialisation and deserialisation code here which makes it harder to read given the TLV specific wrapping that happens here

ellemouton · 2024-10-27T05:14:37Z

Thanks @bitromortac

I tested the migration as well and tested what happens if one runs with the old migration in master and then updates to the new migration, which will probably lead to an error like:

Yes we dont support things if they were running on latest master. If they were, they can do a clear-mc call manually before starting again with this version. If we were supporting on latest master, then we would have added this as a new migration instead.

ellemouton

thanks @bitromortac - updated!

ellemouton · 2024-10-28T08:03:16Z

-		sourcePubKey: route.SourcePubKey,
-		totalAmount:  route.TotalAmount,
-		hops:         extractMCHops(route.Hops),
+		sourcePubKey: tlv.NewRecordT[tlv.TlvType0, route.Vertex](r.SourcePubKey),


thanks - struggling to get my editor to do the thing....

ellemouton · 2024-10-28T08:04:23Z

+
+func encodeMCRoute(w io.Writer, val interface{}, _ *[8]byte) error {
+	if v, ok := val.(*mcRoute); ok {
+		return serializeRoute(w, v)


cool yeah think im gonna leave this one just cause of the mismatch in signatures. So would need to put the full serialisation and deserialisation code here which makes it harder to read given the TLV specific wrapping that happens here

ellemouton · 2024-10-28T08:07:37Z

 // contains information required to update mission control.
-func interpretResult(rt *mcRoute, success bool, failureSrcIdx *int,
-	failure lnwire.FailureMessage) *interpretedResult {
+func interpretResult(rt *mcRoute, success bool,


hmmm I dont think they are duplicates/redundancies. There are times when we want to report a failure but we don't know the index of the failure. See this call where we are reporting a failure but we set the source index and failure messages to nil

bitromortac

Yes we dont support things if they were running on latest master. If they were, they can do a clear-mc call manually before starting again with this version. If we were supporting on latest master, then we would have added this as a new migration instead.

Right, just in case anybody updates without knowing about the migrations beforehand, it's possible to get stuck, because one can't resetmc then due to the rpc server not being available, just wanted to have an answer for that case.

LGTM ⚡ (pending some clarification on a suggestion)

bitromortac · 2024-10-29T09:19:27Z

 // contains information required to update mission control.
-func interpretResult(rt *mcRoute, success bool, failureSrcIdx *int,
-	failure lnwire.FailureMessage) *interpretedResult {
+func interpretResult(rt *mcRoute, success bool,


right, but in the end we only have successes or failures, so we could combine the failure index and failure message into an optional TLV sub-struct whose presence encodes if it was a failure? but non-blocking, as it's not a huge gain in savings, but it would enforce that we can't have a success and failures set in the same result

ellemouton · 2024-10-30T12:54:27Z

Right, just in case anybody updates without knowing about the migrations beforehand, it's possible to get stuck, because one can't resetmc then due to the rpc server not being available, just wanted to have an answer for that case.

Yeah, I think in that case they need to manually drop the bucket (kvdb) or write a cascading delete (sql). Let's defs confirm with @Roasbeef though.

I just think that if we do want to support users in this state, then we should have gone with adding a separate migration all together. If we made the decision about editing an existing migration, then to me that means not supporting this state.

ellemouton · 2024-10-30T12:55:27Z

@bitromortac - in the mean time, I've updated the PR to use the implicit success/fail struct you suggested - mind having a look at the new structure? Once I get the ACK from you, then I'll ask Oli to also take another look given the structure is quite different now

bitromortac

The latest changes look good 🙏, leaving some comments, but will also do another round of testing.

Add the TrueBoolean type along with its Record method. Also update the Millisatoshi type with a Record method. Both of these will be used in an upcoming commit which adjusts a mission control migration to use pure TLV types.

So that we can use it in TLV encoding. Also add this to the codec for channeldb migration 32 since we will be using it there in an upcoming adjustment commit.

guggero

re-tACK, LGTM 🎉

guggero · 2024-11-01T08:20:25Z

-	result2.timeReply = result1.timeReply.Add(time.Hour)
-	result2.timeFwd = result1.timeReply.Add(time.Hour)
-	result2.id = 2
+	result2 := newPaymentResult(


Can we also add a test with a failure that has no details? Just to show that works too (didn't see one, but perhaps I didn't look hard enough).

indeed - good idea! added

In this commit, we update an existing migration which at the time of writing has not been included in a release. We update it so that it converts the format used for MissionControl result encoding to use pure TLV instead. The 3 structs that have been updated are: `mcHop`, `mcRoute` and `paymentResult`.

ellemouton force-pushed the mcEncodingToTLV branch 3 times, most recently from 9057d4e to b666a60 Compare October 8, 2024 13:49

ellemouton requested a review from guggero October 8, 2024 13:50

ellemouton added the blinded paths label Oct 8, 2024

ellemouton requested a review from Roasbeef October 8, 2024 13:50

ellemouton added this to the v0.19.0 milestone Oct 8, 2024

ellemouton self-assigned this Oct 8, 2024

saubyk requested a review from bitromortac October 15, 2024 16:40

ellemouton removed the request for review from Roasbeef October 15, 2024 19:31

guggero approved these changes Oct 18, 2024

View reviewed changes

bitromortac reviewed Oct 23, 2024

View reviewed changes

ellemouton force-pushed the mcEncodingToTLV branch 2 times, most recently from 9154415 to 4d8aa29 Compare October 23, 2024 10:24

ellemouton commented Oct 23, 2024

View reviewed changes

ellemouton force-pushed the mcEncodingToTLV branch from 4d8aa29 to bc491a6 Compare October 23, 2024 10:25

ellemouton requested a review from bitromortac October 23, 2024 13:02

ellemouton force-pushed the mcEncodingToTLV branch from bc491a6 to 1ecd942 Compare October 24, 2024 06:43

bitromortac reviewed Oct 25, 2024

View reviewed changes

ellemouton force-pushed the mcEncodingToTLV branch 2 times, most recently from ca16b85 to 44950d0 Compare October 28, 2024 08:10

ellemouton commented Oct 28, 2024

View reviewed changes

ellemouton requested a review from bitromortac October 28, 2024 09:56

bitromortac approved these changes Oct 29, 2024

View reviewed changes

ellemouton force-pushed the mcEncodingToTLV branch 2 times, most recently from d9ebbd5 to 01d8285 Compare October 30, 2024 12:53

ellemouton force-pushed the mcEncodingToTLV branch from 01d8285 to 68de584 Compare October 30, 2024 13:03

bitromortac reviewed Oct 31, 2024

View reviewed changes

Comment thread routing/missioncontrol.go

Comment thread routing/result_interpretation.go Outdated

ellemouton added 2 commits November 1, 2024 08:59

lnwire21: update Msat and TrueBoolean

a5f3fa1

Add the TrueBoolean type along with its Record method. Also update the Millisatoshi type with a Record method. Both of these will be used in an upcoming commit which adjusts a mission control migration to use pure TLV types.

routing+channeldb: let Vertex implement tlv.RecordProducer

d9a073a

So that we can use it in TLV encoding. Also add this to the codec for channeldb migration 32 since we will be using it there in an upcoming adjustment commit.

ellemouton force-pushed the mcEncodingToTLV branch from 68de584 to 11fc268 Compare November 1, 2024 07:00

guggero approved these changes Nov 1, 2024

View reviewed changes

ellemouton added 2 commits November 1, 2024 12:28

docs: update release notes

30e0d6c

ellemouton force-pushed the mcEncodingToTLV branch from 11fc268 to 30e0d6c Compare November 1, 2024 10:28

guggero merged commit 22ae3e5 into lightningnetwork:master Nov 1, 2024

ellemouton deleted the mcEncodingToTLV branch November 1, 2024 12:16

bitromortac mentioned this pull request Apr 28, 2025

[bug]: mission control migration fails for nil payment failure message #9769

Closed

Conversation

ellemouton commented Oct 8, 2024

Uh oh!

coderabbitai Bot commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton commented Oct 27, 2024

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

coderabbitai Bot commented Oct 8, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)