lnrpc: Add UpdateNodeAnnouncement endpoint #5587

positiveblue · 2021-07-29T18:53:28Z

Allow users to change nodeAnnouncement values such Color, Alias, Features or
Addresses and broadcast the update to its peers without having to
restart the node.

Fixes #5466

positiveblue

Also, should I add release notes to 0.13.x or to 0.14.x?

positiveblue · 2021-07-29T18:59:01Z

lnrpc/lightning.proto

@@ -3711,6 +3718,22 @@ message ListPermissionsResponse {
    map<string, MacaroonPermissionList> method_permissions = 1;
 }

+message NodeAnnouncementUpdateRequest {


features and addresses have the problem that they are sets. I do not have the full context about what kind of features are out there but we need to decided if the request will directly substitute the current features with the ones in the request or if we need a mechanism to perform join/add/subtract of single features

we need to decided if the request will directly substitute the current features with the ones in the request or if we need a mechanism to perform join/add/subtract of single features

Yeah this is what I was hinting at re knowing which fields are actually being updated. Seems we could try to handle the two (?) cases explicitly. So in one mode, it's a wholesale replacement of w/e feature bits we already have advertised. In the other case, a user can specify that it wants a particular bit to be added or removed.

One possible way to do this would be to have something like an "action" defined for each entry, so something like:

enum FeatureBitAction { ADD = 0; REMOVE = 1; } map<FeatureBit, FeatureBitActiion> FeatureBitUpdate = 1;

This way we don't force the caller to fully specify all the existing feature bits, instead they say if they want a bit to be removed or added.

positiveblue · 2021-07-29T19:01:27Z

lnrpc/lightning.proto

+    string alias = 3;
+
+    // Addresses is a slice of all the node's known addresses.
+    repeated NodeAddress addresses = 4;


Similar problem that the one with features. Should we simply substitute the current node addresses with the ones that are provided in the request or we need to keep the ones in the conf file for example.
Also, should we use the default port for those addresses that are only the IP, or we should not support not providing it

I think the same "action" scheme I outlined above could work here, so turning it into a map of actions.

positiveblue · 2021-07-29T19:02:15Z

lnrpc/lightning.proto

+    // Addresses is a slice of all the node's known addresses.
+    repeated NodeAddress addresses = 4;
+}
+message NodeAnnouncementUpdateResponse {


Not sure if it is worth to return all the data that will be broadcasted to other nodes

Could be useful to just send what was actually updated? So like if you try to add a new address, but it's already advertised, then you get back an empty response telling you it was effectively a noop.

Related to this response handling, is that we likely want to return a gRPC status/error code, to indicate if a user tries to do something like a set a feature bit that we actually don't understand. On the other-hand, we could allow such behavior as it may be useful when paired with something like: #5346

lnrpc/lightning.yaml

positiveblue · 2021-07-29T19:04:11Z

lntest/itest/lnd_channel_graph_test.go

+		t.Fatalf("faild to validate an invalid alias for an update node announcement request: %v", err)
+	}
+
+	// TODO(positiveblue): Add checks for features and addresses


There is still not validation for these two parameters in the server implementation

So for feature here, we'll need to make a decision (for the validation logic), if we want to allow them to advertise feature bits unknown to us or not.

For addresses, as is we rely on net ResolveTCPAddr (or the onion counter part) to make sure they're actually a valid address.

Roasbeef · 2021-07-30T20:28:49Z

Also, should I add release notes to 0.13.x or to 0.14.x?

0.14.1!

Roasbeef

Solid first time contribution! Did an initial pass over the diff, and I think the main thing that needs to be sorted out is how we handle partial updates to the set of advertised feature bits and/or addresses. See my comments inline for some suggestions.

lnrpc/lightning.proto

Roasbeef · 2021-07-30T20:35:23Z

lnrpc/lightning.proto

@@ -3711,6 +3718,22 @@ message ListPermissionsResponse {
    map<string, MacaroonPermissionList> method_permissions = 1;
 }

+message NodeAnnouncementUpdateRequest {


we need to decided if the request will directly substitute the current features with the ones in the request or if we need a mechanism to perform join/add/subtract of single features

Yeah this is what I was hinting at re knowing which fields are actually being updated. Seems we could try to handle the two (?) cases explicitly. So in one mode, it's a wholesale replacement of w/e feature bits we already have advertised. In the other case, a user can specify that it wants a particular bit to be added or removed.

One possible way to do this would be to have something like an "action" defined for each entry, so something like:

enum FeatureBitAction { ADD = 0; REMOVE = 1; } map<FeatureBit, FeatureBitActiion> FeatureBitUpdate = 1;

This way we don't force the caller to fully specify all the existing feature bits, instead they say if they want a bit to be removed or added.

Roasbeef · 2021-07-30T20:35:42Z

lnrpc/lightning.proto

+    string alias = 3;
+
+    // Addresses is a slice of all the node's known addresses.
+    repeated NodeAddress addresses = 4;


I think the same "action" scheme I outlined above could work here, so turning it into a map of actions.

Roasbeef · 2021-07-30T20:37:46Z

lnrpc/lightning.proto

+    // Addresses is a slice of all the node's known addresses.
+    repeated NodeAddress addresses = 4;
+}
+message NodeAnnouncementUpdateResponse {


Could be useful to just send what was actually updated? So like if you try to add a new address, but it's already advertised, then you get back an empty response telling you it was effectively a noop.

Related to this response handling, is that we likely want to return a gRPC status/error code, to indicate if a user tries to do something like a set a feature bit that we actually don't understand. On the other-hand, we could allow such behavior as it may be useful when paired with something like: #5346

rpcserver.go

Roasbeef · 2021-07-30T20:44:17Z

lntest/itest/lnd_channel_graph_test.go

+		t.Fatalf("faild to validate an invalid alias for an update node announcement request: %v", err)
+	}
+
+	// TODO(positiveblue): Add checks for features and addresses


So for feature here, we'll need to make a decision (for the validation logic), if we want to allow them to advertise feature bits unknown to us or not.

For addresses, as is we rely on net ResolveTCPAddr (or the onion counter part) to make sure they're actually a valid address.

lntest/itest/lnd_channel_graph_test.go

positiveblue · 2021-07-30T21:17:51Z

Thank you so much for the feedback @Roasbeef I think it is enough for the next iteration, I will ping you after applying the changes

guggero

Gave this PR a quick first-pass review, mostly to answer some of your questions.
Nice work so far, definitely looks like you're on the right track. Looking forward to the next iteration.

lnrpc/lightning.proto

lnrpc/lightning.yaml

rpcserver.go

lntest/itest/lnd_channel_graph_test.go

docs/release-notes/release-notes-0.14.0.md

positiveblue · 2021-08-09T05:15:03Z

rpcserver.go

+	ops := &lnrpc.Op{Entity: "features"}
+
+	fsets := make(map[feature.Set]*lnwire.RawFeatureVector)
+	sets := []feature.Set{feature.SetNodeAnn}


@Roasbeef should the endpoint allow the user to update ANY feature set? (I think yes... but I am not sure)
In that case, we should include it in the proto request (set-bit-action)

Yeah I think any feature, other than features we have set by default (existing in the feature deps set).

positiveblue · 2021-08-09T05:15:26Z

rpcserver.go

+		// Ensure that all of our feature sets properly set any
+		// dependent features.
+		fv := lnwire.NewFeatureVector(raw, lnwire.Features)
+		err := feature.ValidateDeps(fv)


Do we need to validate anything else?

See above comment, this ensures the features are well defined, but can possibly allow a user to over write a feature we set (like anchor channels), which we want to prevent IMO.

positiveblue · 2021-08-09T05:19:48Z

rpcserver.go

+	removeAddr := make([]net.Addr, 0, len(updates))
+	addAddr := make([]net.Addr, 0, len(updates))
+	for _, update := range updates {
+		addr, err := net.ResolveTCPAddr(update.Address.GetNetwork(), update.Address.GetAddr())


How should I make this work with tor? Should I instead use parseAddr using the node configuration?

Yeah, using parseAddr will ensure that if Tor is active, the DNS request is tunneled over that.

rpcserver.go

positiveblue · 2021-08-09T05:22:40Z

rpcserver.go

+		LastUpdate:           time.Unix(int64(newNodeAnn.Timestamp), 0),
+		Addresses:            newNodeAnn.Addresses,
+		Alias:                newNodeAnn.Alias.String(),
+		Features: lnwire.NewFeatureVector(


this is what causes me a bit of discomfort, the endpoint will broadcast the node announcement changes but it can also be used for updating other feature sets?

What do you mean other feature sets? You mean things that we don't actually understand?

lntest/itest/lnd_channel_graph_test.go

sputn1ck · 2021-09-07T12:57:37Z

rpcserver.go

+	}
+
+	if len(nodeModifiers) == 0 {
+		return nil, fmt.Errorf("unable detect any new values to update " +


When only updating features, UpdateNodeAnnouncement returns an error (but still updates the featurebit) .

Thank you for noticing @sputn1ck

Actually this can be tricky because even if when we have updated some parts of the node it is not until the end of the function that we update the on-disk version of our announcement so other sub-systems don't become out of sync + announce those changes to our peers.

I will return resp instead of nil so the user always have track of what operations were performed. Even so, if some operations succeed but the endpoint fails before completing the full flow the user will have to call it again until it succeeds to ensure that the copy on disk + our peers have the correct node announcement version

New endpoint for the `Peers` subrpc.

positiveblue · 2022-04-02T01:30:46Z

@Roasbeef I added the new command for lndcli. For the macaroon, we had some discussion about not having it's own. I think this is ready to go 🚀

What new uses for the sub-server do you have in mind 👀?

Basic logic for the endpoint: - Get the current nodeAnn information - Calculate modifications - Apply modifications - Return changes

lightninglabs-deploy · 2022-04-11T00:56:30Z

@Roasbeef: review reminder
@positiveblue, remember to re-request review from reviewers when ready

Roasbeef · 2022-04-12T02:28:36Z

What new uses for the sub-server do you have in mind 👀?

See this comment: #6262 (comment). More generically, this sub-server lets us move towards an architectures where a main lnd node doesn't actually have a gossiper included and instead uses this API to push out updates and either obtain updates for the graph, or have those updates applied to a read-only mirror it maintains.

Roasbeef

LGTM 🌾

joostjager · 2022-10-31T06:27:10Z

@positiveblue I noticed that changing an alias using UpdateNodeAnnouncement doesn't survive a restart. The node will revert to the configured alias again. Is that intentional?

Roasbeef · 2022-10-31T23:40:28Z

The node will revert to the configured alias again. Is that intentional?

As is yeah, and imo that's the correct behavior, as arguably a user shouldn't set the config level param if they want to have it be updated dynamically. There also isn't a great way to distinguish between a particular field of the node announcement that should ignore the static config values.

joostjager · 2022-11-01T09:04:15Z

If you don't set the config level parameters, the dynamically set alias is still forgotten after a restart. The node will revert to a truncated pubkey as the alias.

How can one use this api in a way that no default alias or other node announcement fields can leak during restarts?

Roasbeef · 2022-11-02T23:53:15Z

Ah yeah I think that's an oversight, we should correct that. The gap here is that we didn't add a test case for node restart.

positiveblue commented Jul 29, 2021

View reviewed changes

positiveblue force-pushed the 5466-update-node-announcement-data branch 3 times, most recently from 41decb4 to 6aa0dc4 Compare July 29, 2021 20:28

Roasbeef requested changes Jul 30, 2021

View reviewed changes

guggero reviewed Aug 2, 2021

View reviewed changes

lnrpc/lightning.proto Outdated Show resolved Hide resolved

lnrpc/lightning.yaml Outdated Show resolved Hide resolved

rpcserver.go Outdated Show resolved Hide resolved

alexbosworth reviewed Aug 3, 2021

View reviewed changes

lntest/itest/lnd_channel_graph_test.go Outdated Show resolved Hide resolved

alexbosworth reviewed Aug 3, 2021

View reviewed changes

lntest/itest/lnd_channel_graph_test.go Outdated Show resolved Hide resolved

positiveblue force-pushed the 5466-update-node-announcement-data branch from 6aa0dc4 to cf1e095 Compare August 9, 2021 05:07

positiveblue requested a review from Roasbeef August 9, 2021 05:23

positiveblue force-pushed the 5466-update-node-announcement-data branch from cf1e095 to fca3156 Compare August 9, 2021 05:29

positiveblue commented Aug 9, 2021

View reviewed changes

ErikEk reviewed Aug 19, 2021

View reviewed changes

lntest/itest/lnd_channel_graph_test.go Outdated Show resolved Hide resolved

Roasbeef added the P3 might get fixed, nice to have label Aug 31, 2021

Roasbeef added this to the v0.14.0 milestone Aug 31, 2021

sputn1ck reviewed Sep 7, 2021

View reviewed changes

positiveblue mentioned this pull request Oct 13, 2021

Issue 5466 #5857

Closed

Roasbeef modified the milestones: v0.14.0, v0.15.0 Oct 19, 2021

positiveblue mentioned this pull request Oct 25, 2021

multi: script lease enforcement lightninglabs/pool#300

Merged

positiveblue force-pushed the 5466-update-node-announcement-data branch 8 times, most recently from 13d3537 to 2b4bafb Compare January 8, 2022 20:30

positiveblue force-pushed the 5466-update-node-announcement-data branch from c334ca8 to fd32aa8 Compare April 2, 2022 01:13

lnrpc: add updatenodeannouncement to Peers service

5ab0cbd

New endpoint for the `Peers` subrpc.

positiveblue force-pushed the 5466-update-node-announcement-data branch from fd32aa8 to 2ada770 Compare April 2, 2022 01:27

positiveblue requested a review from Roasbeef April 2, 2022 01:28

positiveblue added 3 commits April 1, 2022 18:36

lnrpc/peers: skeleton logic for updateNodeAnnouncement

e4e0935

Basic logic for the endpoint: - Get the current nodeAnn information - Calculate modifications - Apply modifications - Return changes

lnrpc/peers: handle alias changes in updateNodeAnnouncement

ce48139

lnrpc/peers: handle color changes in updateNodeAnnouncement

aacb256

positiveblue force-pushed the 5466-update-node-announcement-data branch 4 times, most recently from 8c9f798 to 78a1a93 Compare April 3, 2022 01:47

positiveblue added 4 commits April 2, 2022 19:09

lnrpc/peers: handle net address changes in updateNodeAnnouncement

76196db

lnrpc/peers: handle feature bit changes in updateNodeAnnouncement

ae2aa56

lncli: add peers updatenodeannouncement

f3fdf8d

docs: add release notes

fdfdd10

positiveblue force-pushed the 5466-update-node-announcement-data branch from 78a1a93 to fdfdd10 Compare April 3, 2022 02:11

Roasbeef approved these changes Apr 12, 2022

View reviewed changes

Roasbeef merged commit 2fd900b into lightningnetwork:master Apr 12, 2022

sputn1ck mentioned this pull request Jun 30, 2022

Use feature flag to announce Peerswap ElementsProject/peerswap#82

Closed

joostjager mentioned this pull request Nov 2, 2022

[feature]: allow feature bits to be sent using peersrpc.UpdateNodeAnnouncement #7094

Closed

joostjager mentioned this pull request Nov 7, 2022

[feature]: Allow persisting dynamic configured alias when not set in lnd.conf or arguments #7123

Open

carlaKC mentioned this pull request Nov 17, 2022

multi: Add ability to set custom node announcement feature bits in config #7168

Closed

7 tasks

lnrpc: Add UpdateNodeAnnouncement endpoint #5587

lnrpc: Add UpdateNodeAnnouncement endpoint #5587

Conversation

positiveblue commented Jul 29, 2021 • edited by Roasbeef Loading

positiveblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Jul 30, 2021

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

positiveblue commented Jul 30, 2021

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

positiveblue commented Apr 2, 2022

lightninglabs-deploy commented Apr 11, 2022

Roasbeef commented Apr 12, 2022 • edited Loading

Roasbeef left a comment

Choose a reason for hiding this comment

joostjager commented Oct 31, 2022

Roasbeef commented Oct 31, 2022

joostjager commented Nov 1, 2022

Roasbeef commented Nov 2, 2022

positiveblue commented Jul 29, 2021 •

edited by Roasbeef

Loading

Roasbeef commented Apr 12, 2022 •

edited

Loading