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

[hebasto]

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

Also a simple refactoring applied.

[fanquake]

ASAN job:

2022-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
Assertion failed: lock m_cs_banned not held in banman.cpp:176; locks held:
make[3]: *** [Makefile:20916: test/banman_tests.cpp.test] Error 1

[theStack]

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.

[ajtowns]

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

[maflcko]

A constructor shouldn't need to lock a member mutex. Maybe we should fade out AssertLockHeld and remove it here?

[hebasto]

Reworked: c4d0613 -> f317ae1 (pr25149.01 -> pr25149.02).

PR description has been updated as well.

[ajtowns]

Doing:

BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time)
    : m_client_interface(client_interface), m_ban_db(std::move(ban_file)), m_default_ban_time(default_ban_time)
{
    LoadBanlist();
    DumpBanlist();
}

void BanMan::LoadBanlist()
{
    LOCK(m_cs_banned);

    if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated);

    int64_t n_start = GetTimeMillis();
    if (m_ban_db.Read(m_banned)) {
        SweepBanned(); // sweep out unused entries

        LogPrint(BCLog::NET, "Loaded %d banned node addresses/subnets  %dms\n", m_banned.size(),
                 GetTimeMillis() - n_start);
    } else {
        LogPrintf("Recreating the banlist database\n");
        m_banned = {};
        m_is_dirty = true;
    }
}

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

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

• #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.

[w0xlt]

Concept ACK

[hebasto]

Applied @ajtowns's suggestion which was supported by @w0xlt with 👍 .

[ajtowns]

ACK ab75388

[w0xlt]

ACK ab75388

[theStack]

Code-review ACK ab75388

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