banman: schedule sweep at ban expiry instead of polling by Bortlesboat · Pull Request #277 · bitcoinknots/bitcoin

Bortlesboat · 2026-03-06T12:17:59Z

The ban table only refreshed on BannedListChanged events (active ban/unban), not when bans expired naturally. This left stale entries visible in the GUI after their ban duration elapsed.

Give BanMan access to CScheduler so it can fire SweepBanned() at the exact moment the next ban expires, following the same StartScheduledTasks() pattern as PeerManager. Ban() compares the new ban's expiry directly against the tracked m_next_sweep_time and only reschedules when moving the time forward (O(1), no loop over all bans).

A generation counter (m_sweep_seq) invalidates previously-queued callbacks when the schedule changes. Stale callbacks check their captured sequence number and bail immediately without locking or sweeping.

Also fixes an off-by-one in SweepBanned(): uses >= instead of > so entries are swept at the exact expiry second, consistent with how IsBanned() checks current_time < nBanUntil.

Fixes #273

kwsantiago

utACK adbd0a1

luke-jr · 2026-03-16T14:30:42Z

Seems like we should instead trigger BannedListChanged when they expire?

Bortlesboat · 2026-03-16T15:00:04Z

Yeah I thought about that. The tricky part is BanMan doesn't have any timer infrastructure — it just sweeps lazily whenever IsBanned()/GetBanned() happen to get called. To fire on actual expiry we'd need to hook into CScheduler and recalculate the next-expiry time on every ban add/remove.

Went with the GUI timer since it keeps the change small and matches how PeerTableModel already handles its own refresh. But I can see the argument for making BanMan expiry-aware — would benefit bitcoind too, not just Qt. Happy to rework if you'd prefer that direction.

luke-jr · 2026-03-16T16:40:43Z

Makes more sense than polling IMO

Bortlesboat · 2026-03-16T19:00:56Z

Reworked per feedback. Replaced the GUI-level QTimer polling with event-driven expiry at the BanMan level.

BanMan now takes a CScheduler reference via StartScheduledTasks() (same pattern as PeerManager). When a ban is added or on startup, ScheduleNextSweep() finds the earliest nBanUntil and schedules a one-shot SweepBanned() for that exact time. The sweep fires BannedListChanged which the GUI already listens to.

Multiple outstanding scheduled sweeps are harmless since SweepBanned() is idempotent. No cancellation needed — stale sweeps are no-ops.

Shutdown is safe: scheduler->stop() joins the thread before banman.reset().

src/banman.cpp

luke-jr · 2026-03-20T15:51:48Z

src/banman.cpp

        if (m_banned[sub_net].nBanUntil < ban_entry.nBanUntil) {
            m_banned[sub_net] = ban_entry;
            m_is_dirty = true;
+            ScheduleNextSweep();


Don't we need to cancel/replace the existing schedule? Otherwise we will end up with a scheduled recurring task for every ban we make?

Also, we shouldn't need to loop over all bans here, just determine if we're moving the task forward (compare to previous soonest-expiring).

Good catches, both fixed. Added m_next_sweep_time to track the currently-scheduled expiry. Ban() now just compares ban_entry.nBanUntil < m_next_sweep_time — only reschedules when moving it forward, no loop. SweepBannedAndSchedule() resets the tracker before finding the next one.

CScheduler doesn't have a cancel API, so a stale callback can still fire if we reschedule earlier, but it's a no-op — SweepBanned() finds nothing to remove and ScheduleNextSweep() sees the existing schedule is already correct.

Also noticed SweepBanned() was using now > while IsBanned() uses now < — off-by-one at the boundary. Changed to >= so the sweep actually removes the entry when the scheduler fires at the exact expiry second.

I don't think this solves the multiple-callback-loop issue.

Pushed a rework addressing all three:

Removed the dead guard, callers always reset m_next_sweep_time first.

Split the >= fix into its own commit.

Added m_sweep_seq generation counter. Each schedule increments the counter and the callback captures it. Stale callbacks check expected_seq != m_sweep_seq and bail immediately — no sweep, no reschedule cascade.

luke-jr · 2026-03-20T16:53:39Z

src/banman.cpp

+        }
+    }
+
+    if (earliest >= m_next_sweep_time) return;


m_next_sweep_time is always std::numeric_limits<int64_t>::max() when we get here

luke-jr · 2026-03-20T16:54:26Z

src/banman.cpp

-        if (!sub_net.IsValid() || now > ban_entry.nBanUntil) {
+        if (!sub_net.IsValid() || now >= ban_entry.nBanUntil) {


Suggest giving this its own commit (in this same PR)

IsBanned() considers a ban inactive when current_time >= nBanUntil (it checks current_time < nBanUntil), but SweepBanned() only removed entries when now > nBanUntil. This left a one-second window where a ban was no longer enforced but the entry had not yet been swept. Use >= so entries are swept at the exact second the ban expires, consistent with the enforcement check.

luke-jr · 2026-03-29T02:14:52Z

src/init.cpp

        banman->DumpBanlist();
    }, DUMP_BANS_INTERVAL);

+    banman->StartScheduledTasks(scheduler);


Do we need this outside of the GUI? Maybe defer it until the first time handleBannedListChanged (node/interfaces.cpp) is called? (The first call shouldcheck m_next_sweep_time (which needs to be updated unconditionally) and trigger SweepBanned)

Reworked. init.cpp now just stores the scheduler pointer via SetScheduler(). Scheduling activates lazily on first handleBannedListChanged connection through an idempotent EnsureSweepScheduled(). Ban() won't queue sweep callbacks until a GUI consumer has connected. Headless nodes never schedule sweeps.

The ban table only refreshed on BannedListChanged events (active ban/unban), not when bans expired naturally. This left stale entries visible in the GUI after their ban duration elapsed. Give BanMan access to CScheduler so it can fire SweepBanned() at the exact moment the next ban expires. Ban() compares the new ban's expiry directly against the tracked m_next_sweep_time and only reschedules when moving the time forward (O(1), no loop over all bans). Sweep scheduling is deferred until the first GUI consumer connects via handleBannedListChanged, so headless nodes never schedule sweeps. EnsureSweepScheduled() is idempotent — a m_sweep_started flag prevents duplicate activation from multiple handler connections. A generation counter (m_sweep_seq) invalidates previously-queued callbacks when the schedule changes. Stale callbacks check their captured sequence number and bail immediately without locking or sweeping. Fixes bitcoinknots#273

luke-jr

ACK 60244b2 (with some minor suggestions)

luke-jr · 2026-04-10T19:12:41Z

src/banman.cpp

+    SweepBanned();
+    m_next_sweep_time = std::numeric_limits<int64_t>::max();
+    ScheduleNextSweep();


Just call SweepBannedAndSchedule?

luke-jr · 2026-04-10T19:13:00Z

src/banman.cpp

+    LOCK(m_banned_mutex);
+    if (expected_seq != m_sweep_seq) return;
+    SweepBanned();
+    m_next_sweep_time = std::numeric_limits<int64_t>::max();


This line feels like it belongs inside ScheduleNextSweep

IsBanned() considers a ban inactive when current_time >= nBanUntil (it checks current_time < nBanUntil), but SweepBanned() only removed entries when now > nBanUntil. This left a one-second window where a ban was no longer enforced but the entry had not yet been swept. Use >= so entries are swept at the exact second the ban expires, consistent with the enforcement check. Github-Pull: #277 Rebased-From: 13398b9

The ban table only refreshed on BannedListChanged events (active ban/unban), not when bans expired naturally. This left stale entries visible in the GUI after their ban duration elapsed. Give BanMan access to CScheduler so it can fire SweepBanned() at the exact moment the next ban expires. Ban() compares the new ban's expiry directly against the tracked m_next_sweep_time and only reschedules when moving the time forward (O(1), no loop over all bans). Sweep scheduling is deferred until the first GUI consumer connects via handleBannedListChanged, so headless nodes never schedule sweeps. EnsureSweepScheduled() is idempotent — a m_sweep_started flag prevents duplicate activation from multiple handler connections. A generation counter (m_sweep_seq) invalidates previously-queued callbacks when the schedule changes. Stale callbacks check their captured sequence number and bail immediately without locking or sweeping. Fixes #273 Github-Pull: #277 Rebased-From: 60244b2

Ari4ka approved these changes Mar 6, 2026

View reviewed changes

kwsantiago approved these changes Mar 6, 2026

View reviewed changes

Bortlesboat force-pushed the fix-ban-table-refresh branch from adbd0a1 to f22692a Compare March 16, 2026 19:00

Bortlesboat changed the title ~~qt: Add periodic refresh to ban table to remove expired bans~~ banman: schedule sweep at ban expiry instead of polling Mar 16, 2026

rleed suggested changes Mar 18, 2026

View reviewed changes

src/banman.cpp Show resolved Hide resolved

rleed approved these changes Mar 19, 2026

View reviewed changes

Ari4ka approved these changes Mar 19, 2026

View reviewed changes

luke-jr requested changes Mar 20, 2026

View reviewed changes

Bortlesboat force-pushed the fix-ban-table-refresh branch from f22692a to c231d4d Compare March 20, 2026 16:35

luke-jr requested changes Mar 20, 2026

View reviewed changes

Ari4ka approved these changes Mar 20, 2026

View reviewed changes

Bortlesboat force-pushed the fix-ban-table-refresh branch from c231d4d to 87325b4 Compare March 20, 2026 19:15

luke-jr reviewed Mar 29, 2026

View reviewed changes

Bortlesboat force-pushed the fix-ban-table-refresh branch from 87325b4 to 60244b2 Compare March 29, 2026 02:38

Ari4ka approved these changes Mar 29, 2026

View reviewed changes

luke-jr added this to the 29.3 (2nd) milestone Apr 3, 2026

luke-jr approved these changes Apr 10, 2026

View reviewed changes

		if (!sub_net.IsValid() \|\| now > ban_entry.nBanUntil) {
		if (!sub_net.IsValid() \|\| now >= ban_entry.nBanUntil) {

Conversation

Bortlesboat commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kwsantiago left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Mar 16, 2026

Uh oh!

Bortlesboat commented Mar 16, 2026

Uh oh!

luke-jr commented Mar 16, 2026

Uh oh!

Bortlesboat commented Mar 16, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Bortlesboat commented Mar 6, 2026 •

edited

Loading