[addrman] De-duplicate Add() function #22627

This PR merges the two definitions of this overloaded function to reduce code duplication.

When these functions were introduced in https://github.com/bitcoin/bitcoin/commit/5fee401fe14aa6459428a26a82f764db70a6a0b9, there were multiple places that invoked Add() with a single addr and a vector of addrs each, so it made sense to overload the function. I could see how the small difference in log statement was more meaningful when a peer was added via IRC :)

Now, the definition of Add() that takes in a single address is only invoked from the hidden/test-only RPC addpeeraddress. These changes should not cause any observable difference, and are covered by the existing tests that use this RPC endpoint.

when a peer was added via IRC :)

IRC?

Before DNS seeds existed, bitcoin would connect to a specific IRC server, join a channel, and look for other nodes joining that channel, and then insert those to its IP address database.

Concept ACK

+3 −17 is nice

Before DNS seeds existed, bitcoin would connect to a specific IRC server, join a channel, and look for other nodes joining that channel, and then insert those to its IP address database.

Interesting. TIL. Thanks @sipa

I think Joinmarket does something similar for peers participating in coinjoin.

Concept ACK

Concept ACK https://github.com/bitcoin/bitcoin/pull/22627/commits/6ef302effb2f99a538855011f781481d0eb208e8

However this change breaks test\addrman_tests.cpp test. Maybe keeping the removed Add(const CAddress &addr ...) method as a proxy to bool Add(const std::vector<CAddress> &vAddr, ...) can prevent this.

bool Add(const CAddress &addr, const CNetAddr& source, int64_t nTimePenalty = 0)
        EXCLUSIVE_LOCKS_REQUIRED(!cs)
{
    std::vector<CAddress> vAddr{addr};
    return Add(vAddr, source, nTimePenalty);
}

oops, tests! marking as draft until I resolve 😛

Concept ACK

Concept ACK

The two CI failures seem unrelated. They both fail with:

test/miner_tests.cpp:219:32: error: use of undeclared identifier 'VERSIONBITS_TOP_BITS'

This PR doesn't touch miner_tests, and that file does not call CAddrMan::Add(). It passes locally on this branch as well as rebased on master. I suspect its something strange with the auto-merge to master, since #22277 was just merged.

That said, I'm going to leave this in a draft so it doesn't cause conflict with #20233- that change is more important :)

Concept ACK

The CI failures were fixed with #22630.

Concept ACK

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22697 (addrman: Remove CAddrMan::Clear() function by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

#20233 has been merged. I'm happy to review this once it's been rebased on master.

rebased on master, ready for review!

Code review ACK 60e0cbdd574bb9109bcad1e0c27c7936a534a0e7

Nice cleanup

crACK 60e0cbd

LGTM 🧉

ACK 60e0cbdd574bb9109bcad1e0c27c7936a534a0e7

This has been merged in f3dbd1c2b2bc2b8edae657f79a2d31820b428e86.