multi: accept memo note when opening channel #7668

shaurya947 · 2023-05-04T18:07:17Z

Change Description

Fixes #7639 by allowing a note ("memo") to go along with channel creation when using the openchannel CLI/RPC/JSON command. We accept a string with max length of 500 (command is rejected if length is exceeded) and persist it in a hex-encoded format as part of the TLV stream. When channels are queried, we retrieve the value and decode it before passing the info to the caller. We also add a new itest to verify both the happy and unhappy path.

Steps to Test

When using openchannel through CLI/RPC/JSON, pass in a memo note using the memo argument. If length is valid (<= 500), it should persist and be retrievable in listchannels call. If length is invalid, the command should be rejected with an appropriate error message.

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

shaurya947 · 2023-05-05T15:40:43Z

Looks like TestFetchHistoricalChannel is failing inside db_test.go and I'm not quite sure why. It fails when deep-comparing channel with histChannel:

lnd/channeldb/db_test.go

Line 693 in 04314d3

if !reflect.DeepEqual(histChannel, channel) {

The test seems to pass if I attach a Memo here:

lnd/channeldb/channel_test.go

Line 313 in 04314d3

return &OpenChannel{

But that shouldn't be required right? Am I doing something wrong with the TLV stream which messes up (de)serialization when the Memo field is not present?

guggero

Very nice feature and well structured PR, thanks a lot!

rpcserver.go

channeldb/channel.go

lnrpc/lightning.proto

rpcserver.go

itest/list_on_test.go

itest/lnd_open_channel_test.go

shaurya947 · 2023-05-11T18:50:32Z

Hey @guggero I updated the PR to address all your feedback:

Ditch hex encoding
Combine dbchannel changes into a single commit
Combine lnrpc changes into a single commit
Reuse existing itest

Also thanks so much for helping with the unit test failure. Your suggestion seems to have worked 🙂

guggero

Just a couple of more nits, otherwise LGTM 🎉

cmd/lncli/cmd_open_channel.go

itest/lnd_open_channel_test.go

shaurya947 · 2023-05-15T00:57:50Z

Hey @guggero addressed all your feedback as well as fixed a silly mistake I made in the itest (used ht.Alice/ht.Bob instead of the alice and bob arguments)

shaurya947 · 2023-05-17T13:53:33Z

Hey @guggero the windows itest check seems to fail because of a process lock on a file it seems? Is that expected?

Also any suggestions as to whom I could tag for a second review?

guggero · 2023-05-17T14:35:52Z

Hey @guggero the windows itest check seems to fail because of a process lock on a file it seems? Is that expected?

Yeah, unfortunately some itests are still flaky, we are working on that.

Also any suggestions as to whom I could tag for a second review?

Good question. Looking for volunteers, @ffranr, @positiveblue, @sputn1ck, @bitromortac.

positiveblue

LGTM 🚀

This is such a quick win. I can see a lot of usage form external software that opens channels automatically for example.

I think we should make the UX more coherent by also adding this field to the PendingChannels proto message but nothing blocking.

positiveblue · 2023-05-17T17:56:50Z

rpcserver.go

@@ -2169,6 +2169,11 @@ func (r *rpcServer) parseOpenChannelReq(in *lnrpc.OpenChannelRequest,
 			in.CommitmentType)
 	}

+	if len(in.Memo) > 500 {


nit: create a constant for this magic number

nit: consider adding a comment to explain why the length needs to be limited to 500.

positiveblue · 2023-05-17T18:09:38Z

lnrpc/lightning.proto

+    useful information. This is only ever stored locally and in no way impacts
+    the channel's operation.
+    */
+    string memo = 36;


nit: we should add the same field to PendingChannels

Will address in a follow-up PR if that's ok. Want to make sure I can handle all cases around pendingopen/pendingforceclose/pendingwaitingclose properly and not inadvertently cause any more regressions in this PR.

lnrpc/lightning.proto

ffranr

I enjoyed walking through the commits, nice work!

ffranr · 2023-05-17T19:44:08Z

rpcserver.go

@@ -2169,6 +2169,11 @@ func (r *rpcServer) parseOpenChannelReq(in *lnrpc.OpenChannelRequest,
 			in.CommitmentType)
 	}

+	if len(in.Memo) > 500 {


nit: consider adding a comment to explain why the length needs to be limited to 500.

ffranr · 2023-05-17T20:11:40Z

itest/lnd_open_channel_test.go

+	// Bob shouldn't see the memo since it's for Alice only.
+	channel = ht.QueryChannelByChanPoint(bob, chanPoints[0])
+	require.Empty(ht, channel.Memo, "Memo is not empty")


This commit adds the memo field to the protobuf message and generates the new go files.

This commit simply adds a new memo flag to the openchannel command. Memo could be any note-to-self kind of useful information to go along with the channel. The value isn't used yet, will be in the next commit.

shaurya947 · 2023-05-18T17:00:00Z

Thanks for the review @positiveblue and @ffranr ! 🙂

I fixed the one commit message and created a constant for the magic number (along with some comments). I will defer the PendingChannels change for a follow-up PR if that's ok. I've made a branch for it, I just want to make sure I can handle all the different pending states properly and not cause any more inadvertent regressions in this PR.

In this commit we ensure that the string length of the memo field specified as part of the OpenChannelRequest is no longer than 500.

We add byte-array field called Memo to the InitFundingMsg struct. We also provide a value for this from rpcserver.go#parseOpenChannelReq().

We also pass the value of Memo from funding/manager.go.

We add a Memo field to the OpenChannel DB struct. We also persist it using a tlv record. We then pass the Memo value from the InitFundingReserveMsg when creating a new reservation for the channel. Finally, we also read Memo field when fetching channel from DB.

This allows us the memo to be returned in responses such as listchannels.

We test both the happy path (valid memo is returned when querying), as well as the unhappy path (invalid memo rejects the open action). To accomplish this, we update the OpenChannelParams struct inside the harness to accept the Memo.

ffranr

Looks good to me! Detailed commit messages 👍

Note that I haven't tested.

shaurya947 · 2023-05-19T19:13:48Z

Thank you folks! My first PR 🥲

shaurya947 mentioned this pull request May 4, 2023

[feature]: allow user-specified label when opening channels #7639

Closed

Crypt-iQ added rpc Related to the RPC interface channels labels May 4, 2023

guggero reviewed May 8, 2023

View reviewed changes

shaurya947 force-pushed the open-channel-memo branch from cd2bea9 to 6692435 Compare May 11, 2023 18:47

guggero self-requested a review May 11, 2023 20:58

guggero approved these changes May 12, 2023

View reviewed changes

cmd/lncli/cmd_open_channel.go Show resolved Hide resolved

itest/lnd_open_channel_test.go Outdated Show resolved Hide resolved

guggero reviewed May 12, 2023

View reviewed changes

itest/lnd_open_channel_test.go Outdated Show resolved Hide resolved

shaurya947 force-pushed the open-channel-memo branch from 6692435 to a69bad9 Compare May 15, 2023 00:57

guggero requested a review from Crypt-iQ May 15, 2023 08:35

Crypt-iQ removed their request for review May 15, 2023 14:10

ffranr self-requested a review May 17, 2023 14:57

positiveblue approved these changes May 17, 2023

View reviewed changes

ffranr requested changes May 17, 2023

View reviewed changes

lnrpc/lightning.proto Show resolved Hide resolved

ffranr reviewed May 17, 2023

View reviewed changes

shaurya947 added 2 commits May 18, 2023 12:50

lnrpc: add memo field to OpenChannelRequest and ListChannelsResponse

9feb3dd

This commit adds the memo field to the protobuf message and generates the new go files.

lncli: add optional memo flag to openchannel cmd

373e445

This commit simply adds a new memo flag to the openchannel command. Memo could be any note-to-self kind of useful information to go along with the channel. The value isn't used yet, will be in the next commit.

shaurya947 force-pushed the open-channel-memo branch from a69bad9 to ce0d924 Compare May 18, 2023 16:56

shaurya947 added 6 commits May 18, 2023 13:02

rpcserver: enforce memo length is less than 500 characters

1724409

In this commit we ensure that the string length of the memo field specified as part of the OpenChannelRequest is no longer than 500.

funding: add Memo to InitFundingMsg struct

7c6faa4

We add byte-array field called Memo to the InitFundingMsg struct. We also provide a value for this from rpcserver.go#parseOpenChannelReq().

lnwallet: add Memo to InitFundingReserveMsg

db44924

We also pass the value of Memo from funding/manager.go.

rpcserver: attach channel Memo to *lnrpc.Channel struct

c8e4d8e

This allows us the memo to be returned in responses such as listchannels.

docs: update release notes for 0.17.0

be82091

shaurya947 force-pushed the open-channel-memo branch from ce0d924 to be82091 Compare May 18, 2023 17:02

guggero requested a review from ffranr May 19, 2023 07:30

ffranr approved these changes May 19, 2023

View reviewed changes

guggero merged commit ac3e7be into lightningnetwork:master May 19, 2023
20 of 24 checks passed

shaurya947 deleted the open-channel-memo branch May 24, 2023 21:58

shaurya947 mentioned this pull request May 25, 2023

lnrpc+itest: return channel Memo for Pending channels #7728

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: accept memo note when opening channel #7668

multi: accept memo note when opening channel #7668

shaurya947 commented May 4, 2023 •

edited by Crypt-iQ

shaurya947 commented May 5, 2023 •

edited

guggero left a comment

shaurya947 commented May 11, 2023

guggero left a comment

shaurya947 commented May 15, 2023

shaurya947 commented May 17, 2023

guggero commented May 17, 2023

positiveblue left a comment

positiveblue May 17, 2023

ffranr May 17, 2023

positiveblue May 17, 2023

shaurya947 May 18, 2023

ffranr left a comment

ffranr May 17, 2023

ffranr May 17, 2023

shaurya947 commented May 18, 2023

ffranr left a comment

shaurya947 commented May 19, 2023

multi: accept memo note when opening channel #7668

multi: accept memo note when opening channel #7668

Conversation

shaurya947 commented May 4, 2023 • edited by Crypt-iQ

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

shaurya947 commented May 5, 2023 • edited

guggero left a comment

Choose a reason for hiding this comment

shaurya947 commented May 11, 2023

guggero left a comment

Choose a reason for hiding this comment

shaurya947 commented May 15, 2023

shaurya947 commented May 17, 2023

guggero commented May 17, 2023

positiveblue left a comment

Choose a reason for hiding this comment

positiveblue May 17, 2023

Choose a reason for hiding this comment

ffranr May 17, 2023

Choose a reason for hiding this comment

positiveblue May 17, 2023

Choose a reason for hiding this comment

shaurya947 May 18, 2023

Choose a reason for hiding this comment

ffranr left a comment

Choose a reason for hiding this comment

ffranr May 17, 2023

Choose a reason for hiding this comment

ffranr May 17, 2023

Choose a reason for hiding this comment

shaurya947 commented May 18, 2023

ffranr left a comment

Choose a reason for hiding this comment

shaurya947 commented May 19, 2023

shaurya947 commented May 4, 2023 •

edited by Crypt-iQ

shaurya947 commented May 5, 2023 •

edited