refactor: Add thread safety annotation to BanMan::SweepBanned() #25149

1. 
   hebasto commented at 8:19 pm on May 16, 2022: member

   This PR adds a proper thread safety annotation to BanMan::SweepBanned().

   Also a simple refactoring applied.

2. DrahtBot added the label Refactoring on May 16, 2022
3. 
   fanquake commented at 9:42 pm on May 16, 2022: member

   ASAN job:

   02022-05-16T21:20:13.025586Z (mocktime: 1970-01-01T00:12:57Z) [test] [net_types.cpp:63] [BanMapFromJson] Dropping entry with unknown version (2) from ban list
   1Assertion failed: lock m_cs_banned not held in banman.cpp:176; locks held:
   2make[3]: *** [Makefile:20916: test/banman_tests.cpp.test] Error 1
   

4. 
   theStack commented at 10:37 pm on May 16, 2022: member

   Concept ACK

   Looks like a m_cs_banned lock acquire needs to add before the the SweepBanned call in the BanMan constructor to get rid of the failed assertion.

5. 
   ajtowns commented at 3:21 am on May 17, 2022: member

   Moving the code that’s in the constructor into a separate function, so that the constructor is just { LoadBanlist(); DumpBanlist(); } causes the compiler to give errors about the missing locks in LoadBanlist().
6. 
   MarcoFalke commented at 6:13 am on May 17, 2022: member

   A constructor shouldn’t need to lock a member mutex. Maybe we should fade out AssertLockHeld and remove it here?
7. hebasto force-pushed on May 17, 2022
8. hebasto force-pushed on May 17, 2022
9. 
   hebasto commented at 10:14 am on May 17, 2022: member

   Reworked: c4d0613bdf3beaf48fd35625cf2dc6018b059dbb -> f317ae181dc03cbe7004b63996dc7cf2c85c84db (pr25149.01 -> pr25149.02).

   PR description has been updated as well.

10. 
    ajtowns commented at 2:53 pm on May 17, 2022: member

    Doing:

     0BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time)
     1    : m_client_interface(client_interface), m_ban_db(std::move(ban_file)), m_default_ban_time(default_ban_time)
     2{
     3    LoadBanlist();
     4    DumpBanlist();
     5}
     6
     7void BanMan::LoadBanlist()
     8{
     9    LOCK(m_cs_banned);
    10
    11    if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated);
    12
    13    int64_t n_start = GetTimeMillis();
    14    if (m_ban_db.Read(m_banned)) {
    15        SweepBanned(); // sweep out unused entries
    16
    17        LogPrint(BCLog::NET, "Loaded %d banned node addresses/subnets  %dms\n", m_banned.size(),
    18                 GetTimeMillis() - n_start);
    19    } else {
    20        LogPrintf("Recreating the banlist database\n");
    21        m_banned = {};
    22        m_is_dirty = true;
    23    }
    24}
    

    looks cleaner to me, fwiw. Just explicitly taking the lock in the constructor seems simpler than delegating to SweepBanMan() which takes the lock then calls SweepBanMan_() too…

11. 
    DrahtBot commented at 5:35 am on May 18, 2022: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24097 (Replace RecursiveMutex m_cs_banned with Mutex, and rename it by hebasto)

    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.

12. 
    w0xlt commented at 4:32 pm on May 18, 2022: contributor

    Concept ACK
13. refactor: Move code from ctor into private `BanMan::LoadBanlist()`

    Co-authored-by: Anthony Towns <aj@erisian.com.au>

    3919059deb
14. refactor: Add thread safety annotation to `BanMan::SweepBanned()` 52c0b3e859
15. refactor: Remove redundant scope in `BanMan::SweepBanned()` ab75388320
16. hebasto force-pushed on May 20, 2022
17. 
    hebasto commented at 1:58 pm on May 20, 2022: member

    Applied @ajtowns’s suggestion which was supported by @w0xlt with :+1: .
18. 
    ajtowns commented at 3:52 pm on May 20, 2022: member

    ACK ab7538832043a6c15e45a178fb6bb6298a00108f
19. w0xlt approved
20. 
    w0xlt commented at 4:22 pm on May 20, 2022: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/25149/commits/ab7538832043a6c15e45a178fb6bb6298a00108f
21. theStack approved
22. 
    theStack commented at 1:40 pm on May 23, 2022: member

    Code-review ACK ab7538832043a6c15e45a178fb6bb6298a00108f

    Happy to see that another RecursiveMutex will bite the dust soon.

23. MarcoFalke merged this on May 24, 2022
24. MarcoFalke closed this on May 24, 2022

    ---

25. hebasto deleted the branch on May 24, 2022
26. sidhujag referenced this in commit 42d1144d6e on May 28, 2022
27. DrahtBot locked this on May 24, 2023

Contributors
hebasto fanquake theStack ajtowns MarcoFalke DrahtBot w0xlt

Labels
Refactoring