Feature bits

[GeorgeTsagk]

Description

Adds the necessary subsystem to handle custom feature bit negotiation for tap channels. This allows us to introduce new channel features that would otherwise break backwards compatibility. We piggy-back on the existing LND messages (init & reestablish) to nest the tlv.Blob that encodes the CustomChannelType (uint64) type.

The set of active features over a channel is the intersection of the locally supported features and the features that the peer provided.

Currently we follow a "global" approach on the feature bits that we support. We don't make any distinctions between peers or channels. The locally supported feature bits is a static set.

We also don't allow reading/writing feature bits per channel, but per peer. For now the funding outpoint / funding blob arguments are ignored. When we establish a feature-set with a peer, that applies to all of our channels with that peer.

An older tapd node (i.e a node that does not contain the contents of this PR) will be treated as if they provided an empty feature set (uint64 with 0's)

Dependencies

LND lightningnetwork/lnd#10182
Lndclient lightninglabs/lndclient#239
LiT itest: lightninglabs/lightning-terminal#1138

Closes #1573

[coveralls]

Pull Request Test Coverage Report for Build 18007703532

Details

• 57 of 162 (35.19%) changed or added relevant lines in 8 files are covered.
• No unchanged relevant lines lost coverage.
• Overall first build on feature-bits at 56.52%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapchannel/aux_traffic_shaper.go	0	5	0.0%
tapfeatures/aux_feature_bits.go	8	14	57.14%
server.go	0	37	0.0%
tapfeatures/aux_channel_negotiator.go	21	78	26.92%

Totals
Change from base Build 17982497706:	56.5%
Covered Lines:	63685
Relevant Lines:	112676

---

💛 - Coveralls

[jtobin]

A few initial questions and comments, mostly about the AuxChanNegotiator handling in the RFQ system.

[jtobin]

Made another pass, with some additional questions and suggestions. I think everything on my end should be resolved after this.

[jtobin]

LGTM 👍 👍

[Roasbeef]

I think w/ the latest direction, this can now just return the feature vector directly, to be merged by lnd.

[GeorgeTsagk]

Wdym? To merge this feature vector with the one that LND uses in lnwire.Init?

Might be a nice convenience since that message already uses a feature vec, but can't see the opportunities. We'd skip a few lines (just for lnwire.Init) which encode/decode the aux features within ExtraData, which is a bit more lean, but then again for the other messages we'd have to maintain the current approach as they don't have a feature-vec field.

[GeorgeTsagk]

Also guarded the new hooks with the s.waitForReady call to make sure that the subsystems have been initialized, as we're using AuxChanNegotiator in the new hooks

[GeorgeTsagk]

Pushed an update that uses the new LND aux negotiator interface.

[GeorgeTsagk]

Will have to update golang to v1.24.6, will probably create a separate PR for that and rebase

[Roasbeef]

Can be rebased now!

[Roasbeef]

Do we actually need this here? IIUC GetInitRecords() will just read some static variables.

We need to be careful here to not introduce yet another deadlock bug when lnd is trying to start up but tapd hasn't been fully initialized yet.

I don't think any of these need waitForReady, as they don't interact with any outside daemon/sub-system.

[GeorgeTsagk]

We're always using the aux chan negotiator, which needs to be initialized by the server.

Without this we'll be racing against the server, occasionally panicking because of nil dereference.

For the init records we could actually have it be static and detached from the negotiator -- the rest of the calls I'm not so sure about.

[Roasbeef]

Without this we'll be racing against the server, occasionally panicking because of nil dereference.

Why would we panic?

I'm referring to issues like this: #1780.

Init can def be static.

Do we have a reason to include this? Given that we've seen deadlock instances when this wasn't needed?

[GeorgeTsagk]

Rebased on main with upgraded go to v1.24.6, CI should be more green now (LiT is still expected to fail, see corresponding PR for status of LiT itests)

[jtobin]

(Just noting for posterity that this will resolve #1573.)

[GeorgeTsagk]

(Just noting for posterity that this will resolve #1573.)

Thanks, added a Closes clause on the PR description.

[lightninglabs-deploy]

@Roasbeef: review reminder

[Roasbeef]

LGTM 🪻

[GeorgeTsagk]

On the concerns related to servers reaching a deadlock waiting for each other to start:

Currently the new aux hooks in tapd related to aux features are guarded by s.waitForReady() which means that the tapd server must has been properly initialized and started.

On the LND side the callsites for these new hooks reside within brontide and the htlcswitch links (created by brontide). Any other LND sub-server does not have its start-up affected by these new call-sites, which are also ran asynchronously in a goroutine.

This PR is now ready to merge into main, but since the corresponding LiT itest PR is not yet reviewed/approved I'll not merge this yet, as then we'd have to block any other PR due to the failing LiT itest CI step.