p2p: 26847 fixups (AddrMan totals) #27015

1. 
   mzumsande commented at 3:41 pm on February 1, 2023: contributor

   Two fixups for #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 #26847 (comment))
   • Use std::nullopt instead of {} for default args (suggested in #26847 (review))
2. test: Remove AddrMan unit test that fails consistency checks

   Now that Size() performs internal consistency checks,
   it will rightfully fail (and assert) when dealing with
   a corrupted AddrMan. Therefore remove this check.

   59cc66abb9
3. addrman: Use std::nullopt instead of {} dc70c1eb08
4. 
   DrahtBot commented at 3:42 pm on February 1, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	MarcoFalke

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

5. DrahtBot added the label P2P on Feb 1, 2023
6. fanquake requested review from vasild on Feb 1, 2023
7. fanquake requested review from achow101 on Feb 1, 2023
8. fanquake requested review from naumenkogs on Feb 1, 2023
9. 
   maflcko commented at 3:46 pm on February 1, 2023: member

   lgtm ACK dc70c1eb08ba8f0e77ac0810312a67468ade9419
10. 
    fanquake commented at 3:56 pm on February 1, 2023: member

    Going to merge this now to avoid further spurious CI failures.
11. fanquake merged this on Feb 1, 2023
12. fanquake closed this on Feb 1, 2023

    ---

13. sidhujag referenced this in commit 4732901cd5 on Feb 1, 2023
14. in src/addrman.h:109 in dc70c1eb08

    105@@ -106,7 +106,7 @@ class AddrMan
    106     * @param[in] in_new           Select addresses only from one table (true = new, false = tried, nullopt = both)
    107     * @return                     Number of unique addresses that match specified options.
    108     */
    109-    size_t Size(std::optional<Network> net = {}, std::optional<bool> in_new = {}) const;
    110+    size_t Size(std::optional<Network> net = std::nullopt, std::optional<bool> in_new = std::nullopt) const;
    

    ---

    ---

    

    LarryRuane commented at 11:59 pm on February 2, 2023:

    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?

    ---

    

    vasild commented at 2:44 pm on February 9, 2023:

    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().

    ---

    

    LarryRuane commented at 4:46 pm on February 9, 2023:

    You’re right, ignore my comment.
15. 
    LarryRuane commented at 0:02 am on February 3, 2023: contributor

    post-merge ACK
16. mzumsande deleted the branch on May 4, 2023
17. bitcoin locked this on May 3, 2024

Contributors
mzumsande DrahtBot maflcko fanquake LarryRuane vasild

Review Requested
vasild achow101 naumenkogs

Labels
P2P