From ce2e43401da17482c22900f5e72981255ea99f25 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Fri, 2 Oct 2026 18:14:21 -0600 Subject: [PATCH 1/2] net: --connect peers are manual, as in Core getpeerinfo reported a --connect peer as outbound-full-relay. Core opens every -connect address as ConnectionType::MANUAL. Both dial paths now use Manual: the tip-follow dial of the pinned targets and the redial of remembered --connect hostnames. The type is read by more than the RPC label, so the readers follow Core: - is_preferred_download counts Manual, as Core's fPreferredDownload does. Otherwise a node dialled only through --connect (or addnode, as today) never replaces a stalling headers-sync peer. - The 10 s --seednode fallback no longer runs under --connect. It fired when fewer than 2 outbound-full-relay peers were live, which manual --connect peers would never satisfy. Core never dials seednodes under -connect. - Manual peers are not held to the NODE_NETWORK service check, as in Core's ExpectServicesFromConn. Fixes #819. Co-Authored-By: Claude Opus 5.5 (1M context) --- TESTING.md | 2 +- changelog.d/connect-peers-manual.md | 9 +++ crates/rbitcoin-net/src/peers.rs | 66 ++++++++++++------- crates/rbitcoin-net/src/service.rs | 13 +++- crates/rbitcoin-node/src/run.rs | 55 ++++++++++++++-- .../tests/integration_multinode.rs | 3 +- 6 files changed, 118 insertions(+), 30 deletions(-) create mode 100644 changelog.d/connect-peers-manual.md diff --git a/TESTING.md b/TESTING.md index b7e10ed50..676a32d5b 100644 --- a/TESTING.md +++ b/TESTING.md @@ -373,7 +373,7 @@ Prefer **one high-level scenario** per behavior cluster. Delete lower-level test | `end_of_ibd_follow` | P2P (**default**) | Miner plus `--sh-index` syncer. One mature regtest: coinbases pay script A, one spend pays script B. IBD reaches that tip, leaves IBD, and Electrum history matches. The next block tip-follows. Restart does not rewrite the scripthash pack mark; one more block arrives by write-behind. Cancelling IBD once the height is below the miner, then `run_p2p`, finishes the same tip and history. With the syncer caught up, dropping the miner leaves tip follow (not IBD). A partial datadir whose miner is down does not open Electrum and stays short of that tip; the same miner address coming back lets that datadir finish | | `end_of_ibd_sh_interrupt` | P2P (**default**) | Same miner and scripts. The syncer first catches up with `--sh-index` off, then the datadir is frozen after pass 1 (`DONE.keys`, no `DONE.post`). Resume builds the index and Electrum's first answer is the full A/B history; restart does not rewrite the pack mark. A block mined after the freeze is in that history. A copy that already has the block, with `include_hwm` at the new tip and that create appended on the unsealed head, still serves the same history and does not open Electrum early | | `end_of_ibd_work_fork` | P2P (**default**) | Two miners on a regtest with retargeting and min-difficulty. One chain retargets harder and stops shorter. The other forks after that retarget, resets to the pow limit, and grows taller with less work. The syncer IBD-adopts the heavier tip, keeps it across a reopen, and follows one more heavy block while the tall chain is still ahead | -| `node_run_p2p_short` | Node (**default**) | Product `run_p2p` `--blocks-only` `--connect` to a live seeder (`--max-tip-age` so the 3-block pad is not stale IBD); process `getpeerinfo` / `getconnectioncount` / `getnetworkinfo` / `getnettotals` / `ping` while connected (v2 outbound-full-relay; handshake `startingheight` equals the seeder tip; `timeoffset` present; `synced_headers`/`synced_blocks` stay `-1` until the peer announces a header hash (empty getheaders at tip does not copy VERSION height); `servicesnames` present; `getnetworkinfo.timeoffset` present); after catch-up `localrelay` / mempool `relay_enabled` stay false and `sendrawtransaction` is not `relay disabled`; Electrum `broadcast` and Esplora `POST /tx` admit decode/consensus errors (not hub-missing / not `relay disabled`); `addconnection inbound` refuses; `disconnectnode` unknown `nodeid` / empty params error then a real addr drops that row from the next `getpeerinfo`, the seeder sees it go, and a second `disconnectnode` is `-29`; `addnode onetry` reconnects as `manual` and the seeder sees the inbound; seeder inbound `tx` then disconnects. Exit via `stop`. `max_run_secs=0` is `node_listen_and_exit`. Mock-clock `timeoffset` median (odd N, even N upper-middle, inbound-only 0, peer clock behind), connecting dummy `-1`, header-only vs connected `synced_blocks`, query-without-chain, `pingwait` / `NETWORK_LIMITED` / `noban` stay RPC guts | +| `node_run_p2p_short` | Node (**default**) | Product `run_p2p` `--blocks-only` `--connect` to a live seeder (`--max-tip-age` so the 3-block pad is not stale IBD); process `getpeerinfo` / `getconnectioncount` / `getnetworkinfo` / `getnettotals` / `ping` while connected (v2 `manual`, as Core reports `-connect`; handshake `startingheight` equals the seeder tip; `timeoffset` present; `synced_headers`/`synced_blocks` stay `-1` until the peer announces a header hash (empty getheaders at tip does not copy VERSION height); `servicesnames` present; `getnetworkinfo.timeoffset` present); after catch-up `localrelay` / mempool `relay_enabled` stay false and `sendrawtransaction` is not `relay disabled`; Electrum `broadcast` and Esplora `POST /tx` admit decode/consensus errors (not hub-missing / not `relay disabled`); `addconnection inbound` refuses; `disconnectnode` unknown `nodeid` / empty params error then a real addr drops that row from the next `getpeerinfo`, the seeder sees it go, and a second `disconnectnode` is `-29`; `addnode onetry` reconnects as `manual` and the seeder sees the inbound; seeder inbound `tx` then disconnects. Exit via `stop`. `max_run_secs=0` is `node_listen_and_exit`. Mock-clock `timeoffset` median (odd N, even N upper-middle, inbound-only 0, peer clock behind), connecting dummy `-1`, header-only vs connected `synced_blocks`, query-without-chain, `pingwait` / `NETWORK_LIMITED` / `noban` stay RPC guts | Removed (covered by the rows above): `confirm_cross_block_prevout_without_tx_head`, `double_archive_keeps_tx_height_for_coinbase_maturity`, `mega_batch_duplicate_header_is_idempotent`, diff --git a/changelog.d/connect-peers-manual.md b/changelog.d/connect-peers-manual.md new file mode 100644 index 000000000..f3a0da122 --- /dev/null +++ b/changelog.d/connect-peers-manual.md @@ -0,0 +1,9 @@ +Fixed + +- **`--connect` peers are `manual`, as in Core.** `getpeerinfo` reported a + `--connect` peer as `outbound-full-relay`. Core counts `manual` peers as + preferred download peers, and so does this node now: one whose outbound + peers all come from `--connect` or `addnode` replaces a stalling + headers-sync peer instead of waiting on it. As in Core, a `--connect` peer + is no longer dropped for missing `NODE_NETWORK`, and the 10 s + `--seednode` fallback does not run under `--connect`. diff --git a/crates/rbitcoin-net/src/peers.rs b/crates/rbitcoin-net/src/peers.rs index 592636c75..cec480c2f 100644 --- a/crates/rbitcoin-net/src/peers.rs +++ b/crates/rbitcoin-net/src/peers.rs @@ -1766,10 +1766,13 @@ impl PeerHub { self.forcerelay_perm.load(Ordering::Relaxed) } + /// Core `fPreferredDownload` counts `manual` (`addnode`, `--connect`) + /// outbound peers, so a node dialled only that way can still replace a + /// stalling headers-sync peer. fn is_preferred_download(p: &LivePeer) -> bool { matches!( p.conn_type, - PeerConnType::OutboundFullRelay | PeerConnType::BlockRelay + PeerConnType::OutboundFullRelay | PeerConnType::BlockRelay | PeerConnType::Manual ) } @@ -2381,35 +2384,32 @@ impl PeerHub { } } - fn remembered_redials(&self) -> Vec<(String, PeerConnType)> { - let mut out = Vec::new(); - for host in self + fn remembered_redials(&self) -> Vec { + let mut out: Vec = self .manual_hosts .lock() .unwrap_or_else(|e| e.into_inner()) .iter() - { - out.push((host.clone(), PeerConnType::Manual)); - } - for host in self - .connect_hosts - .lock() - .unwrap_or_else(|e| e.into_inner()) - .iter() - { - out.push((host.clone(), PeerConnType::OutboundFullRelay)); - } + .cloned() + .collect(); + out.extend( + self.connect_hosts + .lock() + .unwrap_or_else(|e| e.into_inner()) + .iter() + .cloned(), + ); out } - /// One dial per resolved endpoint in this pass. `addnode add` and - /// `--connect` of the same host share that dial (Manual wins). A later - /// pass dials again when the session is still not live. DNS lookup is - /// synchronous; callers on a Tokio worker use - /// [`Self::redial_remembered_off_runtime`]. + /// One `manual` dial per resolved endpoint in this pass, as Core dials + /// `-addnode` and `-connect`. `addnode add` and `--connect` of the same + /// host share that dial. A later pass dials again when the session is + /// still not live. DNS lookup is synchronous; callers on a Tokio worker + /// use [`Self::redial_remembered_off_runtime`]. pub fn redial_remembered_with(&self, resolve: impl Fn(&str) -> Result) { let mut seen = HashSet::::new(); - for (host, typ) in self.remembered_redials() { + for host in self.remembered_redials() { let Ok(target) = resolve(&host) else { continue; }; @@ -2419,7 +2419,7 @@ impl PeerHub { if self.is_target_live(&target) { continue; } - let _ = self.dial_target(target, typ); + let _ = self.dial_target(target, PeerConnType::Manual); } } @@ -3147,9 +3147,10 @@ mod tests { } #[test] - fn session_heartbeat_keeps_sole_preferred_headers_sync_peer() { + fn session_heartbeat_replaces_stalled_headers_sync_peer_only_with_another_preferred() { let hub = PeerHub::new(); let a = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 1); + let b = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 2); let outbound = hub.register( a, a, @@ -3166,6 +3167,15 @@ mod tests { "must not disconnect the only preferred download peer" ); assert!(outbound.is_sync_started()); + + // An addnode / --connect peer is `manual`. Core counts it as a + // preferred download peer, so the stalled sync peer can go. + let _manual = hub.register(b, b, &ver("/rbitcoin:0.1.0/"), false, PeerConnType::Manual); + hub.on_session_heartbeat(); + assert!( + outbound.stop.load(Ordering::SeqCst), + "a live manual peer must let the stalled headers-sync peer be replaced" + ); } #[test] @@ -3347,6 +3357,16 @@ mod tests { "addnode and --connect of one endpoint share one dial per pass: {got:?}" ); assert!(matches!(got[0].typ, PeerConnType::Manual)); + + // A --connect host on its own redials as `manual` too (Core `-connect`). + let hub = PeerHub::new(); + let (tx, mut rx) = mpsc::unbounded_channel(); + hub.set_dialer(tx); + hub.set_connect_hosts(vec!["127.0.0.1:18445".into()], 18444); + hub.redial_remembered(); + let got = take_dials(&mut rx); + assert_eq!(got.len(), 1, "{got:?}"); + assert!(matches!(got[0].typ, PeerConnType::Manual), "{got:?}"); } #[tokio::test] diff --git a/crates/rbitcoin-net/src/service.rs b/crates/rbitcoin-net/src/service.rs index 50d4aacbf..8ef6e3b50 100644 --- a/crates/rbitcoin-net/src/service.rs +++ b/crates/rbitcoin-net/src/service.rs @@ -350,6 +350,17 @@ impl P2PNode { } pub async fn follow_from_net(&mut self, peer: crate::NetAddr) -> Result<(), NetError> { + self.follow_from_net_as(peer, PeerConnType::OutboundFullRelay) + .await + } + + /// [`Self::follow_from_net`] with the session's `connection_type` + /// (`Manual` for a `--connect` target). + pub async fn follow_from_net_as( + &mut self, + peer: crate::NetAddr, + typ: PeerConnType, + ) -> Result<(), NetError> { let target = DialTarget::from_net(peer); let prepared = prepare_outbound_session( target, @@ -359,7 +370,7 @@ impl P2PNode { self.peers.clone(), self.user_agent.clone(), self.follow_live.clone(), - PeerConnType::OutboundFullRelay, + typ, self.dialer.clone(), ) .await?; diff --git a/crates/rbitcoin-node/src/run.rs b/crates/rbitcoin-node/src/run.rs index 7348742fc..155b15a1a 100644 --- a/crates/rbitcoin-node/src/run.rs +++ b/crates/rbitcoin-node/src/run.rs @@ -1,4 +1,4 @@ -use crate::config::{parse_btc_to_sat, NodeConfig}; +use crate::config::{parse_btc_to_sat, ListenOpts, NodeConfig}; use crate::error::NodeError; use crate::health::{run_health, NodeStatus, Phase}; use crate::regtest_rpc::HubRegtest; @@ -593,6 +593,7 @@ pub async fn run_p2p(config: NodeConfig) -> Result<(), NodeError> { let mut pinned = config.listen.connect.clone(); pinned.extend(dns_resolved); let targets = follow_dial_targets(&pinned, &addrman, max_out, &occupied); + let follow_type = follow_dial_type(&pinned); let ibd_targets = follow_dial_targets(&pinned, &addrman, candidate_n, &occupied); status.enter(Phase::CatchUp); let catch_up = run_ibd_or_skip( @@ -699,7 +700,7 @@ pub async fn run_p2p(config: NodeConfig) -> Result<(), NodeError> { } } } - if !shutdown.requested() && !config.listen.seednodes.is_empty() && !addrman.is_empty() { + if !shutdown.requested() && seednode_fallback_armed(&config.listen, addrman.is_empty()) { const ADD_NEXT_SEEDNODE_SECS: u64 = 10; let seeds = config.listen.seednodes.clone(); let network = config.network; @@ -746,7 +747,7 @@ pub async fn run_p2p(config: NodeConfig) -> Result<(), NodeError> { let follow_n = targets.len().min(max_out.min(3)); if catch_up.dial_failed_all() { for peer in targets.iter().take(follow_n) { - if let Err(e) = node.peers.dial_net(*peer, PeerConnType::OutboundFullRelay) { + if let Err(e) = node.peers.dial_net(*peer, follow_type) { warn!("node: follow dial {peer}: {e}"); } } @@ -765,7 +766,7 @@ pub async fn run_p2p(config: NodeConfig) -> Result<(), NodeError> { warn!("signal: skip remaining follow connects"); break; } - result = tokio::time::timeout(to, node.follow_from_net(*peer)) => { + result = tokio::time::timeout(to, node.follow_from_net_as(*peer, follow_type)) => { match result { Ok(Ok(())) => { info!( @@ -2108,6 +2109,24 @@ pub(crate) fn follow_dial_targets( } } +/// Session type for the tip-follow dials of `targets`. `--connect` targets +/// are `manual`, as Core dials `-connect` (`getpeerinfo.connection_type`). +pub(crate) fn follow_dial_type(connect: &[rbitcoin_net::NetAddr]) -> PeerConnType { + if connect.is_empty() { + PeerConnType::OutboundFullRelay + } else { + PeerConnType::Manual + } +} + +/// Whether to arm the 10 s `--seednode` addr-fetch fallback (it fires when +/// fewer than 2 outbound full-relay peers are live). Core never dials +/// seednodes under `-connect`, and `--connect` peers are `manual`, so they +/// would never hold those slots off. +pub(crate) fn seednode_fallback_armed(listen: &ListenOpts, book_empty: bool) -> bool { + !listen.seednodes.is_empty() && !book_empty && !listen.has_pinned_connect() +} + /// Whether this wake should run the stale-tip redial check. /// /// Perf (5s) and RPC-stop (50ms) ticks must still evaluate stale. A one-shot @@ -2252,6 +2271,34 @@ mod tests { 8333, ))]; assert_eq!(follow_dial_targets(&connect, &am, 8, &occupied), want); + // Core reports `-connect` peers as `manual`; addrman picks are full relay. + assert_eq!(follow_dial_type(&connect), PeerConnType::Manual); + assert_eq!(follow_dial_type(&[]), PeerConnType::OutboundFullRelay); + } + + #[test] + fn seednode_fallback_is_off_under_connect() { + let mut listen = NodeConfig::default().listen; + assert!(!seednode_fallback_armed(&listen, false), "no seednodes"); + listen.seednodes = vec!["127.0.0.1:18444".into()]; + assert!(seednode_fallback_armed(&listen, false)); + assert!( + !seednode_fallback_armed(&listen, true), + "empty book takes the startup path" + ); + listen.connect = vec![rbitcoin_net::NetAddr::Ip( + "127.0.0.1:18445".parse().unwrap(), + )]; + assert!( + !seednode_fallback_armed(&listen, false), + "Core skips seednodes under -connect" + ); + listen.connect.clear(); + listen.connect_dns = vec!["localhost:18445".into()]; + assert!( + !seednode_fallback_armed(&listen, false), + "a --connect hostname pins too" + ); } #[test] diff --git a/crates/rbitcoin-test/tests/integration_multinode.rs b/crates/rbitcoin-test/tests/integration_multinode.rs index 5c7ef55a3..c1bd7bf68 100644 --- a/crates/rbitcoin-test/tests/integration_multinode.rs +++ b/crates/rbitcoin-test/tests/integration_multinode.rs @@ -2839,7 +2839,8 @@ async fn node_run_p2p_short() { let rows = peers["result"].as_array().expect("getpeerinfo array"); assert_eq!(rows.len(), 1, "{peers}"); assert_eq!(rows[0]["inbound"], false, "{peers}"); - assert_eq!(rows[0]["connection_type"], "outbound-full-relay", "{peers}"); + // Core reports a `-connect` peer as `manual`. + assert_eq!(rows[0]["connection_type"], "manual", "{peers}"); assert_eq!(rows[0]["transport_protocol_type"], "v2", "{peers}"); assert!( rows[0]["bytesrecv"].as_u64().unwrap_or(0) > 0, From 69c3ab2454c51ceb5257f65d6ecade48339af6f8 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Fri, 2 Oct 2026 18:24:30 -0600 Subject: [PATCH 2/2] node: no --seednode dial at all under --connect The previous commit kept the 10 s seednode fallback off under --connect, but the startup dial (empty addrman) still ran. That happens when every --connect target is a hostname that does not resolve and peers.dat is empty. Both paths now share seednodes_allowed, which is false under --connect, as Core never dials seednodes under -connect. Co-Authored-By: Claude Opus 5.5 (1M context) --- changelog.d/connect-peers-manual.md | 4 ++-- crates/rbitcoin-node/src/run.rs | 37 +++++++++++++---------------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/changelog.d/connect-peers-manual.md b/changelog.d/connect-peers-manual.md index f3a0da122..0d3fb5ac9 100644 --- a/changelog.d/connect-peers-manual.md +++ b/changelog.d/connect-peers-manual.md @@ -5,5 +5,5 @@ Fixed preferred download peers, and so does this node now: one whose outbound peers all come from `--connect` or `addnode` replaces a stalling headers-sync peer instead of waiting on it. As in Core, a `--connect` peer - is no longer dropped for missing `NODE_NETWORK`, and the 10 s - `--seednode` fallback does not run under `--connect`. + is no longer dropped for missing `NODE_NETWORK`, and `--seednode` is not + dialled under `--connect`. diff --git a/crates/rbitcoin-node/src/run.rs b/crates/rbitcoin-node/src/run.rs index 155b15a1a..5dc1d5c77 100644 --- a/crates/rbitcoin-node/src/run.rs +++ b/crates/rbitcoin-node/src/run.rs @@ -684,7 +684,11 @@ pub async fn run_p2p(config: NodeConfig) -> Result<(), NodeError> { ); } - if tip_follow_ready && !shutdown.requested() && addrman.is_empty() { + if tip_follow_ready + && !shutdown.requested() + && addrman.is_empty() + && seednodes_allowed(&config.listen) + { for raw in &config.listen.seednodes { let addr = match resolve_seednode(raw, config.network) { Ok(a) => a, @@ -700,7 +704,7 @@ pub async fn run_p2p(config: NodeConfig) -> Result<(), NodeError> { } } } - if !shutdown.requested() && seednode_fallback_armed(&config.listen, addrman.is_empty()) { + if !shutdown.requested() && !addrman.is_empty() && seednodes_allowed(&config.listen) { const ADD_NEXT_SEEDNODE_SECS: u64 = 10; let seeds = config.listen.seednodes.clone(); let network = config.network; @@ -2119,12 +2123,12 @@ pub(crate) fn follow_dial_type(connect: &[rbitcoin_net::NetAddr]) -> PeerConnTyp } } -/// Whether to arm the 10 s `--seednode` addr-fetch fallback (it fires when -/// fewer than 2 outbound full-relay peers are live). Core never dials -/// seednodes under `-connect`, and `--connect` peers are `manual`, so they -/// would never hold those slots off. -pub(crate) fn seednode_fallback_armed(listen: &ListenOpts, book_empty: bool) -> bool { - !listen.seednodes.is_empty() && !book_empty && !listen.has_pinned_connect() +/// Whether `--seednode` may be dialled as addr-fetch: at startup with an +/// empty addrman, or by the 10 s fallback when fewer than 2 outbound +/// full-relay peers are live. Core never dials seednodes under `-connect`. +/// `--connect` peers are `manual`, so they would never hold the fallback off. +pub(crate) fn seednodes_allowed(listen: &ListenOpts) -> bool { + !listen.seednodes.is_empty() && !listen.has_pinned_connect() } /// Whether this wake should run the stale-tip redial check. @@ -2277,28 +2281,21 @@ mod tests { } #[test] - fn seednode_fallback_is_off_under_connect() { + fn seednodes_are_off_under_connect() { let mut listen = NodeConfig::default().listen; - assert!(!seednode_fallback_armed(&listen, false), "no seednodes"); + assert!(!seednodes_allowed(&listen), "no seednodes"); listen.seednodes = vec!["127.0.0.1:18444".into()]; - assert!(seednode_fallback_armed(&listen, false)); - assert!( - !seednode_fallback_armed(&listen, true), - "empty book takes the startup path" - ); + assert!(seednodes_allowed(&listen)); listen.connect = vec![rbitcoin_net::NetAddr::Ip( "127.0.0.1:18445".parse().unwrap(), )]; assert!( - !seednode_fallback_armed(&listen, false), + !seednodes_allowed(&listen), "Core skips seednodes under -connect" ); listen.connect.clear(); listen.connect_dns = vec!["localhost:18445".into()]; - assert!( - !seednode_fallback_armed(&listen, false), - "a --connect hostname pins too" - ); + assert!(!seednodes_allowed(&listen), "a --connect hostname pins too"); } #[test]