From 5e20e9ec3859205c220867ca49efb752b8edaacc Mon Sep 17 00:00:00 2001
From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Date: Wed, 26 Jan 2022 16:25:58 +0200
Subject: [PATCH 1/4] Prevent possible concurrent CBanDB::Write() calls

---
 src/banman.cpp | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/banman.cpp b/src/banman.cpp
index 95b927c1ff1..7fa489b0514 100644
--- a/src/banman.cpp
+++ b/src/banman.cpp
@@ -7,6 +7,7 @@
 
 #include <netaddress.h>
 #include <node/ui_interface.h>
+#include <sync.h>
 #include <util/system.h>
 #include <util/time.h>
 #include <util/translation.h>
@@ -39,6 +40,9 @@ BanMan::~BanMan()
 
 void BanMan::DumpBanlist()
 {
+    static Mutex dump_mutex;
+    LOCK(dump_mutex);
+
     SweepBanned(); // clean unused entries (if bantime has expired)
 
     if (!BannedSetIsDirty()) return;

From 33bda6ab87cc1b569e96da337296eb3e9ce6db1a Mon Sep 17 00:00:00 2001
From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Date: Wed, 26 Jan 2022 16:38:21 +0200
Subject: [PATCH 2/4] Fix data race condition in BanMan::DumpBanlist()

The m_is_dirty value being read in BannedSetIsDirty() can differ from
the value being set in SweepBanned(), i.e., be inconsistent with a
BanMan instance internal state.
---
 src/banman.cpp | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/banman.cpp b/src/banman.cpp
index 7fa489b0514..2b029198db3 100644
--- a/src/banman.cpp
+++ b/src/banman.cpp
@@ -43,9 +43,11 @@ void BanMan::DumpBanlist()
     static Mutex dump_mutex;
     LOCK(dump_mutex);
 
-    SweepBanned(); // clean unused entries (if bantime has expired)
-
-    if (!BannedSetIsDirty()) return;
+    {
+        LOCK(m_cs_banned);
+        SweepBanned();
+        if (!BannedSetIsDirty()) return;
+    }
 
     int64_t n_start = GetTimeMillis();
 

From 83c76467157bbca023bffda0f0bc2f01eb76a040 Mon Sep 17 00:00:00 2001
From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Date: Tue, 18 Jan 2022 19:13:40 +0200
Subject: [PATCH 3/4] Avoid calling BanMan::SweepBanned() twice in a row

---
 src/banman.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/banman.cpp b/src/banman.cpp
index 2b029198db3..87c1bbc89e2 100644
--- a/src/banman.cpp
+++ b/src/banman.cpp
@@ -43,16 +43,15 @@ void BanMan::DumpBanlist()
     static Mutex dump_mutex;
     LOCK(dump_mutex);
 
+    banmap_t banmap;
     {
         LOCK(m_cs_banned);
         SweepBanned();
         if (!BannedSetIsDirty()) return;
+        banmap = m_banned;
     }
 
     int64_t n_start = GetTimeMillis();
-
-    banmap_t banmap;
-    GetBanned(banmap);
     if (m_ban_db.Write(banmap)) {
         SetBannedSetDirty(false);
     }

From 99a6b699cd650f13d7200d344bf5e2d4b45b20ac Mon Sep 17 00:00:00 2001
From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Date: Wed, 26 Jan 2022 16:58:32 +0200
Subject: [PATCH 4/4] Fix race condition for SetBannedSetDirty() calls

Another thread can `SetBannedSetDirty(true)` while `CBanDB::Write()`
call being executed. The following `SetBannedSetDirty(false)`
effectively makes `m_is_dirty` flag value inconsistent with the actual
`m_banned` state. Such behavior can result in data loss, e.g., during
shutdown.
---
 src/banman.cpp | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/banman.cpp b/src/banman.cpp
index 87c1bbc89e2..b28e3f7f7cc 100644
--- a/src/banman.cpp
+++ b/src/banman.cpp
@@ -49,11 +49,12 @@ void BanMan::DumpBanlist()
         SweepBanned();
         if (!BannedSetIsDirty()) return;
         banmap = m_banned;
+        SetBannedSetDirty(false);
     }
 
     int64_t n_start = GetTimeMillis();
-    if (m_ban_db.Write(banmap)) {
-        SetBannedSetDirty(false);
+    if (!m_ban_db.Write(banmap)) {
+        SetBannedSetDirty(true);
     }
 
     LogPrint(BCLog::NET, "Flushed %d banned node addresses/subnets to disk  %dms\n", banmap.size(),