diff --git a/changelog.d/addrfetch-learns-reply.md b/changelog.d/addrfetch-learns-reply.md new file mode 100644 index 0000000000..c430388e06 --- /dev/null +++ b/changelog.d/addrfetch-learns-reply.md @@ -0,0 +1,8 @@ +Fixed + +- **An addr-fetch session keeps the addresses it asked for.** The `getaddr` + reply from an addr-fetch peer (the `--proxy` DNS seeds, `-seednode`) was + dropped before AddrMan and scored as misbehavior. As in Core, the + `getaddr` now buys `MAX_ADDR_TO_SEND` more tokens, the reply (`addrv2` or + v1 `addr`) is learned and not relayed, and more than one address ends the + session with a plain disconnect. diff --git a/crates/rbitcoin-net/src/peer.rs b/crates/rbitcoin-net/src/peer.rs index ed5175e98d..3a13a93e09 100644 --- a/crates/rbitcoin-net/src/peer.rs +++ b/crates/rbitcoin-net/src/peer.rs @@ -455,6 +455,10 @@ impl AddrTokenBucket { self.tokens -= take as f64; take } + + pub(crate) fn credit(&mut self, n: usize) { + self.tokens += n as f64; + } } #[doc(hidden)] @@ -2148,6 +2152,7 @@ pub fn maybe_queue_addrfetch_getaddr( if session.conn_type != crate::peers::PeerConnType::AddrFetch { return false; } + session.credit_getaddr_reply(); let _ = queue_out(out, NetworkMessage::GetAddr); true } @@ -2971,7 +2976,7 @@ pub fn handle_peer_inventory_msg( return Ok(()); } match payload { - NetworkMessage::Addr(list) => on_addr_list(follow, session, list.len())?, + NetworkMessage::Addr(list) => on_addr_list(follow, session, list)?, NetworkMessage::AddrV2(list) => on_addrv2(follow, session, list)?, NetworkMessage::GetHeaders(gh) => on_getheaders(hub, out_tx, follow, session, gh)?, NetworkMessage::GetBlocks(gb) => on_getblocks(hub, out_tx, follow, session, gb)?, @@ -3082,19 +3087,55 @@ pub fn on_sendaddrv2(follow: &mut PeerFollowState, session: Option<&crate::peers punish_disconnect(&mut follow.ban_score, session); } +/// v1 `addr` is learned only as an addr-fetch session's `getaddr` reply. #[doc(hidden)] pub fn on_addr_list( follow: &mut PeerFollowState, session: Option<&crate::peers::LivePeer>, - n: usize, + list: &[(u32, bitcoin::p2p::address::Address)], ) -> Result<(), NetError> { - if session.is_some_and(|s| s.conn_type == crate::peers::PeerConnType::AddrFetch && n > 1) { - punish_disconnect(&mut follow.ban_score, session); + let Some(s) = session.filter(|s| s.conn_type == crate::peers::PeerConnType::AddrFetch) else { + return Ok(()); + }; + if list.len() > MAX_ADDR_TO_SEND { + misbehaving(&mut follow.ban_score, session); return Ok(()); } + let v2: Vec<_> = list + .iter() + .filter_map(|(time, a)| { + let sock = a.socket_addr().ok()?; + Some(bitcoin::p2p::address::AddrV2Message { + time: *time, + services: a.services, + addr: crate::NetAddr::from_socket(sock).to_addrv2(), + port: sock.port(), + }) + }) + .collect(); + learn_addrfetch_reply(s, &v2, list.len()); Ok(()) } +/// Core `ProcessMessage` ADDR/ADDRV2 on an addr-fetch session: the reply to +/// the `getaddr` it sent is learned and not relayed (`m_getaddr_sent`). +/// More than one address ends the session, without misbehavior. +fn learn_addrfetch_reply( + s: &crate::peers::LivePeer, + list: &[bitcoin::p2p::address::AddrV2Message], + n: usize, +) { + if let Some(ph) = s.peer_hub() { + // Node time, as the bucket's start (`connected_at`) is. + let now_ms = s.clock_now().saturating_mul(1000); + let allow = s.take_addr_tokens(list.len(), now_ms); + ph.learn_addrv2(&list[..allow]); + } + if n > 1 { + s.request_disconnect(); + } +} + #[doc(hidden)] pub fn on_addrv2( follow: &mut PeerFollowState, @@ -3111,8 +3152,8 @@ pub fn on_addrv2( misbehaving(&mut follow.ban_score, session); return Ok(()); } - if session.is_some_and(|s| s.conn_type == crate::peers::PeerConnType::AddrFetch && n > 1) { - punish_disconnect(&mut follow.ban_score, session); + if let Some(s) = session.filter(|s| s.conn_type == crate::peers::PeerConnType::AddrFetch) { + learn_addrfetch_reply(s, list, n); return Ok(()); } if let Some(s) = session { diff --git a/crates/rbitcoin-net/src/peers.rs b/crates/rbitcoin-net/src/peers.rs index 429ee268b6..e97c893779 100644 --- a/crates/rbitcoin-net/src/peers.rs +++ b/crates/rbitcoin-net/src/peers.rs @@ -983,6 +983,15 @@ impl LivePeer { *self.out_tx.lock().unwrap_or_else(|e| e.into_inner()) = Some(tx); } + /// Core `m_addr_token_bucket += MAX_ADDR_TO_SEND` on `getaddr`: the + /// reply may exceed the bucket cap. + pub(crate) fn credit_getaddr_reply(&self) { + self.addr_bucket + .lock() + .unwrap_or_else(|e| e.into_inner()) + .credit(crate::peer::MAX_ADDR_TO_SEND); + } + /// How many of the `want` addresses in one message this peer may have /// processed (learned and relayed). The rest count as rate limited. A /// peer with the `addr` permission spends tokens but is never limited. diff --git a/crates/rbitcoin-net/tests/peer/mod.rs b/crates/rbitcoin-net/tests/peer/mod.rs index 7a561c7ccb..223eb56257 100644 --- a/crates/rbitcoin-net/tests/peer/mod.rs +++ b/crates/rbitcoin-net/tests/peer/mod.rs @@ -5075,3 +5075,123 @@ fn compact_filter_ranges_past_core_limits_disconnect() { } mod log_text_tests; + +/// Core `ProcessMessage` ADDR/ADDRV2: the `getaddr` an addr-fetch session +/// sends buys `MAX_ADDR_TO_SEND` more addresses. The reply, v1 or v2, is +/// learned and not relayed (`m_getaddr_sent`). More than one address ends the +/// session without misbehavior; a one-address self-announce does not. +#[tokio::test] +async fn addrfetch_learns_the_getaddr_reply_then_disconnects() { + use bitcoin::p2p::address::{AddrV2, AddrV2Message, Address}; + use bitcoin::p2p::message_network::VersionMessage; + use bitcoin::p2p::ServiceFlags; + use std::sync::atomic::Ordering; + + let (dir, hub) = rbitcoin_net::chain::tiny_regtest_hub_labeled("addrfetch-reply"); + let peers = rbitcoin_net::peers::PeerHub::new(); + let book = std::sync::Arc::new(std::sync::Mutex::new(rbitcoin_net::seeds::AddrMan::new())); + peers.set_addrman(std::sync::Arc::clone(&book)); + let register = |port: u16, typ: rbitcoin_net::peers::PeerConnType| { + let addr = std::net::SocketAddr::from(([127, 0, 0, 1], port)); + let ver = VersionMessage { + version: bitcoin::p2p::PROTOCOL_VERSION, + services: ServiceFlags::NETWORK | ServiceFlags::WITNESS, + timestamp: 0, + receiver: Address::new(&addr, ServiceFlags::NONE), + sender: Address::new(&addr, ServiceFlags::NONE), + nonce: 0, + user_agent: "/rbitcoin:test/".into(), + start_height: 0, + relay: true, + }; + peers.register(addr, addr, &ver, false, typ) + }; + let neighbor = register(18459, rbitcoin_net::peers::PeerConnType::OutboundFullRelay); + neighbor.set_wants_addrv2(); + let (relay_tx, mut relayed) = mpsc::unbounded_channel(); + neighbor.attach_out(relay_tx); + let book_len = || book.lock().unwrap().len(); + let routable = |i: u32| AddrV2::Ipv4(std::net::Ipv4Addr::from(0x0b00_0001 + (i << 16))); + + let seed = register(18460, rbitcoin_net::peers::PeerConnType::AddrFetch); + let (out_tx, _out_rx) = mpsc::unbounded_channel(); + assert!(maybe_queue_addrfetch_getaddr(&out_tx, &seed)); + let time = seed.clock_now() as u32; + let mut follow = PeerFollowState::new(); + push( + &hub, + &out_tx, + &mut follow, + Some(seed.as_ref()), + NetworkMessage::AddrV2(vec![AddrV2Message { + time, + services: ServiceFlags::NETWORK, + addr: AddrV2::Ipv4(std::net::Ipv4Addr::new(9, 9, 9, 9)), + port: 8333, + }]), + ) + .await; + assert_eq!(book_len(), 1, "a self-announce is learned"); + assert!( + !seed.stop.load(Ordering::SeqCst), + "one address is not the reply" + ); + let reply = (0..MAX_ADDR_TO_SEND as u32) + .map(|i| AddrV2Message { + time, + services: ServiceFlags::NETWORK | ServiceFlags::WITNESS, + addr: routable(i), + port: 18444, + }) + .collect(); + push( + &hub, + &out_tx, + &mut follow, + Some(seed.as_ref()), + NetworkMessage::AddrV2(reply), + ) + .await; + let learned = book_len() - 1; + assert!( + learned >= MAX_ADDR_TO_SEND * 9 / 10, + "the getaddr reply is learned past one token: {learned}" + ); + assert!( + seed.stop.load(Ordering::SeqCst), + "the reply ends addr-fetch" + ); + assert_eq!(follow.ban_score, 0, "a getaddr reply is not misbehavior"); + assert!( + relayed.try_recv().is_err(), + "an addr-fetch reply is not relayed" + ); + + let v1_seed = register(18461, rbitcoin_net::peers::PeerConnType::AddrFetch); + assert!(maybe_queue_addrfetch_getaddr(&out_tx, &v1_seed)); + let before = book_len(); + let v1 = |last: u8| { + let addr = std::net::SocketAddr::from(([12, 0, 0, last], 18444)); + (time, Address::new(&addr, ServiceFlags::NETWORK)) + }; + let mut follow = PeerFollowState::new(); + push( + &hub, + &out_tx, + &mut follow, + Some(v1_seed.as_ref()), + NetworkMessage::Addr(vec![v1(1), v1(2)]), + ) + .await; + assert!(book_len() > before, "a v1 getaddr reply is learned"); + assert!( + v1_seed.stop.load(Ordering::SeqCst), + "a v1 reply ends addr-fetch" + ); + assert_eq!(follow.ban_score, 0, "a v1 getaddr reply is not misbehavior"); + assert!( + relayed.try_recv().is_err(), + "a v1 addr-fetch reply is not relayed" + ); + let _ = std::fs::remove_dir_all(dir.path()); +}