Remove loopback and duplicate addrs in net peers output

[FrancisMurillo]

Summary of changes
Changes introduced in this pull request:

• Remove loopback and duplicate addrs in net peers output
  • This does not filter localhost alias, onlyl loopback like 127.0.0.1
  • Peers without connections are omitted

Reference issue to close (if applicable)
Closes #1184

Other information and links

[CLAassistant]

All committers have signed the CLA.

[leviathanbeak]

it's up to the reviewers, personally I'd just collect to HashSet<String> then to Vec<String> in order to get unique addresses, instead of explicitly creating let mut seen_addrs: HashSet<Multiaddr> = HashSet::default();

[cryptoquick]

Very nice! I really like this approach, and it works well!

Compare:

With:

Againly, nicely done!

it's up to the reviewers, personally I'd just collect to HashSet<String> then to Vec<String> in order to get unique addresses, instead of explicitly creating let mut seen_addrs: HashSet<Multiaddr> = HashSet::default();

@leviathanbeak I'm not sure I understand this feedback, do you think you could provide an example or maybe suggest a change?

[connormullett]

@leviathanbeak I'm not sure I understand this feedback, do you think you could provide an example or maybe suggest a change?

HashSets cannot contain duplicates. Personally I think that approach is much simpler then containing another iterable with seen addresses, so I would like to see if we can incorporate a HashSet. Nice work!

[connormullett]

See previous comment on HashSet approach

[cryptoquick]

@leviathanbeak I'm not sure I understand this feedback, do you think you could provide an example or maybe suggest a change?

HashSets cannot contain duplicates. Personally I think that approach is much simpler then containing another iterable with seen addresses, so I would like to see if we can incorporate a HashSet. Nice work!

Right, but isn't that the approach already being used?
https://github.com/ChainSafe/forest/pull/1199/files#diff-a4693340c77dcb6dcb5dd651478de1755657fb815fea64d1cc1cd4626f81002cR6

[leviathanbeak]

@cryptoquick I meant something along these lines (I'm not at my PC)

let addresses: Vec<String> = info
                                .addrs
                                .into_iter()
                                .filter_map(|multi_addr| filter loopback addresses)
                                .collect::<HashSet<String>()
                                .into_iter()
                                .collect();

[connormullett]

@cryptoquick I meant something along these lines (I'm not at my PC)

let addresses: Vec<String> = info
                                .addrs
                                .into_iter()
                                .filter_map(|multi_addr| filter loopback addresses)
                                .collect::<HashSet<String>()
                                .into_iter()
                                .collect();

Yup, much cleaner.

[cryptoquick]

@cryptoquick I meant something along these lines (I'm not at my PC)

let addresses: Vec<String> = info
                                .addrs
                                .into_iter()
                                .filter_map(|multi_addr| filter loopback addresses)
                                .collect::<HashSet<String>()
                                .into_iter()
                                .collect();

Clever, I like it. @FrancisMurillo Care to make the changes?

[FrancisMurillo]

@cryptoquick I meant something along these lines (I'm not at my PC)

let addresses: Vec<String> = info
                                .addrs
                                .into_iter()
                                .filter_map(|multi_addr| filter loopback addresses)
                                .collect::<HashSet<String>()
                                .into_iter()
                                .collect();

Clever, I like it. @FrancisMurillo Care to make the changes?

@cryptoquick Happy to make the changes. Just to clarify with let mut seen_addrs, there is no duplicate Multiaddrs globally or among all peers. This approach is taken by lotus so that is where it came from. With .collect::<HashSet<_>>(), there is no duplicate in a peer but can occur in others. So is the filtering global or per peer?

[cryptoquick]

@cryptoquick Happy to make the changes. Just to clarify with let mut seen_addrs, there is no duplicate Multiaddrs globally or among all peers. This approach is taken by lotus so that is where it came from. With .collect::<HashSet<_>>(), there is no duplicate in a peer but can occur in others. So is the filtering global or per peer?

Ah, makes sense. Are you saying the "no duplicate Multiaddrs" portion of the AC isn't necessary if we filter for only external addresses? That makes sense, since, we shouldn't have duplicate addresses on IP, haha.

If I remove that requirement, could the code be simplified?

[FrancisMurillo]

@connormullett Updated to use .collect::<HashSet<_>>()

[cryptoquick]

Just reviewed the latest, still works as expected. Thanks for responding to the feedback so quickly, @FrancisMurillo. The code is definitely more succinct now.

@connormullett How's that look?

[connormullett]

Much cleaner, thanks for this work! Approved