Skip to content

Include non-permanently connected peers in list_peers() #95

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

Merged
merged 2 commits into from
Jun 13, 2023

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented May 12, 2023

Closes #62, based on #85

This is a follow-up to #56, as we now are able to include non-permanently connected peers in list_peers().
We also include a number of minor doc fixes as a second commit.

@tnull tnull added this to the 0.1 milestone May 12, 2023
@tnull tnull mentioned this pull request May 12, 2023
47 tasks
@tnull tnull force-pushed the 2023-05-list-all-peers branch 4 times, most recently from 1bfbfe2 to 7b6ac82 Compare May 23, 2023 17:34
@tnull
Copy link
Collaborator Author

tnull commented May 23, 2023

Rebased after #85 landed.

@tnull tnull force-pushed the 2023-05-list-all-peers branch from 7b6ac82 to a4da746 Compare June 6, 2023 10:38
@tnull
Copy link
Collaborator Author

tnull commented Jun 6, 2023

Rebased on main.

(Some(con_addr), Some(_stored_addr)) => NetAddress(con_addr),
(None, Some(stored_addr)) => stored_addr,
(Some(con_addr), None) => NetAddress(con_addr),
(None, None) => continue,
Copy link
Contributor

Choose a reason for hiding this comment

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

When would this happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As we're using lightning-net-tokio and don't support OnionV3 currently, it shouldn't happen. In particular the conn_addr_opt should always be Some. However, I'd rather skip than unwrap here, as it doesn't hurt and is safer if something ever were to change.

src/types.rs Outdated
/// The channel's ID (prior to funding transaction generation, this is a random 32 bytes,
/// thereafter this is the transaction ID of the funding transaction XOR the funding transaction
/// output).
/// The channel ID (prior to funding transaction generation, this is a random 32 byte
Copy link
Contributor

Choose a reason for hiding this comment

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

s/32 byte/32-byte

/// that if we broadcast a revoked state, our counterparty can punish us by claiming at least
/// this value on chain.
/// The value, in satoshis, of this channel as it appears in the funding output.
pub channel_value_sats: u64,
Copy link
Contributor

Choose a reason for hiding this comment

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

We use the plural sats but the singular msat below, it seems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mh, true. Possibly we should decide one way and clean up all at once. This is however doesn't feel like the right place to do it?

@tnull tnull force-pushed the 2023-05-list-all-peers branch from a4da746 to efec308 Compare June 13, 2023 09:23
@tnull tnull force-pushed the 2023-05-list-all-peers branch from 0dfee49 to b10591a Compare June 13, 2023 16:43
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM. Please squash fixups.

@tnull tnull force-pushed the 2023-05-list-all-peers branch from b10591a to f562395 Compare June 13, 2023 17:43
@tnull
Copy link
Collaborator Author

tnull commented Jun 13, 2023

LGTM. Please squash fixups.

Squashed without further changes.

@tnull tnull merged commit 56fe49c into lightningdevkit:main Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include non-permanently connected peers in list_peers
2 participants