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.

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

lnrpc: expose network name in GetInfo #2399

Merged
merged 1 commit into from Jan 8, 2019

Conversation

@joostjager
Copy link
Collaborator

@joostjager joostjager commented Jan 2, 2019

Previously only a testnet boolean was available which
made it impossible to distinguish between regtest and
mainnet.

@joostjager joostjager force-pushed the getinfo-network-name branch 2 times, most recently from 98f2b78 to 6385567 Jan 4, 2019
Copy link
Collaborator

@cfromknecht cfromknecht left a comment

LGTM! Been wanting this for a while :)

Loading

@cfromknecht
Copy link
Collaborator

@cfromknecht cfromknecht commented Jan 5, 2019

needs rebase tho

Loading

lnrpc/rpc.proto Outdated
@@ -1151,6 +1151,9 @@ message GetInfoResponse {

/// Number of inactive channels
uint32 num_inactive_channels = 15 [json_name = "num_inactive_channels"];

/// The network the node is connected to
string network = 16 [json_name = "network"];
Copy link
Member

@Roasbeef Roasbeef Jan 5, 2019

Choose a reason for hiding this comment

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

Can instead extend the chain field to be something like: bitcoin:testnet? (right now it just says bitcoin)

Loading

Copy link
Collaborator Author

@joostjager joostjager Jan 5, 2019

Choose a reason for hiding this comment

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

If we want that, I'd rather make a separate field for it. In a new PR, because this is all I need currently.

Loading

Copy link
Member

@Roasbeef Roasbeef Jan 7, 2019

Choose a reason for hiding this comment

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

If we add this new field, then the response contains redundant data: we'll have a testnet bool as well as a network enum-like file. Instead, we can expand the scope of the chain response to also specify the network. This would rectify a flaw in the existing response (unable to determine precise deployment environment). Using the chain field also fits within the existing schema put in place to prep lnd for a multi-chain future (having bitcoin:testnet and bitcoin:mainnet active at once for example). With the chain field modified, we can then remove the testnet field all together as it adds no new information.

Loading

Copy link
Collaborator Author

@joostjager joostjager Jan 7, 2019

Choose a reason for hiding this comment

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

Yes, the testnet bool should be marked deprecated.

Expanding the chains field to report chain:network is a breaking change. Can that be merged?

Loading

Copy link
Collaborator

@cfromknecht cfromknecht Jan 7, 2019

Choose a reason for hiding this comment

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

When would we ever want to have testnet and mainnet running on the same node? IMO mainnet/testnet/simnet should be a single daemon-level configuration, and then any number of currencies can be used on that network

Loading

@joostjager joostjager force-pushed the getinfo-network-name branch 2 times, most recently from ad29b42 to d6f3e88 Jan 7, 2019
@Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Jan 7, 2019

Loading

@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Jan 7, 2019

@Roasbeef I am fine with extending the existing chains list, but it is a breaking change. Let me know your thoughts on that.

Loading

@Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Jan 7, 2019

Yeah I think that's fine as this will land in a major release.

Loading

@joostjager joostjager force-pushed the getinfo-network-name branch from d6f3e88 to e5b1358 Jan 7, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Jan 7, 2019

@Roasbeef have another look.

To me this multi chain field is premature, but alright, it does the job.

Loading

@joostjager joostjager force-pushed the getinfo-network-name branch 2 times, most recently from 1c2e64a to a2e8b5f Jan 7, 2019
Previously only a testnet boolean was available which
made it impossible to distinguish between regtest and
mainnet.
@joostjager joostjager force-pushed the getinfo-network-name branch from a2e8b5f to 6494080 Jan 7, 2019
Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 🧬

Loading

@Roasbeef Roasbeef merged commit 0c893c6 into lightningnetwork:master Jan 8, 2019
1 of 2 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants