Normalizes node features so they use LDK NodeFeatures #77
Conversation
|
Some manual testing to make sure the features are properly parsed will be appreciated (I'm looking at you @okjodom, AKA fast-merger lol) |
sr-gi
force-pushed
the
normalize-node-features
branch
2 times, most recently
from
11ca6b6
to
b5e2566
Compare
|
I've opened a PR upstream to add the regular feature bit parsing. We should be able to simplify this if that gets merged (I have code locally for it). I think we could squash this and go with the current approach for now. For reference: lightningdevkit/rust-lightning#2522 |
| .getinfo(GetinfoRequest {}) | ||
| .await | ||
| .map_err(|err| LightningError::GetInfoError(err.to_string()))? | ||
| .into_inner(); | ||
|
|
||
| //FIXME: our_features is returning None, but it should not :S |
There was a problem hiding this comment.
Does this comment still apply?
There was a problem hiding this comment.
Yep, and I'm not sure why :S
sim-lib/src/lnd.rs
Outdated
| // FIXME: Do better with this error | ||
| // TODO: We could even generalize this to parse any type of Features | ||
| /// Parses the node features from the format returned by LND gRPC to LDK NodeFeatures | ||
| fn parse_node_features(features: HashSet<&u32>) -> Result<NodeFeatures, String> { |
There was a problem hiding this comment.
Just return NodeFeatures since this doesn't error?
There was a problem hiding this comment.
Yep, you're right. I mixed this with the previous commit
sim-lib/src/lnd.rs
Outdated
| @@ -47,7 +67,8 @@ impl LndNode { | |||
| info: NodeInfo { | |||
| pubkey: PublicKey::from_str(&identity_pubkey) | |||
| .map_err(|err| LightningError::GetInfoError(err.to_string()))?, | |||
| features: features.keys().copied().collect(), | |||
| features: parse_node_features(features.keys().collect()) | |||
There was a problem hiding this comment.
Rather than parse_node_features taking &u32 could we do keys().cloned().collect() here? Seems like a more normal function signature.
There was a problem hiding this comment.
Sure, I don't think that'll be a big deal
sim-lib/src/lnd.rs
Outdated
| fn parse_node_features(features: HashSet<&u32>) -> Result<NodeFeatures, String> { | ||
| let mut flags = vec![0; 256]; | ||
|
|
||
| // Set well known features first |
There was a problem hiding this comment.
This comment doesn't really make sense to me? Looks like we're setting lower value features first, not known ones?
Perhaps reword to note that < 255 are protocol features, so we allocate enough for the "standard" protocol features first?
There was a problem hiding this comment.
Yep, same, this was from the initial apporach
Currently we were using a pretty hacky way of parsing LND features that only parsed the keysend feature (given is the only we care about atm). This PR properly loads the feature vector returned by LND's GetNodeInfo and parses it so a complete NodeFeatures object can be constructed. Also sets NodeInfo::features to NodeFeatures for consistency This is based on LDK's code for parsing features, there's currently an open PR to merge this upstream, so we can simplify if that gets merged.
sr-gi
force-pushed
the
normalize-node-features
branch
from
f828e2f
to
08f0cca
Compare
|
Fixed @carlaKC comments and squashed both commits. |
|
@sr-gi any reason not to merge this one? I think it's ready to go 🚀 |
I'm trying to figure out this, will merge tomorrow otherwise |
Sounds like a plan! Goood luck to ya 🫡 |
|
For the record: ElementsProject/lightning#6635 We may want to merge this and keep track of the fix, if it doesn't happen to be me doing something weird |
SGTM - created #85 to keep track of it! |
Currently we were using a pretty hacky way of parsing LND features that only parsed the keysend feature (given is the only we care about atm).
This PR properly loads the feature vector returned by LND's GetNodeInfo and parses it so a complete NodeFeatures object can be constructed.
Also sets NodeInfo::features to NodeFeatures for consistency