fix(network): Avoid initiating outbound handshakes with IPs for which Zebra already has an active peer.

[arya2]

Motivation

This prevents some kinds of attacks.

Closes #6982.

Solution

• Adds a most_recent_by_ip field to AddressBook.
• Updates the new field when there's an update with a later last_response time, or when addresses are removed.
• Adds && !self.has_active_peer_with_ip() to the filter in reconnection_peers().
• Tests that the iterator from reconnection_peers() skips over address when there's already a live address with the same IP.

Review

Part of regular scheduled work.

Reviewer Checklist

• Will the PR name make sense to users?
  • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
• Are the PR labels correct?
• Does the code do what the ticket and PR says?
  • Does it change concurrent code, unsafe code, or consensus rules?
• How do you know it works? Does it have tests?

Follow Up Work

• Update AddressBook field types with concurrent collections #7040

[codecov]

Codecov Report

Merging #7029 (b9edfe8) into main (56a7638) will decrease coverage by 0.24%.
The diff coverage is 74.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7029      +/-   ##
==========================================
- Coverage   77.47%   77.24%   -0.24%     
==========================================
  Files         310      310              
  Lines       41647    41913     +266     
==========================================
+ Hits        32268    32377     +109     
- Misses       9379     9536     +157     

[teor2345]

This is a nice minimal design!

I think checking AttemptPending peers is a blocking change. If we decide to put this PR in a later release, that could also be blocking.

The rest of my comments provide context, ask for more context so we can check the design works as expected, or ask for docs so future developers and auditors can do those checks.

Feel free to ask me to find some of these details myself.

[teor2345]

It seems like the scope of this PR has grown a bit. I'm partly responsible for that, I think my suggestion about pending handshakes was out of scope. (And it's difficult to do securely.)

So I'd like to make sure all the code we add in this PR has a strong security motivation. That way, it's less likely to have other negative security impacts.

[teor2345]

Looks great!

I just want to double-check the logic in two places where we might be returning an incorrect value.

[teor2345]

Sorry about the Rust syntax, I didn't check it before suggesting

[arya2]

@Mergifyio refresh

[mergify]

refresh

✅ Pull request refreshed