[addrman] De-duplicate Add() function#22627
[addrman] De-duplicate Add() function#22627amitiuttarwar wants to merge 1 commit intobitcoin:masterfrom
Conversation
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 |
Interesting. TIL. Thanks @sipa I think Joinmarket does something similar for peers participating in coinjoin. |
|
Concept ACK |
lsilva01
left a comment
lsilva01
left a comment
There was a problem hiding this comment.
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);
}
|
oops, tests! marking as draft until I resolve 😛 |
amitiuttarwar
force-pushed
the
2021-08-merge-add
branch
from
6ef302e to
830b5cc
Compare
|
Concept ACK |
|
Concept ACK |
|
The two CI failures seem unrelated. They both fail with:
This PR doesn't touch 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 |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Merge the two definitions of this overloaded function to reduce code duplication.
amitiuttarwar
force-pushed
the
2021-08-merge-add
branch
from
830b5cc to
60e0cbd
Compare
|
rebased on master, ready for review! |
|
Code review ACK 60e0cbd Nice cleanup |
60e0cbd [addrman] Merge the two Add() functions (Amiti Uttarwar) Pull request description: This PR merges the two definitions of this overloaded function to reduce code duplication. When these functions were introduced in bitcoin/bitcoin@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. ACKs for top commit: jnewbery: Code review ACK 60e0cbd Zero-1729: crACK 60e0cbd fanquake: ACK 60e0cbd Tree-SHA512: 782fb2ac6d2d403ba7d7ff543197ca42b610b9a8806952d271e57e2ee3527ad1a94af4ebbad5371b5e95d77df07c56ccc8c1d5a2c82cdecb0d2b5085b3bdd5ee
|
This has been merged in f3dbd1c. |
11 participants
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 RPCaddpeeraddress. These changes should not cause any observable difference, and are covered by the existing tests that use this RPC endpoint.