Page MenuHomePhabricator

Avoid calling CAddrMan::Connected() on block-relay-only peer addresses
ClosedPublic

Authored by PiRK on Dec 20 2021, 12:53.

Details

Summary

PR description:

This PR does two things:

  • Block-relay-only interaction with addrman.
    • Calling CAddrMan::Connected() on an address that was a block-relay-only peer causes the time we report in addr messages containing that peer to be updated; particularly now that we use anchor connections with a our block-relay-only peers, this risks leaking information about those peers. So, stop this.
    • Avoiding calling CAddrMan::Good() on block-relay-only peer addresses causes the addrman logic around maintaining the new and tried table to be less good, and in particular makes it so that block-relay-only peer addresses are more likely to be evicted from the addrman (for no good reason I can think of). So, mark those addresses as good when we connect.
  • Fix test-before-evict bug. There's a bug where if we get a collision in the tried table with an existing address that is one of our current peers, and the connection is long-lived enough, then SelectTriedCollisions() might return that existing peer address to us as a test-before-evict connection candidate. However, our logic for new outbound connections would later prevent us from actually making a connection; the result would be that when we get a collision with a long-lived current peer, that peer's address is likely to get evicted from the tried table. Fix this by checking to see if a test-before-evict candidate is a peer we're currently connected to, and if so, mark it as Good().

Commit description:

Connected() updates the time we serve in addr messages, so avoid leaking block-relay-only peer connections by avoiding these calls.

This is a backport of core#20187 [1/4]
https://github.com/bitcoin/bitcoin/pull/20187/commits/daf55531260833d597ee599e2d289ea1be0b1d9c

Test Plan

ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable