Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sim-lib: Adds LightningNode::get_network #120

Merged
merged 1 commit into from
Oct 4, 2023
Merged

sim-lib: Adds LightningNode::get_network #120

merged 1 commit into from
Oct 4, 2023

Conversation

sr-gi
Copy link
Member

@sr-gi sr-gi commented Oct 3, 2023

The current approach to get the network from a node stores the information in NodeInfo.

However, this is troublesome going forward (after #103) given we will start using NodeInfo to store information about nodes we do not control (and from which we cannot query the network). Furthermore, the network information is worthless once we've sanity-checked that we are running on a network we support and all nodes are running on the same one, so there is no point keeping that around

@sr-gi sr-gi force-pushed the 20231003-validate-network branch from cdaf919 to 58376b4 Compare October 3, 2023 17:14
carlaKC
carlaKC reviewed Oct 3, 2023
View reviewed changes
Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK, just need to deal with the different values from different impls.

sim-lib/src/lib.rs Outdated Show resolved Hide resolved
sim-lib/src/lib.rs Show resolved Hide resolved
@sr-gi sr-gi force-pushed the 20231003-validate-network branch from 58376b4 to 5b46ecc Compare October 3, 2023 18:43
@sr-gi
Copy link
Member Author

sr-gi commented Oct 3, 2023

Amended comments from review

@sr-gi sr-gi requested review from carlaKC and okjodom October 3, 2023 18:44
@sr-gi sr-gi force-pushed the 20231003-validate-network branch from 5b46ecc to 6f3ba4a Compare October 3, 2023 19:08
@sr-gi sr-gi mentioned this pull request Oct 3, 2023
Merged
4 tasks
carlaKC
carlaKC reviewed Oct 3, 2023
View reviewed changes
sim-lib/src/lnd.rs Show resolved Hide resolved
sim-lib/src/lnd.rs Outdated Show resolved Hide resolved
@sr-gi sr-gi force-pushed the 20231003-validate-network branch from 6f3ba4a to 01123bd Compare October 3, 2023 20:58
sr-gi
sr-gi commented Oct 3, 2023
View reviewed changes
sim-lib/src/lnd.rs
Comment on lines +95 to +101
"LND node is not connected any chain".to_string(),
));
} else if info.chains.len() > 1 {
return Err(LightningError::ValidationError(format!(
"LND node is connected to more than one chain: {:?}",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note to self to format this with the NodeId when rebasing for #103.

@sr-gi
Copy link
Member Author

sr-gi commented Oct 3, 2023

Addresses comments from @carlaKC

carlaKC
carlaKC approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 01123bd

okjodom
okjodom approved these changes Oct 4, 2023
View reviewed changes
Copy link
Collaborator

@okjodom okjodom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK.

I think LND get_network failing with ValidationErrors is both odd and very efficient. Like, LND is self validating it's network, but the spec for get_network only says "tell me your network". Not blocking

@sr-gi
Copy link
Member Author

sr-gi commented Oct 4, 2023

tACK.

I think LND get_network failing with ValidationErrors is both odd and very efficient. Like, LND is self validating it's network, but the spec for get_network only says "tell me your network". Not blocking

Well, there's the issue that they have specific variants that cannot be used to construct bitcoin::Network, which is a bit odd by itself. Now that you mention it, however, I think the CLN one should also return ValidationErrors if Network::from_core_arg fails.

@sr-gi sr-gi force-pushed the 20231003-validate-network branch from 01123bd to 4e9cc81 Compare October 4, 2023 18:29
@sr-gi
sim-lib: Adds LightningNode::get_network
def8dd1 
The current approach to get the network from a node stores the information in `NodeInfo`.

However, this is troublesome going forward given we will start using `NodeInfo`
to store information about nodes we do not control (and from which we cannot
query the network). Furthermore, the network information is worthless once we've
sanity-checked that we are running on a network we support and all nodes are running
on the same one, so there is no point keeping that around
@sr-gi
Copy link
Member Author

sr-gi commented Oct 4, 2023

Updated CLN to return ValidationError if the network cannot be built (which should not happen, but for consistency)

@sr-gi sr-gi force-pushed the 20231003-validate-network branch from 4e9cc81 to def8dd1 Compare October 4, 2023 18:33
@sr-gi
Copy link
Member Author

sr-gi commented Oct 4, 2023

Rebased

carlaKC
carlaKC approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ def8dd1

@sr-gi sr-gi merged commit f376643 into bitcoin-dev-project:main Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlaKC carlaKC carlaKC approved these changes

@okjodom okjodom okjodom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sr-gi @okjodom @carlaKC