feat(wire)!: Add a new `BitcoinSocketAddr` struct to unify address parsing by Davidson-Souza · Pull Request #984 · getfloresta/Floresta

Davidson-Souza · 2026-04-23T01:12:54Z

Description and Notes

Our code represents addresses in a scattered and messy way. Some places we use SocketAddr, some AddrV2 and some Strings. SocketAddr and Strings already wraps ports, while AddrV2 and IpAddr don't. Therefore some functions will ask for a u16 and other won't. We also have parsing logic for addresses all over the code, repeating logic and creating inconsistecies. Finally, in some APIs we support names, and others don't, which is frustrating.

This type will encapsulate both addresses and ports, as AddrV2 it will work with non-ip addresses such as Tor and I2P. It also supports name resolution.

It exposes a few conversion primitives to make it into other types if this is required in specific scopes, but overall we should use this type to represent Bitcoin node addresses.

Note: This used to be part of another PR to implement Onion addresses (we currently can't connect to .onion addresses), but got too big and I decided to split it up. Without these changes, it would be impossible to have those as well.

luisschwab · 2026-04-23T05:24:04Z

cACK. I was actually thinking about this lately. To me, it would make sense to eventually upstream this to bitcoin-p2p.

Davidson-Souza · 2026-04-29T21:19:39Z

Ready for a first round of reviews

lorenzolfm · 2026-05-01T16:45:10Z

+        // Don't connect to the same peer twice
+        let is_connected = |(_, peer_addr): (_, &LocalPeerView)| peer_addr.address == peer_address;
+
+        if self.common.peers.iter().any(is_connected) {
+            return Err(WireError::PeerAlreadyExists(peer_address));


NIT
You can use .values() to avoid the clunky |(_, peer_addr): (_, &LocalPeerView)| peer_addr.address == peer_address; syntax

:P

// Don't connect to the same peer twice if self .common .peers .values() .any(|p| p.address == peer_address) { return Err(WireError::PeerAlreadyExists(peer_address)); }

like you did here
https://github.com/getfloresta/Floresta/pull/984/changes#diff-44a1dbc03f87d4acd2d99bf96644515095dd1aed5bfd34b5213b6cc72f5e2f10R516

lorenzolfm · 2026-05-01T16:53:54Z

+                    0,
+                    AddressState::NeverTried,
+                    ServiceFlags::NONE,
+                    0,


maybe dumb question, but why is id hardcoded to zero?

These come from the AddrMan and we use it to ban/update that address. But since this is coming from somewhere else, we don't know if AddrMan have it or what's the internal id for it.

lorenzolfm · 2026-05-01T17:01:40Z

+        debug!("Trying to remove peer {addr}");

-        let address = self.to_addr_v2(addr);
        let index = self


NIT

let index = self .added_peers .iter() .position(|info| addr == info.address) .ok_or(WireError::PeerNotFoundAtAddress(addr))?; self.added_peers.remove(index);

if your in for a more idiomatic rust approach

lorenzolfm · 2026-05-01T17:05:55Z

    /// Handles the node request for immediate disconnection from a peer.
-    pub fn handle_disconnect_peer(&mut self, addr: IpAddr, port: u16) -> Result<(), WireError> {
+    pub fn handle_disconnect_peer(&mut self, addr: BitcoinSocketAddr) -> Result<(), WireError> {
        // Get the peer's index in the [`AddressMan`]'s list, if it exists.


also nit :p

// Get the peer's index in the [`AddressMan`]'s list, if it exists. let peer_id = self .peers .iter() .find_map(|(&id, peer)| (*peer.address.as_bitcoin_socket_addr() == addr).then_some(id)) .ok_or(WireError::PeerNotFoundAtAddress(addr))?; self.send_to_peer(peer_id, NodeRequest::Shutdown)

lorenzolfm · 2026-05-01T17:16:45Z

            .write_all(&[SOCKS_VERSION, 1, SOCKS_AUTH_METHOD_NONE])
-            .await
-            .unwrap();
+            .await?;


lorenzolfm

ACK ✅

lorenzolfm · 2026-05-01T17:35:53Z

+            Network::Signet => 38333,
+            Network::Bitcoin => 8333,
+            Network::Testnet => 18333,
+            Network::Regtest => 48444,


18444 i think

@Davidson-Souza use the get_p2p_port function I added I while back

@Davidson-Souza use the get_p2p_port function I added I while back

Where is this function? Maybe it was removed

Where is this function? Maybe it was removed

Floresta/crates/floresta-wire/src/p2p_wire/node/conn.rs#L475

It's named get_port. Maybe you could rename it to get_p2p_port.

Davidson-Souza · 2026-05-07T18:21:27Z

Pushed 7635a87 applying @lorenzolfm's code styling suggestions.

csgui · 2026-05-08T11:08:47Z

+            false,
+            "Valid address with out-of-range port",
+        )
+    }


Missing test coverage for CJDNS addresses.

Nice catch! Will add!

csgui · 2026-05-08T11:10:54Z

Those PRs changing several files are hard to reason about, but it seems consistent 👍 Maybe we should tackle validation with more tests and corner cases.

Davidson-Souza · 2026-05-08T17:52:52Z

Those PRs changing several files are hard to reason about, but it seems consistent 👍 Maybe we should tackle validation with more tests and corner cases.

Yeah, it's annoying. But I can't break it further because it wouldn't compile. Most of the changes however, are just changing types, so this is easy to use.

As for testing; most of the API parts changes are covered by our functional tests (addnode and connect mainly), I've added several parsing tests and fuzzing to make sure it won't break. Do you have any further suggestion for how to test it?

Davidson-Souza · 2026-05-11T16:39:41Z

Added tests for CJDNS

luisschwab · 2026-05-11T20:19:11Z

Needs rebase thanks to ~~@JoseSK999~~ @moisesPompilio

Davidson-Souza · 2026-05-12T13:46:35Z

Needs rebase thanks to @JoseSK999

It's actually @moisesPompilio's fault. @JoseSK999 didn't touch RPC.

luisschwab · 2026-05-12T14:05:14Z

Fixed lol

Our code represents addresses in a scattered and messy way. Some places we use SocketAddr, some AddrV2 and some Strings. SocketAddr and Strings already wraps ports, while AddrV2 and IpAddr don't. Therefore some functions will ask for a `u16` and other won't. We also have parsing logic for addresses all over the code, repeating logic and creating inconsistecies. Finally, in some APIs we support names, and others don't, which is frustrating. This type will encapsulate both addresses and ports, as AddrV2 it will work with non-ip addresses such as Tor and I2P. It also supports name resolution. It exposes a few conversion primitives to make it into other types if this is required in specific scopes, but overall we should use this type to represent Bitcoin node addresses.

Use BitcoinSocketAddr where we need to represent a node addressess, simplify some code and improve debug message for addresses.

…nSocketAddr This function was just calling BitcoinSocketAddr, and didn't have any reason to exist. The tests also tested BitcoinSocketAddr, instead of something inside `conn.rs`, so I've moved them.

Davidson-Souza · 2026-05-12T17:31:42Z

Rebased and fixed tests.

Davidson-Souza self-assigned this Apr 23, 2026

Davidson-Souza added this to Floresta Apr 23, 2026

Davidson-Souza added the reliability Related to runtime reliability, stability and production readiness label Apr 23, 2026

github-project-automation Bot moved this to Backlog in Floresta Apr 23, 2026

Davidson-Souza added the architecture label Apr 23, 2026

Davidson-Souza force-pushed the refactor/BitcoinSocketAddr branch from 2d92b13 to 45785e1 Compare April 28, 2026 23:37

Davidson-Souza changed the title ~~[Seeking Conceptual Review]feat(wire)!: Add a new BitcoinSocketAddr struct to unify address parsing~~ feat(wire)!: Add a new BitcoinSocketAddr struct to unify address parsing Apr 28, 2026

Davidson-Souza force-pushed the refactor/BitcoinSocketAddr branch 4 times, most recently from 1646cab to 4017323 Compare April 29, 2026 20:17

Davidson-Souza marked this pull request as ready for review April 29, 2026 21:19

Davidson-Souza moved this from Backlog to In progress in Floresta Apr 29, 2026

lorenzolfm reviewed May 1, 2026

View reviewed changes

Davidson-Souza force-pushed the refactor/BitcoinSocketAddr branch from 4017323 to 7635a87 Compare May 7, 2026 18:18

csgui reviewed May 8, 2026

View reviewed changes

csgui added this to the Q2/2026 milestone May 8, 2026

Davidson-Souza force-pushed the refactor/BitcoinSocketAddr branch from 7635a87 to 3381963 Compare May 11, 2026 16:21

Davidson-Souza moved this from In progress to Needs review in Floresta May 11, 2026

Davidson-Souza added 4 commits May 12, 2026 13:36

feat(wire)!: Use BitcoinSocketAddr everywhere

9b4564a

Use BitcoinSocketAddr where we need to represent a node addressess, simplify some code and improve debug message for addresses.

fuzz: add a bitcoin_socket_addr target

fde16ed

refactor(wire)!: remove resolve_connect_host and move tests to Bitcoi…

8470e4d

…nSocketAddr This function was just calling BitcoinSocketAddr, and didn't have any reason to exist. The tests also tested BitcoinSocketAddr, instead of something inside `conn.rs`, so I've moved them.

Davidson-Souza force-pushed the refactor/BitcoinSocketAddr branch from 3381963 to 8470e4d Compare May 12, 2026 16:57

Davidson-Souza mentioned this pull request May 14, 2026

refactor(wire): make node interface a trait #1035

Open

Conversation

Davidson-Souza commented Apr 23, 2026

Description and Notes

Uh oh!

luisschwab commented Apr 23, 2026

Uh oh!

Davidson-Souza commented Apr 29, 2026

Uh oh!

lorenzolfm May 1, 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!

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!

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

Choose a reason for hiding this comment

Uh oh!

luisschwab May 12, 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!

Davidson-Souza commented May 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csgui commented May 8, 2026

Uh oh!

Davidson-Souza commented May 8, 2026

Uh oh!

Davidson-Souza commented May 11, 2026

Uh oh!

luisschwab commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Davidson-Souza commented May 12, 2026

Uh oh!

luisschwab commented May 12, 2026

Uh oh!

Davidson-Souza commented May 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lorenzolfm May 1, 2026 •

edited

Loading

luisschwab May 12, 2026 •

edited

Loading

luisschwab commented May 11, 2026 •

edited

Loading