p2p: 26847 fixups (AddrMan totals)#27015
Conversation
Now that Size() performs internal consistency checks, it will rightfully fail (and assert) when dealing with a corrupted AddrMan. Therefore remove this check.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
lgtm ACK dc70c1e |
|
Going to merge this now to avoid further spurious CI failures. |
dc70c1e addrman: Use std::nullopt instead of {} (Martin Zumsande) 59cc66a test: Remove AddrMan unit test that fails consistency checks (Martin Zumsande) Pull request description: Two fixups for bitcoin#26847: * Now that `AddrMan::Size()` performs internal consistency tests (it didn't before), we can't call it in the `load_addrman_corrupted` unit tests, where we deal with an artificially corrupted `AddrMan`. This would fail the test when using `-checkaddrman=1` (leading to spurious CI fails). Therefore remove the tests assertion, which is not particularly helpful anyway (in production we abort init when peers.dat is corrupted instead of querying AddrMan in its corrupted state). (See bitcoin#26847 (comment)) * Use `std::nullopt` instead of `{}` for default args (suggested in bitcoin#26847 (comment)) ACKs for top commit: MarcoFalke: lgtm ACK dc70c1e Tree-SHA512: dd8a988e23d71a66d3dd30560bb653c9ad17db6915abfa5f722818b0ab18921051ec9223bfbc75d967df8bcd204dfe473d680bf68e8a8e4e4998fbb91dc973c5
| * @return Number of unique addresses that match specified options. | ||
| */ | ||
| size_t Size(std::optional<Network> net = {}, std::optional<bool> in_new = {}) const; | ||
| size_t Size(std::optional<Network> net = std::nullopt, std::optional<bool> in_new = std::nullopt) const; |
There was a problem hiding this comment.
This is more clear, but I'm wondering, since the default constructor for std::optional is nullopt (see case 1 here, this seems completely intuitive), would have been better to leave off these initializers?
There was a problem hiding this comment.
That would mean all callers have to always pass the arguments, even if they are std::nullopt, would not be possible to call it like addrman.Size().
There was a problem hiding this comment.
You're right, ignore my comment.
6 participants
Two fixups for #26847:
AddrMan::Size()performs internal consistency tests (it didn't before), we can't call it in theload_addrman_corruptedunit tests, where we deal with an artificially corruptedAddrMan. This would fail the test when using-checkaddrman=1(leading to spurious CI fails). Therefore remove the tests assertion, which is not particularly helpful anyway (in production we abort init when peers.dat is corrupted instead of querying AddrMan in its corrupted state).(See p2p: track AddrMan totals by network and table, improve precision of adding fixed seeds #26847 (comment))
std::nulloptinstead of{}for default args (suggested in p2p: track AddrMan totals by network and table, improve precision of adding fixed seeds #26847 (comment))