[addrman] De-duplicate Add() function

[amitiuttarwar]

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

When these functions were introduced in 5fee401, 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.

[ghost]

when a peer was added via IRC :)

IRC?

[sipa]

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.

[practicalswift]

Concept ACK

+3 −17 is nice

[ghost]

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.

[Zero-1729]

Concept ACK

[lsilva01]

Concept ACK 6ef302e

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);
}

[amitiuttarwar]

oops, tests! marking as draft until I resolve 😛

[0xB10C]

Concept ACK

[theStack]

Concept ACK

[amitiuttarwar]

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 :)

[jnewbery]

Concept ACK

[mzumsande]

The CI failures were fixed with #22630.

Concept ACK

[DrahtBot]

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

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.

[jnewbery]

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

[amitiuttarwar]

rebased on master, ready for review!

[jnewbery]

Code review ACK 60e0cbd

Nice cleanup

[Zero-1729]

crACK 60e0cbd

LGTM 🧉

[fanquake]

ACK 60e0cbd

[fanquake]

This has been merged in f3dbd1c.