Add config options for custom feature bits #7568

carlaKC · 2023-04-04T07:27:32Z

Change Description

Restrict updatenodeannouncement from making changes to LND's standard feature bits.
Move management of feature bits into feature manager.
Add the ability to configure custom Init / NodeAnn / Invoice features at startup, and disallow modifying of these features via API (we consider them "hard set" like LND's defined feature bits). AMP isn't included because I really can't see a use case for it, but happy to tack it on!

Fixes: #7094, Replaces #7168.

Steps to Test

Disallow unsetting of defined feature bits:

lncli peers updatenodeannouncement --feature_bit_remove=5 -> can't unset feature bit 5 (upfront-shutdown-script): feature is used in standard protocol set

Disallow setting of defined feature bits:

lncli peers updatenodeannouncement --feature_bit_remove=3 -> can't set feature bit 3 (initial-routing-sync): feature is used in standard protocol set

Configure Init feature:

Build LND with dev tag
Run alice with custom init bit: lnd --protocol.custom-init=555, run bob as usual and connect to alice
lncli-bob listpeers -> 555 listed in alice's features

Configure NodeAnn feature:

Build LND with dev tag
Run alice with custom bit: lnd --protocol.custom-nodeann=777
Run bob as usual, connect to alice and open a channel (required to be added to the graph)
lncli-bob describgraph -> alice listed with 777 feature (might take a sec to update, gossip batching)
Demonstrate "hard set" config feature: lncli-alice peers updatenodeannouncement --feature_bit_remove=777 -> can't unset configured feature

Configure Invoice feature:

Build LND with dev tag
lnd --protocol.custom-invoice=999
lncli addinvoice -> lnbc...
lncli decodepayreq lnbc... -> 999 listed in features

carlaKC · 2023-04-04T07:38:36Z

Getting back to this - looking for an apprach ACK then I'll add some itest converge :)

lightninglabs-deploy · 2023-04-12T12:07:17Z

@positiveblue: review reminder

ziggie1984 · 2023-04-13T11:45:34Z

Hi Carla I am interested in hosting this PR in the Review Club on may 25th. Do you consider it a good idea, or is it still to early?

carlaKC · 2023-04-13T13:21:17Z

Hi Carla I am interested in hosting this PR in the Review Club on may 25th. Do you consider it a good idea, or is it still to early?

No objections from me! PR is ready for review + has itest coverage for new features.

positiveblue

LGTM @carlaKC 🎉 Thank you very much for making a "sane" version of getNodeAnnouncement

The itests are failing but you just need to close the channel that alice opened ht.CloseChannel(ht.Alice, chanPoint) (you can get the chanPoint from the OpenChannel call)

netann/node_announcement.go

lnwire/features_test.go

server.go

carlaKC · 2023-04-19T15:07:04Z

Rebased + addressed first round of review!

The itests are failing but you just need to close the channel that alice opened ht.CloseChannel(ht.Alice, chanPoint) (you can get the chanPoint from the OpenChannel call)

Interesting, I swear these passed locally 🤔 Thanks for the tip!

positiveblue · 2023-04-20T02:19:41Z

LGTM 🚀

ziggie1984

Very nice PR, really enlightened my understanding regarding features in lightning in general. Thanks for your work on this :)

During review I found that the cmd: lncli peers updatenodeannouncement has a typo in the description which could also be fixed in this context?

--feature_bit_remove value a feature bit that needs to be disabledCan be set multiple times in the same command => ... disabled. Can ...

netann/node_announcement.go

feature/manager.go

server.go

lnwire/features_test.go

feature/manager.go

lncfg/protocol_experimental_on.go

feature/manager_internal_test.go

itest/lnd_custom_features.go

ziggie1984

Went through the tests today, and I must say I find it weird, that config bits set by us are labeled as unkown in our view when calling lncli getinfo. I think we need to mark them maybe custom or something rather than unknown?

Started lnd with: --protocol.custom-init=555 and then calling lncli getinfo. I mean we are setting them so I would expect they are at least known to us?

       "555": {
            "name": "unknown",
            "is_required": false,
            "is_known": false
        },

Moreover I think we need to maybe disallow setting feature bits which are greater 10 bits at least for the invoice custom features:

Started lnd with: --protocol.custom-invoice=1117
Then tried to add an invoice which resulted in the following error:

rlncli-wt addinvoice
[lncli] rpc error: code = Unknown desc = data length too big to fit within 10 bits: 1117

If we cannot encode such big features, should we maybe not allow to set them in the first place?

carlaKC · 2023-04-24T13:44:09Z

If we cannot encode such big features, should we maybe not allow to set them in the first place?

Great catch, thanks for the review! Will add some validation on existing RPC and config.

Went through the tests today, and I must say I find it weird, that config bits set by us are labeled as unkown in our view when calling lncli getinfo. I think we need to mark them maybe custom or something rather than unknown?

I'll take a look at how much more code this would be. While I agree it's odd, it is how we currently have it for UpdateNodeAnnouncement and what I'd consider a "frill" for an advanced feature - if you're buidling with dev setting custom feature bits, you damn well should know what they're for 😬

ziggie1984 · 2023-04-24T19:51:45Z

I'll take a look at how much more code this would be. While I agree it's odd, it is how we currently have it for UpdateNodeAnnouncement and what I'd consider a "frill" for an advanced feature - if you're buidling with dev setting custom feature bits, you damn well should know what they're for 😬

Agree with you, just see whether its a big change and leave it out if it's too much. Just a small nit and definitely not worth it if the change is not small.

carlaKC · 2023-04-25T15:38:30Z

@ziggie1984 are you sure you ran into rpc error: code = Unknown desc = data length too big to fit within 10 bits: 1117 with Started lnd with: --protocol.custom-invoice=1117? Did some digging into maximum feature bit we can fit in an invoice tagged field, and by my count we only hit issues for features > 5114 (if my reasoning is correct)?

Added a new commit to enforce a maximum custom feature for invoices, if we try to configure LND with custom invoice features out of the allowed range it will now error:

lnd --protocol.custom-invoice=5115 -> feature bit: 5115 exceeds set: SetInvoice maximum: 5114

ziggie1984

Sorry for confusion just saw the log message with 1117 in my lnd instance and wrote the comment. You are correct I must have started my lnd instance with something like [11175 - 5 ~ 11175).

You covered all my comments LGTM ⚡️

feature/set.go

lnwire/features.go

feature/set.go

lnwire/features.go

feature/manager.go

yyforyongyu

Very nice PR🎉 and thanks for taking the time to refactor the code! I think it's very close, just a few questions/typos/nits.

discovery/gossiper.go

lnwire/features_test.go

lnwire/features.go

yyforyongyu · 2023-04-27T11:55:36Z

lnrpc/peersrpc/peers_server.go

+	nodeAnnFeatures := currentNodeAnn.Features
+	featureUpdates := len(req.FeatureUpdates) > 0
+	if featureUpdates {
+		var (


hmm don't think we need to declare them here?

nodeAnnFeatures needs to be declared outside so that we can use it on 396, and if we use := it's shadowed and we get an unused var error.

yyforyongyu · 2023-04-27T11:57:08Z

netann/host_ann.go

@@ -168,8 +168,8 @@ func (h *HostAnnouncer) hostWatcher() {
 // NodeAnnUpdater describes a function that's able to update our current node
 // announcement on disk. It returns the updated node announcement given a set
 // of updates to be applied to the current node announcement.
-type NodeAnnUpdater func(modifier ...NodeAnnModifier,
-) (lnwire.NodeAnnouncement, error)
+type NodeAnnUpdater func(modifier ...NodeAnnModifier) (


nit: wrong commit?

feature/manager.go

itest/lnd_custom_features.go

yyforyongyu · 2023-04-27T15:03:45Z

itest/lnd_custom_features.go

+
+// assertNotSet checks that the features provided aren't contained in a feature
+// map.
+func assertNotSet(ht *lntest.HarnessTest, features []uint32,


nit: could make it a helper closure inside the test as it's only used by this test(and likely to be the only case).

would prefer to leave as is - imo closures can cultter up an itest vs a well named function which makes it clear what it's doing. Plus we do one itest / one file nowadays so it won't get in anyone's way here.

Plus we do one itest / one file nowadays so it won't get in anyone's way here.

Not sure about that, but this is not gonna be scalable and don't think we'll go that route.

closures can cultter up an itest vs a well named function

The issue with the current func is it can get in others' way, as assertNotSet is a broad term and can mean many things - just imagine another itest also wanna assert something is not set.

Non-blocking tho, maybe we could address this and similar issues in a dedicated PR.

Updated to assertFeatureNotInSet to keep out of the way 👍

carlaKC · 2023-04-28T14:54:42Z

Thanks for the review @yyforyongyu, addressed nits in latest push.
Just the modifiers question outstanding for discussion IMO.

yyforyongyu · 2023-05-04T01:34:57Z

itest/lnd_custom_features.go

+
+// assertNotSet checks that the features provided aren't contained in a feature
+// map.
+func assertNotSet(ht *lntest.HarnessTest, features []uint32,


Plus we do one itest / one file nowadays so it won't get in anyone's way here.

Not sure about that, but this is not gonna be scalable and don't think we'll go that route.

closures can cultter up an itest vs a well named function

The issue with the current func is it can get in others' way, as assertNotSet is a broad term and can mean many things - just imagine another itest also wanna assert something is not set.

Non-blocking tho, maybe we could address this and similar issues in a dedicated PR.

cmd/lncli/peersrpc_active.go

docs/release-notes/release-notes-0.17.0.md

lnrpc/peersrpc/peers_server.go

yyforyongyu

LGTM🥳

guggero · 2023-05-04T10:42:21Z

Needs a rebase!

In preparation for moving feature bit modification inside of the feature manager, separate node modification from signing.

In preparation for a more complex function signature for set node announcement, separate get and set so that readonly callers don't need to handle the extra arguments.

Disallow update of any features that are defined by LND (since just updating the feature, but not the functionality will result in strange behavior). All known feature bits should be toggled using protocol config options.

Base 32 encoded bolt 11 invoices only allow 10 bits to express the length of the feature vector in a tagged field, so there is a much lower limit on the values invoice custom features can hold. Other places in the protocol are theoretically limited by the maximum message size, but since we express a feature bit as u16 we don't need to be concerned about this. The decision is made to track maximum per-set in the feature manager, which is conceptually aware of sets and then validate in lnwire/features against some arbitrary maximum value provided to the caller to keep the base features package unaware of sets.

Move merge of our features into the feature manager, rather than updating our node announcement then retrospectively setting the feature manger's set to the new vector. This allows us to use the feature vector as our single source of truth for announcements.

carlaKC · 2023-05-04T14:42:40Z

Rebased + addressed last nits!

SeverinAlexB · 2023-05-05T05:04:12Z

Thanks for the work/merge guys!

carlaKC mentioned this pull request Apr 4, 2023

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

Closed

carlaKC requested a review from positiveblue April 4, 2023 07:36

SeverinAlexB mentioned this pull request Apr 5, 2023

Transport layer proposal BitcoinAndLightningLayerSpecs/lsp#23

Merged

carlaKC force-pushed the 7094-updatefeaturemanager branch 4 times, most recently from f7c712b to e5e2a82 Compare April 5, 2023 11:18

saubyk added this to the v0.17.0 milestone Apr 13, 2023

saubyk assigned carlaKC Apr 13, 2023

saubyk added gossip feature bits wire protocol Encoding of messages for the communication between nodes labels Apr 13, 2023

positiveblue reviewed Apr 14, 2023

View reviewed changes

netann/node_announcement.go Show resolved Hide resolved

lnwire/features_test.go Outdated Show resolved Hide resolved

server.go Outdated Show resolved Hide resolved

carlaKC force-pushed the 7094-updatefeaturemanager branch 2 times, most recently from f2924f1 to a9ec486 Compare April 19, 2023 15:01

carlaKC requested a review from positiveblue April 19, 2023 16:46

yyforyongyu self-requested a review April 20, 2023 11:00

positiveblue approved these changes Apr 20, 2023

View reviewed changes

ziggie1984 reviewed Apr 22, 2023

View reviewed changes

ziggie1984 reviewed Apr 23, 2023

View reviewed changes

carlaKC force-pushed the 7094-updatefeaturemanager branch from a9ec486 to 8b76556 Compare April 24, 2023 17:57

carlaKC mentioned this pull request Apr 25, 2023

Custom Feature Bits lightningdevkit/rust-lightning#2131

Closed

carlaKC force-pushed the 7094-updatefeaturemanager branch from 8b76556 to da450d9 Compare April 25, 2023 15:27

carlaKC requested a review from ziggie1984 April 25, 2023 15:38

ziggie1984 approved these changes Apr 26, 2023

View reviewed changes

feature/set.go Show resolved Hide resolved

lnwire/features.go Show resolved Hide resolved

feature/set.go Show resolved Hide resolved

lnwire/features.go Show resolved Hide resolved

feature/manager.go Show resolved Hide resolved

yyforyongyu requested changes Apr 27, 2023

View reviewed changes

carlaKC force-pushed the 7094-updatefeaturemanager branch 2 times, most recently from 611bdda to 15363f3 Compare April 28, 2023 14:43

carlaKC requested a review from yyforyongyu April 28, 2023 14:53

yyforyongyu reviewed May 4, 2023

View reviewed changes

yyforyongyu approved these changes May 4, 2023

View reviewed changes

carlaKC added 10 commits May 4, 2023 10:35

multi/refactor: move node ann modification outside of sign method

c6263c2

In preparation for moving feature bit modification inside of the feature manager, separate node modification from signing.

multi/refactor: separate methods for get and set node announcement

3f9f0ea

In preparation for a more complex function signature for set node announcement, separate get and set so that readonly callers don't need to handle the extra arguments.

lnwire: add feature vector update validation helpers

1a9d743

feature: add update function for safe feature update

5a139fd

Disallow update of any features that are defined by LND (since just updating the feature, but not the functionality will result in strange behavior). All known feature bits should be toggled using protocol config options.

feature/refactor: unexport set raw

4e98f4d

multi: add custom feature bits to config options

4e36e2f

features: disallow unsetting of config-set features

fe7cad2

itest: add coverage for custom features

019da31

carlaKC force-pushed the 7094-updatefeaturemanager branch from 15363f3 to 820920a Compare May 4, 2023 14:42

lncli: fix typo in updatenodeannouncement

ad6ccb4

docs: add release notes

820920a

guggero merged commit 7284526 into lightningnetwork:master May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config options for custom feature bits #7568

Add config options for custom feature bits #7568

carlaKC commented Apr 4, 2023 •

edited by saubyk

Loading

carlaKC commented Apr 4, 2023

lightninglabs-deploy commented Apr 12, 2023

ziggie1984 commented Apr 13, 2023 •

edited

Loading

carlaKC commented Apr 13, 2023

positiveblue left a comment

carlaKC commented Apr 19, 2023

positiveblue commented Apr 20, 2023

ziggie1984 left a comment

ziggie1984 left a comment •

edited

Loading

carlaKC commented Apr 24, 2023

ziggie1984 commented Apr 24, 2023

carlaKC commented Apr 25, 2023

ziggie1984 left a comment

yyforyongyu left a comment

yyforyongyu Apr 27, 2023

carlaKC Apr 28, 2023

yyforyongyu Apr 27, 2023

yyforyongyu Apr 27, 2023

carlaKC Apr 28, 2023

yyforyongyu May 4, 2023

carlaKC May 4, 2023

carlaKC commented Apr 28, 2023 •

edited

Loading

yyforyongyu May 4, 2023

yyforyongyu left a comment

guggero commented May 4, 2023

carlaKC commented May 4, 2023

SeverinAlexB commented May 5, 2023

Add config options for custom feature bits #7568

Add config options for custom feature bits #7568

Conversation

carlaKC commented Apr 4, 2023 • edited by saubyk Loading

Change Description

Steps to Test

carlaKC commented Apr 4, 2023

lightninglabs-deploy commented Apr 12, 2023

ziggie1984 commented Apr 13, 2023 • edited Loading

carlaKC commented Apr 13, 2023

positiveblue left a comment

Choose a reason for hiding this comment

carlaKC commented Apr 19, 2023

positiveblue commented Apr 20, 2023

ziggie1984 left a comment

Choose a reason for hiding this comment

ziggie1984 left a comment • edited Loading

Choose a reason for hiding this comment

carlaKC commented Apr 24, 2023

ziggie1984 commented Apr 24, 2023

carlaKC commented Apr 25, 2023

ziggie1984 left a comment

Choose a reason for hiding this comment

yyforyongyu 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

carlaKC commented Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

guggero commented May 4, 2023

carlaKC commented May 4, 2023

SeverinAlexB commented May 5, 2023

carlaKC commented Apr 4, 2023 •

edited by saubyk

Loading

ziggie1984 commented Apr 13, 2023 •

edited

Loading

ziggie1984 left a comment •

edited

Loading

carlaKC commented Apr 28, 2023 •

edited

Loading