Skip to content

Conversation

@mzumsande
Copy link
Contributor

@mzumsande mzumsande commented Oct 19, 2022

While testing #20018, I noticed that we sometimes make an AddrFetch connection to a node a DNS seed resolves to (if we have a -proxy set), but then disconnect them because we don't like their services. The same could happen for a manually added -seednode.
However, we only want addresses from these peers, which are not related to any of the current service flags, so I don't see a point in aborting an AddrFetch attempt due to undesirable services.

We only want a GETADDR response from them, which is
not related to any of the current service flags, so
there is no point in disconnecting them for undesirable
services.
@DrahtBot DrahtBot added the P2P label Oct 19, 2022
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 19, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@dergoegge
Copy link
Member

However, we only want addresses from these peers, which are not related to any of the current service flags, so I don't see a point in aborting an AddrFetch attempt due to undesirable services.

MayHaveUsefulAddressDB makes me think we that we are assuming nodes signaling NODE_NETWORK or NODE_NETWORK_LIMITED to have a useful addr db.

bitcoin/src/protocol.h

Lines 352 to 359 in 2ac71d2

/**
* Checks if a peer with the given service flags may be capable of having a
* robust address-storage DB.
*/
static inline bool MayHaveUsefulAddressDB(ServiceFlags services)
{
return (services & NODE_NETWORK) || (services & NODE_NETWORK_LIMITED);
}

@mzumsande
Copy link
Contributor Author

mzumsande commented Oct 20, 2022

MayHaveUsefulAddressDB makes me think we that we are assuming nodes signaling NODE_NETWORK or NODE_NETWORK_LIMITED to have a useful addr db.

Interesting! I guess we could leave ExpectServicesFromConn as is but check for MayHaveUsefulAddressDB() instead of HasAllDesirableServiceFlags() for an AddrFetch connections when deciding whether to accept the peer.

However, I'm not sure this would be ideal, for the following reasons:

  • MayHaveUsefulAddressDB is used to decide whether we want to connect to a feeler, and whether to process a received addr at all. In neither of these situations we particularly care about the peer's address db (we don't even request addrs from feelers), we care whether it is a (pruned or not) full node, so the function might better be named IsFullNode().
  • MayHaveUsefulAddressDB just makes the assumption that SPV clients don't know about address relay when they can't serve blocks - that may be the case for most current software, but I don't see why a SPV node shouldn't implement addr relay.
  • When we do an AddrFetch connection, we usually don't have a bunch of alternative addrs (otherwise there would be no need to make such a connection in the first place) - we either manually specified this peer (-seednode), it's a peer a DNS seed resolves to, or (if net: Make AddrFetch connections to fixed seeds #26114 makes it in) it's a fixed seed. When we do make an AddrFetch connection and the peer doesn't deliver addrs, we disconnect them after a few minutes. So I can't see the hurt in trying, even if the service bits don't match, if we don't have a better alternative.

@mzumsande
Copy link
Contributor Author

closing, there doesn't seem to be much interest and even though I still think it could make sense, it's certainly not critical.

@mzumsande mzumsande closed this Apr 3, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Apr 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants