feat(wire): implement BanMan and make it single source of truth for bans by Micah-Shallom · Pull Request #926 · getfloresta/Floresta

Micah-Shallom · 2026-04-02T16:55:20Z

Description and Notes

Introduces BanMan for centralized ban management and makes it the single source of truth for bans. Removes AddressState::Banned and replaces scattered ban logic across AddressMan and peer_man.

The following are some of the main changes to get a good grasp:

rearrange_buckets accepts &BanMan and removes the banned IPs each cycle using retain()
disconnect_and_ban now removes banned IPs from addresses, good_addresses, and good_peers_by_service so banned peers aren't selected for connection
between rearranging cycles,, conn.rs ban check catches and regects a banned Ip trying to connect that way no banned peer actually connects even before the next cleanup cycle runs.

This PR is a consolidation of #892 and #912.
PR #899 tries to introduce support for CIDR/Subnet into BanMAN

To verify changes

cargo test -p floresta-wire

Closes #820.

moisesPompilio · 2026-04-02T17:03:36Z

Even if the PRs are being consolidated, it should have a clearer description of what’s being done. If possible, follow the PR template we have.

Micah-Shallom · 2026-04-02T17:05:13Z

okay @moisesPompilio ...editing the description right away

Vedd-Patel · 2026-04-06T20:57:57Z

        };

+        let net_address = peer_address.get_net_address();
+        if self.ban_man.is_banned(net_address) {


add a unit test that seeds a banned IP and verifies something like create_connection returns PeerBanned and no connection attempt is started yet

Vedd-Patel · 2026-04-06T20:58:09Z

+
+        // Purge banned IPs from the address database
+        self.addresses
+            .retain(|_, address| !banned_ips.contains(&address.get_net_address()));


banned IP cleanup in rearrange_buckets remove the entries only of addresses but doesnt synchronize good_addresses /good_peers_by_service or similars
this can leave stale ids and make enough_addresses overcounty which can alter peer-acquisition behaviors
while purging the ban address collecct the removed ids and then remove them from all indexes would fix this

Vedd-Patel · 2026-04-06T20:58:12Z

    }

+    /// Removes all addresses matching the given IP from the address book.
+    pub fn remove_address_by_ip(&mut self, ip: IpAddr) {


remove_address_by_ip removed from addresses/good_addresses etc but not peers_by_service
this creates stale service index entries and can slowly degrade lookup quality/performance
retain-filter all vectors in peers_by_service for removed ids also would help

Vedd-Patel · 2026-04-06T20:58:19Z

+
+        assert!(second_ban > first_ban);
+    }
+}


lint is unhappy due to missing empty line at the end of file ig

vedpatel@VEDs-MacBook-Air Floresta % cargo +nightly fmt --all -- --check Diff in /Users/vedpatel/Downloads/Floresta/crates/floresta-wire/src/p2p_wire/ban_man.rs:165: assert!(second_ban > first_ban); } } +

Davidson-Souza · 2026-04-08T16:32:34Z

    pub(crate) state: PeerStatus,

    /// An id identifying this peer's address in our address manager
+    #[allow(dead_code)]


a317a87

If it's not being used, just remove the field

This is not really fixed. Adding a _ is the same as allowing dead code.

the address_id field has been removed.... @Davidson-Souza

Davidson-Souza · 2026-04-08T16:33:09Z

                PeerStatus::Banned => {
-                    self.address_man
-                        .update_set_state(idx, AddressState::Banned(RunningNode::BAN_TIME));
+                    // Already removed from address_man in disconnect_and_ban
                }


a317a87

If this status isn't needed anymore, you can remove it

removed the comment but had to leave the PeerStatus::Banned => {} arm there bcus the enum requires all arms matched

But do we even need a PeerStatus banned?

@Davidson-Souza ....well looking around we actually do not need it anymore...it is majorly referenced in 2 places

the assignment here happens to be redundant

this can be replaced with ban_man.is_banned(peer.address) but will need to pass in either &self or &banman

passing it will affect only one call site on the codebase

so after alot of back and forths with the compiler.... i tried removing PeerStatus::Banned and replacing the check in is_peer_good with ban_man.is_banned(),,,but i kept getting borrow conflicts...since peers and ban_man live in NODECOMMON and go through DerefMut, the compiler didnt let me borrow them at the same time.....

the workaround was to extract the peer_address before get_mut and check ban status upfront...this ended up removing is_peer_good entirely and creating an inline check....

NOTE: this was an AI suggested change @Davidson-Souza

Wait, the behavior we want now if for peers to be removed from the addressmanager right ?

Davidson-Souza · 2026-04-08T16:34:56Z

+        // Purge banned IPs from the address database
+        self.addresses
+            .retain(|_, address| !banned_ips.contains(&address.get_net_address()));


a317a87

Why purging, tho? I think they can be removed from the good addresses buckets. However, if we prune them here, we will simply forget it forever.

nice catch @Davidson-Souza ...
purging was a bad design choice here... like you suggested, i introduced a quarantine move instead
quarantine removes banned_ip from good_addresses and good_peers_by_service and marks the banned ip as Failed...this way we still posses info about bannedIPs and this will prevent us from forgetting them..

Davidson-Souza · 2026-04-08T16:35:45Z


 impl BanMan {
-    /// Creates a new empty Banman
+    /// Creates a new empty `Banman`.


a317a87

Suggested change

/// Creates a new empty `Banman`.

/// Creates a new empty [`Banman`].

Davidson-Souza · 2026-04-14T18:13:10Z

+        if let Err(e) = self.ban_man.dump_bans(&self.datadir) {
+            tracing::warn!("Failed to dump bans: {e}");
+        }
+


Propagate this

this has been propagated @Davidson-Souza

Davidson-Souza · 2026-04-14T18:13:28Z

@@ -0,0 +1,215 @@
+use std::collections::HashMap;


This file needs a SPDX license identifier

Davidson-Souza · 2026-04-16T16:27:55Z

        &mut self,
        kind: ConnectionKind,
-        peer_id: usize,
+        _peer_id: usize,


Please remove all unused values (variables, structs, enum variants). Don't add an _ or allow deadcode.

fixed @Davidson-Souza

Micah-Shallom force-pushed the feat/wire-banman-consolidated branch from da6ad69 to 4a44223 Compare April 2, 2026 17:00

Micah-Shallom force-pushed the feat/wire-banman-consolidated branch from 4a44223 to a317a87 Compare April 2, 2026 18:12

moisesPompilio added the reliability Related to runtime reliability, stability and production readiness label Apr 4, 2026

moisesPompilio assigned Micah-Shallom Apr 4, 2026

Vedd-Patel reviewed Apr 6, 2026

View reviewed changes

Davidson-Souza requested changes Apr 8, 2026

View reviewed changes

Micah-Shallom force-pushed the feat/wire-banman-consolidated branch 5 times, most recently from 87043dc to bf0aabd Compare April 10, 2026 02:01

Micah-Shallom requested a review from Davidson-Souza April 10, 2026 20:37

Davidson-Souza requested changes Apr 14, 2026

View reviewed changes

Micah-Shallom force-pushed the feat/wire-banman-consolidated branch 3 times, most recently from 9423ba7 to 8f078a4 Compare April 15, 2026 06:18

Davidson-Souza reviewed Apr 16, 2026

View reviewed changes

Micah-Shallom added 2 commits April 17, 2026 01:30

feat(wire): implement BanMan for centralized ban management

4159efe

refactor(wire): make BanMan single source of truth for bans

22ae270

Micah-Shallom force-pushed the feat/wire-banman-consolidated branch from 8f078a4 to 22ae270 Compare April 17, 2026 00:56

	/// Creates a new empty `Banman`.
	/// Creates a new empty [`Banman`].

Conversation

Micah-Shallom commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description and Notes

To verify changes

Uh oh!

moisesPompilio commented Apr 2, 2026

Uh oh!

Micah-Shallom commented Apr 2, 2026

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!

Vedd-Patel Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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

Micah-Shallom commented Apr 2, 2026 •

edited

Loading

Vedd-Patel Apr 6, 2026 •

edited

Loading