Replace RecursiveMutex m_cs_banned with Mutex, and rename it

[hebasto]

This PR is an alternative to #24092. Last two commit have been cherry-picked from the latter.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, vasild, maflcko, achow101
Concept ACK	shaavan
Stale ACK	w0xlt, promag

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

Conflicts

No conflicts as of last run.

[w0xlt]

tACK 39e101f

In addition to the refactoring, this PR:

. Adds lock acquisition in BanMan::DumpBanlist before calling SweepBanned().

. Replaces BanMan::SetBannedSetDirty() with WITH_LOCK(m_cs_banned, m_is_dirty = false); (write operation) or simply m_is_dirty (non-blocking read operation).

. SweepBanned() no longer locks the mutex, just checks that it is held. This function is now called with WITH_LOCK(m_banned_mutex, SweepBanned());.

. BanMan::DumpBanlist called BanMan::SweepBanned() twice (directly and via BanMan::GetBanned). Now it gets rid of BanMan::GetBanned and handles banmap_t banmap within the lock scope.

[shaavan]

Code Review ACK 39e101f

I prefer that the SweepBanned function, instead of locking m_cs_banned by itself, now asserts it being locked by its caller function. This prevents any need for double locking, and hence m_cs_banned could be safely converted from RecursiveMutex to Mutex.

The renaming of m_cs_banned to m_banned_mutex is an improvement, and I agree with it.

The conversion of m_banned_mutex from RecursiveMutex to Mutex is very cleanly implemented and is clutter-free.

[promag]

Code review ACK 39e101f.

There is a behavior change where m_client_interface->BannedListChanged() is now called with the lock held. The only place that connects to that signal is in ClientModel where updateBanlist is called asynchronously, so I'd say this change is deadlock-free.

[hebasto]

@promag

There is a behavior change where m_client_interface->BannedListChanged() is now called with the lock held.

The same behavior is on the master branch:

bitcoin/src/banman.cpp

Lines 160 to 166 in d0bf9bb

	void BanMan::GetBanned(banmap_t& banmap)
	{
	LOCK(m_cs_banned);
	// Sweep the banlist so expired bans are not returned
	SweepBanned();
	banmap = m_banned; //create a thread safe copy
	}

isn't it?

[promag]

@hebasto right! Only checked that from DumpBanlist the lock isn't held, but from GetBanned it is. So this change makes it consistent 🎉

[hebasto]

@MarcoFalke

Strictly speaking, fixing the logical race is a bugfix, so it might be better to do it first to allow easier cherry-picking to older commits. Would you mind going with the fix first?

I split bugfixes into #24168.

---

Fellow reviewers, please review #24168 first.

Converting this to a draft for now.

[hebasto]

Rebased 39e101f -> 0ad0288 (pr24097.02 -> pr24097.03) on top of the merged #24168.

[shaavan]

Concept ACK

I had a nit suggestion and a doubt that I would like to discuss.

[hebasto]

Converted to draft for now. Please review #25149 first.

[hebasto]

@maflcko Mind having a look at this PR please?

[maflcko]

ACK 37d150d 🎾

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 🎾
6hI0SRliStMofz4F+1OOUsdVFjJFjIicGUMlRpqAROJJGilc1hwLblhiZg1bxrodqlBVD+O8wLbCbdwCzCQPCA==

[hebasto]

@fanquake @achow101

Is there anything else to do here?

[achow101]

ACK 37d150d