Skip to content

Expose peer addresses in PeerManager #2019

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
Feb 10, 2023

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Feb 7, 2023

Closes #2018

In this PR we expose the NetAddresses of connected peers in PeerManager.

@TheBlueMatt
Copy link
Collaborator

Maybe let's just include this in the list peers method? That way users don't have to call twice and correlate (and handle races where you have inconsistent data)

@tnull tnull force-pushed the 2023-02-expose-peer-addrs branch from b92b51f to cec565a Compare February 7, 2023 17:39
@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2023

Codecov Report

Base: 91.06% // Head: 91.04% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (903dc8d) compared to base (137b77c).
Patch has no changes to coverable lines.

❗ Current head 903dc8d differs from pull request most recent head 2e8f73e. Consider uploading reports for the commit 2e8f73e to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2019      +/-   ##
==========================================
- Coverage   91.06%   91.04%   -0.03%     
==========================================
  Files          99       99              
  Lines       51724    51724              
  Branches    51724    51724              
==========================================
- Hits        47105    47093      -12     
- Misses       4619     4631      +12     
Impacted Files Coverage Δ
lightning/src/ln/peer_handler.rs 55.73% <ø> (ø)
lightning/src/util/events.rs 28.86% <0.00%> (-0.70%) ⬇️
lightning/src/ln/functional_tests.rs 96.87% <0.00%> (-0.25%) ⬇️
lightning/src/ln/channelmanager.rs 87.43% <0.00%> (+0.02%) ⬆️
lightning/src/chain/onchaintx.rs 95.74% <0.00%> (+1.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tnull
Copy link
Contributor Author

tnull commented Feb 7, 2023

Maybe let's just include this in the list peers method? That way users don't have to call twice and correlate (and handle races where you have inconsistent data)

I assume you're referring to PeerManager::get_peer_node_ids?

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Feel free to squash.

@tnull tnull force-pushed the 2023-02-expose-peer-addrs branch from cec565a to 4ce6474 Compare February 8, 2023 20:31
@tnull
Copy link
Contributor Author

tnull commented Feb 8, 2023

Merged both methods into get_peer_nodeids and squashed commits.

@tnull tnull force-pushed the 2023-02-expose-peer-addrs branch from 4ce6474 to c275f72 Compare February 8, 2023 20:46
@TheBlueMatt
Copy link
Collaborator

Needs rebase, sorry.

@tnull tnull force-pushed the 2023-02-expose-peer-addrs branch from c275f72 to 903dc8d Compare February 9, 2023 08:29
@tnull
Copy link
Contributor Author

tnull commented Feb 9, 2023

Rebased.

@arik-so
Copy link
Contributor

arik-so commented Feb 10, 2023

Looks good to me. Do you think perhaps this might benefit from tests?

@tnull tnull force-pushed the 2023-02-expose-peer-addrs branch from 903dc8d to 2e8f73e Compare February 10, 2023 17:02
@tnull
Copy link
Contributor Author

tnull commented Feb 10, 2023

Now included a note mentioning that the NetAddresses will only be returned if the user gave them preivously:

> git diff-tree -U2 903dc8df 2e8f73e5
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 19e5bdbe..4577e4a8 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -742,4 +742,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
        /// handshake has completed and we are sure the remote peer has the private key for the given
        /// [`PublicKey`].
+       ///
+       /// The returned `Option`s will only be `Some` if an address had been previously given via
+       /// [`Self::new_outbound_connection`] or [`Self::new_inbound_connection`].
        pub fn get_peer_node_ids(&self) -> Vec<(PublicKey, Option<NetAddress>)> {
                let peers = self.peers.read().unwrap();

@tnull
Copy link
Contributor Author

tnull commented Feb 10, 2023

Looks good to me. Do you think perhaps this might benefit from tests?

Now checking the given NetAddresses are actually returned from get_peer_nodeids.

@TheBlueMatt TheBlueMatt merged commit 90bb3f9 into lightningdevkit:main Feb 10, 2023
@wpaulino wpaulino mentioned this pull request Mar 3, 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.

Expose Peer.their_net_address in PeerManager
4 participants