feat(wire): allow NodeInterface to request peer banning by Micah-Shallom · Pull Request #872 · getfloresta/Floresta

Micah-Shallom · 2026-03-06T21:07:48Z

Description

Closes #809.

Adds ban_peer() to NodeInterface, allowing external consumers to ban a peer by address and port. Follows the same pattern as the existing disconnect_peer() flow. Internally calls the existing disconnect_and_ban() method.

AI was used for codebase understanding; but all code was written manually.

jaoleal

Thanks for taking a look at this.

jaoleal · 2026-03-08T21:50:49Z

crates/floresta-wire/src/p2p_wire/node/user_req.rs

+
+            UserRequest::BanPeer((addr, port)) => {
+                let node_response = match self.handle_ban_peer(addr, port) {
+                    Ok(_) => {


Suggested change

Ok(_) => {

Ok(()) => {

jaoleal · 2026-03-08T22:05:57Z

crates/floresta-wire/src/p2p_wire/node/peer_man.rs

+    pub fn handle_ban_peer(&mut self, addr: IpAddr, port: u16) -> Result<(), WireError> {
+        let peer_id = self
+            .peers
+            .iter()
+            .find(|(_, peer)| addr == peer.address && port == peer.port)
+            .map(|(&peer_id, _)| peer_id);
+
+        match peer_id {
+            Some(peer_id) => {
+                self.disconnect_and_ban(peer_id)?;
+                Ok(())
+            }
+            None => Err(WireError::PeerNotFoundAtAddress(addr, port)),
+        }
+    }


Here some suggestions:

I think that you can make port optional, you can also simplify the handling on 518.

/// Bans a peer for `T::BAN_TIME`. pub fn handle_ban_peer(&mut self, addr: IpAddr, port: Option<u16>) -> Result<(), WireError> { let peer_id = self .peers .iter() .find(|(_, peer)| { addr == peer.address && port.unwrap_or(get_port(&self.network)) == peer.port }) .map(|(&peer_id, _)| peer_id); match peer_id { Some(peer_id) => self.disconnect_and_ban(peer_id), None => Err(WireError::PeerNotFoundAtAddress( addr, port.unwrap_or(get_port(&self.network)), )), } }

Also, it appears that Bitcoin Core allows banning peers that arent known by the time. cc @Davidson-Souza

Micah-Shallom · 2026-03-09T01:35:02Z

hi @jaoleal ...thank you for your review.. i have addressed your suggestions.
Also regarding banning unknown peers, i will be happy to implement that if @Davidson-Souza confirms it's in scope for this PR, or i can follow up in a seperate PR

jaoleal · 2026-03-09T14:22:10Z

Okay, the changes LGTM.

Davidson-Souza · 2026-03-09T19:20:52Z

crates/floresta-wire/src/p2p_wire/node/peer_man.rs

        Ok(())
    }

+    /// Bans a peer for `T::BAN_TIME`.


This works only for connected peers, right? I think the comment should mention that

Davidson-Souza · 2026-03-09T19:21:46Z

crates/floresta-wire/src/p2p_wire/node/peer_man.rs

+            .peers
+            .iter()
+            .find(|(_, peer)| {
+                addr == peer.address && port.unwrap_or(Self::get_port(self.network)) == peer.port


Perhaps if a port isn't provided, we should ban all peers on that ip?

Wdyt @luisschwab?

Just chiming in.

It seems that banning all peers is the way to go. Right now None is the default port number.

ban_peer(1.2.3.4, None) -> ban_peer(1.2.3.4, 8333)

But an attacker could reconnect on 1.2.3.4:50000

Davidson-Souza · 2026-03-09T19:22:11Z

crates/floresta-wire/src/p2p_wire/node/peer_man.rs

+            Some(peer_id) => self.disconnect_and_ban(peer_id),
+            None => Err(WireError::PeerNotFoundAtAddress(
+                addr,
+                port.unwrap_or(Self::get_port(self.network)),


And return an option here

Davidson-Souza · 2026-03-09T19:22:45Z

crates/floresta-wire/src/p2p_wire/node/user_req.rs

+            UserRequest::BanPeer((addr, port)) => {
+                let node_response = match self.handle_ban_peer(addr, port) {
+                    Ok(()) => {
+                        info!("Banned peer {addr}:{}",port.unwrap_or(Self::get_port(self.network)));


Suggested change

info!("Banned peer {addr}:{}",port.unwrap_or(Self::get_port(self.network)));

info!("Banned peer {addr}:{}", port.unwrap_or(Self::get_port(self.network)));

luisschwab · 2026-03-09T19:30:11Z

I think we should have an actual BanMan before tackling this one.

jaoleal · 2026-03-09T19:42:49Z

I think we should have an actual BanMan before tackling this one.

I think the proposed PR aggregates on floresta as it is. Cant we work on a BanMan after ? I say that because i thought this would enable setban rpc method, by looking at is specs it appears that such a BanMan needs its own PR.

We could open a tracking issue for that, I did some looking around of what we need to satisfy setban and the changes are trivial.

Davidson-Souza · 2026-03-09T21:17:53Z

@luisschwab

I think we should have an actual BanMan before tackling this one.

This interface (which is pretty much what #809 asks for) is not the same as setban. setban takes in an ip address only, and idk if it disconnects already connected peers. Maybe using just this could be useful? For bonsai this would basically be what you need IIRC.

Since you opened the issue, it's up to you whether this is useful or not.

Edit: forgot to quote

luisschwab · 2026-03-09T21:31:46Z

setban takes in an ip address only

No, it takes extra arguments: https://developer.bitcoin.org/reference/rpc/setban.html

I think these should also be exposed to the NodeInterface instead of just banning a peer absolutely. This approach also won't allow banning a whole subnet, as we can only pass an IP address.

Davidson-Souza · 2026-03-09T21:37:11Z

setban takes in an ip address only

No, it takes extra arguments: https://developer.bitcoin.org/reference/rpc/setban.html

I meant it doesn't take a port, just an ip. I like the idea of adding a bantime. Banning forever isn't a good idea.

Since the way to go seems to be adding the BanMan to ban a range, we should merge #820 and #809 and better define the approach.

luisschwab · 2026-03-09T22:08:12Z

we should merge #820 and #809 and better define the approach.

done

Micah-Shallom · 2026-03-10T10:34:20Z

thanks for your reviews @jaoleal @Davidson-Souza @luisschwab ...seeing that the conversation is pretty much drifting into the banman implementation #820 . If y'all will be okay with me looking into it, i could go through the docs and the existing banman code over the next few days and see what i can come up with.. if thats fine i'll appreciate being assigned to it @luisschwab 🙏🏾🙏🏾

csgui · 2026-03-10T11:00:42Z

crates/floresta-wire/src/p2p_wire/node/peer_man.rs

+            .peers
+            .iter()
+            .find(|(_, peer)| {
+                addr == peer.address && port.unwrap_or(Self::get_port(self.network)) == peer.port


Just chiming in.

It seems that banning all peers is the way to go. Right now None is the default port number.

ban_peer(1.2.3.4, None) -> ban_peer(1.2.3.4, 8333)

But an attacker could reconnect on 1.2.3.4:50000

csgui · 2026-03-10T11:04:23Z

crates/floresta-wire/src/p2p_wire/node/peer_man.rs

+            Some(peer_id) => self.disconnect_and_ban(peer_id),
+            None => Err(WireError::PeerNotFoundAtAddress(
+                addr,
+                port.unwrap_or(Self::get_port(self.network)),


port.unwrap_or(Self::get_port(self.network)) is being computed twice.

csgui · 2026-03-10T11:12:52Z

crates/floresta-wire/src/p2p_wire/node_interface.rs

    TransactionBroadcastResult(Result<Txid, AcceptToMempoolError>),
+
+    /// A response indicating whether a peer was successfully banned.
+    BanPeer(bool),


Have you considered a richer type instead a bool?

Only bool loses information. What happens with the error computed in handle_ban_peer if the bool is false?

csgui · 2026-03-10T11:18:31Z

crates/floresta-wire/src/p2p_wire/node_interface.rs

    Disconnect((IpAddr, u16)),

+    /// Ban a peer by its address.
+    BanPeer((IpAddr, Option<u16>)),


This look odd to users of the API. If the default port inference is useful here, it should probably also apply to Disconnect (and also other operations like Remove) since they are "similar" operations.

Should we keep port an optional? Any take on that @Davidson-Souza, @luisschwab, @jaoleal ?

Micah-Shallom force-pushed the wire/add-ban-peer-to-node-interface branch from 65d637f to 2256574 Compare March 6, 2026 21:21

moisesPompilio assigned Micah-Shallom Mar 7, 2026

moisesPompilio added enhancement New feature or request BDK Floresta Issues related to the integration of Floresta in BDK labels Mar 7, 2026

jaoleal reviewed Mar 8, 2026

View reviewed changes

Micah-Shallom force-pushed the wire/add-ban-peer-to-node-interface branch from 2256574 to 2beb1b9 Compare March 9, 2026 01:31

jaoleal requested review from Davidson-Souza, JoseSK999 and luisschwab and removed request for JoseSK999 March 9, 2026 14:22

Davidson-Souza reviewed Mar 9, 2026

View reviewed changes

csgui reviewed Mar 10, 2026

View reviewed changes

feat(wire): allow NodeInterface to request peer banning

3ba5b8e

Micah-Shallom force-pushed the wire/add-ban-peer-to-node-interface branch from 2beb1b9 to 3ba5b8e Compare March 10, 2026 11:51

	info!("Banned peer {addr}:{}",port.unwrap_or(Self::get_port(self.network)));
	info!("Banned peer {addr}:{}", port.unwrap_or(Self::get_port(self.network)));

Conversation

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

Uh oh!

Description

Uh oh!

jaoleal 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!

Micah-Shallom commented Mar 9, 2026

Uh oh!

jaoleal commented Mar 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luisschwab commented Mar 9, 2026

Uh oh!

jaoleal commented Mar 9, 2026

Uh oh!

Davidson-Souza commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luisschwab commented Mar 9, 2026

Uh oh!

Davidson-Souza commented Mar 9, 2026

Uh oh!

luisschwab commented Mar 9, 2026

Uh oh!

Micah-Shallom commented Mar 10, 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Micah-Shallom commented Mar 6, 2026 •

edited

Loading

Davidson-Souza commented Mar 9, 2026 •

edited

Loading