fix: another bundle of small fixes for how we select peers by Davidson-Souza · Pull Request #904 · getfloresta/Floresta

Davidson-Souza · 2026-03-18T22:46:50Z

Description and Notes

This PR bundles a couple of small changes intended to make our node more effective at finding Utreexo peers. See each commit message for more details about what they are changing and why.

Changelog:

fix(addr_man: don't return connected addresses in get_address_by_service
fix: update update_set_service_flag to require NETWORK_LIMITED
test: add a new test for adding fixed peers on addr_man
fix(sync_ctx): make it more aggressive at oppening utreexo connections
fix: update excess peer's data
fix: fix service bits for some hardcoded addresses

JoseSK999 · 2026-03-18T23:08:05Z

crates/floresta-wire/src/p2p_wire/seeds/signet_seeds.json

            "Tried": 0
        },
-        "services": 1037,
+        "services": 1033,


Currently we can't connect to these, if these don't support P2Pv2. Just noting this.

We need to test all those peers to check if they are still alive. A little offtopic for this PR tho

Opened #907 for this

JoseSK999 · 2026-03-18T23:14:14Z

In cf61fbb you say "don't try to create a normal connection
if we are short on utreexo ones.", but I only see one code change which is the interval reduction

JoseSK999 · 2026-03-18T23:17:43Z

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

+            // Don't allow our node to have more than T::MAX_OUTGOING_PEERS, unless this is a
+            // manual peer, those can exceed our quota.
+


Nit: remove these lines, you already added one updated comment below

JoseSK999 · 2026-03-18T23:19:09Z

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

+
+            // We allow utreexo and manual peers to bypass our connection limis
+            let is_utreexo_peer =
+                version.kind == ConnectionKind::Regular(service_flags::UTREEXO.into());


I think this is incorrect since you can have other service bits, so this will not be equal even if this is a bridge node

JoseSK999 · 2026-03-18T23:22:26Z

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


        let idx = rand::random::<usize>() % peers.len();
-        let utreexo_peer = peers.get(idx)?;
+        let utreexo_peer = *peers.get(idx)?;


Nit, this shouldn't assume the peer is utreexo, we can use other service bits

luisschwab · 2026-03-19T18:57:30Z

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

    fn get_address_by_service(&self, service: ServiceFlags) -> Option<(usize, LocalAddress)> {
-        let peers = self.good_peers_by_service.get(&service)?;
+        let peers: Vec<_> = self
+            .good_peers_by_service
+            .get(&service)?
+            .iter()
+            .filter_map(|address| {
+                let local_address = self.addresses.get(address)?;
+                if local_address.state == AddressState::Connected {
+                    return None;
+                }
+
+                Some(local_address.id)
+            })
+            .collect();
+
        if peers.is_empty() {
            return None;
        }

        let idx = rand::random::<usize>() % peers.len();
-        let utreexo_peer = peers.get(idx)?;
+        let address_id = *peers.get(idx)?;

-        Some((*utreexo_peer, self.addresses.get(utreexo_peer)?.to_owned()))
+        Some((peer_id, self.addresses.get(&address_id)?.to_owned()))
    }


Missing documentation

luisschwab · 2026-03-19T18:59:53Z

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

    pub fn update_set_service_flag(&mut self, idx: usize, flags: ServiceFlags) -> &mut Self {
        // if this peer turns out to not have the minimum required services, we remove it
-        if !flags.has(ServiceFlags::NETWORK) || !flags.has(ServiceFlags::WITNESS) {
+        if !flags.has(ServiceFlags::NETWORK_LIMITED) || !flags.has(ServiceFlags::WITNESS) {


Does NETWORK also imply NETWORK_LIMITED? If not, NETWORK should remain there and NETWORK_LIMITED should be added.

If we require NETWORK we will drop all pruned nodes from addr_man

luisschwab · 2026-03-19T19:01:37Z

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

+        let signet_address =
+            load_addresses_from_json("./src/p2p_wire/seeds/signet_seeds.json").unwrap();


signet_addresses

luisschwab · 2026-03-19T19:03:41Z

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

        ServiceFlags::NETWORK | ServiceFlags::WITNESS | service_flags::UTREEXO_ARCHIVE.into()
    }

-    const TRY_NEW_CONNECTION: u64 = 30; // 30 seconds


On 9be7162 you just removed TRY_NEW_CONNECTION, but where is the new 10 second value coming from?

In node_context.rs it is set to 10s by default

luisschwab · 2026-03-19T19:04:48Z

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

        self.inflight
            .insert(InflightRequests::GetAddresses, (peer, Instant::now()));

+        let good_peers_count = self.connected_peers();


self.connected_peers() should return the actual peers, not the count.

Or be renamed to reflect that it returns a count, we use it in several places already. Fells a bit off-topic for this PR tho

JoseSK999 · 2026-03-19T23:43:43Z

Trying this tomorrow morning🫡

Davidson-Souza · 2026-03-20T14:39:47Z

I forgot to push some fixes suggested by @luisschwab

JoseSK999 · 2026-03-20T19:49:50Z

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

    fn get_address_by_service(&self, service: ServiceFlags) -> Option<(usize, LocalAddress)> {
-        let peers = self.good_peers_by_service.get(&service)?;
+        let peers: Vec<_> = self
+            .good_peers_by_service
+            .get(&service)?
+            .iter()
+            .filter_map(|address| {
+                let local_address = self.addresses.get(address)?;
+                if local_address.state == AddressState::Connected {
+                    return None;
+                }
+
+                Some(local_address.id)
+            })
+            .collect();
+
        if peers.is_empty() {
            return None;
        }

        let idx = rand::random::<usize>() % peers.len();
-        let utreexo_peer = peers.get(idx)?;
+        let address_id = *peers.get(idx)?;

-        Some((*utreexo_peer, self.addresses.get(utreexo_peer)?.to_owned()))
+        Some((address_id, self.addresses.get(&address_id)?.to_owned()))
    }


Compact version, avoiding calling self.addresses.get twice

fn get_address_by_service(&self, service: ServiceFlags) -> Option<(usize, LocalAddress)> { let candidates = self.good_peers_by_service.get(&service)?; candidates .iter() .filter_map(|id| { let addr = self.addresses.get(id)?; (addr.state != AddressState::Connected).then_some((id, addr)) }) .choose(&mut rand::thread_rng()) .map(|(id, addr)| (*id, addr.to_owned())) }

JoseSK999 · 2026-03-20T20:20:25Z

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

+                self.peers.entry(peer).and_modify(|p| {
+                    p.kind = ConnectionKind::Feeler;
+                });
+            }
+        }
+
        if version.kind == ConnectionKind::Feeler {


I think you didn't actually fix it, we modify the kind to be feeler on self.peers, but then we check against the Version message which won't be feeler.

Also what if this is an extra connection, we shouldn't convert it into a feeler even if we have excess peers.

…vice Currently, `get_address_by_service` will return addresses that we are already connected. Since the address selection logic first calls this function, and only if it returns `None`, tries a random (not known-to-be-good peer), we end up only getting connected peers in the main loop of `get_address_to_connect`. In turn, since we don't return connected addresses from that function, we return a large amount of `None`'s if we try to get address for a poorly populated service. This commit now filters out connected peers, so we don't even consider them. If there's no peers besides the connected ones, then we fallback to trying the "not good" addresses.

We previously required NETWORK, which excludes pruned nodes. `push_addresses` also requires NETWORK_LIMITED.

This test adds fixed addresses to the address manager and checks whether they actually have been added.

This commit reduces the interval between connection attempts to 10 secods (the default for NodeContext).

Davidson-Souza · 2026-03-23T16:26:20Z

Pushed a6c831d:

Changed version in handle_peer_ready to fully fix the bug in e91f329
Cherry picked f6f37ce from [WIP] feat: assume-valid swift sync #837, since it helps with cases where redo inflight requests fail
Increased timeout to two minutes on SyncNode, I had several cases where I've banned all my utreexo peers due to timeout 😢

JoseSK999 · 2026-03-23T23:24:18Z

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

    }

+    #[test]
+    fn test_adding_fixed_peer() {


I noticed this supersedes test_add_fixed_addresses, right? We should just add here the not empty check to keep this strictly a super-set of checks

Not really, test_add_fixed_addresses is using AddressMan::add_fixed_addresses, this new test does it manually to have access to the raw data

JoseSK999 · 2026-03-24T08:39:37Z

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


+        let good_peers_count = self.connected_peers();
+        if good_peers_count > T::MAX_OUTGOING_PEERS {
+            // We allow utreexo and manual peers to bypass our connection limis


Suggested change

// We allow utreexo and manual peers to bypass our connection limis

// We allow utreexo and manual peers to bypass our connection limits

JoseSK999 · 2026-03-24T08:47:48Z

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

+            let is_utreexo_peer = matches!(version.kind, ConnectionKind::Regular(services) if services.has(service_flags::UTREEXO.into()));
+            let is_manual_peer = version.kind == ConnectionKind::Manual;
+
+            if !(is_utreexo_peer || is_manual_peer) {


My comment on the extra peer is unresolved. I think we will convert any extra peer into feeler here, we should exclude those as well.

Now it's fixed

Before this commit, we would turn peers that exceeds our maximum peers count into a Feeler connection. However, we didn't update the services and state for that address, making our node believe that was a `Failed` connection, rather than a `Tried` one. This commit fixes this by removing the early return and move that code to before the logic that handles feeler connections.

… peers This will make sure we can create more than one utreexo connection, currently if we have only one utreexo peer, we won't use hardcoded addresses, and potentially have only one for the whole IBD, slowing things down.

Davidson-Souza · 2026-03-24T13:55:15Z

Pushed 857a80b:

Fixed a typo on docs
Don't mutate Extra peers into Feeler if they are above the limit
Use hardcoded addresses if AddressMan::enough_addresses return false

JoseSK999

ACK 857a80b; works well on signet

luisschwab

ACK 857a80b

Davidson-Souza added this to the v0.9.0 milestone Mar 18, 2026

Davidson-Souza self-assigned this Mar 18, 2026

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

Davidson-Souza force-pushed the fix-stuff branch from d922a3e to 77fd684 Compare March 18, 2026 22:49

JoseSK999 reviewed Mar 18, 2026

View reviewed changes

Davidson-Souza mentioned this pull request Mar 18, 2026

0.9.0 priorities #850

Closed

20 tasks

Davidson-Souza force-pushed the fix-stuff branch from 77fd684 to 09dc21c Compare March 19, 2026 18:44

luisschwab self-requested a review March 19, 2026 18:53

luisschwab suggested changes Mar 19, 2026

View reviewed changes

Davidson-Souza force-pushed the fix-stuff branch from 09dc21c to ed858fd Compare March 19, 2026 19:58

This comment was marked as outdated.

Sign in to view

luisschwab requested a review from JoseSK999 March 19, 2026 23:23

Davidson-Souza force-pushed the fix-stuff branch from ed858fd to 9e69e92 Compare March 20, 2026 14:39

Davidson-Souza force-pushed the fix-stuff branch from 9e69e92 to 92f3c38 Compare March 20, 2026 15:54

This comment was marked as outdated.

Sign in to view

JoseSK999 reviewed Mar 20, 2026

View reviewed changes

Davidson-Souza added 4 commits March 23, 2026 13:09

fix: update update_set_service_flag to require NETWORK_LIMITED

2efbbec

We previously required NETWORK, which excludes pruned nodes. `push_addresses` also requires NETWORK_LIMITED.

test: add a new test for adding fixed peers on addr_man

04ba097

This test adds fixed addresses to the address manager and checks whether they actually have been added.

fix(sync_ctx): make it more aggressive at oppening utreexo connections

af42edc

This commit reduces the interval between connection attempts to 10 secods (the default for NodeContext).

Davidson-Souza force-pushed the fix-stuff branch from 92f3c38 to a6c831d Compare March 23, 2026 16:21

Davidson-Souza requested review from JoseSK999 and luisschwab March 23, 2026 17:42

JoseSK999 reviewed Mar 23, 2026

View reviewed changes

JoseSK999 reviewed Mar 24, 2026

View reviewed changes

Davidson-Souza and others added 5 commits March 24, 2026 10:40

fix: fix service bits for some hardcoded addresses

9caa04f

fix: don't drop inflight requests before completion

ed87428

chore: increase timeout to two minutes

b108023

Davidson-Souza force-pushed the fix-stuff branch from a6c831d to 857a80b Compare March 24, 2026 13:45

Davidson-Souza requested a review from JoseSK999 March 25, 2026 12:15

JoseSK999 approved these changes Mar 25, 2026

View reviewed changes

luisschwab approved these changes Mar 25, 2026

View reviewed changes

Davidson-Souza merged commit 249a32c into getfloresta:master Mar 25, 2026
11 checks passed

		// Don't allow our node to have more than T::MAX_OUTGOING_PEERS, unless this is a
		// manual peer, those can exceed our quota.

		let signet_address =
		load_addresses_from_json("./src/p2p_wire/seeds/signet_seeds.json").unwrap();

	// We allow utreexo and manual peers to bypass our connection limis
	// We allow utreexo and manual peers to bypass our connection limits

Conversation

Davidson-Souza commented Mar 18, 2026

Description and Notes

Uh oh!

JoseSK999 Mar 18, 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!

JoseSK999 commented Mar 18, 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!

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!

This comment was marked as outdated.

Uh oh!

JoseSK999 commented Mar 19, 2026

Uh oh!

Davidson-Souza commented Mar 20, 2026

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Davidson-Souza commented Mar 23, 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!

Davidson-Souza commented Mar 24, 2026

Uh oh!

JoseSK999 left a comment

Choose a reason for hiding this comment

Uh oh!

luisschwab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JoseSK999 Mar 18, 2026 •

edited

Loading