-
Notifications
You must be signed in to change notification settings - Fork 133
Feature bits #1748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature bits #1748
Conversation
eb267a6
to
77f9420
Compare
Pull Request Test Coverage Report for Build 17639932889Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few initial questions and comments, mostly about the AuxChanNegotiator handling in the RFQ system.
77f9420
to
74d98bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made another pass, with some additional questions and suggestions. I think everything on my end should be resolved after this.
74d98bf
to
611a9fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 👍
} | ||
|
||
// ourFeatures returns a slice containing all of the locally supported features. | ||
func ourFeatures() []lnwire.FeatureBit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be a constant instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in a const array? Not aware of how we do this in the codebase
If you mean just var ourFeatures[...]lnwire.FeatureBit{a, b, c, d}
I wanted to avoid that since many different goroutines may be accessing it. The current approach with the func returns a new slice so each caller gets their own pointer
// custom feature bits to include in the init message TLVs. The implementation | ||
// can decide which features to advertise based on the peer's identity. | ||
func (n *AuxChannelNegotiator) GetInitFeatures( | ||
_ route.Vertex) (tlv.Blob, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think w/ the latest direction, this can now just return the feature vector directly, to be merged by lnd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// to drop the connection (by returning an error right here). | ||
err = checkRequiredBits(getLocalFeatureVec(), peerVec) | ||
if err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lnd
doesn't need to handle this error right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we expect LND to fail the peer connection, preventing any channels from becoming active
611a9fc
to
3d4c18f
Compare
3d4c18f
to
f9e10d7
Compare
Also guarded the new hooks with the |
@Roasbeef: review reminder |
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 theCustomChannelType
(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
with0
's)Dependencies
LND lightningnetwork/lnd#10182
Lndclient lightninglabs/lndclient#239
LiT itest: lightninglabs/lightning-terminal#1138