From eb7f3e46941440ab8ceffb2e46a972932c0c23c0 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Wed, 7 Oct 2026 16:18:18 -0600 Subject: [PATCH 01/13] p2p: apply Core's message size and count limits, and bound queued sends per peer - v1 frames are capped at Core's MAX_PROTOCOL_MESSAGE_LENGTH (4,000,000 bytes) instead of 32 MiB. The v1 and v2 receive buffers grow as payload bytes arrive, at most 256 KiB ahead of them (V1Transport::readData, V2Transport MAX_RESERVE_AHEAD), and a large v1 buffer is released after its message. - inv/getdata over MAX_INV_SZ (50,000), headers over MAX_HEADERS_RESULTS (2000) and addr/addrv2 over MAX_ADDR_TO_SEND (1000) are misbehaviour, as in Core; a notfound over 5016 entries is ignored. - A peer's queue counts its bytes. getdata is served as Core's ProcessGetData does: entries are kept in a per-peer backlog and served until the queue passes 1,000,000 bytes (-maxsendbuffer's default), and the rest as it drains. The peer's next message is not read while a getdata of its is being served or its queue is over the limit (ProcessMessages). -maxuploadtarget is charged only for blocks queued. - The write loop watches the disconnect signal during a write, and drops a peer whose socket takes no bytes for 20 minutes (TIMEOUT_INTERVAL, "socket sending timeout"). Sent bytes are counted as the socket takes them. - A version whose user agent is over MAX_SUBVERSION_LENGTH is dropped, as Core's LIMITED_STRING read does; the user agent and unknown message types are sanitized with Core's SanitizeString before they are stored or logged. - getcfilters/getcfheaders/getcfcheckpt requests that Core's PrepareBlockFilterRequest refuses now disconnect instead of being ignored. Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/net/connection.rs | 104 +++- node/src/net/connection_bounds_tests.rs | 116 ++++ node/src/net/limits.rs | 60 ++ node/src/net/manager.rs | 642 ++++++++++++++------ node/src/net/manager_p2pbounds_tests.rs | 726 +++++++++++++++++++++++ node/src/net/mod.rs | 2 + node/src/net/peer.rs | 6 +- node/src/net/send_queue.rs | 227 +++++++ node/src/net/send_queue_tests.rs | 110 ++++ node/src/net/v2transport.rs | 39 +- node/src/net/v2transport_bounds_tests.rs | 82 +++ satd/tests/p2p_limits.rs | 221 +++++++ satd/tests/regtest.rs | 49 +- 13 files changed, 2143 insertions(+), 241 deletions(-) create mode 100644 node/src/net/connection_bounds_tests.rs create mode 100644 node/src/net/limits.rs create mode 100644 node/src/net/manager_p2pbounds_tests.rs create mode 100644 node/src/net/send_queue.rs create mode 100644 node/src/net/send_queue_tests.rs create mode 100644 node/src/net/v2transport_bounds_tests.rs create mode 100644 satd/tests/p2p_limits.rs diff --git a/node/src/net/connection.rs b/node/src/net/connection.rs index bec935593..fbc4e67ef 100644 --- a/node/src/net/connection.rs +++ b/node/src/net/connection.rs @@ -6,7 +6,7 @@ use std::io; use std::pin::Pin; use std::sync::Arc; use std::task::{Context, Poll}; -use tokio::io::{AsyncRead, AsyncReadExt, AsyncWriteExt, ReadBuf, ReadHalf, WriteHalf}; +use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt, ReadBuf, ReadHalf, WriteHalf}; use tokio::net::TcpStream; use crate::net::stats::PeerStats; @@ -225,8 +225,17 @@ pub struct V1Writer { /// Size of a P2P message header: 4 (magic) + 12 (command) + 4 (length) + 4 (checksum). const HEADER_SIZE: usize = 24; -/// Maximum message payload size (32 MB). -const MAX_PAYLOAD_SIZE: usize = 32 * 1024 * 1024; +/// Maximum message payload size: Core's `MAX_PROTOCOL_MESSAGE_LENGTH` +/// (`net.h`). A header announcing more ends the connection +/// (`V1Transport::readHeader`); satd took up to 32 MiB. +const MAX_PAYLOAD_SIZE: usize = 4_000_000; +/// How far ahead of the payload bytes actually received the receive buffer +/// is grown: Core's `V1Transport::readData` ("Allocate up to 256 KiB ahead, +/// but never more than the total message size"). +const PAYLOAD_ALLOC_STEP: usize = 256 * 1024; +/// Receive-buffer capacity kept for the next message. A buffer grown past it +/// by a larger message is released once that message is decoded. +const RECV_BUF_KEEP: usize = 64 * 1024; /// Maximum bytes to scan when resyncing to magic after stream misalignment. const MAX_RESYNC_BYTES: usize = 256 * 1024; @@ -326,15 +335,39 @@ impl V1Writer { pub async fn send(&mut self, msg: NetworkMessage) -> io::Result<()> { let raw = RawNetworkMessage::new(self.magic, msg); let bytes = serialize(&raw); - self.stream.write_all(&bytes).await?; + write_counted(&mut self.stream, &bytes, self.counters.as_ref()).await?; if let Some(c) = &self.counters { - c.record_sent(bytes.len()); c.attribute_sent(raw.cmd(), bytes.len()); } Ok(()) } } +/// Write `bytes` in full, counting each chunk as the socket takes it. +/// +/// Core moves `nSendBytes` and `m_last_send` on every partial send +/// (`SocketSendData`). Counting only whole messages made a large message that +/// a peer reads slowly look like no progress at all, and the write loop's send +/// timeout is measured against these counts. +pub(crate) async fn write_counted( + w: &mut W, + bytes: &[u8], + counters: Option<&Arc>, +) -> io::Result<()> { + let mut written = 0; + while written < bytes.len() { + let n = w.write(&bytes[written..]).await?; + if n == 0 { + return Err(io::ErrorKind::WriteZero.into()); + } + written += n; + if let Some(c) = counters { + c.record_sent(n); + } + } + Ok(()) +} + impl V1Reader { /// Receive the next network message. Skips messages that fail to deserialize. /// @@ -383,10 +416,19 @@ async fn recv_message( )); } - // Read payload - let mut payload = vec![0u8; payload_len]; - if payload_len > 0 { - stream.read_exact(&mut payload).await?; + // Read the payload into `buf`, behind the header, growing it as the + // bytes arrive and never more than PAYLOAD_ALLOC_STEP ahead of them + // (Core's `V1Transport::readData`). Sizing it from the header alone + // let a peer that announced a large payload and sent nothing hold + // that much memory per connection. + buf.clear(); + buf.extend_from_slice(&header); + let frame_len = HEADER_SIZE + payload_len; + while buf.len() < frame_len { + let start = buf.len(); + let end = frame_len.min(start + PAYLOAD_ALLOC_STEP); + buf.resize(end, 0); + stream.read_exact(&mut buf[start..end]).await?; } // Record on-wire bytes for this framed message (counted even if it @@ -399,23 +441,16 @@ async fn recv_message( c.record_recv(wire_len); } - // Combine header + payload and deserialize - buf.clear(); - buf.extend_from_slice(&header); - buf.extend_from_slice(&payload); - - match deserialize::(buf) { + let result = match deserialize::(buf) { + Ok(raw) if *raw.magic() != magic => Some(Err(io::Error::new( + io::ErrorKind::InvalidData, + "wrong network magic", + ))), Ok(raw) => { - if *raw.magic() != magic { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - "wrong network magic", - )); - } if let Some(c) = counters { c.attribute_recv(raw.cmd(), wire_len); } - return Ok(raw.payload().clone()); + Some(Ok(raw.into_payload())) } Err(_) if &header[4..16] == b"pong\0\0\0\0\0\0\0\0" && payload_len < 8 => { // Too short to hold a nonce. Core still acts on it -- it @@ -424,20 +459,29 @@ async fn recv_message( if let Some(c) = counters { c.attribute_recv("pong", wire_len); } - return Ok(NetworkMessage::Unknown { + Some(Ok(NetworkMessage::Unknown { command: bitcoin::p2p::message::CommandString::try_from_static("pong") .expect("static command"), - payload, - }); + payload: buf[HEADER_SIZE..].to_vec(), + })) } Err(_) => { if let Some(c) = counters { c.attribute_recv(crate::net::stats::MSG_TYPE_OTHER, wire_len); } - let cmd = String::from_utf8_lossy(&header[4..16]); - tracing::debug!(cmd = %cmd.trim_end_matches('\0'), "Skipping unparseable message"); - continue; + // The peer's own text: sanitized as Core's + // `SanitizeString(msg_type)`, so it cannot break a log line. + let cmd = crate::net::limits::sanitize_string(&String::from_utf8_lossy(&header[4..16])); + tracing::debug!(cmd = %cmd, "Skipping unparseable message"); + None } + }; + // Kept for the next message, but not at the size of a large one. + if buf.capacity() > RECV_BUF_KEEP { + *buf = Vec::new(); + } + if let Some(result) = result { + return result; } } } @@ -500,3 +544,7 @@ async fn resync_to_magic( } } } + +#[cfg(test)] +#[path = "connection_bounds_tests.rs"] +mod bounds_tests; diff --git a/node/src/net/connection_bounds_tests.rs b/node/src/net/connection_bounds_tests.rs new file mode 100644 index 000000000..f76054c4b --- /dev/null +++ b/node/src/net/connection_bounds_tests.rs @@ -0,0 +1,116 @@ +//! The v1 frame limit, the receive buffer's growth and release, and send +//! progress, against Bitcoin Core's `V1Transport` and `SocketSendData`. + +use super::*; +use bitcoin::p2p::message::CommandString; +use std::io::Cursor; +use std::time::Duration; + +/// A reader that records the largest read it was asked for. +struct MaxAsk { + inner: R, + max: usize, +} + +impl AsyncRead for MaxAsk { + fn poll_read( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &mut ReadBuf<'_>, + ) -> Poll> { + self.max = self.max.max(buf.remaining()); + Pin::new(&mut self.inner).poll_read(cx, buf) + } +} + +fn unknown(len: usize) -> NetworkMessage { + NetworkMessage::Unknown { + command: CommandString::try_from_static("bigmsg").unwrap(), + payload: vec![0x5a; len], + } +} + +fn frame(msg: NetworkMessage) -> Vec { + serialize(&RawNetworkMessage::new(Magic::REGTEST, msg)) +} + +/// Core's `MAX_PROTOCOL_MESSAGE_LENGTH` is 4,000,000 bytes, and a header +/// announcing more ends the connection (`V1Transport::readHeader`). satd +/// took frames up to 32 MiB. +#[tokio::test] +async fn a_frame_over_four_million_bytes_is_refused() { + let mut buf = Vec::new(); + let mut ok = Cursor::new(frame(unknown(4_000_000))); + let got = recv_message(&mut ok, Magic::REGTEST, &mut buf, None).await; + assert!(matches!(got, Ok(NetworkMessage::Unknown { ref payload, .. }) if payload.len() == 4_000_000)); + + let mut over = Cursor::new(frame(unknown(4_000_001))); + let err = recv_message(&mut over, Magic::REGTEST, &mut buf, None) + .await + .expect_err("a 4,000,001-byte payload is refused"); + assert_eq!(err.kind(), io::ErrorKind::InvalidData, "{err}"); +} + +/// Core grows the receive buffer as payload bytes arrive, never more than +/// 256 KiB ahead of them (`V1Transport::readData`). satd allocated the whole +/// announced payload before reading any of it, so a peer could make it hold +/// a full frame per connection by announcing one and sending nothing. +#[tokio::test] +async fn the_payload_buffer_grows_with_the_bytes_received() { + let mut bytes = frame(unknown(4_000_000)); + bytes.truncate(HEADER_SIZE + 1000); + let mut reader = MaxAsk { inner: Cursor::new(bytes), max: 0 }; + let mut buf = Vec::new(); + let err = recv_message(&mut reader, Magic::REGTEST, &mut buf, None) + .await + .expect_err("the payload never arrives"); + assert_eq!(err.kind(), io::ErrorKind::UnexpectedEof); + assert!(reader.max <= PAYLOAD_ALLOC_STEP, "asked for {} bytes at once", reader.max); + assert!(buf.capacity() <= HEADER_SIZE + 2 * PAYLOAD_ALLOC_STEP, "holding {} bytes", buf.capacity()); +} + +/// The receive buffer is reused between messages, but not at the size of +/// the largest message the connection ever carried. +#[tokio::test] +async fn the_receive_buffer_is_released_after_a_large_message() { + let mut bytes = frame(unknown(1_000_000)); + bytes.extend(frame(NetworkMessage::Ping(7))); + let mut reader = Cursor::new(bytes); + let mut buf = Vec::new(); + let big = recv_message(&mut reader, Magic::REGTEST, &mut buf, None).await.unwrap(); + assert!(matches!(big, NetworkMessage::Unknown { .. })); + assert!(buf.capacity() <= RECV_BUF_KEEP, "kept {} bytes after a 1 MB message", buf.capacity()); + let small = recv_message(&mut reader, Magic::REGTEST, &mut buf, None).await.unwrap(); + assert_eq!(small, NetworkMessage::Ping(7)); +} + +/// Bytes are counted as the socket takes them, not once the whole message +/// is out: Core's `m_last_send` and `nSendBytes` move on every partial send +/// (`SocketSendData`), and the send timeout is measured against that. +#[tokio::test] +async fn a_send_counts_its_bytes_as_the_socket_takes_them() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let (dialed, accepted) = tokio::join!( + TcpStream::connect(listener.local_addr().unwrap()), + listener.accept() + ); + // The far end never reads. + let (_far, _) = accepted.unwrap(); + let conn = Connection::with_magic(dialed.unwrap(), Magic::REGTEST); + let (_reader, mut writer) = conn.split(); + let stats = PeerStats::new(crate::net::stats::NetTotals::new()); + writer.set_counters(stats.clone()); + + let msg_len = frame(unknown(3_900_000)).len() as u64; + let mut completed = 0u64; + while let Ok(r) = tokio::time::timeout(Duration::from_millis(500), writer.send(unknown(3_900_000))).await { + r.unwrap(); + completed += msg_len; + assert!(completed < 512 * 1024 * 1024, "the socket never filled"); + } + assert!( + stats.bytes_sent() > completed, + "a send the socket only partly took is not counted: {} sent, {completed} in whole messages", + stats.bytes_sent() + ); +} diff --git a/node/src/net/limits.rs b/node/src/net/limits.rs new file mode 100644 index 000000000..283556999 --- /dev/null +++ b/node/src/net/limits.rs @@ -0,0 +1,60 @@ +//! Bitcoin Core's limits on what one P2P message may carry, and its +//! sanitizing of peer-supplied text. +//! +//! rust-bitcoin decodes a vector of any length the frame can hold +//! (`MAX_INV_SIZE` is documented as "not currently enforced"), so these are +//! applied to the decoded message, with Core's reaction to each. + +/// Core's `MAX_HEADERS_RESULTS` (`net_processing.h`): the most headers a +/// `headers` message may carry. More is `Misbehaving` ("headers message +/// size = N"). +pub const MAX_HEADERS_RESULTS: usize = 2000; + +/// Core's `MAX_ADDR_TO_SEND` (`net_processing.cpp`): the most entries an +/// `addr` or `addrv2` message may carry. More is `Misbehaving` ("addr +/// message size = N", `ProcessAddrs`). +pub const MAX_ADDR_TO_SEND: usize = 1000; + +/// The most entries of a `notfound` Core looks at: `MAX_PEER_TX_ANNOUNCEMENTS +/// + MAX_BLOCKS_IN_TRANSIT_PER_PEER` (5000 + 16, `net_processing.cpp` +/// `NOTFOUND`). A longer one is ignored whole, with no penalty. +pub const MAX_NOTFOUND_SZ: usize = 5000 + 16; + +/// Characters Core's `SanitizeString` keeps under `SAFE_CHARS_DEFAULT` +/// (`util/strencodings.cpp`): alphanumerics plus ` .,;-_/:?@()`. +fn is_safe_char(c: char) -> bool { + c.is_ascii_alphanumeric() || " .,;-_/:?@()".contains(c) +} + +/// Core's `SanitizeString(str)`: drop every character outside +/// `SAFE_CHARS_DEFAULT`. Core applies it to a peer's user agent before it +/// stores or logs it (`cleanSubVer`), and to message types in log lines, so +/// nothing a peer sends can put a line break, a control character or an +/// escape sequence into the log or into `getpeerinfo`. +pub fn sanitize_string(s: &str) -> String { + s.chars().filter(|c| is_safe_char(*c)).collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Core's `SAFE_CHARS_DEFAULT`, character for character: everything it + /// lists survives, and nothing else in the byte range does. + #[test] + fn sanitize_string_keeps_exactly_cores_safe_chars() { + let core_safe: String = ('a'..='z') + .chain('A'..='Z') + .chain('0'..='9') + .chain(" .,;-_/:?@()".chars()) + .collect(); + assert_eq!(sanitize_string(&core_safe), core_safe); + for b in 0u8..=255 { + let c = char::from(b); + let kept = sanitize_string(&c.to_string()); + assert_eq!(kept.is_empty(), !core_safe.contains(c), "byte {b:#04x}"); + } + assert_eq!(sanitize_string("/Satoshi:27.0.0/\nfake log line\x1b[31m"), "/Satoshi:27.0.0/fake log line31m"); + assert_eq!(sanitize_string("caf\u{e9}"), "caf"); + } +} diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index aea389ed4..f883cd5e0 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -411,6 +411,12 @@ pub enum NetEvent { id: PeerId, msg: NetworkMessage, }, + /// The peer's queue has drained back under the send buffer while some + /// of its `getdata` entries are still unserved: serve more of them. Sent + /// by the peer's write loop; see [`crate::net::send_queue`]. + GetDataResume { + id: PeerId, + }, } /// A block handed to the block processor: the peer it came from, that peer's @@ -445,7 +451,9 @@ struct RecentBlock { /// Handle for sending messages to a specific peer. struct PeerHandle { info: PeerInfo, - msg_tx: mpsc::Sender, + /// The peer's outbound queue. It counts the bytes queued, which Core's + /// send-buffer limit is measured in; see [`crate::net::send_queue`]. + msg_tx: crate::net::send_queue::PeerSender, /// Last time we sent this peer a `getheaders`, for rate-limiting /// announcement-triggered header discovery (anti-DoS). `None` until the /// first send. @@ -479,10 +487,12 @@ struct PeerHandle { flow: Arc, } -/// A message's type as Core names it in a log line. +/// A message's type as Core names it in a log line. A type satd does not +/// know is the peer's own text, sanitized as Core's `SanitizeString(msg_type)` +/// does, so it cannot break a log line. fn handshake_msg_type(msg: &NetworkMessage) -> String { match msg { - NetworkMessage::Unknown { command, .. } => command.to_string(), + NetworkMessage::Unknown { command, .. } => crate::net::limits::sanitize_string(command.as_ref()), m => m.cmd().to_string(), } } @@ -494,15 +504,16 @@ fn log_received(id: PeerId, msg: &NetworkMessage) { if !tracing::enabled!(tracing::Level::DEBUG) { return; } - let cmd = match msg { - NetworkMessage::Unknown { command, .. } => command.to_string(), - m => m.cmd().to_string(), - }; + let cmd = handshake_msg_type(msg); let raw = bitcoin::p2p::message::RawNetworkMessage::new(bitcoin::p2p::Magic::REGTEST, msg.clone()); let payload = bitcoin::consensus::serialize(&raw).len().saturating_sub(24); tracing::debug!("received: {cmd} ({payload} bytes) peer={id}"); } +#[cfg(test)] +#[path = "manager_p2pbounds_tests.rs"] +mod p2pbounds_tests; + /// How a transaction is announced to a peer: `MSG_WTX` carrying the wtxid /// to a peer that negotiated BIP 339 wtxid relay, the txid to the rest. fn tx_announcement(wtxid_relay: bool, txid: bitcoin::Txid, wtxid: bitcoin::Wtxid) -> Inventory { @@ -2948,7 +2959,7 @@ impl PeerManager { id, PeerHandle { info, - msg_tx, + msg_tx: msg_tx.into(), disconnect: Arc::new(tokio::sync::Notify::new()), flow: Arc::new(crate::net::flow::PeerFlow::new()), last_getheaders_sent: None, @@ -3745,6 +3756,9 @@ impl PeerManager { Ok(NetEvent::PeerDisconnected { id }) => { self.handle_peer_disconnected(id); } + Ok(NetEvent::GetDataResume { id }) => { + self.resume_getdata(id); + } Ok(NetEvent::MessageReceived { id, msg }) => { // The socket task counted this message in as it was // handed over; the guard counts it out once the work @@ -4255,9 +4269,20 @@ impl PeerManager { // against that peer's outstanding ping and does not forward it. NetworkMessage::Pong(_) => {} NetworkMessage::Inv(inventory) => { + // Core's `MAX_INV_SZ` (`net_processing.cpp` INV). + if inventory.len() > MAX_INV_PER_MSG { + self.add_ban_score(id, 100, &format!("inv message size = {}", inventory.len())); + return; + } self.handle_inv(id, inventory); } NetworkMessage::Headers(headers) => { + // Core's `MAX_HEADERS_RESULTS`, checked before any header in + // the message is looked at. + if headers.len() > crate::net::limits::MAX_HEADERS_RESULTS { + self.add_ban_score(id, 100, &format!("headers message size = {}", headers.len())); + return; + } self.handle_headers(id, headers); } NetworkMessage::Block(block) => { @@ -4277,6 +4302,12 @@ impl PeerManager { } NetworkMessage::GetData(inv) => { self.handle_getdata(id, inv); + // The write loop stopped reading this peer when it handed the + // request over. Whatever is left unserved keeps it stopped; + // see `serve_getdata`. + if let Some(sender) = self.peer_sender(id) { + sender.queue().note_getdata_handled(); + } } NetworkMessage::SendCmpct(msg) => { // Only version 2 (witness) compact blocks are spoken; Core @@ -4328,6 +4359,13 @@ impl PeerManager { // Otherwise this is the message that latches the link's // addr relay on, inbound included. let relay_addrs = self.setup_address_relay(id); + // Core's `ProcessAddrs`: past `MAX_ADDR_TO_SEND` entries the + // message is misbehaviour and nothing in it is stored. A link + // that relays no addresses ignores it whatever its size. + if relay_addrs && addrs.len() > crate::net::limits::MAX_ADDR_TO_SEND { + self.add_ban_score(id, 100, &format!("addr message size = {}", addrs.len())); + return; + } let source = self.peer_ip(id); for (_, addr) in &addrs { if relay_addrs @@ -4477,6 +4515,11 @@ impl PeerManager { // As above: a block-relay-only link relays no addresses in // either direction, and anything else latches on. let relay_addrs = self.setup_address_relay(id); + // As for `addr`. + if relay_addrs && addrs.len() > crate::net::limits::MAX_ADDR_TO_SEND { + self.add_ban_score(id, 100, &format!("addrv2 message size = {}", addrs.len())); + return; + } let source = self.peer_ip(id); for addr_msg in addrs.iter().filter(|_| relay_addrs) { match &addr_msg.addr { @@ -4522,6 +4565,13 @@ impl PeerManager { self.disconnect_by_id(id); } NetworkMessage::NotFound(inventory) => { + // Core looks at a `notfound` only up to + // `MAX_PEER_TX_ANNOUNCEMENTS + MAX_BLOCKS_IN_TRANSIT_PER_PEER` + // entries, and ignores a longer one whole, without penalty. + if inventory.len() > crate::net::limits::MAX_NOTFOUND_SZ { + tracing::debug!(id, count = inventory.len(), "ignoring an oversized notfound"); + return; + } // Authoritative "I don't have this" from the peer. Until // this commit we logged at debug and did nothing, so the // height stayed in_flight for the full 60s @@ -4581,24 +4631,20 @@ impl PeerManager { handle.info.prefers_headers = true; } } + // Each request is checked as Core's `PrepareBlockFilterRequest` + // does, including whether filters are served at all. #[cfg(feature = "block-filter-index")] - NetworkMessage::GetCFilters(req) => { - if self.peer_serve_filters_ready() { - self.handle_get_cfilters(id, req); - } - // Silent drop per BIP 157 when not serving. - } + NetworkMessage::GetCFilters(req) => self.handle_get_cfilters(id, req), #[cfg(feature = "block-filter-index")] - NetworkMessage::GetCFHeaders(req) => { - if self.peer_serve_filters_ready() { - self.handle_get_cfheaders(id, req); - } - } + NetworkMessage::GetCFHeaders(req) => self.handle_get_cfheaders(id, req), #[cfg(feature = "block-filter-index")] - NetworkMessage::GetCFCheckpt(req) => { - if self.peer_serve_filters_ready() { - self.handle_get_cfcheckpt(id, req); - } + NetworkMessage::GetCFCheckpt(req) => self.handle_get_cfcheckpt(id, req), + // Built without filters: no filter type is served, and Core + // disconnects a peer that asks for one it does not serve. + #[cfg(not(feature = "block-filter-index"))] + NetworkMessage::GetCFilters(_) | NetworkMessage::GetCFHeaders(_) | NetworkMessage::GetCFCheckpt(_) => { + tracing::debug!("peer requested unsupported block filter type, disconnecting peer={id}"); + self.disconnect_by_id(id); } _ => {} } @@ -7690,42 +7736,104 @@ impl PeerManager { ); } - /// `getcfilters` handler per BIP 157. - /// Validates filter type, range bounds, and active-chain stop hash; - /// silent-drops any violation. On success, replies with one - /// `CFilter` per height in the requested range (BIP 157 specifies - /// per-height responses, not a batched form). + /// Core's `PrepareBlockFilterRequest`: the checks a BIP 157 request + /// passes before it is served. Returns the stop block's height and the + /// index to serve from. + /// + /// A request satd will not answer as asked disconnects the peer, as in + /// Core (`fDisconnect`, no misbehaviour): a filter type it does not serve + /// (any type, while it serves no filters), a stop hash it does not know + /// or would not serve, a start above the stop, or a range of + /// `max_height_diff` heights or more. satd used to ignore them all. + /// + /// A stop hash off the active chain that Core would still serve (a block + /// its `BlockRequestAllowed` admits) is ignored without a disconnect: + /// satd keeps filters for the active chain only, so it has nothing to + /// send, but the request is not one Core would hold against the peer. + #[cfg(feature = "block-filter-index")] + fn prepare_block_filter_request( + &self, + id: PeerId, + filter_type: u8, + start_height: u32, + stop_hash: bitcoin::BlockHash, + max_height_diff: u32, + ) -> Option<(u32, Arc)> { + if filter_type != node_filter_index::FILTER_TYPE_BASIC || !self.peer_serve_filters_ready() { + tracing::debug!( + "peer requested unsupported block filter type: {filter_type}, disconnecting peer={id}" + ); + self.disconnect_by_id(id); + return None; + } + let stop_height = match self.chain_state.get_block_index(&stop_hash) { + Some(e) if self.chain_state.active_chain_contains(&stop_hash, e.height) => e.height, + Some(e) if self.stale_block_request_allowed(&e) => { + tracing::debug!("no filters for block {stop_hash} off the active chain, peer={id}"); + return None; + } + _ => { + tracing::debug!("peer requested invalid block hash: {stop_hash}, disconnecting peer={id}"); + self.disconnect_by_id(id); + return None; + } + }; + if start_height > stop_height { + tracing::debug!( + "peer sent invalid getcfilters/getcfheaders with start height {start_height} and \ + stop height {stop_height}, disconnecting peer={id}" + ); + self.disconnect_by_id(id); + return None; + } + if stop_height - start_height >= max_height_diff { + tracing::debug!( + "peer requested too many cfilters/cfheaders: {} / {max_height_diff}, disconnecting peer={id}", + u64::from(stop_height - start_height) + 1 + ); + self.disconnect_by_id(id); + return None; + } + // Core: "Filter index for supported type not found", no disconnect. + self.filter_index.get().cloned().map(|idx| (stop_height, idx)) + } + + /// The half of Core's `BlockRequestAllowed` that admits a block off the + /// active chain: one this node validated, no more than + /// `STALE_RELAY_AGE_LIMIT` (30 days) older than the best header. + /// `BlockStatus::Valid` is written only by `connect_block` and survives + /// the reorg that takes the block off the active chain, as Core's + /// `BLOCK_VALID_SCRIPTS` does. Core also bounds the block's + /// work-equivalent age; that bound is not applied, so this admits a little + /// more than Core does. + #[cfg(feature = "block-filter-index")] + fn stale_block_request_allowed(&self, entry: &crate::storage::blockindex::BlockIndexEntry) -> bool { + const STALE_RELAY_AGE_LIMIT: i64 = 30 * 24 * 60 * 60; + if entry.status != crate::storage::blockindex::BlockStatus::Valid { + return false; + } + self.chain_state + .get_block_index(&self.chain_state.best_header_hash()) + .is_some_and(|best| { + i64::from(best.header.time) - i64::from(entry.header.time) < STALE_RELAY_AGE_LIMIT + }) + } + + /// `getcfilters` handler per BIP 157, checked by + /// [`Self::prepare_block_filter_request`]. Replies with one `CFilter` per + /// height in the requested range (BIP 157 specifies per-height + /// responses, not a batched form). #[cfg(feature = "block-filter-index")] fn handle_get_cfilters(&self, id: PeerId, req: bitcoin::p2p::message_filter::GetCFilters) { use crate::index::filter::lookups::MAX_GETCFILTERS_SIZE; - use node_filter_index::FILTER_TYPE_BASIC; let bitcoin::p2p::message_filter::GetCFilters { filter_type, start_height, stop_hash, } = req; - // Filter type guard. - if filter_type != FILTER_TYPE_BASIC { - return; - } - // Resolve stop_hash → stop_height via block_index. - let Some(stop_entry) = self.chain_state.get_block_index(&stop_hash) else { - return; - }; - let stop_height = stop_entry.height; - // BIP 157: stop_height ≥ start_height, range < 1000. - if stop_height < start_height { - return; - } - if stop_height - start_height >= MAX_GETCFILTERS_SIZE { - return; - } - // Active-chain check: stop_hash must match the active chain at stop_height. - match self.chain_state.get_block_hash_by_height(stop_height) { - Some(h) if h == stop_hash => {} - _ => return, - } - let Some(idx) = self.filter_index.get() else { + let Some((stop_height, idx)) = + self.prepare_block_filter_request(id, filter_type, start_height, stop_hash, MAX_GETCFILTERS_SIZE) + else { return; }; // Stream responses via an async task with backpressure. The @@ -7743,7 +7851,6 @@ impl PeerManager { return; }; let chain_state = self.chain_state.clone(); - let idx = idx.clone(); tokio::spawn(async move { for h in start_height..=stop_height { let Some(block_hash) = chain_state.get_block_hash_by_height(h) else { @@ -7769,7 +7876,8 @@ impl PeerManager { }); } - /// `getcfheaders` handler per BIP 157. + /// `getcfheaders` handler per BIP 157, checked by + /// [`Self::prepare_block_filter_request`]. /// Replies with a single `CFHeaders` carrying /// `previous_filter_header` plus per-height filter hashes (computed /// on the fly from the stored filter blob — see plan §"Filter-hash @@ -7780,32 +7888,20 @@ impl PeerManager { use crate::index::filter::lookups::MAX_GETCFHEADERS_SIZE; use bitcoin::bip158::FilterHash; use bitcoin::hashes::Hash; - use node_filter_index::FILTER_TYPE_BASIC; let bitcoin::p2p::message_filter::GetCFHeaders { filter_type, start_height, stop_hash, } = req; - if filter_type != FILTER_TYPE_BASIC { - return; - } - let Some(stop_entry) = self.chain_state.get_block_index(&stop_hash) else { - return; - }; - let stop_height = stop_entry.height; - if stop_height < start_height { - return; - } // Bitcoin Core / BIP 157 cap getcfheaders at 2000, not the 1000 // that applies to getcfilters. Review 2026-05-04 M1. - if stop_height - start_height >= MAX_GETCFHEADERS_SIZE { - return; - } - match self.chain_state.get_block_hash_by_height(stop_height) { - Some(h) if h == stop_hash => {} - _ => return, - } - let Some(idx) = self.filter_index.get() else { + let Some((stop_height, idx)) = self.prepare_block_filter_request( + id, + filter_type, + start_height, + stop_hash, + MAX_GETCFHEADERS_SIZE, + ) else { return; }; // previous_filter_header: header at start_height - 1, or all-zeros for height 0. @@ -7839,27 +7935,19 @@ impl PeerManager { /// `getcfcheckpt` handler per BIP 157 — filter headers at every /// 1000-block boundary up to (and including) the highest 1000-block - /// boundary ≤ `stop_height`. + /// boundary ≤ `stop_height`. Checked by + /// [`Self::prepare_block_filter_request`] with no range limit, as Core + /// does. #[cfg(feature = "block-filter-index")] fn handle_get_cfcheckpt(&self, id: PeerId, req: bitcoin::p2p::message_filter::GetCFCheckpt) { use bitcoin::hashes::Hash; - use node_filter_index::FILTER_TYPE_BASIC; let bitcoin::p2p::message_filter::GetCFCheckpt { filter_type, stop_hash, } = req; - if filter_type != FILTER_TYPE_BASIC { - return; - } - let Some(stop_entry) = self.chain_state.get_block_index(&stop_hash) else { - return; - }; - let stop_height = stop_entry.height; - match self.chain_state.get_block_hash_by_height(stop_height) { - Some(h) if h == stop_hash => {} - _ => return, - } - let Some(idx) = self.filter_index.get() else { + let Some((stop_height, idx)) = + self.prepare_block_filter_request(id, filter_type, 0, stop_hash, u32::MAX) + else { return; }; let max_idx = stop_height / 1000; @@ -8023,91 +8111,164 @@ impl PeerManager { } fn handle_getdata(&self, id: PeerId, inventory: Vec) { + // Core's `MAX_INV_SZ` (`net_processing.cpp` GETDATA), checked before + // anything in the request is looked at. + if inventory.len() > MAX_INV_PER_MSG { + self.add_ban_score(id, 100, &format!("getdata message size = {}", inventory.len())); + return; + } // Core's `ProcessMessage` line for every getdata. match inventory.as_slice() { [inv] => tracing::debug!("received getdata for: {} peer={id}", inv_to_string(inv)), invs => tracing::debug!("received getdata ({} invsz) peer={id}", invs.len()), } + let Some(sender) = self.peer_sender(id) else { + return; + }; + // Queued behind anything still unserved and served from there: + // Core's `m_getdata_requests` and `ProcessGetData`. + sender.queue().push_getdata(inventory); + self.serve_getdata(id, &sender); + } + + /// [`NetEvent::GetDataResume`]: the peer's queue has drained enough to + /// serve more of its `getdata` backlog. + fn resume_getdata(&self, id: PeerId) { + let Some(sender) = self.peer_sender(id) else { + return; + }; + // Before looking at the queue: a write that drains it after this + // point asks again rather than finding a request already waiting. + sender.queue().resume_taken(); + self.serve_getdata(id, &sender); + } + + /// The sending end of a peer's queue, while the peer is connected. + fn peer_sender(&self, id: PeerId) -> Option { + self.peers.read().get(&id).map(|h| h.msg_tx.clone()) + } + + /// Core's `ProcessGetData`: serve the peer's unserved `getdata` entries, + /// oldest first, until none are left or its queue is past the send + /// buffer (`fPauseSend`). + /// + /// The rest waits for the queue to drain. The peer's write loop asks for + /// more as it does ([`NetEvent::GetDataResume`]), and takes no other + /// message from the peer until the backlog is empty, which keeps the + /// answers in the order asked and the backlog to one request. + /// + /// satd served every entry at once, reading and deserializing each block + /// and queueing it with a `try_send` whose failure it ignored. One + /// request could hold a peer's whole queue of blocks in memory for as + /// long as the peer did not read them, and the blocks that did not fit + /// were read, dropped, and still charged to `-maxuploadtarget`. + fn serve_getdata(&self, id: PeerId, sender: &crate::net::send_queue::PeerSender) { + let queue = sender.queue(); let mut not_found = Vec::new(); - for inv in inventory { - match inv { - // `MSG_CMPCT_BLOCK` (BIP 152). A block within - // `MAX_CMPCTBLOCK_DEPTH` of the tip is answered with a - // `cmpctblock` (`serve_compact_block`). Anything deeper, or - // anything while we are still syncing, gets the full `block`, - // which BIP 152 permits and Core itself sends; Core accepts - // it against its in-flight compact request. Letting the - // request fall through to `_ => {}` instead would silently - // drop it, and a Core peer never re-requests. - Inventory::Block(hash) - | Inventory::WitnessBlock(hash) - | Inventory::CompactBlock(hash) => { - if matches!(inv, Inventory::CompactBlock(_)) && self.serve_compact_block(id, &hash) { - continue; - } - if let Some(block) = self.chain_state.get_block(&hash) { - // -maxuploadtarget: decline historical blocks once - // the rolling budget is spent (download/noban peers - // and recent blocks are exempt). - if !self.upload_permits_block(id, &block) { - tracing::debug!(id, %hash, "maxuploadtarget reached; declining historical block"); - not_found.push(inv); - continue; - } - self.record_upload(block.total_size() as u64); - self.send_to_peer(id, NetworkMessage::Block(block)); - } else { - not_found.push(inv); - } + while !sender.paused() { + let Some(inv) = queue.front_getdata() else { + break; + }; + if !self.serve_getdata_entry(id, sender, inv, &mut not_found) { + // The queue filled after the check above, or the peer is + // gone. The entry stays first; the write loop asks again once + // there is room. + break; + } + queue.pop_getdata(); + } + if !not_found.is_empty() { + let _ = sender.try_send(NetworkMessage::NotFound(not_found)); + } + if queue.getdata_backlog() == 0 { + queue.wake_reader(); + } + } + + /// Answer one `getdata` entry: queue what it asks for, or add it to the + /// `notfound`. False when nothing could be queued and the entry is still + /// to be served. + fn serve_getdata_entry( + &self, + id: PeerId, + sender: &crate::net::send_queue::PeerSender, + inv: Inventory, + not_found: &mut Vec, + ) -> bool { + match inv { + // `MSG_CMPCT_BLOCK` (BIP 152). A block within + // `MAX_CMPCTBLOCK_DEPTH` of the tip is answered with a + // `cmpctblock` (`serve_compact_block`). Anything deeper, or + // anything while we are still syncing, gets the full `block`, + // which BIP 152 permits and Core itself sends; Core accepts + // it against its in-flight compact request. Letting the + // request fall through to `_ => {}` instead would silently + // drop it, and a Core peer never re-requests. + Inventory::Block(hash) + | Inventory::WitnessBlock(hash) + | Inventory::CompactBlock(hash) => { + if matches!(inv, Inventory::CompactBlock(_)) && self.serve_compact_block(id, &hash) { + return true; + } + let Some(block) = self.chain_state.get_block(&hash) else { + not_found.push(inv); + return true; + }; + // -maxuploadtarget: decline historical blocks once + // the rolling budget is spent (download/noban peers + // and recent blocks are exempt). + if !self.upload_permits_block(id, &block) { + tracing::debug!(id, %hash, "maxuploadtarget reached; declining historical block"); + not_found.push(inv); + return true; + } + let size = block.total_size() as u64; + if sender.try_send(NetworkMessage::Block(block)).is_err() { + return false; } - Inventory::Transaction(txid) | Inventory::WitnessTransaction(txid) => { - // Relay-quarantined txs are not served via `getdata`: - // announce-suppression without serve-suppression would be - // incoherent — a peer that learned the txid elsewhere must - // not be able to pull it from us (design §6.1). Report it - // as not-found, exactly as if we did not hold it. - if let Some(entry) = self - .mempool - .get(&txid) - .filter(|e| e.scope.assists_relay()) - { - // The peer pulled this tx from us. If it's a pending - // local broadcast, that's the primary proof it has - // propagated — count the peer toward stopping - // rebroadcast. Only if the Tx was actually enqueued: - // a try_send drop (slow peer, full channel) means the - // peer never received it, and retiring the tx on a - // dropped send would defeat the exact failure mode - // rebroadcast exists to cover. - if self.send_to_peer(id, NetworkMessage::Tx(entry.tx)) { - self.note_broadcast_witness(id, txid); - } - } else { - not_found.push(inv); - } + // Charged for a block that was queued, not one that was read. + self.record_upload(size); + } + Inventory::Transaction(txid) | Inventory::WitnessTransaction(txid) => { + // Relay-quarantined txs are not served via `getdata`: + // announce-suppression without serve-suppression would be + // incoherent — a peer that learned the txid elsewhere must + // not be able to pull it from us (design §6.1). Report it + // as not-found, exactly as if we did not hold it. + let Some(entry) = self.mempool.get(&txid).filter(|e| e.scope.assists_relay()) else { + not_found.push(inv); + return true; + }; + // The peer pulled this tx from us. If it's a pending + // local broadcast, that's the primary proof it has + // propagated — count the peer toward stopping + // rebroadcast. Only once the Tx is actually enqueued: + // retiring the tx on a send that did not happen would + // defeat the exact failure mode rebroadcast exists to cover. + if sender.try_send(NetworkMessage::Tx(entry.tx)).is_err() { + return false; } - // BIP 339: the same, looked up by wtxid. A resident - // transaction with the same txid and another witness is not - // the one asked for, so the answer is `notfound`. - Inventory::WTx(wtxid) => { - if let Some((txid, entry)) = self - .mempool - .get_by_wtxid(&wtxid) - .filter(|(_, e)| e.scope.assists_relay()) - { - if self.send_to_peer(id, NetworkMessage::Tx(entry.tx)) { - self.note_broadcast_witness(id, txid); - } - } else { - not_found.push(inv); - } + self.note_broadcast_witness(id, txid); + } + // BIP 339: the same, looked up by wtxid. A resident + // transaction with the same txid and another witness is not + // the one asked for, so the answer is `notfound`. + Inventory::WTx(wtxid) => { + let Some((txid, entry)) = + self.mempool.get_by_wtxid(&wtxid).filter(|(_, e)| e.scope.assists_relay()) + else { + not_found.push(inv); + return true; + }; + if sender.try_send(NetworkMessage::Tx(entry.tx)).is_err() { + return false; } - _ => {} + self.note_broadcast_witness(id, txid); } + // An entry of a type that is not served is skipped, as in Core. + _ => {} } - if !not_found.is_empty() { - self.send_to_peer(id, NetworkMessage::NotFound(not_found)); - } + true } /// Accept the header a `cmpctblock` carries, the way a `headers` message @@ -9207,7 +9368,7 @@ impl PeerManager { }; let handle = PeerHandle { info, - msg_tx, + msg_tx: msg_tx.into(), disconnect: Arc::new(tokio::sync::Notify::new()), flow: Arc::new(crate::net::flow::PeerFlow::new()), last_getheaders_sent: None, @@ -9577,13 +9738,16 @@ impl PeerManager { // the connection is torn down anyway. // Also the ping accounting the write loop needs below, so the map is // read once rather than again just before the loop starts. - let (ping_stats, peer_flow, disconnect_signal) = { + let (ping_stats, peer_flow, disconnect_signal, send_queue) = { let peers = self.peers.read(); match peers.get(&id) { - Some(h) => { - (Some(h.stats.clone()), Some(h.flow.clone()), Some(h.disconnect.clone())) - } - None => (None, None, None), + Some(h) => ( + Some(h.stats.clone()), + Some(h.flow.clone()), + Some(h.disconnect.clone()), + Some(h.msg_tx.queue().clone()), + ), + None => (None, None, None, None), } }; if let Some(stats) = &ping_stats { @@ -9732,6 +9896,7 @@ impl PeerManager { peer_flow, Some(self.drain_now.clone()), disconnect_signal, + send_queue, ) .await; @@ -9786,6 +9951,14 @@ impl PeerManager { /// terminate instead of leaving an untracked peer feeding events. /// The earlier `Some(msg) = msg_rx.recv()` pattern silently /// disabled the branch on close — review F2 (PRs #180-#184). + /// - the disconnect signal, or a write that makes no progress for + /// [`crate::net::send_queue::SEND_TIMEOUT`] → exit, even with the + /// write unfinished (`send_watched`). + /// + /// Flow control, after Core's send buffer (`send_queue`): the bytes of + /// every message written are taken off the peer's queue count; while the + /// count is over the limit, or a `getdata` of the peer's is being served, + /// the peer's next message is left unread. #[allow(clippy::too_many_arguments)] async fn peer_write_loop( id: PeerId, @@ -9797,6 +9970,7 @@ impl PeerManager { flow: Option>, drain_now: Option>, disconnect_signal: Option>, + send_queue: Option>, ) -> Result<(), String> { // Bitcoin Core's keepalive cadence (`PING_INTERVAL`, net_processing.h). // The first tick of a tokio interval fires immediately, which is also @@ -9827,6 +10001,12 @@ impl PeerManager { tokio::pin!(disconnect_requested); loop { + // Core's `ProcessMessages` takes nothing more from a peer while a + // `getdata` of its is being served, or while its send buffer is + // full ("Don't bother if send buffer is too full to respond + // anyway"). The message stays in the peer's read queue, and the + // reader task stops reading the socket once that is full. + let reading_paused = send_queue.as_ref().is_some_and(|q| q.reading_paused()); tokio::select! { _ = &mut disconnect_requested => { // Explicit teardown, as opposed to noticing `msg_rx` @@ -9834,6 +10014,14 @@ impl PeerManager { // and closes the socket. return Err("disconnected by manager".to_string()); } + // Woken when the manager has served a `getdata`, to look + // again. The writes below re-check after every message. + _ = async { + match &send_queue { + Some(q) => q.reader_woken().await, + None => std::future::pending::<()>().await, + } + }, if reading_paused => {} _ = ping_timer.tick() => { // A peer that keeps talking but never answers a ping is // dropped here. Returning takes the ordinary teardown @@ -9880,13 +10068,17 @@ impl PeerManager { // it must never go on the wire. let nonce = rand::random::().max(1); stats.ping_sent(nonce); - writer - .send(NetworkMessage::Ping(nonce)) - .await - .map_err(|e| e.to_string())?; + Self::send_watched( + id, + writer, + NetworkMessage::Ping(nonce), + Some(stats), + disconnect_requested.as_mut(), + ) + .await?; } } - msg = read_rx.recv() => { + msg = read_rx.recv(), if !reading_paused => { match msg { // A pong is matched here, on the peer's own task, // rather than forwarded to the manager loop. @@ -9974,10 +10166,14 @@ impl PeerManager { ); } } - writer - .send(NetworkMessage::Pong(nonce)) - .await - .map_err(|e| e.to_string())?; + Self::send_watched( + id, + writer, + NetworkMessage::Pong(nonce), + stats.as_ref(), + disconnect_requested.as_mut(), + ) + .await?; } Some(msg) => { // Counted in here and out wherever the work ends @@ -9987,6 +10183,11 @@ impl PeerManager { if let Some(flow) = &flow { flow.queued(); } + // Nothing more is read until the manager has + // served this request (`reading_paused`). + if let (NetworkMessage::GetData(_), Some(q)) = (&msg, &send_queue) { + q.note_getdata_forwarded(); + } event_tx .send(NetEvent::MessageReceived { id, msg }) .await @@ -10017,7 +10218,23 @@ impl PeerManager { if let (NetworkMessage::Ping(nonce), Some(stats)) = (&msg, &stats) { stats.ping_sent(*nonce); } - writer.send(msg).await.map_err(|e| e.to_string())?; + // What the sender counted the message at. + let size = send_queue + .as_ref() + .map(|_| crate::net::send_queue::queued_size(&msg)); + Self::send_watched(id, writer, msg, stats.as_ref(), disconnect_requested.as_mut()) + .await?; + if let (Some(q), Some(size)) = (&send_queue, size) { + q.sent(size); + // Back under the send buffer with `getdata` + // entries waiting: ask the manager for more. + if q.take_resume() { + event_tx + .send(NetEvent::GetDataResume { id }) + .await + .map_err(|e| e.to_string())?; + } + } } None => { // Manager dropped our handle. Return so the @@ -10034,6 +10251,60 @@ impl PeerManager { } } + /// Write one message to the peer, giving up if the manager drops the peer + /// or the socket takes no bytes for + /// [`crate::net::send_queue::SEND_TIMEOUT`]. + /// + /// A peer that stops reading leaves a write parked for as long as it + /// likes, and the write loop looked at the disconnect signal and the ping + /// deadline only between writes. So `disconnectnode`, a ban and the ping + /// timeout could not reach such a peer, and its socket and everything + /// queued to it stayed. Core drops a peer that has taken no bytes for + /// `TIMEOUT_INTERVAL` (`InactivityCheck`, "socket sending timeout"). + /// Progress is the bytes the socket takes, counted as it takes them, so a + /// slow reader is not mistaken for a stalled one. + async fn send_watched>( + id: PeerId, + writer: &mut ConnectionWriter, + msg: NetworkMessage, + stats: Option<&Arc>, + mut disconnect: std::pin::Pin<&mut F>, + ) -> Result<(), String> { + use crate::net::send_queue::SEND_TIMEOUT; + /// How often a parked write is checked for progress. + const PROGRESS_CHECK: Duration = Duration::from_secs(10); + + let send = writer.send(msg); + tokio::pin!(send); + // Most writes finish at once; only one that has to wait is watched. + tokio::select! { + biased; + res = &mut send => return res.map_err(|e| e.to_string()), + _ = std::future::ready(()) => {} + } + let mut check = tokio::time::interval(PROGRESS_CHECK); + check.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); + let mut sent = stats.map(|s| s.bytes_sent()); + let mut progress_at = tokio::time::Instant::now(); + loop { + tokio::select! { + res = &mut send => return res.map_err(|e| e.to_string()), + _ = disconnect.as_mut() => return Err("disconnected by manager".to_string()), + _ = check.tick() => { + let now_sent = stats.map(|s| s.bytes_sent()); + if now_sent != sent { + sent = now_sent; + progress_at = tokio::time::Instant::now(); + } else if progress_at.elapsed() >= SEND_TIMEOUT { + let secs = progress_at.elapsed().as_secs(); + tracing::debug!("socket sending timeout: {secs}s, disconnecting peer={id}"); + return Err(format!("socket sending timeout: no bytes taken in {secs}s")); + } + } + } + } + } + /// Receive the next handshake message, dropping the peer when Core's /// `InactivityCheck` would: `-peertimeout` seconds after it connected, on /// the node clock, with the handshake still incomplete. @@ -10144,6 +10415,19 @@ impl PeerManager { // with a line naming it. let their_version = loop { match self.recv_handshake(id, conn, connected_at).await? { + // Core reads the user agent as `LIMITED_STRING(strSubVer, + // MAX_SUBVERSION_LENGTH)`. A longer one throws in + // deserialization: the `version` is dropped, and the peer is + // left without a handshake until a well-formed one arrives or + // the connect timeout ends it (`recv_handshake` applies it + // here too). + NetworkMessage::Version(v) if v.user_agent.len() > crate::MAX_SUBVERSION_LENGTH => { + tracing::debug!( + "ignoring version message with a {}-byte user agent (limit {}) from peer={id}", + v.user_agent.len(), + crate::MAX_SUBVERSION_LENGTH + ) + } NetworkMessage::Version(v) => break v, other => tracing::debug!( "non-version message before version handshake. Message \"{}\" from peer={id}", @@ -10867,7 +11151,7 @@ mod tests { let (tx, _rx) = mpsc::channel::(1); PeerHandle { info, - msg_tx: tx, + msg_tx: tx.into(), disconnect: Arc::new(tokio::sync::Notify::new()), flow: Arc::new(crate::net::flow::PeerFlow::new()), last_getheaders_sent: None, @@ -11069,7 +11353,7 @@ mod tests { ( PeerHandle { info, - msg_tx: tx, + msg_tx: tx.into(), disconnect: Arc::new(tokio::sync::Notify::new()), flow: Arc::new(crate::net::flow::PeerFlow::new()), last_getheaders_sent: None, @@ -13487,7 +13771,7 @@ mod tests { 9, PeerHandle { info, - msg_tx: tx, + msg_tx: tx.into(), disconnect: Arc::new(tokio::sync::Notify::new()), flow: Arc::new(crate::net::flow::PeerFlow::new()), last_getheaders_sent: None, diff --git a/node/src/net/manager_p2pbounds_tests.rs b/node/src/net/manager_p2pbounds_tests.rs new file mode 100644 index 000000000..e14865ac6 --- /dev/null +++ b/node/src/net/manager_p2pbounds_tests.rs @@ -0,0 +1,726 @@ +//! Core's per-message limits, the send-buffer bound on `getdata`, and the +//! BIP 157 request checks, against a live `PeerManager`. + +use super::*; +use crate::net::flow::InFlight; +use crate::net::send_queue::{MAX_SEND_BUFFER_BYTES, queued_size}; + +fn mk_pm() -> (Arc, tempfile::TempDir) { + use crate::chain::state::AssumeValid; + use crate::storage::db::InMemoryStore; + use crate::storage::flatfile::FlatFileManager; + use crate::validation::script::NoopVerifier; + + let dir = tempfile::TempDir::new().unwrap(); + let store = Box::new(InMemoryStore::new()); + let flat_files = FlatFileManager::new(&dir.path().join("blocks")).unwrap(); + let chain_state = Arc::new( + ChainState::new( + store, + flat_files, + Network::Regtest, + Box::new(NoopVerifier), + AssumeValid::Disabled, + 450, + 4, + Default::default(), + Default::default(), + Default::default(), + ) + .unwrap(), + ); + let mempool = Arc::new(Mempool::new(1_000_000, 0)); + let fee_estimator = Arc::new(FeeEstimator::new()); + let (_shutdown_tx, shutdown_rx) = tokio::sync::watch::channel(false); + let pm = PeerManager::new(chain_state, mempool, fee_estimator, Network::Regtest, shutdown_rx); + (pm, dir) +} + +/// A connected peer whose queue the test reads, `slots` messages deep. +fn add_peer(pm: &PeerManager, id: PeerId, addr: SocketAddr, slots: usize) -> mpsc::Receiver { + let mut info = PeerInfo::new(id, addr, Direction::Inbound); + info.state = PeerState::Connected; + let (tx, rx) = mpsc::channel::(slots); + pm.peers.write().insert( + id, + PeerHandle { + info, + msg_tx: tx.into(), + disconnect: Arc::new(tokio::sync::Notify::new()), + flow: Arc::new(crate::net::flow::PeerFlow::new()), + last_getheaders_sent: None, + last_mempool_served: None, + fee_filter_sent: None, + stats: PeerStats::new(NetTotals::new()), + }, + ); + rx +} + +fn addr(last: u8) -> SocketAddr { + SocketAddr::from(([10, 61, 0, last], 8333)) +} + +fn deliver(pm: &PeerManager, id: PeerId, msg: NetworkMessage) { + pm.handle_message(id, msg, InFlight::adopt(None)); +} + +fn connected(pm: &PeerManager, id: PeerId) -> bool { + pm.peers.read().contains_key(&id) +} + +/// Mine `n` blocks of roughly `size` bytes each (the coinbase carries an +/// OP_RETURN of that size) and return their hashes in height order. +fn mine_big(pm: &PeerManager, n: usize, size: usize) -> Vec { + let mempool = Mempool::new(1_000_000, 0); + (0..n) + .map(|i| { + let data = bitcoin::script::PushBytesBuf::try_from(vec![i as u8; size]).unwrap(); + let block = crate::mining::miner::build_block_to_script( + &pm.chain_state, + &mempool, + bitcoin::ScriptBuf::new_op_return(&data), + None, + ) + .expect("mine regtest block"); + pm.chain_state.accept_block(&block).expect("accept block"); + block.block_hash() + }) + .collect() +} + +fn drain(rx: &mut mpsc::Receiver) -> Vec { + std::iter::from_fn(|| rx.try_recv().ok()).collect() +} + +// ---- Message count limits ---- + +/// Core's `MAX_INV_SZ`: an `inv` or `getdata` of more than 50,000 entries is +/// `Misbehaving` (`net_processing.cpp` INV / GETDATA). 50,000 is fine. +#[test] +fn inv_and_getdata_past_max_inv_sz_are_misbehaviour() { + let (pm, _dir) = mk_pm(); + let entry = Inventory::Unknown { inv_type: 99, hash: [7u8; 32] }; + for (i, (make, name)) in [ + (NetworkMessage::Inv as fn(Vec) -> NetworkMessage, "inv"), + (NetworkMessage::GetData as fn(Vec) -> NetworkMessage, "getdata"), + ] + .into_iter() + .enumerate() + { + let (ok, over) = (2 * i as u8 + 1, 2 * i as u8 + 2); + let _rx_ok = add_peer(&pm, ok.into(), addr(ok), 8); + deliver(&pm, ok.into(), make(vec![entry; MAX_INV_PER_MSG])); + assert!(connected(&pm, ok.into()), "{name} of {MAX_INV_PER_MSG} entries is allowed"); + + let _rx_over = add_peer(&pm, over.into(), addr(over), 8); + deliver(&pm, over.into(), make(vec![entry; MAX_INV_PER_MSG + 1])); + assert!(!connected(&pm, over.into()), "{name} of {} entries disconnects", MAX_INV_PER_MSG + 1); + assert!(pm.is_addr_banned(&addr(over)), "{name} past MAX_INV_SZ is misbehaviour"); + } +} + +/// Core's `MAX_HEADERS_RESULTS`: a `headers` message of more than 2000 is +/// `Misbehaving`, before any header in it is looked at. +#[test] +fn headers_past_max_headers_results_are_misbehaviour() { + let (pm, _dir) = mk_pm(); + let genesis = pm.chain_state.get_block_index(&pm.chain_state.tip_hash()).unwrap().header; + + let _rx = add_peer(&pm, 1, addr(1), 8); + deliver(&pm, 1, NetworkMessage::Headers(vec![genesis; 2000])); + assert!(connected(&pm, 1), "2000 headers are allowed"); + + let _rx = add_peer(&pm, 2, addr(2), 8); + deliver(&pm, 2, NetworkMessage::Headers(vec![genesis; 2001])); + assert!(!connected(&pm, 2), "2001 headers disconnect"); + assert!(pm.is_addr_banned(&addr(2)), "2001 headers are misbehaviour"); +} + +/// Core's `MAX_ADDR_TO_SEND`: an `addr` or `addrv2` of more than 1000 entries +/// is `Misbehaving` (`ProcessAddrs`), and none of it is stored. +#[test] +fn addr_and_addrv2_past_max_addr_to_send_are_misbehaviour() { + use bitcoin::p2p::address::{AddrV2, AddrV2Message}; + let (pm, _dir) = mk_pm(); + let v1 = |n: usize| { + NetworkMessage::Addr( + (0..n) + .map(|i| { + let a = SocketAddr::from(([44, 1, (i / 256) as u8, (i % 256) as u8], 8333)); + (1_700_000_000, Address::new(&a, ServiceFlags::NETWORK)) + }) + .collect(), + ) + }; + let v2 = |n: usize| { + NetworkMessage::AddrV2( + (0..n) + .map(|i| AddrV2Message { + time: 1_700_000_000, + services: ServiceFlags::NETWORK, + addr: AddrV2::Ipv4(std::net::Ipv4Addr::new(45, 1, (i / 256) as u8, (i % 256) as u8)), + port: 8333, + }) + .collect(), + ) + }; + for (i, make) in [&v1 as &dyn Fn(usize) -> NetworkMessage, &v2].into_iter().enumerate() { + let (ok, over) = (2 * i as u8 + 1, 2 * i as u8 + 2); + let _rx = add_peer(&pm, ok.into(), addr(ok), 8); + deliver(&pm, ok.into(), make(1000)); + assert!(connected(&pm, ok.into()), "case {i}: 1000 addresses are allowed"); + + let _rx = add_peer(&pm, over.into(), addr(over), 8); + deliver(&pm, over.into(), make(1001)); + assert!(!connected(&pm, over.into()), "case {i}: 1001 addresses disconnect"); + assert!(pm.is_addr_banned(&addr(over)), "case {i}: 1001 addresses are misbehaviour"); + } +} + +/// Core looks at a `notfound` only up to `MAX_PEER_TX_ANNOUNCEMENTS + +/// MAX_BLOCKS_IN_TRANSIT_PER_PEER` entries and ignores a longer one whole, +/// with no penalty. satd acts on the block entries of a `notfound` (it +/// releases those heights for another peer), so a longer one releases +/// nothing. +#[test] +fn notfound_past_the_limit_is_ignored() { + use crate::net::limits::MAX_NOTFOUND_SZ; + let (pm, _dir) = mk_pm(); + let hashes = mine_big(&pm, 4, 10); + let filler = Inventory::Unknown { inv_type: 99, hash: [7u8; 32] }; + let notfound = |len: usize| { + let mut inv: Vec = hashes.iter().map(|h| Inventory::WitnessBlock(*h)).collect(); + inv.resize(len, filler); + NetworkMessage::NotFound(inv) + }; + for (len, released) in [(MAX_NOTFOUND_SZ, true), (MAX_NOTFOUND_SZ + 1, false)] { + let _rx = add_peer(&pm, 1, addr(1), 8); + let mut sched = IbdScheduler::new(4, 0, &pm.chain_state, 1024); + let assigned = sched.assign_blocks(1); + assert_eq!(assigned.len(), 4, "every height in flight to peer 1"); + *pm.ibd.write() = Some(sched); + + deliver(&pm, 1, notfound(len)); + let ibd = pm.ibd.read(); + let still = (1..=4).filter(|h| ibd.as_ref().unwrap().in_flight_contains(*h)).count(); + assert_eq!(still == 0, released, "notfound of {len} entries: {still} heights still in flight"); + drop(ibd); + assert!(connected(&pm, 1), "notfound of {len} entries is not misbehaviour"); + *pm.ibd.write() = None; + } +} + +// ---- getdata against the send buffer ---- + +/// Core stops serving a `getdata` once the bytes queued to the peer pass +/// `-maxsendbuffer` (`ProcessGetData` breaks on `fPauseSend`). satd read, +/// deserialized and queued every block asked for, so one request held a +/// peer's whole queue of blocks in memory. +#[test] +fn getdata_stops_serving_past_the_send_buffer() { + let (pm, _dir) = mk_pm(); + let hashes = mine_big(&pm, 30, 100_000); + let mut rx = add_peer(&pm, 1, addr(1), 256); + + pm.handle_getdata(1, hashes.iter().map(|h| Inventory::WitnessBlock(*h)).collect()); + let queued = drain(&mut rx); + let bytes: usize = queued.iter().map(queued_size).sum(); + let largest = queued.iter().map(queued_size).max().unwrap_or(0); + assert!( + bytes <= MAX_SEND_BUFFER_BYTES + largest, + "{} blocks, {bytes} bytes queued at once; the bound is {MAX_SEND_BUFFER_BYTES} plus one block", + queued.len() + ); + assert!(!queued.is_empty(), "the first blocks are served at once"); +} + +/// What the bound holds back is not lost: as the peer's queue drains, the +/// rest of the request is served, in the order asked, and the peer's next +/// message is read only once all of it has been. Each block is charged to +/// `-maxuploadtarget` once, as it is queued. +#[test] +fn getdata_resumes_as_the_queue_drains() { + let (pm, _dir) = mk_pm(); + let hashes = mine_big(&pm, 30, 100_000); + let mut rx = add_peer(&pm, 1, addr(1), 256); + let queue = pm.peer_sender(1).unwrap().queue().clone(); + // A budget large enough never to refuse, so every block is counted. + pm.set_max_upload_target(u64::MAX / 2); + let charged_before = pm.upload_bytes.load(Ordering::Relaxed); + + // As the write loop hands the request over. + queue.note_getdata_forwarded(); + deliver(&pm, 1, NetworkMessage::GetData(hashes.iter().map(|h| Inventory::WitnessBlock(*h)).collect())); + assert!(queue.getdata_backlog() > 0, "the bound held part of the request back"); + assert!(queue.reading_paused(), "nothing more is read from the peer meanwhile"); + + // The write loop, writing and asking for more. + let mut served = Vec::new(); + let mut resumes = 0; + while let Ok(msg) = rx.try_recv() { + queue.sent(queued_size(&msg)); + match msg { + NetworkMessage::Block(b) => served.push(b.block_hash()), + other => panic!("unexpected {other:?}"), + } + if queue.take_resume() { + resumes += 1; + pm.resume_getdata(1); + } + } + assert_eq!(served, hashes, "every block, in the order asked"); + assert!(resumes > 0); + assert_eq!(queue.getdata_backlog(), 0); + assert!(!queue.reading_paused(), "the peer is read again"); + assert_eq!(queue.queued_bytes(), 0); + let charged: usize = served.iter().map(|h| pm.chain_state.get_block(h).unwrap().total_size()).sum(); + assert_eq!(pm.upload_bytes.load(Ordering::Relaxed) - charged_before, charged as u64); +} + +// ---- Peer text ---- + +/// Core keeps a peer's user agent only after `SanitizeString` (`cleanSubVer`) +/// and reports that in `getpeerinfo.subver`; satd logged and reported the raw +/// string, line breaks and escape sequences included. +#[test] +fn peer_user_agent_is_sanitized() { + let mut info = PeerInfo::new(1, addr(1), Direction::Inbound); + let zero: SocketAddr = "0.0.0.0:0".parse().unwrap(); + info.set_version(VersionMessage { + version: 70016, + services: ServiceFlags::NETWORK, + timestamp: 0, + receiver: Address::new(&zero, ServiceFlags::NONE), + sender: Address::new(&zero, ServiceFlags::NONE), + nonce: 1, + user_agent: "/Satoshi:27.0.0/\n2026-01-01T00:00:00Z INFO fake\x1b[0m".into(), + start_height: 0, + relay: true, + }); + assert_eq!(info.user_agent, "/Satoshi:27.0.0/2026-01-01T00:00:00Z INFO fake0m"); +} + +/// Core reads the user agent as `LIMITED_STRING(strSubVer, +/// MAX_SUBVERSION_LENGTH)`. A longer one throws in deserialization, so the +/// `version` is dropped and the peer is left without a handshake until the +/// connect timeout; a later, well-formed `version` is accepted. satd took the +/// oversized one. +#[tokio::test] +async fn version_with_an_oversized_user_agent_is_ignored() { + use crate::net::connection::Connection; + use bitcoin::p2p::Magic; + + fn version(user_agent: String) -> NetworkMessage { + let services = ServiceFlags::NETWORK | ServiceFlags::WITNESS; + let zero: SocketAddr = "0.0.0.0:0".parse().unwrap(); + NetworkMessage::Version(VersionMessage { + version: 70016, + services, + timestamp: crate::time::now_secs() as i64, + receiver: Address::new(&zero, ServiceFlags::NONE), + sender: Address::new(&zero, services), + nonce: 0x5eed, + user_agent, + start_height: 0, + relay: true, + }) + } + + let (pm, _dir) = mk_pm(); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let dialer = tokio::net::TcpStream::connect(listener.local_addr().unwrap()); + let (dialed, accepted) = tokio::join!(dialer, listener.accept()); + let (dialed, (accepted, peer_addr)) = (dialed.unwrap(), accepted.unwrap()); + let mut info = PeerInfo::new(1, peer_addr, Direction::Inbound); + info.state = PeerState::Connecting; + let (tx, _rx) = mpsc::channel::(1); + pm.peers.write().insert( + 1, + PeerHandle { + info, + msg_tx: tx.into(), + disconnect: Arc::new(tokio::sync::Notify::new()), + flow: Arc::new(crate::net::flow::PeerFlow::new()), + last_getheaders_sent: None, + last_mempool_served: None, + fee_filter_sent: None, + stats: PeerStats::new(NetTotals::new()), + }, + ); + let mut ours = Connection::with_magic(accepted, Magic::REGTEST); + let mut theirs = Connection::with_magic(dialed, Magic::REGTEST); + + let handshake = pm.perform_handshake(1, &mut ours, Direction::Inbound, crate::time::now_secs()); + let remote = async { + theirs.send(version("a".repeat(crate::MAX_SUBVERSION_LENGTH + 1))).await.unwrap(); + // An inbound node answers a `version` with its own. Nothing comes + // back for the oversized one. + let reply = tokio::time::timeout(Duration::from_millis(500), theirs.recv()).await; + assert!(reply.is_err(), "an oversized user agent must not be answered, got {reply:?}"); + // A user agent of exactly the limit is fine, and completes the handshake. + theirs.send(version("b".repeat(crate::MAX_SUBVERSION_LENGTH))).await.unwrap(); + loop { + if matches!(theirs.recv().await.unwrap(), NetworkMessage::Verack) { + break; + } + } + theirs.send(NetworkMessage::Verack).await.unwrap(); + }; + let (handshake, ()) = tokio::join!(handshake, remote); + let theirs_version = handshake.expect("the well-formed version completes the handshake"); + assert_eq!(theirs_version.user_agent.len(), crate::MAX_SUBVERSION_LENGTH); +} + +// ---- BIP 157 requests (Core's `PrepareBlockFilterRequest`) ---- + +#[cfg(feature = "block-filter-index")] +mod bip157 { + use super::*; + use bitcoin::hashes::Hash; + use bitcoin::p2p::message_filter::{GetCFCheckpt, GetCFHeaders, GetCFilters}; + use node_filter_index::{FilterIndex, IndexError}; + + /// A complete index of empty filters. + struct EmptyFilters; + + impl FilterIndex for EmptyFilters { + fn filter_at(&self, _: u8, _: u32) -> Result, IndexError> { + Ok(Vec::new()) + } + fn header_at(&self, _: u8, _: u32) -> Result<[u8; 32], IndexError> { + Ok([0u8; 32]) + } + fn headers_range(&self, _: u8, _: u32, _: u32) -> Result, IndexError> { + Ok(Vec::new()) + } + fn checkpoints_to(&self, _: u8, _: u32) -> Result, IndexError> { + Ok(Vec::new()) + } + fn is_complete(&self) -> bool { + true + } + } + + fn serving_pm() -> (Arc, tempfile::TempDir) { + let (pm, dir) = mk_pm(); + pm.set_filter_index(Arc::new(EmptyFilters), true); + (pm, dir) + } + + fn cfheaders(start_height: u32, stop_hash: bitcoin::BlockHash) -> NetworkMessage { + NetworkMessage::GetCFHeaders(GetCFHeaders { filter_type: 0, start_height, stop_hash }) + } + + /// The request is answered and the peer kept: the must-succeed half of + /// every refusal below. + #[test] + fn a_valid_request_is_served() { + let (pm, _dir) = serving_pm(); + let chain = mine_big(&pm, 5, 10); + let mut rx = add_peer(&pm, 1, addr(1), 8); + deliver(&pm, 1, cfheaders(1, chain[4])); + assert!(connected(&pm, 1)); + match rx.try_recv() { + Ok(NetworkMessage::CFHeaders(h)) => assert_eq!(h.filter_hashes.len(), 5), + other => panic!("expected cfheaders, got {other:?}"), + } + deliver(&pm, 1, NetworkMessage::GetCFCheckpt(GetCFCheckpt { filter_type: 0, stop_hash: chain[4] })); + assert!(matches!(rx.try_recv(), Ok(NetworkMessage::CFCheckpt(_)))); + assert!(connected(&pm, 1)); + } + + /// Core disconnects (`fDisconnect`, no misbehaviour) a request satd cannot + /// answer as asked: a filter type it does not serve, a stop hash it does + /// not know or would not serve, a start above the stop, or a range over + /// the message's limit. satd ignored every one of them. + #[test] + fn bad_requests_disconnect() { + let (pm, _dir) = serving_pm(); + let chain = mine_big(&pm, 5, 10); + let unknown = bitcoin::BlockHash::from_byte_array([0xab; 32]); + let cases: Vec<(&str, NetworkMessage)> = vec![ + ( + "unsupported filter type", + NetworkMessage::GetCFilters(GetCFilters { filter_type: 0x99, start_height: 0, stop_hash: chain[4] }), + ), + ("unknown stop hash", cfheaders(0, unknown)), + ("start above stop", cfheaders(5, chain[3])), + ( + "unsupported filter type, getcfcheckpt", + NetworkMessage::GetCFCheckpt(GetCFCheckpt { filter_type: 1, stop_hash: chain[4] }), + ), + ("unknown stop hash, getcfcheckpt", NetworkMessage::GetCFCheckpt(GetCFCheckpt { filter_type: 0, stop_hash: unknown })), + ]; + for (i, (case, msg)) in cases.into_iter().enumerate() { + let id = 10 + i as PeerId; + let mut rx = add_peer(&pm, id, addr(10 + i as u8), 8); + deliver(&pm, id, msg); + assert!(!connected(&pm, id), "{case}: the peer is disconnected"); + assert!(!pm.is_addr_banned(&addr(10 + i as u8)), "{case}: disconnected, not banned"); + assert!(rx.try_recv().is_err(), "{case}: nothing is served"); + } + } + + /// A node that does not serve filters disconnects a peer that asks for + /// them: Core's "peer requested unsupported block filter type" when + /// `NODE_COMPACT_FILTERS` is not among the services it offered. + #[test] + fn a_request_to_a_node_not_serving_filters_disconnects() { + let (pm, _dir) = mk_pm(); + let chain = mine_big(&pm, 2, 10); + let _rx = add_peer(&pm, 1, addr(1), 8); + deliver(&pm, 1, cfheaders(0, chain[1])); + assert!(!connected(&pm, 1)); + } + + /// The range limits: 1000 heights for `getcfilters`, 2000 for + /// `getcfheaders` (Core's `MAX_GETCFILTERS_SIZE`, `MAX_GETCFHEADERS_SIZE`). + #[tokio::test] + async fn a_range_past_the_limit_disconnects() { + let (pm, _dir) = serving_pm(); + let chain = mine_big(&pm, 1001, 1); + let _rx = add_peer(&pm, 1, addr(1), 2048); + deliver(&pm, 1, NetworkMessage::GetCFilters(GetCFilters { filter_type: 0, start_height: 2, stop_hash: chain[1000] })); + assert!(connected(&pm, 1), "1000 filters are allowed"); + let _rx = add_peer(&pm, 2, addr(2), 8); + deliver(&pm, 2, NetworkMessage::GetCFilters(GetCFilters { filter_type: 0, start_height: 1, stop_hash: chain[1000] })); + assert!(!connected(&pm, 2), "1001 filters disconnect"); + } + + /// A stop hash off the active chain: Core serves one it would relay (a + /// block it validated, no more than a month older than the best header, + /// `BlockRequestAllowed`) and disconnects for anything else. satd keeps + /// filters for the active chain only, so it cannot serve the first kind; + /// it ignores the request and keeps the peer. A header it has not + /// validated is the second kind. + #[test] + fn an_off_chain_stop_hash() { + let (pm, _dir) = serving_pm(); + let chain = mine_big(&pm, 3, 10); + // A heavier branch from height 2, built on a second node and handed + // over, so the block at height 3 is connected and then reorged out: + // validated, and stale. + let (other, _other_dir) = mk_pm(); + for h in &chain[..2] { + other.chain_state.accept_block(&pm.chain_state.get_block(h).unwrap()).unwrap(); + } + let branch = mine_big(&other, 2, 12); + for h in &branch { + pm.chain_state.accept_block(&other.chain_state.get_block(h).unwrap()).unwrap(); + } + assert_eq!(pm.chain_state.tip_hash(), branch[1], "the heavier branch is active"); + let stale = pm.chain_state.get_block_index(&chain[2]).unwrap(); + assert_eq!(stale.status, crate::storage::blockindex::BlockStatus::Valid); + + let mut rx = add_peer(&pm, 1, addr(1), 8); + deliver(&pm, 1, cfheaders(1, chain[2])); + assert!(connected(&pm, 1), "a recent, validated stale block is not grounds to disconnect"); + assert!(rx.try_recv().is_err(), "but satd has no filter for it"); + + // A header with no block behind it. + let tip = pm.chain_state.tip_hash(); + let mut header = pm.chain_state.get_block_index(&tip).unwrap().header; + header.prev_blockhash = tip; + header.time += 1; + while header.validate_pow(header.target()).is_err() { + header.nonce += 1; + } + pm.chain_state.accept_headers(&[header]).1.map_or(Ok(()), Err).expect("header accepted"); + let _rx = add_peer(&pm, 2, addr(2), 8); + deliver(&pm, 2, cfheaders(0, header.block_hash())); + assert!(!connected(&pm, 2), "a header satd never validated is not a block it would serve"); + } +} + +// ---- The write loop ---- + +/// A connected socket pair whose far end the test controls, and the near +/// end's halves with counters attached. +async fn socket_pair() -> ( + crate::net::connection::ConnectionReader, + ConnectionWriter, + tokio::net::TcpStream, + Arc, +) { + use crate::net::connection::Connection; + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let (dialed, accepted) = + tokio::join!(tokio::net::TcpStream::connect(listener.local_addr().unwrap()), listener.accept()); + let (far, _) = accepted.unwrap(); + let conn = Connection::with_magic(dialed.unwrap(), bitcoin::p2p::Magic::REGTEST); + let (reader, mut writer) = conn.split(); + let stats = PeerStats::new(NetTotals::new()); + writer.set_counters(stats.clone()); + (reader, writer, far, stats) +} + +fn big(len: usize) -> NetworkMessage { + NetworkMessage::Unknown { + command: bitcoin::p2p::message::CommandString::try_from_static("bigmsg").unwrap(), + payload: vec![0x5a; len], + } +} + +/// Everything a write loop needs, for a peer whose far end the test holds. +struct Loop { + id: PeerId, + event_rx: mpsc::Receiver, + msg_tx: crate::net::send_queue::PeerSender, + read_tx: mpsc::Sender, + disconnect: Arc, + stats: Arc, + far: tokio::net::TcpStream, + task: tokio::task::JoinHandle>, +} + +async fn run_loop() -> Loop { + let (_reader, mut writer, far, stats) = socket_pair().await; + let (event_tx, event_rx) = mpsc::channel(64); + let (msg_tx, mut msg_rx) = mpsc::channel::(256); + let msg_tx = crate::net::send_queue::PeerSender::from(msg_tx); + let (read_tx, mut read_rx) = mpsc::channel::(64); + let disconnect = Arc::new(tokio::sync::Notify::new()); + let queue = msg_tx.queue().clone(); + let (s, d) = (stats.clone(), disconnect.clone()); + let task = tokio::spawn(async move { + let _keep = _reader; + PeerManager::peer_write_loop( + 7, + &event_tx, + &mut writer, + &mut msg_rx, + &mut read_rx, + Some(s), + Some(Arc::new(crate::net::flow::PeerFlow::new())), + None, + Some(d), + Some(queue), + ) + .await + }); + Loop { id: 7, event_rx, msg_tx, read_tx, disconnect, stats, far, task } +} + +/// Queue big messages until the socket stops taking bytes: the far end is +/// not reading and the write loop is parked in a write. +async fn park(l: &Loop) { + // More than loopback socket buffers hold. + for _ in 0..8 { + l.msg_tx.try_send(big(3_900_000)).unwrap(); + } + let mut last = 0; + loop { + tokio::time::sleep(Duration::from_millis(200)).await; + let now = l.stats.bytes_sent(); + if now == last && now > 0 { + return; + } + last = now; + } +} + +/// `disconnectnode`, a ban and the ping timeout all reach the write loop +/// through the disconnect signal, which it only looked at between writes. A +/// peer that stopped reading kept the loop parked in a write, so the socket +/// and everything queued to it outlived the disconnect. +#[tokio::test] +async fn a_parked_write_ends_on_disconnect() { + let l = run_loop().await; + park(&l).await; + l.disconnect.notify_one(); + let ended = tokio::time::timeout(Duration::from_secs(5), l.task) + .await + .expect("the write loop ends while its write is parked") + .unwrap(); + assert_eq!(ended, Err("disconnected by manager".to_string())); + drop(l.far); +} + +/// Core drops a peer whose socket has taken nothing for `TIMEOUT_INTERVAL` +/// (20 minutes; `InactivityCheck`, "socket sending timeout"). A slow reader +/// is not a stalled one: the clock restarts whenever the socket takes bytes. +#[tokio::test(start_paused = true)] +async fn a_write_that_makes_no_progress_times_out() { + use crate::net::send_queue::SEND_TIMEOUT; + let l = run_loop().await; + park(&l).await; + let parked_at = tokio::time::Instant::now(); + let ended = tokio::time::timeout(SEND_TIMEOUT * 2, l.task) + .await + .expect("a write with no progress ends") + .unwrap(); + let waited = parked_at.elapsed(); + let err = ended.expect_err("the loop ends with an error"); + assert!(err.contains("sending timeout"), "{err}"); + assert!( + waited >= SEND_TIMEOUT - Duration::from_secs(60) && waited <= SEND_TIMEOUT + Duration::from_secs(60), + "dropped after {waited:?}" + ); + drop(l.far); +} + +/// Core takes no further message from a peer while a `getdata` of its is +/// being served (`ProcessMessages`). The loop hands the manager the +/// `getdata` and then reads nothing until the manager has taken it in and +/// served all of it. +#[tokio::test] +async fn reading_waits_for_a_getdata_to_be_served() { + let mut l = run_loop().await; + let queue = l.msg_tx.queue().clone(); + l.read_tx.send(NetworkMessage::GetData(vec![])).await.unwrap(); + l.read_tx.send(NetworkMessage::SendHeaders).await.unwrap(); + let first = tokio::time::timeout(Duration::from_secs(5), l.event_rx.recv()).await.unwrap(); + assert!(matches!(first, Some(NetEvent::MessageReceived { msg: NetworkMessage::GetData(_), .. }))); + let early = tokio::time::timeout(Duration::from_millis(300), l.event_rx.recv()).await; + assert!(early.is_err(), "nothing after the getdata is read before it is served"); + + // Taken in with one entry still to serve: still waiting. + queue.push_getdata(vec![Inventory::Unknown { inv_type: 99, hash: [1; 32] }]); + queue.note_getdata_handled(); + let early = tokio::time::timeout(Duration::from_millis(300), l.event_rx.recv()).await; + assert!(early.is_err(), "an unserved entry still holds the peer's next message"); + + queue.pop_getdata(); + queue.wake_reader(); + let next = tokio::time::timeout(Duration::from_secs(5), l.event_rx.recv()).await.unwrap(); + assert!(matches!(next, Some(NetEvent::MessageReceived { msg: NetworkMessage::SendHeaders, .. }))); + l.disconnect.notify_one(); + let _ = l.task.await; +} + +/// The loop asks the manager to serve more of a `getdata` backlog once the +/// queue is back under the send buffer, and holds the peer's messages while +/// it is over. +#[tokio::test] +async fn the_loop_asks_for_more_once_the_queue_drains() { + use tokio::io::AsyncReadExt; + let mut l = run_loop().await; + let queue = l.msg_tx.queue().clone(); + queue.push_getdata(vec![Inventory::Unknown { inv_type: 99, hash: [1; 32] }]); + // Over the buffer: two messages of 600 kB. + l.msg_tx.try_send(big(600_000)).unwrap(); + l.msg_tx.try_send(big(600_000)).unwrap(); + // Let the far end read, so the queue drains. + let mut far = l.far; + let reader = tokio::spawn(async move { + let mut sink = vec![0u8; 1 << 16]; + while far.read(&mut sink).await.is_ok_and(|n| n > 0) {} + }); + let ev = tokio::time::timeout(Duration::from_secs(10), async { + loop { + match l.event_rx.recv().await { + Some(NetEvent::GetDataResume { id }) => return id, + Some(_) => continue, + None => panic!("the loop ended"), + } + } + }) + .await + .expect("a resume is asked for"); + assert_eq!(ev, l.id); + assert_eq!(queue.queued_bytes(), 0); + l.disconnect.notify_one(); + let _ = l.task.await; + reader.abort(); +} diff --git a/node/src/net/mod.rs b/node/src/net/mod.rs index 5069ddd6e..2e3c41835 100644 --- a/node/src/net/mod.rs +++ b/node/src/net/mod.rs @@ -7,10 +7,12 @@ pub mod connection; pub mod dns; pub mod flow; pub mod ibd; +pub mod limits; pub mod manager; pub mod peer; pub mod permissions; pub mod proxy; +pub mod send_queue; pub mod stats; pub mod tor; pub mod sync; diff --git a/node/src/net/peer.rs b/node/src/net/peer.rs index 1f558d584..9cae1f83e 100644 --- a/node/src/net/peer.rs +++ b/node/src/net/peer.rs @@ -338,9 +338,13 @@ impl PeerInfo { } /// Update peer info after receiving their version message. - pub fn set_version(&mut self, version: VersionMessage) { + pub fn set_version(&mut self, mut version: VersionMessage) { self.services = version.services; self.best_height = version.start_height; + // Core keeps the user agent only as `SanitizeString(strSubVer)` + // (`cleanSubVer`): it is logged and reported in `getpeerinfo.subver`, + // and the raw string can carry line breaks and escape sequences. + version.user_agent = crate::net::limits::sanitize_string(&version.user_agent); self.user_agent = version.user_agent.clone(); // The peer's own `fRelay` is kept on the stored `version` message; // `relays_txs()` is the answer to "do *we* relay to them", which diff --git a/node/src/net/send_queue.rs b/node/src/net/send_queue.rs new file mode 100644 index 000000000..97a6f3291 --- /dev/null +++ b/node/src/net/send_queue.rs @@ -0,0 +1,227 @@ +//! Per-peer outbound queue accounting, behind Bitcoin Core's send-buffer +//! limit. +//! +//! Core stops working for a peer once the bytes queued to it pass +//! `-maxsendbuffer` (1,000,000 by default). `fPauseSend` is set +//! (`net.cpp` `SocketSendData` and `PushMessage`), `ProcessGetData` stops +//! serving (`net_processing.cpp` `ProcessGetData`), and `ProcessMessages` +//! takes no further message from that peer until the buffer drains. The +//! unserved rest of a `getdata` stays in `m_getdata_requests` and is served +//! as the buffer empties; until it has all been served, nothing else the peer +//! sent is processed either ("this maintains the order of responses and +//! prevents m_getdata_requests to grow unbounded"). +//! +//! satd's queue to a peer is a bounded channel that the peer's write loop +//! drains. [`PeerSender`] is the sending end: it counts the bytes each +//! message adds. [`SendQueue`] is the state the manager and the write loop +//! share: those bytes, the unserved `getdata` entries, and whether the write +//! loop may hand the manager the peer's next message. + +use std::collections::VecDeque; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::time::Duration; + +use bitcoin::consensus::Encodable; +use bitcoin::p2p::message::NetworkMessage; +use bitcoin::p2p::message_blockdata::Inventory; +use tokio::sync::{Notify, mpsc}; + +/// Core's default `-maxsendbuffer`, `DEFAULT_MAXSENDBUFFER` (1000 kB, +/// `net.h`), in bytes. Past it a peer is not served and not read. +pub const MAX_SEND_BUFFER_BYTES: usize = 1_000_000; + +/// Core's `TIMEOUT_INTERVAL` (`net.h`). A peer whose socket has taken no +/// bytes for this long while a message waits to go out is dropped (Core's +/// `InactivityCheck`, "socket sending timeout"). +pub const SEND_TIMEOUT: Duration = Duration::from_secs(20 * 60); + +/// Size of a v1 message header: magic, command, length and checksum. +const MESSAGE_HEADER_SIZE: usize = 24; + +/// What a message counts for while it waits in a peer's queue: its v1 wire +/// size, header and payload. +/// +/// The sender adds it and the write loop takes it off again, both from the +/// message itself, so the two always agree. A v2 packet is a few bytes +/// longer or shorter on the wire; the budget does not need to know. +pub fn queued_size(msg: &NetworkMessage) -> usize { + // Encoding into a sink cannot fail. + MESSAGE_HEADER_SIZE + msg.consensus_encode(&mut bitcoin::io::sink()).unwrap_or(0) +} + +/// State shared by the manager and one peer's write loop. +#[derive(Debug, Default)] +pub struct SendQueue { + /// Bytes queued to the peer and not yet written to its socket. + queued_bytes: AtomicUsize, + /// `getdata` entries not yet served, oldest first: Core's + /// `m_getdata_requests`. + getdata: parking_lot::Mutex>, + /// `getdata.len()`, readable without the lock. + getdata_len: AtomicUsize, + /// `getdata` messages the write loop has handed the manager that the + /// manager has not yet taken in. + getdata_unhandled: AtomicUsize, + /// A [`crate::net::manager::NetEvent::GetDataResume`] for this peer is + /// waiting in the manager's queue. + resume_queued: AtomicBool, + /// Wakes the write loop when the peer's messages may be read again. + reader_wake: Notify, +} + +impl SendQueue { + pub fn new() -> Self { + Self::default() + } + + /// Bytes queued and not yet written. + pub fn queued_bytes(&self) -> usize { + self.queued_bytes.load(Ordering::Acquire) + } + + /// Core's `fPauseSend`: more than [`MAX_SEND_BUFFER_BYTES`] is queued. + pub fn over_limit(&self) -> bool { + self.queued_bytes() > MAX_SEND_BUFFER_BYTES + } + + fn add(&self, n: usize) { + self.queued_bytes.fetch_add(n, Ordering::AcqRel); + } + + /// The write loop has written (or given up on) a message counted at `n`. + pub fn sent(&self, n: usize) { + let _ = self + .queued_bytes + .fetch_update(Ordering::AcqRel, Ordering::Acquire, |v| Some(v.saturating_sub(n))); + } + + /// Append a `getdata` request's entries to the unserved ones. + pub fn push_getdata(&self, entries: Vec) { + let mut q = self.getdata.lock(); + q.extend(entries); + self.getdata_len.store(q.len(), Ordering::Release); + } + + /// The oldest unserved `getdata` entry, left in place. It stays counted + /// as unserved while the manager works on it, so the write loop never + /// sees an empty backlog and resumes reading before the entry is done. + pub fn front_getdata(&self) -> Option { + self.getdata.lock().front().copied() + } + + /// Drop the oldest unserved `getdata` entry: it has been answered. + pub fn pop_getdata(&self) { + let mut q = self.getdata.lock(); + q.pop_front(); + self.getdata_len.store(q.len(), Ordering::Release); + } + + /// How many `getdata` entries are waiting to be served. + pub fn getdata_backlog(&self) -> usize { + self.getdata_len.load(Ordering::Acquire) + } + + /// The write loop has handed the manager a `getdata`. Until the manager + /// has taken it in and served it, the loop reads nothing more. + pub fn note_getdata_forwarded(&self) { + self.getdata_unhandled.fetch_add(1, Ordering::AcqRel); + } + + /// The manager has taken in a `getdata` (whatever it made of it). + pub fn note_getdata_handled(&self) { + let _ = self + .getdata_unhandled + .fetch_update(Ordering::AcqRel, Ordering::Acquire, |v| Some(v.saturating_sub(1))); + self.wake_reader(); + } + + /// Whether the write loop must leave the peer's next message unread: + /// a `getdata` is still being served, or the queue is over + /// [`MAX_SEND_BUFFER_BYTES`] (Core's `ProcessMessages`). + pub fn reading_paused(&self) -> bool { + self.getdata_unhandled.load(Ordering::Acquire) > 0 + || self.getdata_backlog() > 0 + || self.over_limit() + } + + /// Called by the write loop after a write. True when the manager should + /// be asked to serve more of the backlog: there is some, the queue has + /// room, and no request to serve is already waiting. + pub fn take_resume(&self) -> bool { + self.getdata_backlog() > 0 && !self.over_limit() && !self.resume_queued.swap(true, Ordering::AcqRel) + } + + /// Called by the manager as it starts on a resume request, before it + /// looks at the queue, so a write that drains the queue after that look + /// asks again rather than being lost. + pub fn resume_taken(&self) { + self.resume_queued.store(false, Ordering::Release); + } + + /// Tell the write loop the peer's messages may be readable again. + pub fn wake_reader(&self) { + // `notify_one` stores a permit when the loop is not waiting, so a + // wake sent between its check and its wait is not lost. + self.reader_wake.notify_one(); + } + + /// Resolves after [`Self::wake_reader`]. + pub async fn reader_woken(&self) { + self.reader_wake.notified().await; + } +} + +/// The sending end of a peer's queue. Counts what it queues in the shared +/// [`SendQueue`]. +/// +/// Holds the channel's sender: the write loop learns that the manager has +/// dropped the peer when the last one goes. The write loop itself holds only +/// the [`SendQueue`]. +#[derive(Debug, Clone)] +pub struct PeerSender { + tx: mpsc::Sender, + queue: Arc, +} + +impl From> for PeerSender { + fn from(tx: mpsc::Sender) -> Self { + Self { tx, queue: Arc::new(SendQueue::new()) } + } +} + +impl PeerSender { + /// The state this sender shares with the peer's write loop. + pub fn queue(&self) -> &Arc { + &self.queue + } + + /// Queue a message if there is room, counting its bytes. The signature + /// is the tokio sender's, which this replaces at every call site. + #[allow(clippy::result_large_err)] + pub fn try_send(&self, msg: NetworkMessage) -> Result<(), mpsc::error::TrySendError> { + let n = queued_size(&msg); + // Counted before the message is visible to the write loop, so the + // loop can never take it off before it was put on. + self.queue.add(n); + self.tx.try_send(msg).inspect_err(|_| self.queue.sent(n)) + } + + /// Queue a message, waiting for room, counting its bytes. + #[allow(clippy::result_large_err)] + pub async fn send(&self, msg: NetworkMessage) -> Result<(), mpsc::error::SendError> { + let n = queued_size(&msg); + self.queue.add(n); + self.tx.send(msg).await.inspect_err(|_| self.queue.sent(n)) + } + + /// Whether `getdata` serving must stop for now: the queue is over + /// [`MAX_SEND_BUFFER_BYTES`], or the channel has no free slot. + pub fn paused(&self) -> bool { + self.queue.over_limit() || self.tx.capacity() == 0 + } +} + +#[cfg(test)] +#[path = "send_queue_tests.rs"] +mod tests; diff --git a/node/src/net/send_queue_tests.rs b/node/src/net/send_queue_tests.rs new file mode 100644 index 000000000..fbbc043d0 --- /dev/null +++ b/node/src/net/send_queue_tests.rs @@ -0,0 +1,110 @@ +use super::*; + +fn block_of(size: usize) -> NetworkMessage { + NetworkMessage::Unknown { + command: bitcoin::p2p::message::CommandString::try_from_static("bigmsg").unwrap(), + payload: vec![0; size], + } +} + +/// The count is the message's v1 wire size, exactly. +#[test] +fn queued_size_is_the_v1_wire_size() { + for msg in [NetworkMessage::Verack, NetworkMessage::Ping(9), block_of(123_456)] { + let wire = bitcoin::consensus::serialize(&bitcoin::p2p::message::RawNetworkMessage::new( + bitcoin::p2p::Magic::REGTEST, + msg.clone(), + )); + assert_eq!(queued_size(&msg), wire.len(), "{}", msg.cmd()); + } +} + +/// What goes in is counted, what the write loop takes off is subtracted, +/// and a send that does not happen leaves no trace. +#[test] +fn bytes_are_counted_in_and_out() { + let (tx, mut rx) = mpsc::channel(2); + let sender = PeerSender::from(tx); + let q = sender.queue().clone(); + let a = block_of(600_000); + let b = block_of(500_000); + sender.try_send(a.clone()).unwrap(); + assert_eq!(q.queued_bytes(), queued_size(&a)); + assert!(!q.over_limit(), "600 kB is within the buffer"); + sender.try_send(b.clone()).unwrap(); + assert!(q.over_limit(), "1.1 MB is past it"); + assert!(sender.paused()); + + // The channel is full: the refused message is not counted. + let before = q.queued_bytes(); + assert!(sender.try_send(NetworkMessage::Verack).is_err()); + assert_eq!(q.queued_bytes(), before); + + let first = rx.try_recv().unwrap(); + q.sent(queued_size(&first)); + assert_eq!(q.queued_bytes(), queued_size(&b)); + assert!(!q.over_limit()); + assert!(!sender.paused(), "a free slot and room in the buffer"); +} + +/// A full channel pauses serving even when the bytes are few. +#[test] +fn a_full_channel_pauses() { + let (tx, _rx) = mpsc::channel(1); + let sender = PeerSender::from(tx); + assert!(!sender.paused()); + sender.try_send(NetworkMessage::Verack).unwrap(); + assert!(!sender.queue().over_limit()); + assert!(sender.paused()); +} + +/// Reading stops from the moment a `getdata` is handed over until the +/// manager has taken it in and served all of it, and while the buffer is +/// over the limit. +#[test] +fn reading_pauses_for_an_unserved_getdata() { + let q = SendQueue::new(); + assert!(!q.reading_paused()); + q.note_getdata_forwarded(); + assert!(q.reading_paused(), "forwarded, not yet taken in"); + q.push_getdata(vec![Inventory::Block(bitcoin::BlockHash::from_byte_array([1; 32]))]); + q.note_getdata_handled(); + assert!(q.reading_paused(), "taken in, one entry unserved"); + assert_eq!(q.getdata_backlog(), 1); + assert!(q.front_getdata().is_some()); + assert!(q.reading_paused(), "still unserved while it is being worked on"); + q.pop_getdata(); + assert!(!q.reading_paused()); + + q.add(MAX_SEND_BUFFER_BYTES + 1); + assert!(q.reading_paused(), "over the buffer"); + q.sent(2); + assert!(!q.reading_paused()); +} + +/// One resume request at a time, and only when there is a backlog and room. +#[test] +fn a_resume_is_requested_once_until_taken() { + let q = SendQueue::new(); + assert!(!q.take_resume(), "nothing to resume"); + q.push_getdata(vec![Inventory::Block(bitcoin::BlockHash::from_byte_array([1; 32]))]); + q.add(MAX_SEND_BUFFER_BYTES + 1); + assert!(!q.take_resume(), "no room yet"); + q.sent(MAX_SEND_BUFFER_BYTES + 1); + assert!(q.take_resume()); + assert!(!q.take_resume(), "one is already waiting"); + q.resume_taken(); + assert!(q.take_resume()); +} + +/// A wake sent while nobody waits is kept for the next wait. +#[tokio::test] +async fn a_wake_is_not_lost() { + let q = SendQueue::new(); + q.wake_reader(); + tokio::time::timeout(Duration::from_secs(1), q.reader_woken()) + .await + .expect("the stored wake resolves the next wait"); +} + +use bitcoin::hashes::Hash as _; diff --git a/node/src/net/v2transport.rs b/node/src/net/v2transport.rs index cfb704af4..f635a970b 100644 --- a/node/src/net/v2transport.rs +++ b/node/src/net/v2transport.rs @@ -50,6 +50,9 @@ const MIN_PACKET_LEN: usize = 1 + 16; /// `MAX_PROTOCOL_MESSAGE_LENGTH` (4 MB) payload; `decrypt_packet_len` also /// counts the header byte and the 16-byte tag. const MAX_V2_PACKET_LEN: usize = 1 + 12 + 4_000_000 + 1 + 16; +/// How far a packet buffer is grown ahead of the bytes received: Core's +/// `MAX_RESERVE_AHEAD` (`V2Transport::ReceivedBytes`). +const MAX_RESERVE_AHEAD: usize = 256 * 1024; /// Errors from the v2 message codec. #[derive(Debug, thiserror::Error)] @@ -154,14 +157,19 @@ async fn read_exact_buffered( leftover: &mut Vec, n: usize, ) -> io::Result> { - let mut out = vec![0u8; n]; let from_leftover = leftover.len().min(n); - if from_leftover > 0 { - out[..from_leftover].copy_from_slice(&leftover[..from_leftover]); - leftover.drain(..from_leftover); - } - if from_leftover < n { - stream.read_exact(&mut out[from_leftover..]).await?; + let mut out = Vec::with_capacity(n.min(from_leftover + MAX_RESERVE_AHEAD)); + out.extend_from_slice(&leftover[..from_leftover]); + leftover.drain(..from_leftover); + // Grown as the bytes arrive and never more than MAX_RESERVE_AHEAD past + // them, as Core's `V2Transport` does: sizing it from the length alone let + // a peer that announced a large packet and sent nothing hold that much + // memory per connection. + while out.len() < n { + let start = out.len(); + let end = n.min(start + MAX_RESERVE_AHEAD); + out.resize(end, 0); + stream.read_exact(&mut out[start..end]).await?; } Ok(out) } @@ -230,15 +238,18 @@ async fn recv_v2( } /// Encode, encrypt, and write a single `NetworkMessage` as a genuine v2 -/// packet. Returns the number of on-wire (ciphertext-framed) bytes written. +/// packet, counting the bytes as the socket takes them (see +/// [`crate::net::connection::write_counted`]). Returns the number of on-wire +/// (ciphertext-framed) bytes written. async fn send_v2( stream: &mut S, cipher: &mut OutboundCipher, msg: NetworkMessage, + counters: Option<&Arc>, ) -> io::Result { let contents = encode_message(msg); let packet = cipher.encrypt_to_vec(&contents, PacketType::Genuine, None); - stream.write_all(&packet).await?; + crate::net::connection::write_counted(stream, &packet, counters).await?; Ok(packet.len()) } @@ -457,9 +468,8 @@ impl V2Connection { /// (handshake) path, counted once counters are attached. pub async fn send(&mut self, msg: NetworkMessage) -> io::Result<()> { let cmd = msg.cmd(); - let n = send_v2(&mut self.stream, self.cipher.outbound(), msg).await?; + let n = send_v2(&mut self.stream, self.cipher.outbound(), msg, self.counters.as_ref()).await?; if let Some(c) = &self.counters { - c.record_sent(n); c.attribute_sent(cmd, n); } Ok(()) @@ -493,9 +503,8 @@ impl V2Writer { /// (ciphertext-framed) bytes if counters are set. pub async fn send(&mut self, msg: NetworkMessage) -> io::Result<()> { let cmd = msg.cmd(); - let n = send_v2(&mut self.stream, &mut self.cipher, msg).await?; + let n = send_v2(&mut self.stream, &mut self.cipher, msg, self.counters.as_ref()).await?; if let Some(c) = &self.counters { - c.record_sent(n); c.attribute_sent(cmd, n); } Ok(()) @@ -700,3 +709,7 @@ mod tests { let _ = server.await; } } + +#[cfg(test)] +#[path = "v2transport_bounds_tests.rs"] +mod bounds_tests; diff --git a/node/src/net/v2transport_bounds_tests.rs b/node/src/net/v2transport_bounds_tests.rs new file mode 100644 index 000000000..30ffe3697 --- /dev/null +++ b/node/src/net/v2transport_bounds_tests.rs @@ -0,0 +1,82 @@ +//! The v2 receive buffer's growth and send progress, against Bitcoin Core's +//! `V2Transport::ReceivedBytes` and `SocketSendData`. + +use super::*; +use std::time::Duration; +use tokio::net::TcpListener; + +/// A reader that records the largest read it was asked for. +struct MaxAsk { + inner: R, + max: usize, +} + +impl AsyncRead for MaxAsk { + fn poll_read( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &mut ReadBuf<'_>, + ) -> Poll> { + self.max = self.max.max(buf.remaining()); + Pin::new(&mut self.inner).poll_read(cx, buf) + } +} + +/// Core grows a v2 packet buffer as the bytes arrive, never more than +/// `MAX_RESERVE_AHEAD` (256 KiB) past them. satd allocated the whole +/// announced packet before reading any of it. +#[tokio::test] +async fn a_packet_buffer_grows_with_the_bytes_received() { + let mut reader = MaxAsk { inner: std::io::Cursor::new(vec![7u8; 1000]), max: 0 }; + let mut leftover = vec![1u8; 10]; + let err = read_exact_buffered(&mut reader, &mut leftover, 4_000_000) + .await + .expect_err("the packet never arrives"); + assert_eq!(err.kind(), io::ErrorKind::UnexpectedEof); + assert!(reader.max <= MAX_RESERVE_AHEAD, "asked for {} bytes at once", reader.max); + + // And a packet that does arrive is read whole, leftover first. + let mut reader = std::io::Cursor::new((0..600_000u32).map(|i| i as u8).collect::>()); + let mut leftover = vec![0xee; 3]; + let got = read_exact_buffered(&mut reader, &mut leftover, 500_003).await.unwrap(); + assert_eq!(got.len(), 500_003); + assert_eq!(&got[..3], &[0xee; 3]); + assert_eq!(got[3..], (0..500_000u32).map(|i| i as u8).collect::>()[..]); + assert!(leftover.is_empty()); +} + +/// Bytes are counted as the socket takes them, as on v1. +#[tokio::test] +async fn a_send_counts_its_bytes_as_the_socket_takes_them() { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + let (mut sock, _) = listener.accept().await.unwrap(); + let (cipher, leftover) = responder_handshake(&mut sock, Network::Regtest, &[], 1, None).await.unwrap(); + V2Connection::new(sock, cipher, leftover) + }); + let mut client = TcpStream::connect(addr).await.unwrap(); + let (cipher, leftover) = initiator_handshake(&mut client, Network::Regtest, 2, None).await.unwrap(); + let client = V2Connection::new(client, cipher, leftover); + // The far end never reads. + let _server = server.await.unwrap(); + let (_reader, mut writer) = client.split(); + let stats = PeerStats::new(crate::net::stats::NetTotals::new()); + writer.set_counters(stats.clone()); + + let msg = || NetworkMessage::Unknown { + command: bitcoin::p2p::message::CommandString::try_from_static("bigmsg").unwrap(), + payload: vec![0x5a; 3_900_000], + }; + let mut completed = 0u64; + while let Ok(r) = tokio::time::timeout(Duration::from_millis(500), writer.send(msg())).await { + r.unwrap(); + completed = stats.bytes_sent(); + assert!(completed < 512 * 1024 * 1024, "the socket never filled"); + } + assert!( + stats.bytes_sent() > completed, + "a send the socket only partly took is not counted: {} sent, {completed} in whole messages", + stats.bytes_sent() + ); +} diff --git a/satd/tests/p2p_limits.rs b/satd/tests/p2p_limits.rs new file mode 100644 index 000000000..83edd2c67 --- /dev/null +++ b/satd/tests/p2p_limits.rs @@ -0,0 +1,221 @@ +//! Bitcoin Core's limits on what one P2P message may carry, and its send +//! buffer, exercised against a live regtest node over raw connections. + +mod common; + +use bitcoin::consensus::{deserialize, serialize}; +use bitcoin::hashes::Hash as _; +use bitcoin::p2p::message::{CommandString, NetworkMessage, RawNetworkMessage}; +use bitcoin::p2p::message_blockdata::Inventory; +use bitcoin::p2p::message_network::VersionMessage; +use bitcoin::p2p::{Address, Magic, ServiceFlags}; +use bitcoin::BlockHash; +use common::TestNode; +use std::io::{self, Read, Write}; +use std::net::{SocketAddr, TcpStream}; +use std::time::{Duration, Instant}; + +const ADDR: &str = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqdku202"; + +/// A raw inbound peer, read on the test's own thread. +struct Peer { + stream: TcpStream, +} + +impl Peer { + fn connect(port: u16) -> Self { + let addr: SocketAddr = format!("127.0.0.1:{port}").parse().unwrap(); + let deadline = Instant::now() + Duration::from_secs(30); + let stream = loop { + match TcpStream::connect_timeout(&addr, Duration::from_secs(2)) { + Ok(s) => break s, + Err(_) if Instant::now() < deadline => std::thread::sleep(Duration::from_millis(100)), + Err(e) => panic!("connect to {addr}: {e}"), + } + }; + stream.set_nodelay(true).unwrap(); + stream.set_write_timeout(Some(Duration::from_secs(30))).unwrap(); + let mut peer = Peer { stream }; + peer.send(NetworkMessage::Version(version())); + let mut got_version = false; + loop { + match peer.recv(Duration::from_secs(30)) { + Ok(NetworkMessage::Version(_)) => got_version = true, + Ok(NetworkMessage::Verack) if got_version => break, + Ok(_) => {} + Err(e) => panic!("handshake: {e}"), + } + } + peer.send(NetworkMessage::Verack); + peer + } + + fn send(&mut self, msg: NetworkMessage) { + self.send_raw(&serialize(&RawNetworkMessage::new(Magic::REGTEST, msg))); + } + + fn send_raw(&mut self, bytes: &[u8]) { + // The node may close the socket part way through. + let _ = self.stream.write_all(bytes); + } + + fn recv(&mut self, timeout: Duration) -> io::Result { + self.stream.set_read_timeout(Some(timeout)).unwrap(); + let mut header = [0u8; 24]; + self.stream.read_exact(&mut header)?; + let len = u32::from_le_bytes(header[16..20].try_into().unwrap()) as usize; + let mut buf = header.to_vec(); + buf.resize(24 + len, 0); + self.stream.read_exact(&mut buf[24..])?; + let raw: RawNetworkMessage = + deserialize(&buf).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e.to_string()))?; + Ok(raw.into_payload()) + } + + /// Answer pings until the node sends a message `pred` accepts. + fn recv_until(&mut self, timeout: Duration, pred: impl Fn(&NetworkMessage) -> bool) -> NetworkMessage { + let deadline = Instant::now() + timeout; + loop { + let left = deadline.saturating_duration_since(Instant::now()).max(Duration::from_millis(1)); + match self.recv(left) { + Ok(NetworkMessage::Ping(n)) => self.send(NetworkMessage::Pong(n)), + Ok(m) if pred(&m) => return m, + Ok(_) => {} + Err(e) => panic!("waiting for a message: {e}"), + } + } + } + + /// The connection is still up: a ping gets its pong. + fn alive(&mut self) -> bool { + self.send(NetworkMessage::Ping(0x1234)); + let deadline = Instant::now() + Duration::from_secs(30); + loop { + let left = deadline.saturating_duration_since(Instant::now()).max(Duration::from_millis(1)); + match self.recv(left) { + Ok(NetworkMessage::Pong(0x1234)) => return true, + Ok(NetworkMessage::Ping(n)) => self.send(NetworkMessage::Pong(n)), + Ok(_) => {} + Err(_) => return false, + } + } + } + + /// The node closes the connection within `timeout`. + fn closed_within(&mut self, timeout: Duration) -> bool { + let deadline = Instant::now() + timeout; + loop { + let left = deadline.saturating_duration_since(Instant::now()); + if left.is_zero() { + return false; + } + match self.recv(left) { + Ok(_) => {} + Err(e) if matches!(e.kind(), io::ErrorKind::WouldBlock | io::ErrorKind::TimedOut) => return false, + Err(_) => return true, + } + } + } +} + +fn version() -> VersionMessage { + let services = ServiceFlags::NETWORK | ServiceFlags::WITNESS; + let zero: SocketAddr = "0.0.0.0:0".parse().unwrap(); + VersionMessage { + version: 70016, + services, + timestamp: 1_760_000_000, + receiver: Address::new(&zero, ServiceFlags::NONE), + sender: Address::new(&zero, services), + nonce: rand_nonce(), + user_agent: "/p2p-limits-test/".into(), + start_height: 0, + relay: true, + } +} + +fn rand_nonce() -> u64 { + std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos() as u64 +} + +fn unknown(len: usize) -> NetworkMessage { + NetworkMessage::Unknown { command: CommandString::try_from_static("bigmsg").unwrap(), payload: vec![0x5a; len] } +} + +/// Core's `MAX_PROTOCOL_MESSAGE_LENGTH`: a message of 4,000,000 bytes is +/// taken (and, of an unknown type, ignored); one byte more and the node +/// drops the connection. satd took frames up to 32 MiB. +#[test] +fn a_frame_over_four_million_bytes_ends_the_connection() { + let mut node = TestNode::start(&[]); + let port = node.p2p_port.unwrap(); + + let mut peer = Peer::connect(port); + peer.send(unknown(4_000_000)); + assert!(peer.alive(), "a 4,000,000-byte message is within the limit"); + + let mut peer = Peer::connect(port); + peer.send(unknown(4_000_001)); + assert!(peer.closed_within(Duration::from_secs(30)), "a 4,000,001-byte message ends the connection"); + node.stop(); +} + +/// Core's `MAX_INV_SZ`: an `inv` of 50,000 entries is fine, one of 50,001 is +/// misbehaviour and the peer is dropped. +#[test] +fn an_inv_over_max_inv_sz_ends_the_connection() { + let mut node = TestNode::start(&[]); + let port = node.p2p_port.unwrap(); + let entry = Inventory::Unknown { inv_type: 99, hash: [7u8; 32] }; + + let mut peer = Peer::connect(port); + peer.send(NetworkMessage::Inv(vec![entry; 50_000])); + assert!(peer.alive(), "50,000 entries are within the limit"); + // Misbehaviour bans the address, and every peer here is 127.0.0.1, so + // the refusal goes last. + peer.send(NetworkMessage::Inv(vec![entry; 50_001])); + assert!(peer.closed_within(Duration::from_secs(30)), "50,001 entries end the connection"); + node.stop(); +} + +/// One `getdata` for more data than the socket and the peer's queue can +/// hold is served in full and in order, as the peer reads (Core keeps the +/// unserved rest in `m_getdata_requests`). satd queued blocks with a +/// `try_send` that dropped whatever did not fit, so a peer that read a +/// little late never got the rest of its request. +#[test] +fn a_large_getdata_is_served_in_full_as_the_peer_reads() { + let mut node = TestNode::start(&[]); + let port = node.p2p_port.unwrap(); + const N: u64 = 100; + node.mine_blocks(N, ADDR); + let hashes: Vec = (1..=N) + .map(|h| { + let hex = node.rpc_ok("getblockhash", vec![serde_json::json!(h)]); + let mut bytes = hex::decode(hex.as_str().unwrap()).unwrap(); + bytes.reverse(); + BlockHash::from_byte_array(bytes.try_into().unwrap()) + }) + .collect(); + // A full-size request, each block asked for many times over: about + // 12 MB of blocks, more than loopback socket buffers take. + let asked: Vec = hashes.iter().copied().cycle().take(50_000).collect(); + + let mut peer = Peer::connect(port); + peer.send(NetworkMessage::GetData(asked.iter().map(|h| Inventory::WitnessBlock(*h)).collect())); + // Read nothing for a while: the node fills the socket and its queue, and + // must hold the rest of the request rather than drop it. + std::thread::sleep(Duration::from_secs(3)); + let mut got = Vec::with_capacity(asked.len()); + while got.len() < asked.len() { + match peer.recv_until(Duration::from_secs(60), |m| { + matches!(m, NetworkMessage::Block(_) | NetworkMessage::NotFound(_)) + }) { + NetworkMessage::Block(b) => got.push(b.block_hash()), + other => panic!("after {} blocks: {other:?}", got.len()), + } + } + assert!(got == asked, "every block, in the order asked"); + assert!(peer.alive()); + node.stop(); +} diff --git a/satd/tests/regtest.rs b/satd/tests/regtest.rs index c6c7f42f5..14d272c9c 100644 --- a/satd/tests/regtest.rs +++ b/satd/tests/regtest.rs @@ -15586,12 +15586,17 @@ mod cf_client { None } - /// Assert that no CFilter / CFHeaders / CFCheckpt arrives in the - /// window — the silent-drop contract from BIP 157. - pub fn assert_silent(&mut self, window: Duration) { + /// Assert that the node closes the connection within the window + /// without sending a CFilter / CFHeaders / CFCheckpt: Core's + /// `PrepareBlockFilterRequest` disconnects a peer whose request it + /// will not serve. + pub fn assert_disconnected(&mut self, window: Duration) { let deadline = Instant::now() + window; - while Instant::now() < deadline { + loop { let remaining = deadline.saturating_duration_since(Instant::now()); + if remaining.is_zero() { + panic!("assert_disconnected: still connected after {window:?}"); + } self.stream .set_read_timeout(Some(remaining.max(Duration::from_millis(100)))) .unwrap(); @@ -15599,10 +15604,13 @@ mod cf_client { Ok(NetworkMessage::CFilter(_)) | Ok(NetworkMessage::CFHeaders(_)) | Ok(NetworkMessage::CFCheckpt(_)) => { - panic!("assert_silent: peer sent a filter response unexpectedly"); + panic!("assert_disconnected: peer sent a filter response unexpectedly"); } Ok(_) => continue, - Err(_) => break, + Err(e) if matches!(e.kind(), io::ErrorKind::WouldBlock | io::ErrorKind::TimedOut) => { + panic!("assert_disconnected: still connected after {window:?}"); + } + Err(_) => return, } } } @@ -15857,13 +15865,14 @@ fn test_p2p_getcfcheckpt_thousand_block_intervals() { } #[test] -fn test_p2p_silent_drop_when_peer_serve_disabled() { +fn test_p2p_disconnect_when_peer_serve_disabled() { use bitcoin::hashes::Hash as _; use bitcoin::p2p::message_filter::GetCFHeaders; - // blockfilterindex=basic but peerblockfilters off: handlers must - // silent-drop and the version handshake must NOT advertise - // NODE_COMPACT_FILTERS. + // blockfilterindex=basic but peerblockfilters off: the version + // handshake must NOT advertise NODE_COMPACT_FILTERS, and a request for + // filters disconnects, as Core's does ("peer requested unsupported block + // filter type"). let p2p_port = find_available_port(); let rpcport = find_available_port(); let datadir = fresh_test_datadir("satd-p2p-silent-test"); @@ -15909,13 +15918,13 @@ fn test_p2p_silent_drop_when_peer_serve_disabled() { start_height: 0, stop_hash, }); - client.assert_silent(Duration::from_secs(2)); + client.assert_disconnected(Duration::from_secs(10)); node.stop(); let _ = std::fs::remove_dir_all(&datadir); } #[test] -fn test_p2p_silent_drop_invalid_filter_type() { +fn test_p2p_disconnect_invalid_filter_type() { use bitcoin::hashes::Hash as _; use bitcoin::p2p::message_filter::GetCFilters; @@ -15956,7 +15965,7 @@ fn test_p2p_silent_drop_invalid_filter_type() { start_height: 0, stop_hash, }); - client.assert_silent(Duration::from_secs(2)); + client.assert_disconnected(Duration::from_secs(10)); node.stop(); let _ = std::fs::remove_dir_all(&datadir); } @@ -16088,7 +16097,7 @@ fn test_p2p_getcfheaders_accepts_2000_headers() { resp.filter_hashes.len() ); - // 2001 headers must be silent-dropped (start=0, stop=2000 = 2001). + // 2001 headers disconnect, as in Core (start=0, stop=2000 = 2001). let stop_hex_genesis = node .rpc_call_with_params("getblockhash", vec![serde_json::json!(0)]) .unwrap()["result"] @@ -16098,20 +16107,20 @@ fn test_p2p_getcfheaders_accepts_2000_headers() { let mut _consensus_g = <[u8; 32]>::try_from(hex::decode(&stop_hex_genesis).expect("hex").as_slice()) .expect("32 bytes"); - let _ = tip_hash; // suppress unused var warnings on the silent-drop path + let _ = tip_hash; // suppress unused var warnings on the refusal path client.send_get_cfheaders(GetCFHeaders { filter_type: 0, start_height: 0, stop_hash: stop_hash_2000, }); - client.assert_silent(Duration::from_secs(2)); + client.assert_disconnected(Duration::from_secs(10)); node.stop(); let _ = std::fs::remove_dir_all(&datadir); } #[test] -fn test_p2p_silent_drop_oversized_range() { +fn test_p2p_disconnect_oversized_range() { use bitcoin::hashes::Hash as _; use bitcoin::p2p::message_filter::GetCFilters; @@ -16150,13 +16159,13 @@ fn test_p2p_silent_drop_oversized_range() { start_height: 0, stop_hash, }); - client.assert_silent(Duration::from_secs(2)); + client.assert_disconnected(Duration::from_secs(10)); node.stop(); let _ = std::fs::remove_dir_all(&datadir); } #[test] -fn test_p2p_silent_drop_off_chain_stop_hash() { +fn test_p2p_disconnect_unknown_stop_hash() { use bitcoin::hashes::Hash as _; use bitcoin::p2p::message_filter::GetCFHeaders; @@ -16190,7 +16199,7 @@ fn test_p2p_silent_drop_off_chain_stop_hash() { start_height: 0, stop_hash, }); - client.assert_silent(Duration::from_secs(2)); + client.assert_disconnected(Duration::from_secs(10)); node.stop(); let _ = std::fs::remove_dir_all(&datadir); } From a33a2666c0c7c2b3b93302bbea73402ba20bbeef Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Wed, 7 Oct 2026 17:13:14 -0600 Subject: [PATCH 02/13] p2p: wake the manager when a getdata or a getdata resume is handed over The write loop reads nothing more from a peer until its getdata has been served, so the manager's 500 ms drain tick bounded a peer fetching blocks to about one getdata, or one send buffer's worth of a large one, per tick. Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/net/manager.rs | 14 ++++++++++++-- node/src/net/manager_p2pbounds_tests.rs | 15 ++++++++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index f883cd5e0..1da8eeed9 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -10184,14 +10184,21 @@ impl PeerManager { flow.queued(); } // Nothing more is read until the manager has - // served this request (`reading_paused`). - if let (NetworkMessage::GetData(_), Some(q)) = (&msg, &send_queue) { + // served this request (`reading_paused`), so it + // is served now rather than at the manager's next + // tick: a peer fetching blocks sends one `getdata` + // after another. + let getdata = matches!(msg, NetworkMessage::GetData(_)); + if let (true, Some(q)) = (getdata, &send_queue) { q.note_getdata_forwarded(); } event_tx .send(NetEvent::MessageReceived { id, msg }) .await .map_err(|e| e.to_string())?; + if let (true, Some(wake)) = (getdata, &drain_now) { + wake.notify_one(); + } } None => { // Reader task ended (error or timeout) @@ -10233,6 +10240,9 @@ impl PeerManager { .send(NetEvent::GetDataResume { id }) .await .map_err(|e| e.to_string())?; + if let Some(wake) = &drain_now { + wake.notify_one(); + } } } } diff --git a/node/src/net/manager_p2pbounds_tests.rs b/node/src/net/manager_p2pbounds_tests.rs index e14865ac6..73586a9f6 100644 --- a/node/src/net/manager_p2pbounds_tests.rs +++ b/node/src/net/manager_p2pbounds_tests.rs @@ -569,6 +569,8 @@ struct Loop { msg_tx: crate::net::send_queue::PeerSender, read_tx: mpsc::Sender, disconnect: Arc, + /// The manager's drain wake-up. + drain_now: Arc, stats: Arc, far: tokio::net::TcpStream, task: tokio::task::JoinHandle>, @@ -581,8 +583,9 @@ async fn run_loop() -> Loop { let msg_tx = crate::net::send_queue::PeerSender::from(msg_tx); let (read_tx, mut read_rx) = mpsc::channel::(64); let disconnect = Arc::new(tokio::sync::Notify::new()); + let drain_now = Arc::new(tokio::sync::Notify::new()); let queue = msg_tx.queue().clone(); - let (s, d) = (stats.clone(), disconnect.clone()); + let (s, d, w) = (stats.clone(), disconnect.clone(), drain_now.clone()); let task = tokio::spawn(async move { let _keep = _reader; PeerManager::peer_write_loop( @@ -593,13 +596,13 @@ async fn run_loop() -> Loop { &mut read_rx, Some(s), Some(Arc::new(crate::net::flow::PeerFlow::new())), - None, + Some(w), Some(d), Some(queue), ) .await }); - Loop { id: 7, event_rx, msg_tx, read_tx, disconnect, stats, far, task } + Loop { id: 7, event_rx, msg_tx, read_tx, disconnect, drain_now, stats, far, task } } /// Queue big messages until the socket stops taking bytes: the far end is @@ -672,6 +675,9 @@ async fn reading_waits_for_a_getdata_to_be_served() { l.read_tx.send(NetworkMessage::SendHeaders).await.unwrap(); let first = tokio::time::timeout(Duration::from_secs(5), l.event_rx.recv()).await.unwrap(); assert!(matches!(first, Some(NetEvent::MessageReceived { msg: NetworkMessage::GetData(_), .. }))); + tokio::time::timeout(Duration::from_secs(1), l.drain_now.notified()) + .await + .expect("the manager is woken to serve the getdata, not left to its next tick"); let early = tokio::time::timeout(Duration::from_millis(300), l.event_rx.recv()).await; assert!(early.is_err(), "nothing after the getdata is read before it is served"); @@ -719,6 +725,9 @@ async fn the_loop_asks_for_more_once_the_queue_drains() { .await .expect("a resume is asked for"); assert_eq!(ev, l.id); + tokio::time::timeout(Duration::from_secs(1), l.drain_now.notified()) + .await + .expect("the manager is woken to serve it"); assert_eq!(queue.queued_bytes(), 0); l.disconnect.notify_one(); let _ = l.task.await; From 4c8310536a5f9eca29dec70b97dec77b4c3627a2 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Wed, 7 Oct 2026 21:39:24 -0600 Subject: [PATCH 03/13] p2p: don't ban local, inbound-onion or manual peers for misbehaviour Deal with a peer whose misbehaviour reaches the threshold as Core's MaybeDiscourageAndDisconnect does (net_processing.cpp), in its order: a noban peer and a manual connection are not punished; a peer on a local address (CNetAddr::IsLocal: 127.0.0.0/8, 0.0.0.0/8, ::1) is disconnected without punishing the address; anyone else is disconnected and its address banned for -bantime, as before (Core discourages it). satd banned every peer but noban ones. Every inbound onion peer arrives from the address Tor dialled from, usually 127.0.0.1, as does every unwhitelisted local client, so one such peer's misbehaviour refused all of them until the ban expired, across restarts. A manual peer was banned and then skipped by the redial loop. Peers accepted on the onion listener are now marked (Core's m_inbound_onion), and getpeerinfo reports them on the onion network, as Core's ConnectedThroughNetwork does. An outbound onion peer is banned by its onion host, the address Core punishes, rather than by the 0.0.0.0 placeholder socket every onion peer shares. Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/net/manager.rs | 137 ++++++++---- node/src/net/manager_peerpunish_tests.rs | 261 +++++++++++++++++++++++ node/src/net/mod.rs | 10 + node/src/net/peer.rs | 13 +- satd/tests/compact_blocks.rs | 7 +- satd/tests/p2p_limits.rs | 4 +- satd/tests/p2p_punish.rs | 261 +++++++++++++++++++++++ 7 files changed, 649 insertions(+), 44 deletions(-) create mode 100644 node/src/net/manager_peerpunish_tests.rs create mode 100644 satd/tests/p2p_punish.rs diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index 1da8eeed9..5d7c39788 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -42,6 +42,19 @@ const MAX_OUTBOUND_BLOCK_RELAY: usize = 2; /// since the connection was made. const ADDR_FETCH_TIMEOUT_SECS: u64 = 10 * 30; const BAN_THRESHOLD: u32 = 100; + +/// What becomes of a peer whose misbehaviour reached [`BAN_THRESHOLD`]; see +/// [`PeerManager::misbehaviour_action`]. +#[derive(Debug)] +enum MisbehaviourAction { + /// Neither disconnected nor punished. Names the kind of peer, as Core's + /// log line does. + Keep(&'static str), + /// Disconnected; its address is not banned. + Disconnect, + /// Disconnected, and this banned for `-bantime`. + Ban(crate::net::ban::BanTarget), +} /// Keepalive cadence: how often each peer is sent a `ping` when none is /// outstanding. Bitcoin Core's `PING_INTERVAL` (net_processing.h). /// @@ -514,6 +527,10 @@ fn log_received(id: PeerId, msg: &NetworkMessage) { #[path = "manager_p2pbounds_tests.rs"] mod p2pbounds_tests; +#[cfg(test)] +#[path = "manager_peerpunish_tests.rs"] +mod peerpunish_tests; + /// How a transaction is announced to a peer: `MSG_WTX` carrying the wtxid /// to a peer that negotiated BIP 339 wtxid relay, the txid to the rest. fn tx_announcement(wtxid_relay: bool, txid: bitcoin::Txid, wtxid: bitcoin::Wtxid) -> Inventory { @@ -2947,6 +2964,7 @@ impl PeerManager { let (msg_tx, msg_rx) = mpsc::channel::(256); let mut info = PeerInfo::new(id, addr, Direction::Inbound); info.permissions = perms; + info.inbound_onion = inbound_onion; // `getpeerinfo`'s `addrbind`: which of our listeners this peer // reached us on. Distinct from `-bind` config, since a node may // listen on several addresses. @@ -3666,53 +3684,98 @@ impl PeerManager { self.ban_list.read().is_banned(&addr.ip(), now) } - /// Add ban score to a peer. If the score exceeds BAN_THRESHOLD, the peer - /// is disconnected, removed, and its address is banned. + /// Add ban score to a peer. Once the score reaches [`BAN_THRESHOLD`] the + /// peer has misbehaved (Core's `Misbehaving`), and it is dealt with as + /// [`Self::misbehaviour_action`] says. fn add_ban_score(&self, id: PeerId, score: u32, reason: &str) { let mut peers = self.peers.write(); - let (should_ban, ban_addr) = if let Some(handle) = peers.get_mut(&id) { - // NoBan peers (-whitelist/-whitebind) are never banned or - // disconnected for misbehavior. - if handle.info.permissions.noban { - tracing::debug!(id, addr = %handle.info.addr, reason, "Skipping ban score for noban peer"); + let Some(handle) = peers.get_mut(&id) else { + return; + }; + handle.info.ban_score = handle.info.ban_score.saturating_add(score); + if handle.info.ban_score < BAN_THRESHOLD { + tracing::debug!(id, score = handle.info.ban_score, reason, "Increased ban score"); + return; + } + let ban = match Self::misbehaviour_action(&handle.info) { + MisbehaviourAction::Keep(kind) => { + // Core clears `m_should_discourage` and does nothing else, so + // the next misbehaviour is logged again. + handle.info.ban_score = 0; + tracing::warn!(id, reason, "Not punishing {kind} peer {id}!"); return; } - handle.info.ban_score += score; - if handle.info.ban_score >= BAN_THRESHOLD { - tracing::warn!(id, addr = %handle.info.addr, score = handle.info.ban_score, reason, "Banning peer"); - (true, Some(handle.info.addr.ip())) - } else { - tracing::debug!(id, score = handle.info.ban_score, reason, "Increased ban score"); - (false, None) - } - } else { - (false, None) - }; - if should_ban { - // Signal, for the same reason `disconnect_by_id` does: dropping - // the handle closes the peer task's `msg_rx` only when the last - // sender goes, and a `getcfilters` stream task can hold a clone. - // Without the signal a banned peer stayed connected -- and kept - // feeding the node -- until that task drained. - if let Some(handle) = peers.remove(&id) { - handle.disconnect.notify_one(); + MisbehaviourAction::Disconnect => { + let kind = if handle.info.inbound_onion { "inbound onion" } else { "local" }; + tracing::debug!(id, reason, "Warning: disconnecting but not banning {kind} peer {id}!"); + None } - if let Some(addr) = ban_addr { - drop(peers); // release peers lock before acquiring ban_list lock - let now = crate::time::now_secs(); - let duration = self.ban_duration_secs.load(Ordering::Relaxed); - let target = crate::net::ban::BanTarget::Net( - ipnet::IpNet::from(addr), + MisbehaviourAction::Ban(target) => { + tracing::warn!( + id, + addr = %handle.info.addr_string(), + score = handle.info.ban_score, + reason, + "Banning peer" ); - // Best-effort: misbehaviour bans are fire-and-forget; if the - // entry already exists (e.g. repeated misbehaviour) the add - // fails silently. - let _ = self.ban_list.write().add(&target, now, now + duration); - self.flush_banlist(); + Some(target) } + }; + // Signal, for the same reason `disconnect_by_id` does: dropping + // the handle closes the peer task's `msg_rx` only when the last + // sender goes, and a `getcfilters` stream task can hold a clone. + // Without the signal a banned peer stayed connected -- and kept + // feeding the node -- until that task drained. + if let Some(handle) = peers.remove(&id) { + handle.disconnect.notify_one(); + } + if let Some(target) = ban { + drop(peers); // release peers lock before acquiring ban_list lock + let now = crate::time::now_secs(); + let duration = self.ban_duration_secs.load(Ordering::Relaxed); + // Best-effort: misbehaviour bans are fire-and-forget; if the + // entry already exists (e.g. repeated misbehaviour) the add + // fails silently. + let _ = self.ban_list.write().add(&target, now, now + duration); + self.flush_banlist(); } } + /// What Core's `MaybeDiscourageAndDisconnect` (`net_processing.cpp`) does + /// with a peer that has misbehaved, in this order: + /// - a `noban` peer is not punished; + /// - nor is a manual connection (`addnode`, `-addnode`, `-connect`); + /// - a peer on a local address is disconnected, but its address is not + /// punished, "since that would discourage all peers on the same local + /// address". Every inbound onion peer arrives from the address Tor + /// dialled from, usually loopback, as does every local integration; + /// - anyone else is disconnected and its address punished. Core + /// discourages it; satd bans it for `-bantime`. + /// + /// satd went straight to the last step for every peer but `noban` ones. + /// One misbehaving inbound onion peer banned 127.0.0.1, which refused + /// every onion peer and unwhitelisted local client until the ban expired, + /// across restarts, and a manual peer was banned and then skipped by the + /// redial loop. + fn misbehaviour_action(info: &PeerInfo) -> MisbehaviourAction { + if info.permissions.noban { + return MisbehaviourAction::Keep("noban"); + } + if info.conn_type == ConnType::Manual { + return MisbehaviourAction::Keep("manually connected"); + } + // An outbound onion peer's `addr` is the `0.0.0.0` placeholder all + // onion peers share. Core's address for it is the onion host, which + // is not a local one, and is what it punishes. + if let Some(host) = &info.onion_host { + return MisbehaviourAction::Ban(crate::net::ban::BanTarget::Onion(host.clone())); + } + if crate::net::is_local(info.addr.ip()) { + return MisbehaviourAction::Disconnect; + } + MisbehaviourAction::Ban(crate::net::ban::BanTarget::Net(ipnet::IpNet::from(info.addr.ip()))) + } + /// Run the main event loop. Returns when shutdown signal is received. pub async fn run(self: &Arc) { let mut event_rx = self.event_rx.lock().await; diff --git a/node/src/net/manager_peerpunish_tests.rs b/node/src/net/manager_peerpunish_tests.rs new file mode 100644 index 000000000..df0ce2edc --- /dev/null +++ b/node/src/net/manager_peerpunish_tests.rs @@ -0,0 +1,261 @@ +//! What happens to a peer whose misbehaviour reaches the threshold: Core's +//! `MaybeDiscourageAndDisconnect` (`net_processing.cpp`), against a live +//! `PeerManager`. + +use super::*; +use crate::net::flow::InFlight; + +fn mk_pm() -> (Arc, tempfile::TempDir) { + use crate::chain::state::AssumeValid; + use crate::storage::db::InMemoryStore; + use crate::storage::flatfile::FlatFileManager; + use crate::validation::script::NoopVerifier; + + let dir = tempfile::TempDir::new().unwrap(); + let store = Box::new(InMemoryStore::new()); + let flat_files = FlatFileManager::new(&dir.path().join("blocks")).unwrap(); + let chain_state = Arc::new( + ChainState::new( + store, + flat_files, + Network::Regtest, + Box::new(NoopVerifier), + AssumeValid::Disabled, + 450, + 4, + Default::default(), + Default::default(), + Default::default(), + ) + .unwrap(), + ); + let mempool = Arc::new(Mempool::new(1_000_000, 0)); + let fee_estimator = Arc::new(FeeEstimator::new()); + let (_shutdown_tx, shutdown_rx) = tokio::sync::watch::channel(false); + let pm = PeerManager::new(chain_state, mempool, fee_estimator, Network::Regtest, shutdown_rx); + (pm, dir) +} + +/// A connected peer at `addr`. +fn peer(id: PeerId, addr: &str, direction: Direction) -> PeerInfo { + let mut info = PeerInfo::new(id, addr.parse().unwrap(), direction); + info.state = PeerState::Connected; + info +} + +fn add_peer(pm: &PeerManager, info: PeerInfo) -> mpsc::Receiver { + let (tx, rx) = mpsc::channel::(8); + pm.peers.write().insert( + info.id, + PeerHandle { + info, + msg_tx: tx.into(), + disconnect: Arc::new(tokio::sync::Notify::new()), + flow: Arc::new(crate::net::flow::PeerFlow::new()), + last_getheaders_sent: None, + last_mempool_served: None, + fee_filter_sent: None, + stats: PeerStats::new(NetTotals::new()), + }, + ); + rx +} + +/// What became of a peer after it misbehaved. +#[derive(Debug, PartialEq, Eq)] +enum Outcome { + /// Still connected, nothing banned. + Kept, + /// Disconnected, nothing banned. + Disconnected, + /// Disconnected, and a ban entry added. + Banned, +} + +fn outcome(pm: &PeerManager, id: PeerId) -> Outcome { + let connected = pm.peers.read().contains_key(&id); + let banned = !pm.list_banned().is_empty(); + match (connected, banned) { + (true, false) => Outcome::Kept, + (false, false) => Outcome::Disconnected, + (false, true) => Outcome::Banned, + (true, true) => panic!("peer {id} is still connected, but something was banned"), + } +} + +/// Core's ladder, rung by rung. A `noban` peer and a manual connection are +/// never punished ("Not punishing noban peer", "Not punishing manually +/// connected peer"), whatever their address. A peer on a local address is +/// disconnected but its address is not discouraged, "since that would +/// discourage all peers on the same local address": every inbound onion peer +/// and every local integration shares 127.0.0.1. Anyone else is disconnected +/// and its address punished (satd bans it for `-bantime`). +#[test] +fn misbehaviour_follows_cores_ladder() { + use crate::net::permissions::NetPermissions; + let noban = |mut info: PeerInfo| { + info.permissions = NetPermissions { noban: true, ..NetPermissions::NONE }; + info + }; + let manual = |mut info: PeerInfo| { + info.conn_type = ConnType::Manual; + info + }; + let onion = |mut info: PeerInfo| { + info.inbound_onion = true; + info + }; + let cases: Vec<(&str, PeerInfo, Outcome)> = vec![ + ("an inbound peer", peer(1, "10.62.0.1:50000", Direction::Inbound), Outcome::Banned), + ("an outbound peer", peer(1, "10.62.0.2:8333", Direction::Outbound), Outcome::Banned), + // Private, but not local: Core's `IsLocal` is loopback and 0.0.0.0/8. + ("a peer on a private network", peer(1, "192.168.7.5:8333", Direction::Inbound), Outcome::Banned), + ("a noban peer", noban(peer(1, "10.62.0.3:50000", Direction::Inbound)), Outcome::Kept), + ("a manual peer", manual(peer(1, "10.62.0.4:8333", Direction::Outbound)), Outcome::Kept), + // Manual comes before local in the ladder. + ("a manual peer on loopback", manual(peer(1, "127.0.0.1:8333", Direction::Outbound)), Outcome::Kept), + ("a peer on 127.0.0.1", peer(1, "127.0.0.1:50000", Direction::Inbound), Outcome::Disconnected), + ("a peer elsewhere in 127.0.0.0/8", peer(1, "127.5.6.7:50000", Direction::Inbound), Outcome::Disconnected), + ("a peer on ::1", peer(1, "[::1]:50000", Direction::Inbound), Outcome::Disconnected), + // What a dual-stack listener reports for an IPv4 loopback peer. + ( + "a peer on ::ffff:127.0.0.1", + peer(1, "[::ffff:127.0.0.1]:50000", Direction::Inbound), + Outcome::Disconnected, + ), + ("an inbound onion peer", onion(peer(1, "127.0.0.1:50000", Direction::Inbound)), Outcome::Disconnected), + ("an outbound peer on loopback", peer(1, "127.0.0.1:8333", Direction::Outbound), Outcome::Disconnected), + ]; + for (case, info, expected) in cases { + let (pm, _dir) = mk_pm(); + let _rx = add_peer(&pm, info); + pm.add_ban_score(1, BAN_THRESHOLD, "test"); + assert_eq!(outcome(&pm, 1), expected, "{case}"); + } +} + +/// An outbound onion peer's socket address is the `0.0.0.0` placeholder all +/// onion peers share, not its address. Core punishes the onion address +/// itself, which is not a local one; satd bans that host, and leaves the +/// placeholder alone. +#[test] +fn a_misbehaving_outbound_onion_peer_is_banned_by_its_onion_host() { + let host = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"; + let (pm, _dir) = mk_pm(); + let mut info = peer(1, "0.0.0.0:8333", Direction::Outbound); + info.onion_host = Some(host.to_string()); + let _rx = add_peer(&pm, info); + pm.add_ban_score(1, BAN_THRESHOLD, "test"); + + assert!(!pm.peers.read().contains_key(&1), "the peer is disconnected"); + let now = crate::time::now_secs(); + assert!(pm.ban_list.read().is_onion_banned(host, now), "its onion host is banned"); + assert!( + !pm.is_addr_banned(&"0.0.0.0:8333".parse().unwrap()), + "the shared placeholder is not a peer's address" + ); +} + +/// Neither a `noban` peer nor a manual one is punished however often it +/// misbehaves, and a score that stays under the threshold punishes no one. +#[test] +fn kept_peers_stay_however_often_they_misbehave() { + use crate::net::permissions::NetPermissions; + for case in ["noban", "manual"] { + let (pm, _dir) = mk_pm(); + let mut info = peer(1, "10.62.0.5:8333", Direction::Outbound); + match case { + "noban" => info.permissions = NetPermissions { noban: true, ..NetPermissions::NONE }, + _ => info.conn_type = ConnType::Manual, + } + let _rx = add_peer(&pm, info); + for _ in 0..5 { + pm.add_ban_score(1, BAN_THRESHOLD, "test"); + } + pm.add_ban_score(1, 1, "test"); + assert_eq!(outcome(&pm, 1), Outcome::Kept, "{case}"); + } + + // The must-succeed half: an ordinary peer is punished once its score + // reaches the threshold, and not before. + let (pm, _dir) = mk_pm(); + let _rx = add_peer(&pm, peer(1, "10.62.0.6:8333", Direction::Outbound)); + pm.add_ban_score(1, BAN_THRESHOLD - 1, "test"); + assert_eq!(outcome(&pm, 1), Outcome::Kept, "under the threshold"); + pm.add_ban_score(1, 1, "test"); + assert_eq!(outcome(&pm, 1), Outcome::Banned, "at the threshold"); +} + +/// The case that matters most, through the message handler: a `getblocktxn` +/// naming an index past the end of the block is `Misbehaving` in Core +/// (`SendBlockTransactions`, "getblocktxn with out-of-bounds tx indices"). +/// From an inbound onion peer it disconnects that peer and bans nothing, so +/// the next onion peer still gets in; from an ordinary peer the address is +/// banned as before. +#[test] +fn an_out_of_range_getblocktxn_from_an_inbound_onion_peer_bans_nothing() { + let (pm, _dir) = mk_pm(); + let mempool = Mempool::new(1_000_000, 0); + let block = crate::mining::miner::build_block_to_script( + &pm.chain_state, + &mempool, + bitcoin::ScriptBuf::new_op_return([0x2au8; 4]), + None, + ) + .expect("mine a regtest block"); + pm.chain_state.accept_block(&block).expect("accept the block"); + let request = || { + NetworkMessage::GetBlockTxn(bitcoin::p2p::message_compact_blocks::GetBlockTxn { + txs_request: bitcoin::bip152::BlockTransactionsRequest { + block_hash: block.block_hash(), + indexes: vec![5], + }, + }) + }; + + let mut onion = peer(1, "127.0.0.1:50000", Direction::Inbound); + onion.inbound_onion = true; + let _rx = add_peer(&pm, onion); + pm.handle_message(1, request(), InFlight::adopt(None)); + assert!(!pm.peers.read().contains_key(&1), "the onion peer is disconnected"); + assert!(pm.list_banned().is_empty(), "and nothing is banned"); + assert!( + !pm.is_addr_banned(&"127.0.0.1:50001".parse().unwrap()), + "the next onion peer is not refused" + ); + + let ordinary: SocketAddr = "10.62.0.7:50000".parse().unwrap(); + let _rx = add_peer(&pm, peer(2, "10.62.0.7:50000", Direction::Inbound)); + pm.handle_message(2, request(), InFlight::adopt(None)); + assert!(!pm.peers.read().contains_key(&2), "the ordinary peer is disconnected"); + assert!(pm.is_addr_banned(&ordinary), "and its address banned"); +} + +/// Core's `ConnectedThroughNetwork`: a peer that came in on the onion +/// listener is on the onion network, whatever its socket address, and +/// `getpeerinfo.network` says so (`net.cpp`, `rpc/net.cpp`). +#[test] +fn getpeerinfo_reports_an_inbound_onion_peer_as_onion() { + let stats = PeerStats::new(NetTotals::new()); + let mut info = peer(1, "127.0.0.1:50000", Direction::Inbound); + assert_eq!(info.to_rpc_json(&stats)["network"], "not_publicly_routable"); + info.inbound_onion = true; + assert_eq!(info.to_rpc_json(&stats)["network"], "onion"); +} + +/// The accept path records which listener a peer came in on. +#[tokio::test] +async fn a_peer_accepted_on_the_onion_listener_is_marked_inbound_onion() { + use crate::net::permissions::NetPermissions; + for inbound_onion in [false, true] { + let (pm, _dir) = mk_pm(); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let dialer = tokio::net::TcpStream::connect(listener.local_addr().unwrap()); + let (dialed, accepted) = tokio::join!(dialer, listener.accept()); + let (_dialed, (accepted, peer_addr)) = (dialed.unwrap(), accepted.unwrap()); + pm.accept_inbound_with_perms(accepted, peer_addr, NetPermissions::NONE, inbound_onion); + let peers = pm.peers.read(); + let marked: Vec = peers.values().map(|h| h.info.inbound_onion).collect(); + assert_eq!(marked, vec![inbound_onion], "accepted with inbound_onion = {inbound_onion}"); + } +} diff --git a/node/src/net/mod.rs b/node/src/net/mod.rs index 2e3c41835..44c03e4c9 100644 --- a/node/src/net/mod.rs +++ b/node/src/net/mod.rs @@ -105,6 +105,16 @@ pub fn is_routable(ip: std::net::IpAddr) -> bool { } } +/// Bitcoin Core's `CNetAddr::IsLocal` (`netaddress.cpp`): IPv4 127.0.0.0/8 +/// or 0.0.0.0/8, or IPv6 `::1`. An IPv4-mapped IPv6 address is judged as the +/// IPv4 address it carries, as Core's `CNetAddr` holds it. +pub fn is_local(ip: std::net::IpAddr) -> bool { + match ip.to_canonical() { + std::net::IpAddr::V4(v4) => matches!(v4.octets()[0], 0 | 127), + std::net::IpAddr::V6(v6) => v6 == std::net::Ipv6Addr::LOCALHOST, + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/node/src/net/peer.rs b/node/src/net/peer.rs index 9cae1f83e..f92ce064e 100644 --- a/node/src/net/peer.rs +++ b/node/src/net/peer.rs @@ -276,6 +276,11 @@ pub struct PeerInfo { /// dedup, getpeerinfo, and addrman can distinguish onion peers. `None` /// for clearnet and inbound peers. pub onion_host: Option, + /// The peer came in on the listener a Tor hidden service forwards to + /// (`-bind=…=onion`): Core's `CNode::m_inbound_onion`. Its socket address + /// is the one Tor dialled from, usually loopback, and says nothing about + /// the peer. + pub inbound_onion: bool, /// The local socket address this connection is bound to /// (`getsockname()`), behind `getpeerinfo`'s `addrbind`. `None` until the /// socket exists, and for onion peers, whose local end belongs to the @@ -330,6 +335,7 @@ impl PeerInfo { permissions: crate::net::permissions::NetPermissions::NONE, bind_addr: None, onion_host: None, + inbound_onion: false, conn_type: match direction { Direction::Inbound => ConnType::Inbound, Direction::Outbound => ConnType::OutboundFullRelay, @@ -422,9 +428,10 @@ impl PeerInfo { .as_secs(); let addr_str = self.addr_string(); - // Derive the peer's network from its address. - let network = if let Some(ref onion) = self.onion_host { - let _ = onion; // suppress unused + // Derive the peer's network from its address. A peer that came in on + // the onion listener is on the onion network whatever address Tor + // dialled from: Core's `ConnectedThroughNetwork` (`net.cpp`). + let network = if self.onion_host.is_some() || self.inbound_onion { "onion" } else if !crate::net::is_routable(self.addr.ip()) { "not_publicly_routable" diff --git a/satd/tests/compact_blocks.rs b/satd/tests/compact_blocks.rs index eacc241a4..636e1f40f 100644 --- a/satd/tests/compact_blocks.rs +++ b/satd/tests/compact_blocks.rs @@ -604,7 +604,9 @@ fn blocktxn_with_wrong_count_is_penalised() { transactions: BlockTransactions { block_hash: hash, transactions: vec![b.txdata[1].clone()] }, })); assert!(peer.wait_closed(Duration::from_secs(15)), "a mismatched blocktxn must disconnect the peer"); - poll_until(|| banned_count(&node) == 1, test_timeout(10), "and ban it"); + // The peer is on 127.0.0.1: Core disconnects a local peer without + // punishing its address, which every local peer shares. + assert_eq!(banned_count(&node), 0, "a local peer's address is not banned"); } fn getblocktxn(hash: BlockHash, indexes: Vec) -> NetworkMessage { @@ -662,7 +664,8 @@ fn getblocktxn_out_of_range_index_is_penalised() { // holds only its coinbase. peer.send(getblocktxn(best_hash(&node), vec![5])); assert!(peer.wait_closed(Duration::from_secs(15)), "an out-of-range getblocktxn must disconnect"); - poll_until(|| banned_count(&node) == 1, test_timeout(10), "and ban the peer"); + // As above: a local peer is disconnected, its address not banned. + assert_eq!(banned_count(&node), 0, "a local peer's address is not banned"); } /// Core allows at most three peers to hold a partial reconstruction of the diff --git a/satd/tests/p2p_limits.rs b/satd/tests/p2p_limits.rs index 83edd2c67..80bcea57a 100644 --- a/satd/tests/p2p_limits.rs +++ b/satd/tests/p2p_limits.rs @@ -171,8 +171,8 @@ fn an_inv_over_max_inv_sz_ends_the_connection() { let mut peer = Peer::connect(port); peer.send(NetworkMessage::Inv(vec![entry; 50_000])); assert!(peer.alive(), "50,000 entries are within the limit"); - // Misbehaviour bans the address, and every peer here is 127.0.0.1, so - // the refusal goes last. + // Misbehaviour ends the connection (a peer on 127.0.0.1 is disconnected, + // not banned), so the refusal goes last. peer.send(NetworkMessage::Inv(vec![entry; 50_001])); assert!(peer.closed_within(Duration::from_secs(30)), "50,001 entries end the connection"); node.stop(); diff --git a/satd/tests/p2p_punish.rs b/satd/tests/p2p_punish.rs new file mode 100644 index 000000000..5484ff8f7 --- /dev/null +++ b/satd/tests/p2p_punish.rs @@ -0,0 +1,261 @@ +//! What a regtest node does with a peer that misbehaves, by where the peer +//! connected from: Bitcoin Core's `MaybeDiscourageAndDisconnect` +//! (`net_processing.cpp`), exercised over raw connections. + +mod common; + +use bitcoin::consensus::{deserialize, serialize}; +use bitcoin::p2p::message::{NetworkMessage, RawNetworkMessage}; +use bitcoin::p2p::message_blockdata::Inventory; +use bitcoin::p2p::message_network::VersionMessage; +use bitcoin::p2p::{Address, Magic, ServiceFlags}; +use bitcoin::BlockHash; +use common::{TestNode, find_available_port, poll_until, test_timeout}; +use serde_json::json; +use std::io::{self, Read, Write}; +use std::net::{SocketAddr, TcpListener, TcpStream}; +use std::time::{Duration, Instant}; + +const ADDR: &str = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqdku202"; + +/// A raw peer, read on the test's own thread. +struct Peer { + stream: TcpStream, +} + +impl Peer { + /// Connect to `port` and complete the handshake, or report why not. + fn connect(port: u16) -> io::Result { + let addr: SocketAddr = format!("127.0.0.1:{port}").parse().unwrap(); + let deadline = Instant::now() + Duration::from_secs(30); + let stream = loop { + match TcpStream::connect_timeout(&addr, Duration::from_secs(2)) { + Ok(s) => break s, + Err(_) if Instant::now() < deadline => std::thread::sleep(Duration::from_millis(100)), + Err(e) => return Err(e), + } + }; + Self::handshake(stream) + } + + /// Accept the connection the node dials to `listener`: an outbound peer + /// of the node. + fn accept(listener: &TcpListener) -> Self { + listener.set_nonblocking(true).unwrap(); + let deadline = Instant::now() + Duration::from_secs(30); + let stream = loop { + match listener.accept() { + Ok((s, _)) => break s, + Err(e) if e.kind() == io::ErrorKind::WouldBlock && Instant::now() < deadline => { + std::thread::sleep(Duration::from_millis(20)) + } + Err(e) => panic!("the node never dialled us: {e}"), + } + }; + stream.set_nonblocking(false).unwrap(); + Self::handshake(stream).expect("handshake with the dialling node") + } + + fn handshake(stream: TcpStream) -> io::Result { + stream.set_nodelay(true).unwrap(); + stream.set_write_timeout(Some(Duration::from_secs(30))).unwrap(); + let mut peer = Peer { stream }; + peer.send(NetworkMessage::Version(version())); + let (mut got_version, mut got_verack) = (false, false); + while !(got_version && got_verack) { + match peer.recv(Duration::from_secs(30))? { + NetworkMessage::Version(_) => { + got_version = true; + peer.send(NetworkMessage::Verack); + } + NetworkMessage::Verack => got_verack = true, + _ => {} + } + } + Ok(peer) + } + + fn send(&mut self, msg: NetworkMessage) { + // The node may close the socket part way through. + let _ = self.stream.write_all(&serialize(&RawNetworkMessage::new(Magic::REGTEST, msg))); + } + + fn recv(&mut self, timeout: Duration) -> io::Result { + self.stream.set_read_timeout(Some(timeout)).unwrap(); + let mut header = [0u8; 24]; + self.stream.read_exact(&mut header)?; + let len = u32::from_le_bytes(header[16..20].try_into().unwrap()) as usize; + let mut buf = header.to_vec(); + buf.resize(24 + len, 0); + self.stream.read_exact(&mut buf[24..])?; + let raw: RawNetworkMessage = + deserialize(&buf).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e.to_string()))?; + Ok(raw.into_payload()) + } + + /// Answer pings until the node sends a message `pred` accepts; `None` if + /// the connection ends or nothing arrives in time. + fn recv_until(&mut self, timeout: Duration, pred: impl Fn(&NetworkMessage) -> bool) -> Option { + let deadline = Instant::now() + timeout; + loop { + let left = deadline.saturating_duration_since(Instant::now()).max(Duration::from_millis(1)); + match self.recv(left) { + Ok(NetworkMessage::Ping(n)) => self.send(NetworkMessage::Pong(n)), + Ok(m) if pred(&m) => return Some(m), + Ok(_) => {} + Err(_) => return None, + } + } + } + + /// The node closes the connection within `timeout`. + fn closed_within(&mut self, timeout: Duration) -> bool { + let deadline = Instant::now() + timeout; + loop { + let left = deadline.saturating_duration_since(Instant::now()); + if left.is_zero() { + return false; + } + match self.recv(left) { + Ok(NetworkMessage::Ping(n)) => self.send(NetworkMessage::Pong(n)), + Ok(_) => {} + Err(e) if matches!(e.kind(), io::ErrorKind::WouldBlock | io::ErrorKind::TimedOut) => return false, + Err(_) => return true, + } + } + } + + /// The node still serves this peer: a `getdata` sent now, after whatever + /// was sent before it, is answered with the block. The node handles one + /// peer's messages in order, so the answer also shows that nothing sent + /// earlier ended the connection. + fn served(&mut self, block: BlockHash) -> bool { + self.send(NetworkMessage::GetData(vec![Inventory::WitnessBlock(block)])); + self.recv_until(Duration::from_secs(30), |m| matches!(m, NetworkMessage::Block(b) if b.block_hash() == block)) + .is_some() + } +} + +fn version() -> VersionMessage { + let services = ServiceFlags::NETWORK | ServiceFlags::WITNESS; + let zero: SocketAddr = "0.0.0.0:0".parse().unwrap(); + VersionMessage { + version: 70016, + services, + timestamp: 1_760_000_000, + receiver: Address::new(&zero, ServiceFlags::NONE), + sender: Address::new(&zero, services), + nonce: rand_nonce(), + user_agent: "/p2p-punish-test/".into(), + start_height: 0, + relay: true, + } +} + +/// A distinct version nonce per connection, so the node's self-connection +/// check never mistakes one raw peer for another. +fn rand_nonce() -> u64 { + use std::hash::{BuildHasher, Hasher}; + static COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let mut h = std::collections::hash_map::RandomState::new().build_hasher(); + h.write_u64(COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed)); + h.finish() +} + +fn best_hash(node: &TestNode) -> BlockHash { + node.rpc_ok("getbestblockhash", vec![]).as_str().unwrap().parse().unwrap() +} + +/// `getblocktxn` for an index past the end of `block`: Core's +/// "getblocktxn with out-of-bounds tx indices", `Misbehaving`. +fn out_of_range_getblocktxn(block: BlockHash) -> NetworkMessage { + NetworkMessage::GetBlockTxn(bitcoin::p2p::message_compact_blocks::GetBlockTxn { + txs_request: bitcoin::bip152::BlockTransactionsRequest { block_hash: block, indexes: vec![5] }, + }) +} + +fn banned(node: &TestNode) -> Vec { + node.rpc_ok("listbanned", vec![]).as_array().unwrap().clone() +} + +/// `getpeerinfo` entries for peers that reached the node on `port`. +fn peers_on(node: &TestNode, port: u16) -> Vec { + let suffix = format!(":{port}"); + node.rpc_ok("getpeerinfo", vec![]) + .as_array() + .unwrap() + .iter() + .filter(|p| p["addrbind"].as_str().is_some_and(|b| b.ends_with(&suffix))) + .cloned() + .collect() +} + +/// Core disconnects a misbehaving peer on a local address without punishing +/// the address, since that would punish every peer on it. Every inbound +/// onion peer arrives from the address Tor dialled from, 127.0.0.1 here, as +/// does every local integration. satd banned 127.0.0.1 for `-bantime`, so one +/// such peer shut every other one out until the ban expired, across restarts. +/// +/// Each listener in turn: the peer misbehaves and is disconnected, nothing +/// is banned, and a second peer on the same listener is let in and served. +#[test] +fn a_misbehaving_onion_or_local_peer_is_disconnected_without_a_ban() { + let p2p = find_available_port(); + let onion = find_available_port(); + let mut node = TestNode::start(&[ + &format!("--port={p2p}"), + &format!("--bind=127.0.0.1:{p2p}"), + &format!("--bind=127.0.0.1:{onion}=onion"), + ]); + node.mine_blocks(1, ADDR); + let tip = best_hash(&node); + + for (listener, port, network) in [("onion", onion, "onion"), ("clearnet", p2p, "not_publicly_routable")] { + let mut peer = Peer::connect(port).unwrap_or_else(|e| panic!("{listener}: first peer: {e}")); + poll_until(|| peers_on(&node, port).len() == 1, test_timeout(20), "the peer is listed"); + // Core's `ConnectedThroughNetwork`: an inbound onion peer is on the + // onion network whatever its socket address. + assert_eq!(peers_on(&node, port)[0]["network"], json!(network), "{listener}: getpeerinfo.network"); + + peer.send(out_of_range_getblocktxn(tip)); + assert!(peer.closed_within(test_timeout(30)), "{listener}: the misbehaving peer is disconnected"); + assert_eq!(banned(&node), Vec::::new(), "{listener}: nothing is banned"); + + let mut next = Peer::connect(port) + .unwrap_or_else(|e| panic!("{listener}: a second peer on the same listener is refused: {e}")); + assert!(next.served(tip), "{listener}: the second peer is served"); + } + node.stop(); +} + +/// Core never punishes a manual connection (`addnode`, `-addnode`, +/// `-connect`): it stays connected and its address is not punished. satd +/// banned it like any other peer, and the redial loop then skipped the +/// banned address, so a `-connect` node could be left with no peers. +#[test] +fn a_misbehaving_manual_peer_stays_connected() { + let mut node = TestNode::start(&["--v2transport=0"]); + node.mine_blocks(1, ADDR); + let tip = best_hash(&node); + + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let target = listener.local_addr().unwrap().to_string(); + node.rpc_ok("addnode", vec![json!(target), json!("onetry")]); + let mut peer = Peer::accept(&listener); + poll_until( + || { + node.rpc_ok("getpeerinfo", vec![]) + .as_array() + .unwrap() + .iter() + .any(|p| p["addr"] == json!(target) && p["connection_type"] == json!("manual")) + }, + test_timeout(20), + "the manual peer is listed", + ); + + peer.send(out_of_range_getblocktxn(tip)); + assert!(peer.served(tip), "the manual peer is still connected and served"); + assert_eq!(banned(&node), Vec::::new(), "nothing is banned"); + node.stop(); +} From 02bdb2da1b9b40eaeb94bb121f0544e6893dd070 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Wed, 7 Oct 2026 21:56:36 -0600 Subject: [PATCH 04/13] p2p: keep a getdata cmpctblock entry that could not be queued serve_compact_block reported a MSG_CMPCT_BLOCK entry as answered even when its cmpctblock could not be queued, so the entry left the getdata backlog with nothing sent. It now says whether the message was queued, and an entry that was not stays first, as every other entry does. Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/net/manager.rs | 41 +++++++++++++------------ node/src/net/manager_p2pbounds_tests.rs | 27 ++++++++++++++++ 2 files changed, 48 insertions(+), 20 deletions(-) diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index 1da8eeed9..6eb4b0291 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -8076,38 +8076,32 @@ impl PeerManager { /// Answer a `MSG_CMPCT_BLOCK` getdata with a `cmpctblock`, if the block /// is within [`compact::MAX_CMPCTBLOCK_DEPTH`] of the tip and we are not /// syncing (Core: `can_direct_fetch && pindex->nHeight >= tip->nHeight - - /// MAX_CMPCTBLOCK_DEPTH`). Returns false when the full block should be - /// sent instead. - fn serve_compact_block(&self, id: PeerId, hash: &bitcoin::BlockHash) -> bool { + /// MAX_CMPCTBLOCK_DEPTH`). `None` when the full block should be sent + /// instead; otherwise whether the `cmpctblock` was queued. + fn serve_compact_block(&self, id: PeerId, hash: &bitcoin::BlockHash) -> Option { if self.ibd.read().is_some() || self.is_ibd() { - return false; + return None; } - let Some(entry) = self.chain_state.get_block_index(hash) else { - return false; - }; + let entry = self.chain_state.get_block_index(hash)?; if entry.height.saturating_add(compact::MAX_CMPCTBLOCK_DEPTH) < self.chain_state.tip_height() { - return false; + return None; } let compact = match self.cached_compact(hash) { Some(c) => c, None => { - let Some(block) = self.chain_state.get_block(hash) else { - return false; - }; - match self.compact_for(&block, entry.height, false) { - Some(c) => c, - None => return false, - } + let block = self.chain_state.get_block(hash)?; + self.compact_for(&block, entry.height, false)? } }; let msg = NetworkMessage::CmpctBlock(bitcoin::p2p::message_compact_blocks::CmpctBlock { compact_block: (*compact).clone(), }); - if self.send_to_peer(id, msg) { + let queued = self.send_to_peer(id, msg); + if queued { self.compact_stats.sent_getdata.fetch_add(1, Ordering::Relaxed); self.note_peer_has_block(id, *hash); } - true + Some(queued) } fn handle_getdata(&self, id: PeerId, inventory: Vec) { @@ -8207,8 +8201,12 @@ impl PeerManager { Inventory::Block(hash) | Inventory::WitnessBlock(hash) | Inventory::CompactBlock(hash) => { - if matches!(inv, Inventory::CompactBlock(_)) && self.serve_compact_block(id, &hash) { - return true; + // A `cmpctblock` that could not be queued leaves the entry to + // be served again once the queue drains, as below. + if matches!(inv, Inventory::CompactBlock(_)) + && let Some(queued) = self.serve_compact_block(id, &hash) + { + return queued; } let Some(block) = self.chain_state.get_block(&hash) else { not_found.push(inv); @@ -10272,7 +10270,10 @@ impl PeerManager { /// queued to it stayed. Core drops a peer that has taken no bytes for /// `TIMEOUT_INTERVAL` (`InactivityCheck`, "socket sending timeout"). /// Progress is the bytes the socket takes, counted as it takes them, so a - /// slow reader is not mistaken for a stalled one. + /// slow reader is not mistaken for a stalled one. Without counters, which + /// only a loop whose peer handle was already gone when it started lacks, + /// progress cannot be seen and the deadline runs from the start of the + /// write. async fn send_watched>( id: PeerId, writer: &mut ConnectionWriter, diff --git a/node/src/net/manager_p2pbounds_tests.rs b/node/src/net/manager_p2pbounds_tests.rs index 73586a9f6..4094d5630 100644 --- a/node/src/net/manager_p2pbounds_tests.rs +++ b/node/src/net/manager_p2pbounds_tests.rs @@ -278,6 +278,33 @@ fn getdata_resumes_as_the_queue_drains() { assert_eq!(pm.upload_bytes.load(Ordering::Relaxed) - charged_before, charged as u64); } +/// A `MSG_CMPCT_BLOCK` entry whose `cmpctblock` could not be queued stays +/// first in the backlog, like every other entry, to be served once the +/// queue drains. It was popped as answered with nothing sent. +#[test] +fn a_compact_block_entry_that_cannot_be_queued_is_kept() { + let (pm, _dir) = mk_pm(); + let chain = mine_big(&pm, 3, 10); + let tip = *chain.last().unwrap(); + let mut rx = add_peer(&pm, 1, addr(1), 1); + let sender = pm.peer_sender(1).unwrap(); + sender.try_send(NetworkMessage::Verack).unwrap(); + + let mut not_found = Vec::new(); + assert!( + !pm.serve_getdata_entry(1, &sender, Inventory::CompactBlock(tip), &mut not_found), + "nothing could be queued, so the entry is not answered" + ); + assert!(not_found.is_empty()); + + assert_eq!(rx.try_recv().unwrap(), NetworkMessage::Verack); + assert!(pm.serve_getdata_entry(1, &sender, Inventory::CompactBlock(tip), &mut not_found)); + match rx.try_recv() { + Ok(NetworkMessage::CmpctBlock(c)) => assert_eq!(c.compact_block.header.block_hash(), tip), + other => panic!("expected the cmpctblock, got {other:?}"), + } +} + // ---- Peer text ---- /// Core keeps a peer's user agent only after `SanitizeString` (`cleanSubVer`) From d41ad6e81ce65ee2de00e935d5469390da4bd596 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Thu, 8 Oct 2026 06:24:37 -0600 Subject: [PATCH 05/13] p2p: tidy the MisbehaviourAction docs Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/net/manager.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index 9b07fce4f..5e1356283 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -52,9 +52,10 @@ enum MisbehaviourAction { Keep(&'static str), /// Disconnected; its address is not banned. Disconnect, - /// Disconnected, and this banned for `-bantime`. + /// Disconnected, and the address it names banned for `-bantime`. Ban(crate::net::ban::BanTarget), } + /// Keepalive cadence: how often each peer is sent a `ping` when none is /// outstanding. Bitcoin Core's `PING_INTERVAL` (net_processing.h). /// From 7c8b6ed558368caf3f9682cabed779ece1a99930 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Thu, 8 Oct 2026 07:23:16 -0600 Subject: [PATCH 06/13] p2p: disconnect an inbound onion peer without a ban wherever Tor dials from Core decides the disconnect-only rung on the address alone (pnode.addr.IsLocal(), net_processing.cpp), and Tor on the same host dials from loopback. With Tor on another host (-bind==onion) every onion peer arrives from that host's address. Core discourages it, and still admits it while inbound slots are free (net.cpp); satd's ban would refuse every onion peer for -bantime. An inbound onion peer is now disconnected without a ban whatever address it arrives from. Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/net/manager.rs | 13 +++++++++++-- node/src/net/manager_peerpunish_tests.rs | 8 ++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index 5e1356283..52c7a3f4d 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -3749,7 +3749,9 @@ impl PeerManager { /// - a peer on a local address is disconnected, but its address is not /// punished, "since that would discourage all peers on the same local /// address". Every inbound onion peer arrives from the address Tor - /// dialled from, usually loopback, as does every local integration; + /// dialled from, usually loopback, as does every local integration. + /// satd treats an inbound onion peer this way whatever that address + /// is (see below); /// - anyone else is disconnected and its address punished. Core /// discourages it; satd bans it for `-bantime`. /// @@ -3771,7 +3773,14 @@ impl PeerManager { if let Some(host) = &info.onion_host { return MisbehaviourAction::Ban(crate::net::ban::BanTarget::Onion(host.clone())); } - if crate::net::is_local(info.addr.ip()) { + // Core tests the address alone (`pnode.addr.IsLocal()`), and names + // an inbound onion peer only in its log line: Tor on this host dials + // from loopback. With Tor on another host (`-bind==onion`) + // every onion peer arrives from that host's address. Core discourages + // it, and still admits it while inbound slots are free; satd's ban + // would refuse every onion peer for `-bantime`. So an inbound onion + // peer is disconnected without a ban wherever Tor dials from. + if crate::net::is_local(info.addr.ip()) || info.inbound_onion { return MisbehaviourAction::Disconnect; } MisbehaviourAction::Ban(crate::net::ban::BanTarget::Net(ipnet::IpNet::from(info.addr.ip()))) diff --git a/node/src/net/manager_peerpunish_tests.rs b/node/src/net/manager_peerpunish_tests.rs index df0ce2edc..265daa2f7 100644 --- a/node/src/net/manager_peerpunish_tests.rs +++ b/node/src/net/manager_peerpunish_tests.rs @@ -124,6 +124,14 @@ fn misbehaviour_follows_cores_ladder() { Outcome::Disconnected, ), ("an inbound onion peer", onion(peer(1, "127.0.0.1:50000", Direction::Inbound)), Outcome::Disconnected), + // Tor on another host (`-bind=:=onion`): the + // address is the Tor host's, shared by every onion peer, so it is not + // banned either. See `misbehaviour_action`. + ( + "an inbound onion peer forwarded from another host", + onion(peer(1, "192.168.1.20:50000", Direction::Inbound)), + Outcome::Disconnected, + ), ("an outbound peer on loopback", peer(1, "127.0.0.1:8333", Direction::Outbound), Outcome::Disconnected), ]; for (case, info, expected) in cases { From aec3bebcf2c5c20abf7284f4f742d58c9c1173c4 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Thu, 8 Oct 2026 11:12:47 -0600 Subject: [PATCH 07/13] p2p: bound learned addresses and outbound dials; manual peers outside the automatic cap - Core's address rate limit (ProcessAddrs): a token bucket per peer, one token to start, 0.1 a second up to 1000, plus 1000 when the node sends getaddr; entries are shuffled first and the addr permission is exempt. getpeerinfo reports addr_processed and addr_rate_limited. - A gossiped address joins the dial list only if the address book holds it. The list is a set in arrival order, capped at the book's size. - reserve_dial checks a dial's limit and takes its place under one lock, counting dials in flight: automatic dials against the automatic target, added nodes at most 8 at a time (Core's semAddnode), -connect, -seednode and onetry without a limit. - The reconnect tick redials manual peers whatever the automatic count, then fills the free automatic slots. Automatic dials skip banned addresses, onion hosts included; manual dials do not (Core's OpenNetworkConnection). Gossip naming a banned onion host is not kept. - Manual backoff stops at 60 s; backoff is cleared only by a dial that connects, not by a new tip. - addnode onetry is outside every limit and succeeds for a peer already connected or being dialled. Co-Authored-By: Claude Opus 5.5 --- docs/manual/src/config-reference.md | 6 +- node/src/net/addr_rate.rs | 142 ++++ node/src/net/addrman.rs | 7 +- node/src/net/manager.rs | 821 +++++++++++++++++------- node/src/net/manager_addrdials_tests.rs | 573 +++++++++++++++++ node/src/net/mod.rs | 1 + node/src/net/peer.rs | 8 +- node/src/rpc/server.rs | 8 +- satd/tests/p2p_addr_dials.rs | 308 +++++++++ satd/tests/regtest.rs | 6 +- 10 files changed, 1654 insertions(+), 226 deletions(-) create mode 100644 node/src/net/addr_rate.rs create mode 100644 node/src/net/manager_addrdials_tests.rs create mode 100644 satd/tests/p2p_addr_dials.rs diff --git a/docs/manual/src/config-reference.md b/docs/manual/src/config-reference.md index a01521beb..3a67307dc 100644 --- a/docs/manual/src/config-reference.md +++ b/docs/manual/src/config-reference.md @@ -184,7 +184,7 @@ startup error. | `v2transport` | true | hot | core | Offer/accept BIP 324 v2 encrypted transport (Core default since v26). | | `v2only` | false | hot | satd | Refuse peers that do not speak BIP 324 v2 (privacy hardening). | | `externalip` | none | hot | core | External address to advertise to peers (repeatable). | -| `whitelist` | none | hot | core | Grant net permissions to peers by source subnet (repeatable). `[@]`. **Inbound only unless the permission list carries `out`**, as in Core; an `out` entry applies only to *manual* outbound connections (`connect`, `addnode`, and `seednode`, which satd keeps as a manual connection), never to automatic ones such as the peers the node finds through gossip, `peers.dat` or a DNS seed. `@` with an empty list matches the range and grants nothing. A peer arriving over the Tor hidden service is never matched against this — it reaches the node on a loopback socket Tor forwards to, so matching would grant an anonymous remote peer whatever you granted your own machine. | +| `whitelist` | none | hot | core | Grant net permissions to peers by source subnet (repeatable). `[@]`. **Inbound only unless the permission list carries `out`**, as in Core; an `out` entry applies only to *manual* outbound connections (`connect`, `addnode`, and `seednode`, which satd keeps as a manual connection), never to automatic ones such as the peers the node finds through gossip, `peers.dat` or a DNS seed. `@` with an empty list matches the range and grants nothing. A peer arriving over the Tor hidden service is never matched against this — it reaches the node on a loopback socket Tor forwards to, so matching would grant an anonymous remote peer whatever you granted your own machine. The `addr` permission exempts a peer from Core's address rate limit: otherwise the node takes one address from a peer's `addr`/`addrv2` messages to start and one more every 10 seconds (a thousand more after it sends the peer `getaddr`), and `getpeerinfo` reports the rest as `addr_rate_limited`. | | `whitelistrelay` | on | hot | core | Grant `relay` to whitelisted peers with default permissions (relay their txes even under `-blocksonly`). Entries with an explicit `perms@` prefix are unaffected. | | `whitelistforcerelay` | off | hot | core | Grant `forcerelay` to whitelisted peers with default permissions. Entries with an explicit `perms@` prefix are unaffected. | | `whitebind` | none | restart | core | Bind an extra permissioned P2P listener (repeatable). | @@ -192,10 +192,10 @@ startup error. | `port` | network default | restart | core | P2P listen port. | | `bind` | `0.0.0.0` | restart | core | Bind P2P to this address; **repeatable**. Accepts `addr`, `addr:port`, and `addr[:port]=onion`. An entry with a port uses it; a bare address takes `port`, and a bare `=onion` entry takes `port` + 1 (as in Core). IPv6 literals may be plain (`::1`) or bracketed. Any explicit `bind` replaces the default listener. Cannot be combined with `listen=0`. | | `connect` | none | hot | core | Connect only to specific peer(s) (repeatable). An entry with no port takes the network's default P2P port. Any `connect` stops the node dialling addresses it learns from gossip and soft-sets `listen=0` (see `listen`); `connect=0` — and the command-line negation `-noconnect` — is Core's spelling for "open no outbound connections at all"; a literal `connect=0` mixed with a peer address is refused, since Core keeps the peer and satd will not dial the `0`. Connect-only *exclusivity* is a startup decision (restart to change). | -| `addnode` | none | hot | core | Add a node to connect to (does not disable DNS seeding, and does not affect `listen`). An entry with no port takes the network's default P2P port. | +| `addnode` | none | hot | core | Add a node to connect to (does not disable DNS seeding, and does not affect `listen`). An entry with no port takes the network's default P2P port. As in Core, added nodes are counted apart from the automatic outbound peers and `maxconnections`: at most 8 are connected at a time, and one that is not connected is dialled again at most a minute later, however many automatic peers the node has. A ban does not stop the dial; it does stop automatic dials to that address. | | `uacomment` | none | restart | core | Append a comment to the advertised user agent (**repeatable**; command-line and config-file values accumulate, command line first). Renders as `/satd:(c1; c2)/`. A comment may contain only alphanumerics and `` .,;-_?@`` — the user agent's own delimiters `/`, `:`, `(` and `)` are refused — and the whole user agent may not exceed 256 bytes. Either violation is a startup error, as in Core. | | `seednode` | none | hot | core | One-shot seed peer connected at startup to bootstrap discovery. | -| `maxconnections` | 125 | hot | core | Maximum total connections. `0` (or any value ≤ 0) soft-sets `listen=0`, as in Core — see `listen`. That half is a startup decision: changing `maxconnections` over SIGHUP applies the new cap but does not start or stop the listener. | +| `maxconnections` | 125 | hot | core | Maximum total connections. Manual connections (`connect`, `addnode`, `seednode`, `addnode onetry`) do not count against the automatic outbound peers, as in Core. `0` (or any value ≤ 0) soft-sets `listen=0`, as in Core — see `listen`. That half is a startup decision: changing `maxconnections` over SIGHUP applies the new cap but does not start or stop the listener. | | `maxinboundperip` | 3 | hot | satd | Max simultaneous inbound peers from one source IP (Core-style flood guard; no Core flag). | | `cmpctblockprefill` | false | restart | satd | Announce a new block as a `cmpctblock` with the transactions this node lacked when it arrived prefilled, so a peer that lacks them too rebuilds the block without a `getblocktxn` round trip. Bitcoin Core's proposed design (#35558, not yet merged in Core); off until measurement says otherwise. | | `cmpctblockprefillbytes` | 8192 | restart | satd | Transaction bytes a `cmpctblockprefill` announcement may carry beyond the coinbase. Transactions nobody relayed to this node go first, then replaced or policy-refused ones; one that does not fit is skipped. `0` prefills nothing. | diff --git a/node/src/net/addr_rate.rs b/node/src/net/addr_rate.rs new file mode 100644 index 000000000..28d468115 --- /dev/null +++ b/node/src/net/addr_rate.rs @@ -0,0 +1,142 @@ +//! Bitcoin Core's rate limit on the addresses a peer announces +//! (`net_processing.cpp` `ProcessAddrs`). +//! +//! Every peer has a token bucket. It starts with one token and gains +//! [`MAX_ADDR_RATE_PER_SECOND`] tokens a second, up to +//! [`MAX_ADDR_PROCESSING_TOKEN_BUCKET`]. Each address taken from the peer's +//! `addr` and `addrv2` messages spends one token; an address that finds the +//! bucket empty is dropped, unless the peer has the `addr` permission. +//! Sending the peer a `getaddr` adds [`MAX_ADDR_TO_SEND`] tokens on top, +//! past that limit, so its answer is taken whole. + +use crate::net::limits::MAX_ADDR_TO_SEND; + +/// Core's `MAX_ADDR_RATE_PER_SECOND`: tokens gained per second. +pub const MAX_ADDR_RATE_PER_SECOND: f64 = 0.1; + +/// Core's `MAX_ADDR_PROCESSING_TOKEN_BUCKET`: the bucket refills no +/// further than this. +pub const MAX_ADDR_PROCESSING_TOKEN_BUCKET: f64 = MAX_ADDR_TO_SEND as f64; + +/// One peer's address token bucket and its counters: Core's +/// `Peer::m_addr_token_bucket`, `m_addr_token_timestamp`, +/// `m_addr_processed` and `m_addr_rate_limited`. +#[derive(Debug, Clone)] +pub struct AddrTokenBucket { + tokens: f64, + /// Node time of the last refill, in microseconds + /// (`crate::time::now_micros`). + refilled_at_micros: u64, + /// Addresses from this peer that passed the limit and were looked at + /// (`getpeerinfo.addr_processed`). + pub processed: u64, + /// Addresses from this peer dropped by the limit + /// (`getpeerinfo.addr_rate_limited`). + pub rate_limited: u64, +} + +impl AddrTokenBucket { + /// A new peer's bucket: one token, as in Core. + pub fn new(now_micros: u64) -> Self { + Self { + tokens: 1.0, + refilled_at_micros: now_micros, + processed: 0, + rate_limited: 0, + } + } + + /// Add the tokens earned since the last refill, unless the bucket is + /// already at [`MAX_ADDR_PROCESSING_TOKEN_BUCKET`], and restart the clock + /// either way. Core does this once per message, before taking any of it. + /// A clock that went backwards earns nothing. + pub fn refill(&mut self, now_micros: u64) { + if self.tokens < MAX_ADDR_PROCESSING_TOKEN_BUCKET { + let elapsed_secs = now_micros.saturating_sub(self.refilled_at_micros) as f64 / 1_000_000.0; + self.tokens = (self.tokens + elapsed_secs * MAX_ADDR_RATE_PER_SECOND) + .min(MAX_ADDR_PROCESSING_TOKEN_BUCKET); + } + self.refilled_at_micros = now_micros; + } + + /// Spend a token on one address. Without one the address is dropped + /// and counted as rate limited, unless `exempt` (the `addr` + /// permission), in which case it is taken for free. + pub fn admit(&mut self, exempt: bool) -> bool { + if self.tokens < 1.0 { + if !exempt { + self.rate_limited += 1; + return false; + } + } else { + self.tokens -= 1.0; + } + true + } + + /// A `getaddr` was sent to the peer: room for its answer, past the + /// refill limit. + pub fn getaddr_sent(&mut self) { + self.tokens += MAX_ADDR_TO_SEND as f64; + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const SEC: u64 = 1_000_000; + + fn take(bucket: &mut AddrTokenBucket, n: usize, exempt: bool) -> usize { + (0..n).filter(|_| bucket.admit(exempt)).count() + } + + #[test] + fn a_new_peer_gets_one_address_then_one_every_ten_seconds() { + let mut b = AddrTokenBucket::new(0); + b.refill(0); + assert_eq!(take(&mut b, 1000, false), 1); + assert_eq!(b.rate_limited, 999); + + b.refill(100 * SEC); + assert_eq!(take(&mut b, 1000, false), 10, "0.1 a second"); + + b.refill(105 * SEC); + assert_eq!(take(&mut b, 1, false), 0, "half a token is not one"); + b.refill(110 * SEC); + assert_eq!(take(&mut b, 1, false), 1, "the half carried over"); + } + + #[test] + fn the_bucket_refills_to_a_thousand_and_no_further() { + let mut b = AddrTokenBucket::new(0); + b.refill(1_000_000 * SEC); + assert_eq!(take(&mut b, 2000, false), 1000); + } + + #[test] + fn a_getaddr_makes_room_for_its_answer_past_the_limit() { + let mut b = AddrTokenBucket::new(0); + b.refill(1_000_000 * SEC); + b.getaddr_sent(); + assert_eq!(take(&mut b, 3000, false), 2000); + } + + #[test] + fn the_addr_permission_takes_every_address() { + let mut b = AddrTokenBucket::new(0); + b.refill(0); + assert_eq!(take(&mut b, 1000, true), 1000); + assert_eq!(b.rate_limited, 0); + } + + #[test] + fn a_clock_that_goes_backwards_earns_nothing() { + let mut b = AddrTokenBucket::new(100 * SEC); + b.refill(0); + assert_eq!(take(&mut b, 10, false), 1, "only the first token"); + // And the clock restarted from the earlier time, as Core's does. + b.refill(10 * SEC); + assert_eq!(take(&mut b, 10, false), 1); + } +} diff --git a/node/src/net/addrman.rs b/node/src/net/addrman.rs index 6e62d618c..27f9eaec6 100644 --- a/node/src/net/addrman.rs +++ b/node/src/net/addrman.rs @@ -38,7 +38,7 @@ use std::path::Path; /// Hard cap on total stored addresses (new + tried). Keeps `peers.dat` /// and memory bounded; well above what a single node needs to bootstrap. -const MAX_ENTRIES: usize = 16_384; +pub const MAX_ENTRIES: usize = 16_384; /// Per-network-group cap in the *new* table (eclipse resistance: stop one /// `/16`/ASN from flooding the book with addresses). const MAX_NEW_PER_GROUP: usize = 64; @@ -317,6 +317,11 @@ impl AddrMan { entry.attempts = 0; } + /// Whether the book holds `addr`, in either table. + pub fn contains(&self, addr: &SocketAddr) -> bool { + self.entries.contains_key(addr) + } + /// Record a failed connection attempt. pub fn mark_attempt(&mut self, addr: SocketAddr) { if let Some(e) = self.entries.get_mut(&addr) { diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index 5e1356283..10529d25e 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -97,6 +97,23 @@ const MAX_ONION_DIALS_PER_TICK: usize = 16; /// attempts once a minute (`ThreadOpenAddedConnections`). const MANUAL_TARGET_RELOOKUP: Duration = Duration::from_secs(60); +/// Core's `MAX_ADDNODE_CONNECTIONS` (`net.h`): added nodes (`-addnode`, +/// `addnode add`) connected or being connected at once. Counted apart from +/// the automatic outbound target (`semAddnode`). +const MAX_ADDNODE_CONNECTIONS: usize = 8; + +/// The longest a peer the operator named waits between two dials. Core's +/// `ThreadOpenAddedConnections` tries its added nodes again every 60 +/// seconds; automatic candidates back off to 300 seconds. +const MANUAL_RECONNECT_MAX_BACKOFF: Duration = Duration::from_secs(60); + +/// The longest an automatic dial candidate waits between two dials. +const AUTOMATIC_RECONNECT_MAX_BACKOFF: Duration = Duration::from_secs(300); + +/// Cap on the automatic dial list. Only an address the address book holds +/// gets on it (seeds aside), so this is the book's own size. +const MAX_LEARNED_ADDRS: usize = crate::net::addrman::MAX_ENTRIES; + /// A peer the operator named (`-addnode`, `addnode add`, or a `-connect` /// host name), with the address it currently resolves to. #[derive(Debug, Clone)] @@ -394,15 +411,17 @@ impl ReconnectState { } } - /// Backoff delay: 10s, 20s, 40s, 80s, 160s, capped at 300s. - fn backoff_duration(&self) -> Duration { + /// Backoff delay: 10s, 20s, 40s, 80s, 160s, capped at `max` + /// ([`MANUAL_RECONNECT_MAX_BACKOFF`] or + /// [`AUTOMATIC_RECONNECT_MAX_BACKOFF`]). + fn backoff_duration(&self, max: Duration) -> Duration { let secs = 10u64.saturating_mul(1u64 << self.attempts.min(5)); - Duration::from_secs(secs.min(300)) + Duration::from_secs(secs).min(max) } - fn record_failure(&mut self) { + fn record_failure(&mut self, max: Duration) { self.attempts = self.attempts.saturating_add(1); - self.next_attempt = Instant::now() + self.backoff_duration(); + self.next_attempt = Instant::now() + self.backoff_duration(max); } fn reset(&mut self) { @@ -411,6 +430,82 @@ impl ReconnectState { } } +/// The addresses the node learned for itself, as the reconnect loop dials +/// them: in the order they arrived, each once, at most +/// [`MAX_LEARNED_ADDRS`]. This was a plain `Vec` searched end to end on +/// every insert, with no cap. +#[derive(Debug, Default)] +struct LearnedAddrs { + order: Vec, + members: HashSet, +} + +impl LearnedAddrs { + /// Append `addr`. False, changing nothing, if it is already on the + /// list or the list is full. + fn push(&mut self, addr: SocketAddr) -> bool { + if self.members.len() >= MAX_LEARNED_ADDRS || !self.members.insert(addr) { + return false; + } + self.order.push(addr); + true + } + + #[cfg(test)] + fn contains(&self, addr: &SocketAddr) -> bool { + self.members.contains(addr) + } + + fn iter(&self) -> impl Iterator { + self.order.iter() + } +} + +/// The limit an outbound dial counts against, from the moment it passes the +/// capacity check until its peer is in the peer map, where it is counted +/// instead. Core's counting semaphores: a grant is taken before the dial. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum DialBudget { + /// The node's own connections: the reconnect loop's and the seeds'. + /// Together with the automatic peers already connected, at most the + /// automatic outbound target (8, or 64 during IBD). Core's + /// `semOutbound`. + Automatic, + /// An added node (`-addnode`, `addnode add`): at most + /// [`MAX_ADDNODE_CONNECTIONS`] at once, apart from the automatic target. + /// Core's `semAddnode`. + AddedNode, + /// No limit: the other manual connections (`-connect`, `-seednode`), + /// which Core opens without a grant. + Unlimited, +} + +/// Outbound dials in flight, per [`DialBudget`]. +#[derive(Debug, Default)] +struct DialsInFlight { + automatic: usize, + added_node: usize, +} + +/// A dial's place in its [`DialBudget`], given back when dropped. Owned, so +/// a reservation taken in the reconnect loop can travel with the dial it +/// spawns. +struct DialSlot { + pm: Arc, + budget: DialBudget, +} + +impl Drop for DialSlot { + fn drop(&mut self) { + let mut in_flight = self.pm.dials_in_flight.lock(); + match self.budget { + DialBudget::Automatic => in_flight.automatic = in_flight.automatic.saturating_sub(1), + DialBudget::AddedNode => in_flight.added_node = in_flight.added_node.saturating_sub(1), + DialBudget::Unlimited => {} + } + } +} + /// Event sent from peer tasks to the central manager loop. pub enum NetEvent { PeerConnected { @@ -532,6 +627,10 @@ mod p2pbounds_tests; #[path = "manager_peerpunish_tests.rs"] mod peerpunish_tests; +#[cfg(test)] +#[path = "manager_addrdials_tests.rs"] +mod addrdials_tests; + /// How a transaction is announced to a peer: `MSG_WTX` carrying the wtxid /// to a peer that negotiated BIP 339 wtxid relay, the txid to the rest. fn tx_announcement(wtxid_relay: bool, txid: bitcoin::Txid, wtxid: bitcoin::Wtxid) -> Inventory { @@ -768,13 +867,13 @@ pub struct PeerManager { /// Addresses the node learned for itself: `addr`/`addrv2` gossip, the /// address book at startup, DNS seeds. The reconnect loop dials them only /// while `automatic_outbound` is on, as `outbound-full-relay` - /// connections. Only addresses the address book would take get here - /// (`AddrMan::admits`). + /// connections. A gossiped address gets here only if the address book + /// holds it, a seed only if the book would take it (`AddrMan::admits`). /// /// Kept apart from `manual_addrs`. This list used to hold both, and /// `spawn_peer` called any peer on it `manual`, so every peer reached /// through gossip was reported, whitelisted and counted as one (#866). - learned_addrs: RwLock>, + learned_addrs: RwLock, /// Bitcoin Core's `m_use_addrman_outgoing`. False under `-connect`. automatic_outbound: std::sync::atomic::AtomicBool, /// The clearnet peers the operator named: `-connect`, `-addnode` / @@ -925,6 +1024,10 @@ pub struct PeerManager { /// onion equivalent, stopping the reconnect loop from opening a second /// connection to an onion peer it's already dialing/connected to. pending_onion_dials: RwLock>, + /// Outbound dials in flight, per [`DialBudget`]: counted against the + /// automatic outbound target and the added-node limit along with the + /// peers already connected. See [`Self::reserve_dial`]. + dials_in_flight: parking_lot::Mutex, /// Ban duration in seconds (default: 86400). Atomic for live SIGHUP /// reload (applies to bans created after the change). ban_duration_secs: AtomicU64, @@ -1148,7 +1251,7 @@ impl PeerManager { compact_in_progress: RwLock::new(HashMap::new()), block_refetch: RwLock::new(HashMap::new()), unreadable_refetch_at: parking_lot::Mutex::new(HashMap::new()), - learned_addrs: RwLock::new(Vec::new()), + learned_addrs: RwLock::new(LearnedAddrs::default()), automatic_outbound: std::sync::atomic::AtomicBool::new(true), dns_enabled: std::sync::atomic::AtomicBool::new(true), manual_addrs: RwLock::new(HashSet::new()), @@ -1192,6 +1295,7 @@ impl PeerManager { max_inbound_per_ip: AtomicUsize::new(max_inbound_per_ip), pending_connections: RwLock::new(HashSet::new()), pending_onion_dials: RwLock::new(HashSet::new()), + dials_in_flight: parking_lot::Mutex::new(DialsInFlight::default()), ban_duration_secs: AtomicU64::new(ban_duration_secs), connect_timeout_ms: AtomicU64::new(DEFAULT_CONNECT_TIMEOUT_MS), peer_connect_timeout_secs: AtomicU64::new(DEFAULT_PEER_CONNECT_TIMEOUT_SECS as u64), @@ -1820,13 +1924,22 @@ impl PeerManager { // Linux dials the local host, or, off regtest, one that is not // publicly routable. Core refuses both in `AddrManImpl::AddSingle` // (v31.1 `src/addrman.cpp:554`) and dials only from its book. + // + // Nor does an address the book admits but has no room for (its + // per-group or total cap). Pushing that too put every gossiped + // address on the dial list, past the bounds the book keeps. An + // address the book already holds is pushed again; the list holds + // each address once. { let mut book = self.addrman.write(); if !book.admits(&addr) { tracing::debug!(%addr, "Ignoring a gossiped address the address book does not take"); return; } - book.add_from(addr, now_unix_secs(), services, source); + if !book.add_from(addr, now_unix_secs(), services, source) && !book.contains(&addr) { + tracing::debug!(%addr, "Not dialling a gossiped address the address book has no room for"); + return; + } } self.push_learned(addr); } @@ -1860,10 +1973,7 @@ impl PeerManager { if !self.automatic_outbound.load(Ordering::Relaxed) { return; } - let mut addrs = self.learned_addrs.write(); - if !addrs.contains(&addr) { - addrs.push(addr); - } + self.learned_addrs.write().push(addr); } /// Bitcoin Core's `m_use_addrman_outgoing`: whether the node may dial @@ -1907,6 +2017,11 @@ impl PeerManager { if self.is_onion_connected(&host) { return; } + // Core looks no further at a banned address it is told about, as the + // clearnet arms do; the reconnect loop would not dial it either. + if self.ban_list.read().is_onion_banned(&host, crate::time::now_secs()) { + return; + } let mut addrs = self.connect_peer_addrs.write(); let already = addrs .iter() @@ -1933,9 +2048,7 @@ impl PeerManager { // not been applied yet, and `may_dial` holds these back under it. let mut addrs = self.learned_addrs.write(); for a in picks { - if !addrs.contains(&a) { - addrs.push(a); - } + addrs.push(a); } tracing::info!(loaded = self.addrman.read().len(), "Loaded persistent address book"); } @@ -2396,6 +2509,50 @@ impl PeerManager { .count() } + /// Core's address rate limit (`ProcessAddrs`; see + /// [`crate::net::addr_rate`]): refill the peer's token bucket, then keep + /// the entries of its `addr` / `addrv2` that find a token, taken in a + /// random order so the sender does not choose which ones get through. A + /// peer with the `addr` permission keeps them all. + /// + /// satd took every entry of every message: any peer could put up to a + /// thousand addresses per message into the address book and onto the + /// dial list, as often as it liked. + fn rate_limit_addrs(&self, id: PeerId, mut addrs: Vec) -> Vec { + use rand::seq::SliceRandom; + addrs.shuffle(&mut rand::thread_rng()); + let mut peers = self.peers.write(); + let Some(handle) = peers.get_mut(&id) else { + return Vec::new(); + }; + let exempt = handle.info.permissions.addr; + let bucket = &mut handle.info.addr_rate; + bucket.refill(crate::time::now_micros()); + let before = bucket.rate_limited; + addrs.retain(|_| bucket.admit(exempt)); + let limited = bucket.rate_limited - before; + if limited > 0 { + tracing::debug!("Rate-limited {limited} addresses from peer={id}"); + } + addrs + } + + /// Count `n` addresses from peer `id` as processed + /// (`getpeerinfo.addr_processed`). + fn note_addrs_processed(&self, id: PeerId, n: u64) { + if let Some(handle) = self.peers.write().get_mut(&id) { + handle.info.addr_rate.processed += n; + } + } + + /// Note a `getaddr` sent to peer `id`: its answer may carry another + /// `MAX_ADDR_TO_SEND` addresses past the rate limit, as in Core. + fn note_getaddr_sent(&self, id: PeerId) { + if let Some(handle) = self.peers.write().get_mut(&id) { + handle.info.addr_rate.getaddr_sent(); + } + } + /// An addr-fetch connection exists to collect one batch of addresses and /// go. Core requires more than one entry before treating the answer as /// complete, so a peer that only announces itself does not end the @@ -2508,18 +2665,122 @@ impl PeerManager { .min(max_connections.load(Ordering::Relaxed)) } - /// Check outbound connection limit. - fn check_outbound_limit(&self) -> Result<(), String> { - let max_outbound = if self.is_ibd() { + /// The automatic outbound target: 8, capped by `-maxconnections`, or 64 + /// during IBD. + fn automatic_outbound_target(&self) -> usize { + if self.is_ibd() { MAX_OUTBOUND_IBD } else { self.max_connections.load(Ordering::Relaxed).min(MAX_OUTBOUND) + } + } + + /// Outbound peers that count against the automatic target: every one + /// but the manual connections, which Core counts apart (`semAddnode`, + /// or no limit at all), connected or still in the handshake. + fn automatic_outbound_peers(&self) -> usize { + self.peers + .read() + .values() + .filter(|h| { + h.info.direction == Direction::Outbound + && h.info.conn_type != ConnType::Manual + && h.info.state != PeerState::Disconnected + }) + .count() + } + + /// The added nodes the operator listed, at the addresses they resolve to. + fn added_node_addrs(&self) -> Vec { + self.addnode_entries + .read() + .iter() + .filter(|e| e.listed) + .filter_map(|e| e.resolved.clone()) + .collect() + } + + /// Manual peers that are added nodes, connected or still in the + /// handshake: what [`MAX_ADDNODE_CONNECTIONS`] counts. + fn added_node_peers(&self) -> usize { + let added = self.added_node_addrs(); + self.peers + .read() + .values() + .filter(|h| h.info.conn_type == ConnType::Manual && h.info.state != PeerState::Disconnected) + .filter(|h| { + added.iter().any(|a| match (a, h.info.onion_host.as_deref()) { + (PeerAddr::Socket(sa), None) => *sa == h.info.addr, + (PeerAddr::Onion { host, .. }, Some(peer_host)) => host == peer_host, + _ => false, + }) + }) + .count() + } + + /// The [`DialBudget`] a dial to `target` counts against. `conn_type` is + /// the dial's: `None` for an automatic dial, which is manual after all + /// when the operator named the peer (as `spawn_peer` will type it), + /// `Some(Manual)` for a dial the operator asked for. A typed + /// `addconnection` dial holds its own reservation instead. + fn dial_budget(&self, target: &PeerAddr, conn_type: Option) -> DialBudget { + let manual = match (conn_type, target) { + (Some(ConnType::Manual), _) => true, + (Some(_), _) => return DialBudget::Unlimited, + (None, PeerAddr::Socket(sa)) => self.untyped_outbound_conn_type(sa, None) == ConnType::Manual, + (None, PeerAddr::Onion { host, port }) => { + let placeholder = SocketAddr::from(([0, 0, 0, 0], *port)); + self.untyped_outbound_conn_type(&placeholder, Some(host)) == ConnType::Manual + } }; - let outbound = self.outbound_count(); - if outbound >= max_outbound { - return Err("max outbound connections reached".to_string()); + if !manual { + DialBudget::Automatic + } else if self.added_node_addrs().contains(target) { + DialBudget::AddedNode + } else { + DialBudget::Unlimited } - Ok(()) + } + + /// Take a place for one dial in `budget`, or refuse when the budget is + /// spent. The place is held until the returned slot drops, which a + /// dial that connected does only after its peer is in `peers`. + /// + /// The check counts the dials already in flight along with the peers, + /// and it and the reservation happen under one lock. satd used to count + /// connected outbound peers only, manual ones included: each reconnect + /// tick started a dial for every eligible address at once (the limit + /// could not see them until they connected), and a full set of + /// automatic peers left no room for an added node or `addnode onetry`. + /// Core takes a grant from a counting semaphore before each dial: + /// `semOutbound` for automatic connections, `semAddnode` for added + /// nodes, none for `-connect` and `onetry` (`net.cpp` + /// `ThreadOpenConnections`, `ThreadOpenAddedConnections`; + /// `rpc/net.cpp` `addnode`). + fn reserve_dial(self: &Arc, budget: DialBudget) -> Result { + let mut in_flight = self.dials_in_flight.lock(); + match budget { + DialBudget::Automatic => { + // `addconnection`'s reservations take automatic slots too. + let open = self.automatic_outbound_peers() + + in_flight.automatic + + self.pending_typed_dials.read().len(); + if open >= self.automatic_outbound_target() { + return Err("max outbound connections reached".to_string()); + } + in_flight.automatic += 1; + } + DialBudget::AddedNode => { + if self.added_node_peers() + in_flight.added_node >= MAX_ADDNODE_CONNECTIONS { + return Err(format!( + "addnode connections are limited to {MAX_ADDNODE_CONNECTIONS} at a time" + )); + } + in_flight.added_node += 1; + } + DialBudget::Unlimited => {} + } + Ok(DialSlot { pm: Arc::clone(self), budget }) } /// Connect to an outbound peer with no caller-chosen type, as the @@ -2534,8 +2795,8 @@ impl PeerManager { /// `conn_type` is `Some(Manual)` for a dial the operator asked for /// ([`Self::connect_peer_addr_with`]), and one of `addconnection`'s four /// types for that RPC. `None` is an automatic dial, classified by - /// `spawn_peer`. A manual or automatic dial takes the historical single - /// outbound cap; `addconnection`'s types take Core's per-type ones. + /// `spawn_peer`. A manual or automatic dial counts against its + /// [`DialBudget`]; `addconnection`'s types take Core's per-type limits. pub async fn connect_outbound_as( self: &Arc, addr: SocketAddr, @@ -2624,9 +2885,11 @@ impl PeerManager { Ok(()) } - /// `connect_outbound_as`, with the per-type reservation optional: - /// `add_connection` has already taken it and holds the grant across the - /// spawned dial. + /// `connect_outbound_as`, with the reservation optional. `reserve` false + /// takes no place in any limit here: `add_connection` has already taken + /// its per-type one and the reconnect loop its [`DialSlot`], each held + /// across the spawned dial, and an `addnode onetry` has none, as Core + /// opens one without a grant. async fn connect_outbound_inner( self: &Arc, addr: SocketAddr, @@ -2649,15 +2912,16 @@ impl PeerManager { if !crate::net::is_valid(addr.ip()) { return Err(format!("{addr} is not a valid address to connect to")); } - // Take the capacity check and the per-type reservation together, so - // two callers cannot both pass the same free slot. + // Take the capacity check and the reservation together, so two + // callers cannot both pass the same free slot. + let manual_or_automatic = matches!(conn_type, None | Some(ConnType::Manual)); + let _dial_slot = if manual_or_automatic && reserve { + Some(self.reserve_dial(self.dial_budget(&PeerAddr::Socket(addr), conn_type))?) + } else { + None + }; let _typed_slot = match conn_type { - // A manual dial is capped as an automatic one is, as it always - // was; it has no per-type reservation to take. - None | Some(ConnType::Manual) => { - self.check_outbound_limit()?; - None - } + None | Some(ConnType::Manual) => None, Some(t) if reserve => { let mut pending = self.pending_typed_dials.write(); self.check_outbound_limit_for(t, &pending)?; @@ -2745,11 +3009,28 @@ impl PeerManager { host: &str, port: u16, conn_type: Option, + ) -> Result<(), String> { + self.connect_outbound_onion_inner(host, port, conn_type, true).await + } + + /// `connect_outbound_onion`, with the [`DialSlot`] optional, as in + /// [`Self::connect_outbound_inner`]. + async fn connect_outbound_onion_inner( + self: &Arc, + host: &str, + port: u16, + conn_type: Option, + reserve: bool, ) -> Result<(), String> { if !self.is_network_active() { return Err("networking disabled (networkactive=false)".to_string()); } - self.check_outbound_limit()?; + let _dial_slot = if reserve { + let target = PeerAddr::Onion { host: host.to_string(), port }; + Some(self.reserve_dial(self.dial_budget(&target, conn_type))?) + } else { + None + }; // Dedupe by onion host BEFORE any network I/O. Onion peers all share // the `0.0.0.0` placeholder socket, so `is_addr_connected` / @@ -2817,8 +3098,10 @@ impl PeerManager { /// Dial a peer the operator asked for (either socket or .onion), there /// and then: `-connect`, `-addnode` and `-seednode` at startup and on - /// reload, `addnode add`, `addnode onetry`, and a named peer whose - /// name resolved. The connection is `manual`. + /// reload, `addnode add`, and a named peer whose name resolved. The + /// connection is `manual`; an added node counts against + /// [`MAX_ADDNODE_CONNECTIONS`]. `addnode onetry` is + /// [`Self::connect_peer_addr_onetry`]. pub async fn connect_peer_addr(self: &Arc, addr: &PeerAddr) -> Result<(), String> { self.connect_peer_addr_with(addr, None).await } @@ -2845,11 +3128,53 @@ impl PeerManager { } } + /// `addnode onetry`: one manual dial, outside every limit, as + /// Core's `onetry` opens its connection without a grant (`rpc/net.cpp` + /// `addnode`). A peer already connected, or being dialled, is left as + /// it is and the call succeeds: Core's `OpenNetworkConnection` returns + /// without dialling a host it is connected to, and `onetry` answers + /// null either way. A dial that fails is reported. + /// + /// satd refused `onetry` with "max outbound connections reached" once + /// the automatic outbound peers were all connected, and with "already + /// connected" for a connected peer. + pub async fn connect_peer_addr_onetry( + self: &Arc, + addr: &PeerAddr, + use_v2: Option, + ) -> Result<(), String> { + if self.is_peer_addr_connected(addr) || self.is_dial_in_flight(addr) { + tracing::debug!(%addr, "addnode onetry: already connected or connecting"); + return Ok(()); + } + match addr { + PeerAddr::Socket(sa) => { + self.connect_outbound_inner(*sa, Some(ConnType::Manual), use_v2, false).await + } + PeerAddr::Onion { host, port } => { + self.connect_outbound_onion_inner(host, *port, Some(ConnType::Manual), false).await + } + } + } + + /// Whether a dial to `addr` is under way. + fn is_dial_in_flight(&self, addr: &PeerAddr) -> bool { + match addr { + PeerAddr::Socket(sa) => self.pending_connections.read().contains(sa), + PeerAddr::Onion { host, .. } => self.pending_onion_dials.read().contains(host), + } + } + /// Dial `addr` as one of the node's own automatic dials: the reconnect /// loop's, and a seed's. No type is chosen here; `spawn_peer` reports /// the connection `manual` if the operator named the peer and /// `outbound-full-relay` otherwise. pub async fn connect_peer_addr_automatic(self: &Arc, addr: &PeerAddr) -> Result<(), String> { + // An automatic connection is not opened to a banned address, as in + // Core; a peer the operator named is dialled regardless. + if !self.is_manual_target(addr) && self.is_peer_addr_banned(addr) { + return Err(format!("{addr} is banned")); + } match addr { PeerAddr::Socket(sa) => self.connect_outbound(*sa).await, PeerAddr::Onion { host, port } => self.connect_outbound_onion(host, *port, None).await, @@ -3660,11 +3985,190 @@ impl PeerManager { Self::onion_connected_in(&self.peers.read(), host) } + /// One pass of the reconnect loop, every 10 seconds of the run loop: + /// redial the peers the operator named, then fill the free automatic + /// outbound slots from the addresses the node learned. + /// + /// The two halves are apart, as in Core, where + /// `ThreadOpenAddedConnections` and the `-connect` loop run separately + /// from `ThreadOpenConnections` (`net.cpp`). satd ran one pass, and only + /// while the outbound count, manual peers included, was below the + /// automatic target: on a node with its automatic peers connected, an + /// added node that dropped, or whose first dial failed, was not dialled + /// again. + fn reconnect_tick(self: &Arc) { + let now = Instant::now(); + // Clean expired bans + { + let now_secs = crate::time::now_secs(); + self.ban_list.write().prune_expired(now_secs); + self.flush_banlist(); + } + self.redial_manual_peers(now); + self.dial_automatic_peers(now); + } + + /// Whether `target` is a peer the operator named. + fn is_manual_target(&self, target: &PeerAddr) -> bool { + match target { + PeerAddr::Socket(sa) => self.manual_addrs.read().contains(sa), + PeerAddr::Onion { host, .. } => self.manual_onion_hosts.read().contains(host), + } + } + + /// The manual half of [`Self::reconnect_tick`]: every peer the operator + /// named (`-connect`, `-addnode` / `addnode add`, `-seednode`) that is + /// not connected, whatever the automatic outbound count. Added nodes stop + /// at [`MAX_ADDNODE_CONNECTIONS`], and the one past it waits for a later + /// tick, as Core's added-node thread waits for a grant; the others have + /// no limit. A failed dial backs off to at most + /// [`MANUAL_RECONNECT_MAX_BACKOFF`], Core's added-node retry interval. + /// + /// A ban does not stop these dials. Core opens a manual connection by + /// the name the operator gave and checks bans only for the automatic + /// ones (`OpenNetworkConnection`, `net.cpp`); satd skipped a banned + /// manual peer here. + fn redial_manual_peers(self: &Arc, now: Instant) { + let mut targets: Vec = + self.manual_addrs.read().iter().map(|a| PeerAddr::Socket(*a)).collect(); + let onion: Vec = self.connect_peer_addrs.read().clone(); + targets.extend(onion.into_iter().filter(|a| self.is_manual_target(a))); + for target in targets { + if !self.redial_due(&target, now) { + continue; + } + let Ok(slot) = self.reserve_dial(self.dial_budget(&target, None)) else { + continue; + }; + self.spawn_redial(target, slot, MANUAL_RECONNECT_MAX_BACKOFF); + } + } + + /// The automatic half of [`Self::reconnect_tick`]: learned addresses, + /// then onion peers learned from `addrv2` gossip, one dial each until the + /// automatic outbound target is reached, the dials in flight counted + /// (`reserve_dial`). Under `-connect` the node dials none of them; see + /// [`Self::may_dial`]. A banned address is skipped, an onion host + /// included (`setban .onion`, or a ban for misbehaviour), as Core + /// skips a banned address for an automatic connection; satd checked + /// the bans of clearnet addresses only. + fn dial_automatic_peers(self: &Arc, now: Instant) { + // An address the operator also named is offered by the manual half, + // and `spawn_peer` makes it the manual connection it is: Core makes + // no automatic connection to an added node (`AddedNodesContain`, + // v31.1 `src/net.cpp:2883`). + let learned: Vec = self.learned_addrs.read().iter().copied().collect(); + for addr in learned { + if self.is_manual_target(&PeerAddr::Socket(addr)) || !self.may_dial(&addr) { + continue; + } + let target = PeerAddr::Socket(addr); + if self.is_peer_addr_banned(&target) || !self.redial_due(&target, now) { + continue; + } + match self.reserve_dial(DialBudget::Automatic) { + Ok(slot) => self.spawn_redial(target, slot, AUTOMATIC_RECONNECT_MAX_BACKOFF), + Err(_) => return, + } + } + + // `connect_peer_addrs` is fillable from untrusted addrv2 gossip (up + // to MAX_ONION_CONNECT_ADDRS), and every onion dial holds a circuit + // through the one proxy, so on top of the free slots a tick starts + // no more than a burst of them. + let onion: Vec = self.connect_peer_addrs.read().clone(); + let mut budget = Self::onion_dial_budget( + self.automatic_outbound_target(), + self.automatic_outbound_peers(), + self.dials_in_flight.lock().automatic, + ); + for target in onion.into_iter().filter(|a| !self.is_manual_target(a)) { + // Under `-connect`, only the peers the operator named. + if budget == 0 || !self.automatic_outbound.load(Ordering::Relaxed) { + break; + } + if self.is_peer_addr_banned(&target) || !self.redial_due(&target, now) { + continue; + } + match self.reserve_dial(DialBudget::Automatic) { + Ok(slot) => { + budget -= 1; + self.spawn_redial(target, slot, AUTOMATIC_RECONNECT_MAX_BACKOFF); + } + Err(_) => return, + } + } + } + + /// Whether the reconnect loop should dial `target` now: it is neither + /// connected nor being dialled, and not backing off. Bans are the + /// caller's to check: they hold for automatic dials only. + fn redial_due(&self, target: &PeerAddr, now: Instant) -> bool { + match target { + PeerAddr::Socket(sa) => { + !self.is_addr_connected(sa) + && !self.pending_connections.read().contains(sa) + && self + .reconnect_backoff + .read() + .get(sa) + .is_none_or(|state| now >= state.next_attempt) + } + PeerAddr::Onion { host, .. } => { + !self.is_onion_connected(host) + && !self.pending_onion_dials.read().contains(host) + && self + .onion_reconnect_backoff + .read() + .get(&target.to_string()) + .is_none_or(|state| now >= state.next_attempt) + } + } + } + + /// Dial `target` for the reconnect loop in a task of its own, holding the + /// place `slot` took for it, and keep its backoff: cleared when the dial + /// connects, longer (up to `max_backoff`) when it fails. Only a + /// connection clears it. + fn spawn_redial(self: &Arc, target: PeerAddr, slot: DialSlot, max_backoff: Duration) { + let pm = Arc::clone(self); + tokio::spawn(async move { + let _slot = slot; + let result = match &target { + PeerAddr::Socket(sa) => pm.connect_outbound_inner(*sa, None, None, false).await, + PeerAddr::Onion { host, port } => { + pm.connect_outbound_onion_inner(host, *port, None, false).await + } + }; + if let Err(e) = &result { + tracing::debug!(%target, "Reconnect failed: {e}"); + } + let connected = result.is_ok(); + let update = |state: &mut ReconnectState| { + if connected { + state.reset(); + } else { + state.record_failure(max_backoff); + } + }; + match &target { + PeerAddr::Socket(sa) => { + update(pm.reconnect_backoff.write().entry(*sa).or_insert_with(ReconnectState::new)) + } + PeerAddr::Onion { .. } => update( + pm.onion_reconnect_backoff + .write() + .entry(target.to_string()) + .or_insert_with(ReconnectState::new), + ), + } + }); + } + /// How many new onion dials the reconnect loop may start this tick: the - /// open outbound slots (counting in-flight dials, which `outbound_count` - /// excludes because they aren't `Connected` yet) capped by the per-tick - /// burst limit. Without counting `in_flight`, a single tick could spawn a - /// dial for every gossip-discovered onion at once. + /// open automatic outbound slots (counting dials in flight) capped by the + /// per-tick burst limit. Without counting `in_flight`, a single tick + /// could spawn a dial for every gossip-discovered onion at once. fn onion_dial_budget(target: usize, outbound: usize, in_flight: usize) -> usize { target .saturating_sub(outbound + in_flight) @@ -3685,6 +4189,17 @@ impl PeerManager { self.ban_list.read().is_banned(&addr.ip(), now) } + /// Whether `addr` is banned: a socket address by its IP (and any subnet + /// holding it), an onion one by its host. + fn is_peer_addr_banned(&self, addr: &PeerAddr) -> bool { + match addr { + PeerAddr::Socket(sa) => self.is_addr_banned(sa), + PeerAddr::Onion { host, .. } => { + self.ban_list.read().is_onion_banned(host, crate::time::now_secs()) + } + } + } + /// Add ban score to a peer. Once the score reaches [`BAN_THRESHOLD`] the /// peer has misbehaved (Core's `Misbehaving`), and it is dealt with as /// [`Self::misbehaviour_action`] says. @@ -3869,14 +4384,13 @@ impl PeerManager { let _htip = self.headers_tip.load(Ordering::Relaxed) as u32; // When chain advances, immediately request more blocks (don't wait for timer) + // Reconnect backoff is not reset here: a dial's backoff ends when + // a dial to that address succeeds. Resetting every address's on + // each new tip, which during IBD is every tick, meant no address + // ever backed off. let tip_advanced = tip != last_tip; if tip_advanced { last_tip = tip; - // Reset reconnect backoff on chain progress - let mut backoff = self.reconnect_backoff.write(); - for state in backoff.values_mut() { - state.reset(); - } } // IBD scheduler maintenance @@ -4058,13 +4572,13 @@ impl PeerManager { } } - // Every 20 ticks (10 seconds), reconnect if below outbound target. - // Skipped entirely while networking is paused (`networkactive=0` / - // `setnetworkactive false`): otherwise each tick would spawn dials - // that immediately fail the gate, churning backoff state and logs. - // Named peers are looked up again on the same tick, whatever the - // outbound count: an added node is a manual connection, which - // Core dials outside the outbound target. + // Every 20 ticks (10 seconds), redial the peers the operator + // named and fill the automatic outbound slots + // (`reconnect_tick`). Skipped entirely while networking is paused + // (`networkactive=0` / `setnetworkactive false`): otherwise each + // tick would spawn dials that immediately fail the gate, churning + // backoff state and logs. Named peers are looked up again on the + // same tick. if ticks.is_multiple_of(20) && self.is_network_active() && self.manual_targets_due(Instant::now()) @@ -4072,157 +4586,7 @@ impl PeerManager { tokio::spawn(Arc::clone(self).refresh_manual_targets()); } if ticks.is_multiple_of(20) && self.is_network_active() { - let outbound = self.outbound_count(); - let target = if self.is_ibd() { MAX_OUTBOUND_IBD } else { MAX_OUTBOUND }; - let need_peers = outbound < target; - if need_peers { - // The peers the operator named first, then those the node - // learned. An address on both lists is offered once, and - // `spawn_peer` makes it the manual connection it is: Core - // makes no automatic connection to an added node - // (`AddedNodesContain`, v31.1 `src/net.cpp:2883`). - let manual: HashSet = self.manual_addrs.read().clone(); - let learned: Vec = self - .learned_addrs - .read() - .iter() - .filter(|a| !manual.contains(a)) - .copied() - .collect(); - let addrs: Vec = manual.into_iter().chain(learned).collect(); - - let now = Instant::now(); - - // Clean expired bans - { - let now_secs = crate::time::now_secs(); - self.ban_list.write().prune_expired(now_secs); - self.flush_banlist(); - } - - for addr in addrs { - // Under `-connect`, only the peers the operator named. - if !self.may_dial(&addr) { - continue; - } - // Skip if already connected - if self.is_addr_connected(&addr) { - continue; - } - // Skip if banned - if self.is_addr_banned(&addr) { - continue; - } - // Check backoff timer - { - let backoff = self.reconnect_backoff.read(); - if let Some(state) = backoff.get(&addr) - && now < state.next_attempt { - continue; - } - } - - // Don't exceed target - if self.check_outbound_limit().is_err() { - break; - } - - let pm = Arc::clone(self); - tokio::spawn(async move { - match pm.connect_outbound(addr).await { - Ok(_) => { - let mut backoff = pm.reconnect_backoff.write(); - backoff - .entry(addr) - .or_insert_with(ReconnectState::new) - .reset(); - } - Err(e) => { - tracing::debug!(%addr, "Reconnect failed: {}", e); - let mut backoff = pm.reconnect_backoff.write(); - backoff - .entry(addr) - .or_insert_with(ReconnectState::new) - .record_failure(); - } - } - }); - } - - // Also reconnect .onion peers — with the same discipline the - // clearnet loop above has. `connect_peer_addrs` is fillable - // from untrusted addrv2 gossip (up to MAX_ONION_CONNECT_ADDRS), - // so spawning a dial per entry with no cap would let one peer - // drive hundreds of concurrent SOCKS dials through the single - // proxy every tick. Bound new dials this tick by the open - // slot budget (counting in-flight dials, which - // `outbound_count()` excludes) and a per-tick burst cap; skip - // already-connected / in-flight hosts; and back off failures - // (onion-host-keyed, since `reconnect_backoff` is SocketAddr- - // keyed). - let onion_addrs = self.connect_peer_addrs.read().clone(); - let mut budget = Self::onion_dial_budget( - target, - self.outbound_count(), - self.pending_onion_dials.read().len(), - ); - for peer_addr in onion_addrs { - if budget == 0 { - break; - } - // Under `-connect`, only the peers the operator named. - let manual = match &peer_addr { - PeerAddr::Onion { host, .. } => { - self.manual_onion_hosts.read().contains(host) - } - PeerAddr::Socket(sa) => self.manual_addrs.read().contains(sa), - }; - if !manual && !self.automatic_outbound.load(Ordering::Relaxed) { - continue; - } - let already = match &peer_addr { - PeerAddr::Onion { host, .. } => { - self.is_onion_connected(host) - || self.pending_onion_dials.read().contains(host) - } - PeerAddr::Socket(sa) => self.is_addr_connected(sa), - }; - if already { - continue; - } - let key = peer_addr.to_string(); - { - let backoff = self.onion_reconnect_backoff.read(); - if let Some(state) = backoff.get(&key) - && now < state.next_attempt - { - continue; - } - } - budget -= 1; - let pm = Arc::clone(self); - tokio::spawn(async move { - let key = peer_addr.to_string(); - match pm.connect_peer_addr_automatic(&peer_addr).await { - Ok(_) => { - pm.onion_reconnect_backoff - .write() - .entry(key) - .or_insert_with(ReconnectState::new) - .reset(); - } - Err(e) => { - tracing::debug!(%peer_addr, "Onion reconnect failed: {}", e); - pm.onion_reconnect_backoff - .write() - .entry(key) - .or_insert_with(ReconnectState::new) - .record_failure(); - } - } - }); - } - } + self.reconnect_tick(); } // Drop addr-fetch connections whose peer never sent a usable @@ -4430,12 +4794,20 @@ impl PeerManager { self.add_ban_score(id, 100, &format!("addr message size = {}", addrs.len())); return; } + let count = addrs.len(); + let admitted = if relay_addrs { self.rate_limit_addrs(id, addrs) } else { Vec::new() }; let source = self.peer_ip(id); - for (_, addr) in &addrs { - if relay_addrs - && let Ok(sock_addr) = addr.socket_addr() + let mut processed = 0u64; + for (_, addr) in &admitted { + let sock_addr = addr.socket_addr().ok(); + // Core looks no further at a banned address, and does + // not count it processed. + if sock_addr.is_some_and(|sa| self.is_addr_banned(&sa)) { + continue; + } + processed += 1; + if let Some(sock_addr) = sock_addr && !self.is_addr_connected(&sock_addr) - && !self.is_addr_banned(&sock_addr) { self.add_learned_addr_from( sock_addr, @@ -4444,7 +4816,8 @@ impl PeerManager { ); } } - self.note_addr_fetch_answered(id, addrs.len()); + self.note_addrs_processed(id, processed); + self.note_addr_fetch_answered(id, count); } NetworkMessage::GetAddr => { // Respond with our declared external addresses (-externalip) @@ -4584,8 +4957,11 @@ impl PeerManager { self.add_ban_score(id, 100, &format!("addrv2 message size = {}", addrs.len())); return; } + let count = addrs.len(); + let admitted = if relay_addrs { self.rate_limit_addrs(id, addrs) } else { Vec::new() }; let source = self.peer_ip(id); - for addr_msg in addrs.iter().filter(|_| relay_addrs) { + let mut processed = 0u64; + for addr_msg in &admitted { match &addr_msg.addr { // BIP 155 TorV3: `socket_addr()` can't represent these, // so derive the .onion host and queue it for dialing. @@ -4593,14 +4969,23 @@ impl PeerManager { if onion_routing && addr_msg.port != 0 => { let host = crate::net::peer::torv3_to_onion_host(pubkey); + if self.ban_list.read().is_onion_banned(&host, crate::time::now_secs()) { + continue; + } + processed += 1; self.add_onion_connect_addr(host, addr_msg.port); } // IPv4 / IPv6 (and, under a proxy, anything else with a // socket form) flow through the clearnet address book. _ => { - if let Ok(sock_addr) = addr_msg.socket_addr() + let sock_addr = addr_msg.socket_addr().ok(); + // As for `addr`. + if sock_addr.is_some_and(|sa| self.is_addr_banned(&sa)) { + continue; + } + processed += 1; + if let Some(sock_addr) = sock_addr && !self.is_addr_connected(&sock_addr) - && !self.is_addr_banned(&sock_addr) { self.add_learned_addr_from( sock_addr, @@ -4611,7 +4996,8 @@ impl PeerManager { } } } - self.note_addr_fetch_answered(id, addrs.len()); + self.note_addrs_processed(id, processed); + self.note_addr_fetch_answered(id, count); } NetworkMessage::SendAddrV2 => { let mut peers = self.peers.write(); @@ -9823,6 +10209,8 @@ impl PeerManager { // connection is about to be dropped. let conn_type = self.conn_type_of(id); if conn_type == ConnType::AddrFetch { + // Room for the answer, before it can arrive. + self.note_getaddr_sent(id); writer .send(NetworkMessage::GetAddr) .await @@ -9879,6 +10267,7 @@ impl PeerManager { && conn_type != ConnType::AddrFetch && (self.proxy.is_some() || self.onion_proxy.is_some()) { + self.note_getaddr_sent(id); writer.send(NetworkMessage::GetAddr) .await .map_err(|e| format!("send getaddr: {}", e))?; diff --git a/node/src/net/manager_addrdials_tests.rs b/node/src/net/manager_addrdials_tests.rs new file mode 100644 index 000000000..756b49740 --- /dev/null +++ b/node/src/net/manager_addrdials_tests.rs @@ -0,0 +1,573 @@ +//! The addresses a peer may hand the node, and the outbound dials the node +//! starts from them: Core's address rate limit (`ProcessAddrs`), and the +//! separate limits on automatic and manual connections (`semOutbound`, +//! `semAddnode`; `net.cpp` `ThreadOpenConnections`, +//! `ThreadOpenAddedConnections`), against a live `PeerManager`. + +use super::*; +use crate::net::flow::InFlight; +use std::sync::atomic::AtomicUsize; + +fn mk_pm() -> (Arc, tempfile::TempDir) { + use crate::chain::state::AssumeValid; + use crate::storage::db::InMemoryStore; + use crate::storage::flatfile::FlatFileManager; + use crate::validation::script::NoopVerifier; + + let dir = tempfile::TempDir::new().unwrap(); + let store = Box::new(InMemoryStore::new()); + let flat_files = FlatFileManager::new(&dir.path().join("blocks")).unwrap(); + let chain_state = Arc::new( + ChainState::new( + store, + flat_files, + Network::Regtest, + Box::new(NoopVerifier), + AssumeValid::Disabled, + 450, + 4, + Default::default(), + Default::default(), + Default::default(), + ) + .unwrap(), + ); + (over(chain_state), dir) +} + +fn over(chain_state: Arc) -> Arc { + let mempool = Arc::new(Mempool::new(1_000_000, 0)); + let fee_estimator = Arc::new(FeeEstimator::new()); + let (_shutdown_tx, shutdown_rx) = tokio::sync::watch::channel(false); + PeerManager::new(chain_state, mempool, fee_estimator, Network::Regtest, shutdown_rx) +} + +/// Out of IBD, so the automatic outbound target is 8 rather than 64: a +/// peer has announced a headers tip the node's tip is within 24 of. +fn leave_ibd(pm: &PeerManager) { + pm.headers_tip.store(1, Ordering::Relaxed); + assert!(!pm.is_ibd(), "fixture: out of IBD"); +} + +fn add_peer(pm: &PeerManager, info: PeerInfo) { + let (tx, _rx) = mpsc::channel::(8); + pm.peers.write().insert( + info.id, + PeerHandle { + info, + msg_tx: tx.into(), + disconnect: Arc::new(tokio::sync::Notify::new()), + flow: Arc::new(crate::net::flow::PeerFlow::new()), + last_getheaders_sent: None, + last_mempool_served: None, + fee_filter_sent: None, + stats: PeerStats::new(NetTotals::new()), + }, + ); +} + +/// A connected peer. +fn peer(id: PeerId, addr: SocketAddr, direction: Direction) -> PeerInfo { + let mut info = PeerInfo::new(id, addr, direction); + info.state = PeerState::Connected; + info +} + +/// `n` connected automatic (`outbound-full-relay`) peers, ids from 1000. +fn fill_automatic(pm: &PeerManager, n: usize) { + for i in 0..n { + let addr = SocketAddr::from(([203, 0, 113, i as u8 + 1], 8333)); + add_peer(pm, peer(1000 + i as PeerId, addr, Direction::Outbound)); + } +} + +/// `n` addresses, each in a /16 of its own so the address book's +/// per-group limit does not apply, starting at first octet `base`. +fn spread(base: u8, n: usize) -> Vec { + (0..n) + .map(|i| SocketAddr::from(([base + (i / 250) as u8, (i % 250) as u8, 7, 7], 8333))) + .collect() +} + +fn addr_msg(addrs: &[SocketAddr]) -> NetworkMessage { + NetworkMessage::Addr( + addrs + .iter() + .map(|a| (1_700_000_000, Address::new(a, ServiceFlags::NETWORK | ServiceFlags::WITNESS))) + .collect(), + ) +} + +fn addr_counts(pm: &PeerManager, id: PeerId) -> (u64, u64) { + let peers = pm.peers.read(); + let rate = &peers.get(&id).expect("peer").info.addr_rate; + (rate.processed, rate.rate_limited) +} + +/// A listener on loopback that accepts every connection and keeps it open, +/// counting the accepts. +struct Listener { + addr: SocketAddr, + accepts: Arc, + task: tokio::task::JoinHandle<()>, +} + +impl Listener { + async fn bind() -> Self { + let socket = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = socket.local_addr().unwrap(); + let accepts = Arc::new(AtomicUsize::new(0)); + let counter = accepts.clone(); + let task = tokio::spawn(async move { + let mut held = Vec::new(); + while let Ok((stream, _)) = socket.accept().await { + counter.fetch_add(1, Ordering::SeqCst); + held.push(stream); + } + }); + Self { addr, accepts, task } + } + + fn accepts(&self) -> usize { + self.accepts.load(Ordering::SeqCst) + } +} + +impl Drop for Listener { + fn drop(&mut self) { + self.task.abort(); + } +} + +async fn listeners(n: usize) -> Vec { + let mut out = Vec::new(); + for _ in 0..n { + out.push(Listener::bind().await); + } + out +} + +fn total_accepts(ls: &[Listener]) -> usize { + ls.iter().map(Listener::accepts).sum() +} + +/// Wait until `cond` holds. +async fn wait_for(pm: &PeerManager, cond: impl Fn() -> bool, what: &str) { + let deadline = Instant::now() + Duration::from_secs(30); + while !cond() { + assert!( + Instant::now() < deadline, + "timed out: {what} (in flight {:?}, dialling {:?}, backoff {:?})", + pm.dials_in_flight.lock(), + pm.pending_connections.read(), + pm.reconnect_backoff.read().iter().map(|(a, s)| (*a, s.attempts)).collect::>(), + ); + tokio::time::sleep(Duration::from_millis(20)).await; + } +} + +/// Wait for the dials a reconnect tick started to finish, one way or the +/// other: nothing is in flight any more. +async fn settle(pm: &PeerManager) { + let deadline = Instant::now() + Duration::from_secs(20); + loop { + let in_flight = { + let d = pm.dials_in_flight.lock(); + d.automatic + d.added_node + }; + if in_flight == 0 && pm.pending_connections.read().is_empty() && pm.pending_onion_dials.read().is_empty() { + break; + } + assert!(Instant::now() < deadline, "dials still in flight"); + tokio::time::sleep(Duration::from_millis(20)).await; + } + // Accepts land on the listener tasks a moment after the dial returns. + tokio::time::sleep(Duration::from_millis(200)).await; +} + +// ---- Address rate limit ---- + +/// Core gives each peer a token bucket for the addresses it announces: one +/// token to start, 0.1 a second after that. satd took every entry of every +/// `addr`, so one peer could put a thousand addresses into the address book +/// and onto the dial list per message. A peer with the `addr` permission is +/// not limited, as in Core. +#[test] +fn a_peers_addresses_are_rate_limited_unless_it_has_the_addr_permission() { + let (pm, _dir) = mk_pm(); + let limited = SocketAddr::from(([127, 0, 0, 1], 40001)); + add_peer(&pm, peer(1, limited, Direction::Inbound)); + pm.handle_message(1, addr_msg(&spread(1, 1000)), InFlight::adopt(None)); + assert_eq!(addr_counts(&pm, 1), (1, 999), "one token: one address processed, the rest rate-limited"); + assert_eq!(pm.addrman.read().len(), 1, "one address reached the address book"); + assert_eq!(pm.learned_addrs.read().iter().count(), 1, "and the dial list"); + + let mut exempt = peer(2, SocketAddr::from(([127, 0, 0, 1], 40002)), Direction::Inbound); + exempt.permissions.addr = true; + add_peer(&pm, exempt); + pm.handle_message(2, addr_msg(&spread(20, 1000)), InFlight::adopt(None)); + assert_eq!(addr_counts(&pm, 2), (1000, 0), "the addr permission takes them all"); + assert_eq!(pm.addrman.read().len(), 1001); +} + +/// Core adds `MAX_ADDR_TO_SEND` tokens when it sends a peer `getaddr`, so +/// the answer is taken whole. satd sends one to an addr-fetch peer, and to +/// outbound peers under a proxy. +#[test] +fn the_answer_to_a_getaddr_is_taken_whole() { + let (pm, _dir) = mk_pm(); + add_peer(&pm, peer(1, SocketAddr::from(([198, 51, 100, 1], 8333)), Direction::Outbound)); + pm.note_getaddr_sent(1); + pm.handle_message(1, addr_msg(&spread(1, 1000)), InFlight::adopt(None)); + let (processed, limited) = addr_counts(&pm, 1); + assert_eq!((processed, limited), (1000, 0)); +} + +/// The same limit applies to `addrv2`. +#[test] +fn addrv2_is_rate_limited_too() { + use bitcoin::p2p::address::{AddrV2, AddrV2Message}; + let (pm, _dir) = mk_pm(); + add_peer(&pm, peer(1, SocketAddr::from(([127, 0, 0, 1], 40001)), Direction::Inbound)); + let msg = NetworkMessage::AddrV2( + spread(1, 500) + .into_iter() + .map(|a| AddrV2Message { + time: 1_700_000_000, + services: ServiceFlags::NETWORK, + addr: match a.ip() { + IpAddr::V4(v4) => AddrV2::Ipv4(v4), + IpAddr::V6(v6) => AddrV2::Ipv6(v6), + }, + port: a.port(), + }) + .collect(), + ); + pm.handle_message(1, msg, InFlight::adopt(None)); + assert_eq!(addr_counts(&pm, 1), (1, 499)); +} + +// ---- The automatic dial list ---- + +/// Only an address the address book holds goes on the dial list. The book +/// keeps at most 64 new addresses per network group; satd put every +/// gossiped address on the dial list whether the book took it or not. +#[test] +fn an_address_the_book_has_no_room_for_is_not_dialled() { + let (pm, _dir) = mk_pm(); + let source = IpAddr::from([198, 51, 100, 1]); + let one_group: Vec = + (0..100).map(|i| SocketAddr::from(([44, 1, (i / 250) as u8, (i % 250) as u8 + 1], 8333))).collect(); + for a in &one_group { + pm.add_learned_addr_from(*a, 9, source); + } + let book = pm.addrman.read().len(); + assert_eq!(book, 64, "fixture: the book's per-group limit"); + assert_eq!(pm.learned_addrs.read().iter().count(), 64, "the dial list holds what the book holds"); + for a in &one_group[..64] { + assert!(pm.learned_addrs.read().contains(a)); + } + // An address the book already holds, announced again, stays listed once. + pm.add_learned_addr_from(one_group[0], 9, source); + assert_eq!(pm.learned_addrs.read().iter().count(), 64); +} + +/// The list holds each address once and stops at its cap. +#[test] +fn the_dial_list_stops_at_its_cap() { + let mut list = LearnedAddrs::default(); + let all: Vec = (0..=MAX_LEARNED_ADDRS as u32) + .map(|i| SocketAddr::from((std::net::Ipv4Addr::from(0x0b00_0000 + i), 8333))) + .collect(); + for a in &all[..MAX_LEARNED_ADDRS] { + assert!(list.push(*a)); + } + assert!(!list.push(all[0]), "a duplicate is refused"); + assert!(!list.push(all[MAX_LEARNED_ADDRS]), "past the cap is refused"); + assert_eq!(list.iter().count(), MAX_LEARNED_ADDRS); +} + +// ---- Outbound dials ---- + +/// The automatic limit counts the dials in flight along with the peers. +/// satd counted connected peers only, so a reconnect tick started a dial +/// for every eligible learned address at once: twenty listeners, three +/// automatic peers connected and a target of eight got twenty dials. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_reconnect_tick_starts_no_more_automatic_dials_than_free_slots() { + let (pm, _dir) = mk_pm(); + leave_ibd(&pm); + fill_automatic(&pm, 3); + let ls = listeners(20).await; + for l in &ls { + pm.learned_addrs.write().push(l.addr); + } + + pm.reconnect_tick(); + wait_for(&pm, || total_accepts(&ls) >= 5, "five dials reach a listener").await; + settle(&pm).await; + assert_eq!(total_accepts(&ls), 5, "five free slots, five dials"); + assert_eq!(pm.automatic_outbound_peers(), 8, "the target is reached"); + + // Full now: the next tick starts nothing. + pm.reconnect_tick(); + settle(&pm).await; + assert_eq!(total_accepts(&ls), 5); +} + +/// Manual connections are outside the automatic target, as in Core, where +/// added nodes have their own limit (`semAddnode`). satd counted them +/// against it, and redialled named peers only while the automatic count +/// was below the target: with eight automatic peers, an added node was not +/// dialled at all. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn an_added_node_is_dialled_with_the_automatic_slots_full() { + let (pm, _dir) = mk_pm(); + leave_ibd(&pm); + fill_automatic(&pm, 8); + let l = Listener::bind().await; + assert!(pm.addnode_add(&l.addr.to_string(), PeerAddr::Socket(l.addr))); + + pm.reconnect_tick(); + wait_for(&pm, || l.accepts() == 1, "the added node is dialled").await; + settle(&pm).await; + let conn_type = pm.peers.read().values().find(|h| h.info.addr == l.addr).map(|h| h.info.conn_type); + assert_eq!(conn_type, Some(ConnType::Manual)); + + // The automatic limit still holds for an automatic dial. + let spare = Listener::bind().await; + let err = pm.connect_outbound(spare.addr).await.expect_err("the automatic slots are full"); + assert_eq!(err, "max outbound connections reached"); +} + +/// Core's `MAX_ADDNODE_CONNECTIONS`: eight added nodes at a time. The other +/// manual connections (`-connect`, `-seednode`) have no limit. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn added_nodes_stop_at_eight_and_other_manual_peers_do_not() { + let (pm, _dir) = mk_pm(); + leave_ibd(&pm); + let added = listeners(10).await; + for l in &added { + assert!(pm.addnode_add(&l.addr.to_string(), PeerAddr::Socket(l.addr))); + } + let connect = listeners(10).await; + for l in &connect { + assert!(pm.add_peer_addr(PeerAddr::Socket(l.addr))); + } + + pm.reconnect_tick(); + wait_for( + &pm, + || total_accepts(&added) >= MAX_ADDNODE_CONNECTIONS && total_accepts(&connect) >= 10, + "the dials reach their listeners", + ) + .await; + settle(&pm).await; + assert_eq!(total_accepts(&added), MAX_ADDNODE_CONNECTIONS, "eight added nodes"); + assert_eq!(total_accepts(&connect), 10, "every other manual peer"); + assert_eq!(pm.added_node_peers(), MAX_ADDNODE_CONNECTIONS); + + // One added node goes away: the next tick dials one more. + let gone = pm + .peers + .read() + .iter() + .find(|(_, h)| added.iter().any(|l| l.addr == h.info.addr)) + .map(|(id, _)| *id) + .unwrap(); + pm.peers.write().remove(&gone); + pm.reconnect_tick(); + wait_for(&pm, || total_accepts(&added) > MAX_ADDNODE_CONNECTIONS, "a ninth dial").await; + settle(&pm).await; + assert_eq!(total_accepts(&added), MAX_ADDNODE_CONNECTIONS + 1); +} + +/// `addnode onetry` is outside every limit, as Core's takes no grant, and +/// answers success for a peer already connected, as Core's answers null. +/// satd refused it with "max outbound connections reached" once the +/// automatic peers were all connected, and with "already connected". +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn onetry_is_outside_the_limits_and_succeeds_for_a_connected_peer() { + let (pm, _dir) = mk_pm(); + leave_ibd(&pm); + fill_automatic(&pm, 8); + let l = Listener::bind().await; + let target = PeerAddr::Socket(l.addr); + + pm.connect_peer_addr_onetry(&target, None).await.expect("onetry with the automatic slots full"); + wait_for(&pm, || l.accepts() == 1, "the onetry dial reaches the listener").await; + + pm.connect_peer_addr_onetry(&target, None).await.expect("onetry to a connected peer"); + settle(&pm).await; + assert_eq!(l.accepts(), 1, "no second connection"); + + // A dial that fails is still reported. + let closed = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let closed_addr = closed.local_addr().unwrap(); + drop(closed); + assert!(pm.connect_peer_addr_onetry(&PeerAddr::Socket(closed_addr), None).await.is_err()); +} + +/// A peer the operator named waits at most a minute between dials, Core's +/// added-node retry interval. satd backed it off to five minutes, like a +/// learned address. +#[test] +fn a_manual_peers_backoff_stops_at_a_minute() { + let mut manual = ReconnectState::new(); + let mut automatic = ReconnectState::new(); + for _ in 0..10 { + manual.record_failure(MANUAL_RECONNECT_MAX_BACKOFF); + automatic.record_failure(AUTOMATIC_RECONNECT_MAX_BACKOFF); + } + assert_eq!(manual.backoff_duration(MANUAL_RECONNECT_MAX_BACKOFF), Duration::from_secs(60)); + assert_eq!(automatic.backoff_duration(AUTOMATIC_RECONNECT_MAX_BACKOFF), Duration::from_secs(300)); +} + +/// A failed dial backs off, and in the reconnect loop that is per address: +/// a manual peer's failure uses the manual cap. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_failed_manual_redial_backs_off_by_the_manual_cap() { + let (pm, _dir) = mk_pm(); + leave_ibd(&pm); + let closed = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = closed.local_addr().unwrap(); + drop(closed); + assert!(pm.addnode_add(&addr.to_string(), PeerAddr::Socket(addr))); + for _ in 0..10 { + pm.reconnect_backoff.write().entry(addr).or_insert_with(ReconnectState::new).next_attempt = + Instant::now(); + pm.reconnect_tick(); + settle(&pm).await; + } + let backoff = pm.reconnect_backoff.read(); + let state = backoff.get(&addr).expect("a backoff entry"); + assert_eq!(state.attempts, 10); + assert!( + state.next_attempt <= Instant::now() + MANUAL_RECONNECT_MAX_BACKOFF, + "the next dial is at most a minute away" + ); +} + +/// Backoff ends when a dial to that address connects, and not otherwise. +/// satd cleared every address's backoff whenever the tip moved, which +/// during IBD is every tick, so nothing ever backed off. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_new_tip_leaves_reconnect_backoff_alone() { + use crate::chain::state::tests::{build_test_block, make_chain_state}; + + let (cs, dir) = make_chain_state(); + let chain_state = Arc::new(cs); + let pm = over(chain_state.clone()); + let waiting = SocketAddr::from(([198, 51, 100, 9], 8333)); + { + let mut backoff = pm.reconnect_backoff.write(); + let state = backoff.entry(waiting).or_insert_with(ReconnectState::new); + for _ in 0..3 { + state.record_failure(AUTOMATIC_RECONNECT_MAX_BACKOFF); + } + } + let genesis = bitcoin::constants::genesis_block(Network::Regtest).block_hash(); + chain_state + .accept_block(&build_test_block(genesis, 1, 1_707_000_000)) + .expect("connect block 1"); + + let pm_run = pm.clone(); + let run = tokio::spawn(async move { pm_run.run().await }); + // The run loop sees the new tip on its first maintenance pass, 500 ms in. + tokio::time::sleep(Duration::from_secs(2)).await; + let attempts = pm.reconnect_backoff.read().get(&waiting).map(|s| s.attempts); + run.abort(); + *pm.ibd.write() = None; + let _ = std::fs::remove_dir_all(&dir); + assert_eq!(attempts, Some(3), "a new tip does not clear backoff"); +} + +/// The other half: a dial that connects clears its address's backoff. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_dial_that_connects_clears_its_backoff() { + let (pm, _dir) = mk_pm(); + leave_ibd(&pm); + let l = Listener::bind().await; + pm.learned_addrs.write().push(l.addr); + { + let mut backoff = pm.reconnect_backoff.write(); + let state = backoff.entry(l.addr).or_insert_with(ReconnectState::new); + state.record_failure(AUTOMATIC_RECONNECT_MAX_BACKOFF); + state.record_failure(AUTOMATIC_RECONNECT_MAX_BACKOFF); + state.next_attempt = Instant::now(); + } + pm.reconnect_tick(); + wait_for(&pm, || l.accepts() == 1, "the address is dialled").await; + settle(&pm).await; + assert_eq!(pm.reconnect_backoff.read().get(&l.addr).map(|s| s.attempts), Some(0)); +} + +// ---- Bans and dials ---- + +fn ban(pm: &PeerManager, target: crate::net::ban::BanTarget) { + let now = crate::time::now_secs(); + pm.ban_list.write().add(&target, now, now + 3600).expect("ban"); +} + +/// Core checks bans for automatic connections only: a manual one is opened +/// by the name the operator gave, banned or not (`OpenNetworkConnection`, +/// `net.cpp`). satd skipped a banned added node in the reconnect loop. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_banned_manual_peer_is_dialled_and_a_banned_learned_one_is_not() { + let (pm, _dir) = mk_pm(); + leave_ibd(&pm); + let manual = Listener::bind().await; + let learned = Listener::bind().await; + assert!(pm.addnode_add(&manual.addr.to_string(), PeerAddr::Socket(manual.addr))); + pm.learned_addrs.write().push(learned.addr); + ban(&pm, crate::net::ban::BanTarget::Net(ipnet::IpNet::from(IpAddr::from([127, 0, 0, 1])))); + + pm.reconnect_tick(); + wait_for(&pm, || manual.accepts() == 1, "the banned added node is dialled").await; + settle(&pm).await; + assert_eq!(learned.accepts(), 0, "the banned learned address is not"); + let err = pm + .connect_peer_addr_automatic(&PeerAddr::Socket(learned.addr)) + .await + .expect_err("nor does a seed dial reach it"); + assert!(err.contains("banned"), "{err}"); + assert_eq!(learned.accepts(), 0); +} + +/// The same for onion peers. satd never looked at the ban of an onion host +/// in the reconnect loop or when gossip named one, so `setban .onion`, +/// or a ban for misbehaviour, did not stop automatic dials to it. With no +/// proxy here every onion dial fails, which records a backoff entry: that +/// entry is the trace of a dial attempted. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_banned_onion_host_is_neither_learned_nor_dialled_automatically() { + let (pm, _dir) = mk_pm(); + leave_ibd(&pm); + let host = |c: char| format!("{}.onion", c.to_string().repeat(56)); + let (banned, open, named) = (host('a'), host('b'), host('c')); + + // Learned before the ban, so only the reconnect loop can hold it back. + pm.add_onion_connect_addr(banned.clone(), 8333); + pm.add_onion_connect_addr(open.clone(), 8333); + assert!(pm.add_peer_addr(PeerAddr::Onion { host: named.clone(), port: 8333 })); + for h in [&banned, &named] { + ban(&pm, crate::net::ban::BanTarget::Onion(h.clone())); + } + + pm.reconnect_tick(); + let tried = |h: &str| pm.onion_reconnect_backoff.read().contains_key(&format!("{h}:8333")); + wait_for(&pm, || tried(&open) && tried(&named), "the open host and the named one are dialled").await; + settle(&pm).await; + assert!(!tried(&banned), "the banned learned host is not"); + + // Gossip naming a banned host does not add it. + let later = host('d'); + ban(&pm, crate::net::ban::BanTarget::Onion(later.clone())); + pm.add_onion_connect_addr(later.clone(), 8333); + pm.add_onion_connect_addr(host('e'), 8333); + let listed = |h: &str| { + pm.connect_peer_addrs.read().iter().any(|a| matches!(a, PeerAddr::Onion { host, .. } if host == h)) + }; + assert!(!listed(&later), "a banned onion host is not learned"); + assert!(listed(&host('e')), "an open one is"); +} diff --git a/node/src/net/mod.rs b/node/src/net/mod.rs index 44c03e4c9..a832b2163 100644 --- a/node/src/net/mod.rs +++ b/node/src/net/mod.rs @@ -1,3 +1,4 @@ +pub mod addr_rate; pub mod addrman; pub mod ban; pub mod asmap; diff --git a/node/src/net/peer.rs b/node/src/net/peer.rs index f92ce064e..64f8256ed 100644 --- a/node/src/net/peer.rs +++ b/node/src/net/peer.rs @@ -263,6 +263,9 @@ pub struct PeerInfo { /// lazily, on the first addr-related message the peer sends, so a peer /// that never participates in addr relay is never counted as doing so. pub addr_relay_enabled: bool, + /// The rate limit on the addresses this peer announces, and its + /// `getpeerinfo` counters: Core's address token bucket. + pub addr_rate: crate::net::addr_rate::AddrTokenBucket, /// Peer's minimum fee rate for tx relay (BIP 133 feefilter), in sat/kvB. pub fee_filter: u64, pub conn_time: SystemTime, @@ -330,6 +333,7 @@ impl PeerInfo { wants_addrv2: false, wtxid_relay: false, addr_relay_enabled: false, + addr_rate: crate::net::addr_rate::AddrTokenBucket::new(crate::time::now_micros()), fee_filter: 0, conn_time: std::time::UNIX_EPOCH + std::time::Duration::from_secs(crate::time::now_secs()), permissions: crate::net::permissions::NetPermissions::NONE, @@ -503,8 +507,8 @@ impl PeerInfo { // permissions granted", not "not implemented", which is the // opposite claim for a `-whitelist`ed peer. "permissions": self.permissions.to_strings(), - "addr_processed": 0, - "addr_rate_limited": 0, + "addr_processed": self.addr_rate.processed, + "addr_rate_limited": self.addr_rate.rate_limited, // Core withholds address relay from block-relay-only peers -- // that is the whole point of the connection type. For everyone // else this is latched by `SetupAddressRelay`, not derived from diff --git a/node/src/rpc/server.rs b/node/src/rpc/server.rs index 9b7a7cf24..13ad75b85 100644 --- a/node/src/rpc/server.rs +++ b/node/src/rpc/server.rs @@ -2922,14 +2922,16 @@ pub async fn start( } } "onetry" => { - // A single, un-remembered attempt — block on it and surface the - // result, matching the prior satd behavior (now onion-capable). + // A single, un-remembered attempt — block on it and surface a + // failed dial, as satd always has (now onion-capable). Like + // Core's, it is outside the outbound limits, and a peer + // already connected answers null. let addr = ctx.peer_manager .resolve_peer_target(&addr_str, crate::net::peer::default_p2p_port(ctx.chain_state.network)) .await .map_err(|e| ErrorObjectOwned::owned(-1, e, None::<()>))?; ctx.peer_manager - .connect_peer_addr_with(&addr, v2transport) + .connect_peer_addr_onetry(&addr, v2transport) .await .map_err(|e| ErrorObjectOwned::owned(-1, e, None::<()>))?; } diff --git a/satd/tests/p2p_addr_dials.rs b/satd/tests/p2p_addr_dials.rs new file mode 100644 index 000000000..57ed4dc08 --- /dev/null +++ b/satd/tests/p2p_addr_dials.rs @@ -0,0 +1,308 @@ +//! Addresses a peer hands a regtest node, and the manual connections it +//! opens with its automatic outbound slots full: Bitcoin Core's address rate +//! limit (`net_processing.cpp` `ProcessAddrs`) and its separate limits for +//! manual connections (`net.cpp` `ThreadOpenAddedConnections`, `rpc/net.cpp` +//! `addnode`), exercised over raw connections. + +mod common; + +use bitcoin::consensus::{deserialize, serialize}; +use bitcoin::p2p::message::{NetworkMessage, RawNetworkMessage}; +use bitcoin::p2p::message_network::VersionMessage; +use bitcoin::p2p::{Address, Magic, ServiceFlags}; +use common::{TestNode, find_available_port, poll_until, test_timeout}; +use serde_json::json; +use std::io::{self, Read, Write}; +use std::net::{SocketAddr, TcpListener, TcpStream}; +use std::time::{Duration, Instant}; + +const ADDR: &str = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqdku202"; + +/// A raw peer, read on the test's own thread. +struct Peer { + stream: TcpStream, +} + +impl Peer { + /// Connect to `port` and complete the handshake. + fn connect(port: u16) -> Self { + let addr: SocketAddr = format!("127.0.0.1:{port}").parse().unwrap(); + let deadline = Instant::now() + Duration::from_secs(30); + let stream = loop { + match TcpStream::connect_timeout(&addr, Duration::from_secs(2)) { + Ok(s) => break s, + Err(_) if Instant::now() < deadline => std::thread::sleep(Duration::from_millis(100)), + Err(e) => panic!("connect to {addr}: {e}"), + } + }; + Self::handshake(stream).expect("handshake with the node") + } + + /// Accept the connection the node dials to `listener` within `timeout`. + fn accept(listener: &TcpListener, timeout: Duration) -> Option { + listener.set_nonblocking(true).unwrap(); + let deadline = Instant::now() + timeout; + let stream = loop { + match listener.accept() { + Ok((s, _)) => break s, + Err(e) if e.kind() == io::ErrorKind::WouldBlock && Instant::now() < deadline => { + std::thread::sleep(Duration::from_millis(20)) + } + Err(_) => return None, + } + }; + stream.set_nonblocking(false).unwrap(); + Some(Self::handshake(stream).expect("handshake with the dialling node")) + } + + fn handshake(stream: TcpStream) -> io::Result { + stream.set_nodelay(true).unwrap(); + stream.set_write_timeout(Some(Duration::from_secs(30))).unwrap(); + let mut peer = Peer { stream }; + peer.send(NetworkMessage::Version(version())); + let (mut got_version, mut got_verack) = (false, false); + while !(got_version && got_verack) { + match peer.recv(Duration::from_secs(30))? { + NetworkMessage::Version(_) => { + got_version = true; + peer.send(NetworkMessage::Verack); + } + NetworkMessage::Verack => got_verack = true, + _ => {} + } + } + Ok(peer) + } + + fn send(&mut self, msg: NetworkMessage) { + let _ = self.stream.write_all(&serialize(&RawNetworkMessage::new(Magic::REGTEST, msg))); + } + + fn recv(&mut self, timeout: Duration) -> io::Result { + self.stream.set_read_timeout(Some(timeout)).unwrap(); + let mut header = [0u8; 24]; + self.stream.read_exact(&mut header)?; + let len = u32::from_le_bytes(header[16..20].try_into().unwrap()) as usize; + let mut buf = header.to_vec(); + buf.resize(24 + len, 0); + self.stream.read_exact(&mut buf[24..])?; + let raw: RawNetworkMessage = + deserialize(&buf).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e.to_string()))?; + Ok(raw.into_payload()) + } + + /// Answer pings until the node sends a message `pred` accepts. + fn recv_until(&mut self, timeout: Duration, pred: impl Fn(&NetworkMessage) -> bool) -> Option { + let deadline = Instant::now() + timeout; + loop { + let left = deadline.saturating_duration_since(Instant::now()).max(Duration::from_millis(1)); + match self.recv(left) { + Ok(NetworkMessage::Ping(n)) => self.send(NetworkMessage::Pong(n)), + Ok(m) if pred(&m) => return Some(m), + Ok(_) => {} + Err(_) => return None, + } + } + } + + /// Send `msg` and wait until the node has handled it: the node handles + /// one peer's messages in order, so the `pong` to a `ping` sent after it + /// comes once `msg` is done. + fn send_and_sync(&mut self, msg: NetworkMessage) { + self.send(msg); + let nonce = rand_nonce(); + self.send(NetworkMessage::Ping(nonce)); + assert!( + self.recv_until(test_timeout(30), |m| matches!(m, NetworkMessage::Pong(n) if *n == nonce)) + .is_some(), + "the node answers the ping" + ); + } +} + +fn version() -> VersionMessage { + let services = ServiceFlags::NETWORK | ServiceFlags::WITNESS; + let zero: SocketAddr = "0.0.0.0:0".parse().unwrap(); + VersionMessage { + version: 70016, + services, + timestamp: 1_760_000_000, + receiver: Address::new(&zero, ServiceFlags::NONE), + sender: Address::new(&zero, services), + nonce: rand_nonce(), + user_agent: "/p2p-addr-dials-test/".into(), + start_height: 0, + relay: true, + } +} + +/// A distinct nonce per call, so the node's self-connection check never +/// mistakes one raw peer for another. +fn rand_nonce() -> u64 { + use std::hash::{BuildHasher, Hasher}; + static COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let mut h = std::collections::hash_map::RandomState::new().build_hasher(); + h.write_u64(COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed)); + h.finish() +} + +/// An `addr` of `n` full-node addresses, each in a /16 of its own (so the +/// address book's per-group limit does not apply), from first octet `base`. +fn addr_msg(base: u8, n: usize) -> NetworkMessage { + let services = ServiceFlags::NETWORK | ServiceFlags::WITNESS; + NetworkMessage::Addr( + (0..n) + .map(|i| { + let a = SocketAddr::from(([base + (i / 250) as u8, (i % 250) as u8, 7, 7], 8333)); + (1_760_000_000, Address::new(&a, services)) + }) + .collect(), + ) +} + +/// The one `getpeerinfo` entry for a peer that reached the node on `port`. +fn peer_on(node: &TestNode, port: u16) -> serde_json::Value { + let suffix = format!(":{port}"); + let peers: Vec = node + .rpc_ok("getpeerinfo", vec![]) + .as_array() + .unwrap() + .iter() + .filter(|p| p["addrbind"].as_str().is_some_and(|b| b.ends_with(&suffix))) + .cloned() + .collect(); + assert_eq!(peers.len(), 1, "one peer on {port}: {peers:?}"); + peers[0].clone() +} + +fn addr_counts(node: &TestNode, port: u16) -> (u64, u64) { + let p = peer_on(node, port); + (p["addr_processed"].as_u64().unwrap(), p["addr_rate_limited"].as_u64().unwrap()) +} + +fn book_size(node: &TestNode) -> usize { + node.rpc_ok("getnodeaddresses", vec![json!(0)]).as_array().unwrap().len() +} + +fn now_secs() -> u64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() +} + +/// Core meters the addresses each peer announces with a token bucket: one +/// token to start, then 0.1 a second, and a peer with the `addr` permission +/// is not metered (`ProcessAddrs`; Core's `p2p_addr_relay.py` +/// `rate_limit_tests` makes the same steps). satd took every entry of every +/// message and reported both `getpeerinfo` counters as 0. +#[test] +fn a_peers_addresses_are_rate_limited_unless_it_has_the_addr_permission() { + let p2p = find_available_port(); + let exempt_port = find_available_port(); + let node = TestNode::start(&[ + &format!("--port={p2p}"), + &format!("--bind=127.0.0.1:{p2p}"), + &format!("--whitebind=addr@127.0.0.1:{exempt_port}"), + ]); + let start = now_secs() - 1000; + node.rpc_ok("setmocktime", vec![json!(start)]); + + let mut limited = Peer::connect(p2p); + limited.send_and_sync(addr_msg(1, 1000)); + assert_eq!(addr_counts(&node, p2p), (1, 999), "one token: one address"); + assert_eq!(book_size(&node), 1); + + // A hundred seconds earn ten more. + node.rpc_ok("setmocktime", vec![json!(start + 100)]); + limited.send_and_sync(addr_msg(20, 1000)); + assert_eq!(addr_counts(&node, p2p), (11, 1989)); + assert_eq!(book_size(&node), 11); + + let mut exempt = Peer::connect(exempt_port); + exempt.send_and_sync(addr_msg(40, 1000)); + assert_eq!(addr_counts(&node, exempt_port), (1000, 0), "the addr permission takes them all"); + assert_eq!(book_size(&node), 1011); +} + +/// Core adds a thousand tokens when it sends a peer `getaddr`, so the +/// answer is taken whole. An addr-fetch connection exists for that answer. +#[test] +fn an_addr_fetch_peers_answer_is_taken_whole() { + let node = TestNode::start(&["--v2transport=0"]); + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let target = listener.local_addr().unwrap().to_string(); + node.rpc_ok("addconnection", vec![json!(target), json!("addr-fetch"), json!(false)]); + let mut peer = Peer::accept(&listener, test_timeout(30)).expect("the node dials the addr-fetch peer"); + assert!( + peer.recv_until(test_timeout(30), |m| matches!(m, NetworkMessage::GetAddr)).is_some(), + "the node asks for addresses" + ); + peer.send(addr_msg(1, 600)); + poll_until(|| book_size(&node) == 600, test_timeout(30), "all 600 addresses are learned"); +} + +/// Core counts manual connections apart from the automatic outbound target: +/// `addnode onetry` takes no grant at all, and added nodes have their own +/// limit of eight (`semAddnode`), redialled every minute however many +/// automatic peers there are. satd counted manual connections against the +/// automatic target, so once it was full `onetry` failed with "max outbound +/// connections reached" and an added node whose first dial failed was never +/// dialled again; an `onetry` to a connected peer failed with "already +/// connected", where Core answers null. +#[test] +fn manual_connections_are_outside_the_automatic_outbound_target() { + // `-maxconnections=3` caps the automatic target at three. + let mut node = TestNode::start(&["--v2transport=0", "--maxconnections=3"]); + // Out of IBD: the target is 64 there. + node.mine_blocks(1, ADDR); + + let mut automatic = Vec::new(); + for _ in 0..3 { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let target = listener.local_addr().unwrap().to_string(); + node.rpc_ok("addconnection", vec![json!(target), json!("outbound-full-relay"), json!(false)]); + automatic.push(Peer::accept(&listener, test_timeout(30)).expect("an automatic peer")); + } + let outbound = |node: &TestNode| { + node.rpc_ok("getpeerinfo", vec![]) + .as_array() + .unwrap() + .iter() + .filter(|p| p["inbound"] == json!(false)) + .map(|p| (p["addr"].as_str().unwrap().to_string(), p["connection_type"].as_str().unwrap().to_string())) + .collect::>() + }; + poll_until(|| outbound(&node).len() == 3, test_timeout(20), "three automatic peers"); + + // `onetry`, with the automatic slots full. + let one = TcpListener::bind("127.0.0.1:0").unwrap(); + let one_addr = one.local_addr().unwrap().to_string(); + let out = node.rpc_call_with_params("addnode", vec![json!(one_addr), json!("onetry")]).unwrap(); + assert!(out["error"].is_null(), "onetry is outside the automatic target: {out}"); + let _one = Peer::accept(&one, test_timeout(30)).expect("the onetry dial"); + poll_until( + || outbound(&node).contains(&(one_addr.clone(), "manual".to_string())), + test_timeout(20), + "the onetry peer is a manual connection", + ); + // And again, to the connected peer: null, as in Core. + let out = node.rpc_call_with_params("addnode", vec![json!(one_addr), json!("onetry")]).unwrap(); + assert!(out["error"].is_null(), "onetry to a connected peer answers null: {out}"); + assert_eq!(out["result"], json!(null)); + + // An added node whose first dial fails is dialled again. + let port = find_available_port(); + let added = format!("127.0.0.1:{port}"); + node.rpc_ok("addnode", vec![json!(added), json!("add")]); + std::thread::sleep(Duration::from_secs(1)); + let listener = TcpListener::bind(&added).unwrap(); + let _added = Peer::accept(&listener, test_timeout(40)).expect("the added node is dialled again"); + poll_until( + || outbound(&node).contains(&(added.clone(), "manual".to_string())), + test_timeout(20), + "the added node is a manual connection", + ); + assert_eq!(outbound(&node).len(), 5, "three automatic peers and two manual ones"); + node.stop(); +} diff --git a/satd/tests/regtest.rs b/satd/tests/regtest.rs index 14d272c9c..3d6ed1e65 100644 --- a/satd/tests/regtest.rs +++ b/satd/tests/regtest.rs @@ -25392,7 +25392,9 @@ fn an_address_that_names_no_host_is_neither_learned_nor_dialled() { use serde_json::json; use std::net::{Ipv4Addr, Ipv6Addr, SocketAddr}; - let node = TestNode::start(&["--v2transport=0"]); + // `addr`: the gossiper's addresses are not rate limited (Core's + // address token bucket), so all six entries are looked at. + let node = TestNode::start(&["--v2transport=0", "--whitelist=addr@127.0.0.1"]); // Where a dial to `0.0.0.0:` lands. let local = Inbound::bind(); let port = local.addr.port(); @@ -25545,6 +25547,8 @@ fn off_regtest_a_gossiped_private_address_is_neither_learned_nor_dialled() { "--dnsseed=0", "--fixedseeds=0", &format!("--addnode={}", named.addr), + // Both announced addresses are looked at, past the rate limit. + "--whitelist=addr@127.0.0.1", ], ); let stream = gossip::take_dial(&named, Magic::SIGNET); From b2a34d2cfd2bb142f38376e1157cc72eb134e3b0 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Thu, 8 Oct 2026 11:28:49 -0600 Subject: [PATCH 08/13] p2p: exempt a two-address test peer from the address rate limit Co-Authored-By: Claude Opus 5.5 --- node/src/net/manager.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index 77d222051..cdf374810 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -15043,6 +15043,9 @@ mod tests { pm.peers .write() .insert(1, mk_handle(1, announcer, Direction::Inbound, PeerState::Connected)); + // Two addresses from one peer: past the address rate limit's first + // token, so the peer has the `addr` permission. + pm.peers.write().get_mut(&1).unwrap().info.permissions.addr = true; let v1: SocketAddr = "198.51.100.1:8333".parse().unwrap(); let v1_services = ServiceFlags::NETWORK | ServiceFlags::WITNESS | ServiceFlags::COMPACT_FILTERS; From 68ce859869812cc8b70d12d7b5c87a882115341e Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Thu, 8 Oct 2026 11:41:32 -0600 Subject: [PATCH 09/13] p2p: count only the announced addresses in the addr-fetch test Co-Authored-By: Claude Opus 5.5 --- satd/tests/p2p_addr_dials.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/satd/tests/p2p_addr_dials.rs b/satd/tests/p2p_addr_dials.rs index 57ed4dc08..fa25059f2 100644 --- a/satd/tests/p2p_addr_dials.rs +++ b/satd/tests/p2p_addr_dials.rs @@ -180,8 +180,15 @@ fn addr_counts(node: &TestNode, port: u16) -> (u64, u64) { (p["addr_processed"].as_u64().unwrap(), p["addr_rate_limited"].as_u64().unwrap()) } +/// The addresses `addr_msg` announced that the address book holds (the +/// book also holds the outbound peers the node connected to). fn book_size(node: &TestNode) -> usize { - node.rpc_ok("getnodeaddresses", vec![json!(0)]).as_array().unwrap().len() + node.rpc_ok("getnodeaddresses", vec![json!(0)]) + .as_array() + .unwrap() + .iter() + .filter(|e| e["address"].as_str().is_some_and(|a| a.ends_with(".7.7"))) + .count() } fn now_secs() -> u64 { From d8a9e5698ee7b22956deb465fc129bcd99ade06d Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Tue, 13 Oct 2026 17:39:39 -0600 Subject: [PATCH 10/13] p2p: setban of an onion host disconnects that onion peer Co-Authored-By: Claude Opus 5.5 --- node/src/net/manager.rs | 15 ++++++++++-- node/src/net/manager_addrdials_tests.rs | 31 +++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index 6b4d12c1f..9311dd86c 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -3671,14 +3671,25 @@ impl PeerManager { .already_banned(target, is_subnet, crate::time::now_secs()) } - /// Disconnect every peer whose IP falls within `target`. + /// Disconnect every peer `target` covers: an onion peer by its onion + /// host, any other by its IP. An onion peer's IP is the `0.0.0.0` + /// placeholder they all share, so matching it said nothing about the + /// peer: `setban .onion` left that peer connected. fn disconnect_banned_peers(&self, target: &crate::net::ban::BanTarget) { let ids_to_disconnect: Vec = { let peers = self.peers.read(); peers .iter() .filter_map(|(id, handle)| { - if target.contains_addr(&handle.info.addr.ip()) { + let covered = match (target, handle.info.onion_host.as_deref()) { + (crate::net::ban::BanTarget::Onion(host), Some(peer_host)) => host == peer_host, + (crate::net::ban::BanTarget::Onion(_), None) => false, + (crate::net::ban::BanTarget::Net(_), Some(_)) => false, + (crate::net::ban::BanTarget::Net(_), None) => { + target.contains_addr(&handle.info.addr.ip()) + } + }; + if covered { Some(*id) } else { None diff --git a/node/src/net/manager_addrdials_tests.rs b/node/src/net/manager_addrdials_tests.rs index 756b49740..ca125da36 100644 --- a/node/src/net/manager_addrdials_tests.rs +++ b/node/src/net/manager_addrdials_tests.rs @@ -571,3 +571,34 @@ async fn a_banned_onion_host_is_neither_learned_nor_dialled_automatically() { assert!(!listed(&later), "a banned onion host is not learned"); assert!(listed(&host('e')), "an open one is"); } + +/// `setban .onion` disconnects a connected peer at that onion host, as +/// Core's `setban` disconnects the peers the ban covers. satd matched a ban +/// against peers' IP addresses only, and an onion peer's is the `0.0.0.0` +/// placeholder, so the peer stayed. +#[test] +fn setban_of_an_onion_host_disconnects_that_peer_only() { + let (pm, _dir) = mk_pm(); + let placeholder = SocketAddr::from(([0, 0, 0, 0], 8333)); + let host = |c: char| format!("{}.onion", c.to_string().repeat(56)); + for (id, h) in [(1, host('a')), (2, host('b'))] { + let mut info = peer(id, placeholder, Direction::Outbound); + info.onion_host = Some(h); + add_peer(&pm, info); + } + add_peer(&pm, peer(3, SocketAddr::from(([198, 51, 100, 3], 8333)), Direction::Outbound)); + let now = crate::time::now_secs(); + pm.set_ban_subnet(&crate::net::ban::BanTarget::Onion(host('a')), true, now, now + 3600).unwrap(); + let ids: std::collections::BTreeSet = pm.peers.read().keys().copied().collect(); + assert_eq!(ids, [2, 3].into_iter().collect(), "only the banned onion peer is disconnected"); + + // A ban of an IP range does not reach onion peers through their placeholder. + pm.set_ban_subnet( + &crate::net::ban::BanTarget::Net("0.0.0.0/8".parse().unwrap()), + true, + now, + now + 3600, + ) + .unwrap(); + assert!(pm.peers.read().contains_key(&2), "the other onion peer stays"); +} From fa938d63c1a208309029bddf46c5da0d05f722eb Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Tue, 13 Oct 2026 17:54:50 -0600 Subject: [PATCH 11/13] p2p: a full address book replaces a terrible entry Co-Authored-By: Claude Opus 5.5 --- node/src/net/addrman.rs | 65 ++++++++++++++++++++++--- node/src/net/manager.rs | 20 +++++++- node/src/net/manager_addrdials_tests.rs | 49 +++++++++++++++++++ 3 files changed, 126 insertions(+), 8 deletions(-) diff --git a/node/src/net/addrman.rs b/node/src/net/addrman.rs index 27f9eaec6..c132b9fd8 100644 --- a/node/src/net/addrman.rs +++ b/node/src/net/addrman.rs @@ -92,6 +92,29 @@ pub struct AddrEntry { group: Vec, } +/// Core's `ADDRMAN_HORIZON`: an address not seen for this long is terrible. +const ADDRMAN_HORIZON_SECS: u64 = 30 * 24 * 60 * 60; +/// Core's `ADDRMAN_RETRIES`: failed attempts, with no success ever, that +/// make an address terrible. +const ADDRMAN_RETRIES: u32 = 3; +/// Core's `ADDRMAN_MAX_FAILURES` within `ADDRMAN_MIN_FAIL` (7 days). +const ADDRMAN_MAX_FAILURES: u32 = 10; +const ADDRMAN_MIN_FAIL_SECS: u64 = 7 * 24 * 60 * 60; + +impl AddrEntry { + /// Core's `AddrInfo::IsTerrible`, without its one-minute grace after an + /// attempt (satd does not record when it last tried): seen in the + /// future, not seen for 30 days, tried three times and never reached, + /// or ten failures with no success in a week. + pub fn is_terrible(&self, now: u64) -> bool { + self.last_seen > now + 10 * 60 + || now.saturating_sub(self.last_seen) > ADDRMAN_HORIZON_SECS + || (self.last_success == 0 && self.attempts >= ADDRMAN_RETRIES) + || (now.saturating_sub(self.last_success) > ADDRMAN_MIN_FAIL_SECS + && self.attempts >= ADDRMAN_MAX_FAILURES) + } +} + /// Network group key used for bucketing. IPv4 → the `/16`; IPv6 → the /// `/32`. A custom grouping (e.g. `-asmap` ASN) can be installed via /// [`AddrMan::set_group_fn`]. @@ -251,19 +274,47 @@ impl AddrMan { /// of the peer that announced it. A refresh of a known address updates /// its last-seen time only, as before. pub fn add_from(&mut self, addr: SocketAddr, now: u64, services: u64, source: IpAddr) -> bool { + self.add_from_replacing(addr, now, services, source).0 + } + + /// [`add_from`](Self::add_from), and when the book is full, make room + /// for the new address by dropping one *new* entry that is terrible + /// ([`AddrEntry::is_terrible`]), as Core's address manager replaces + /// terrible entries. Returns whether `addr` was inserted, and the + /// address dropped for it. + /// + /// Without this a full book refused every address it did not already + /// hold, for good: nothing ever left it. + pub fn add_from_replacing( + &mut self, + addr: SocketAddr, + now: u64, + services: u64, + source: IpAddr, + ) -> (bool, Option) { if !self.admits(&addr) { - return false; + return (false, None); } if let Some(e) = self.entries.get_mut(&addr) { e.last_seen = now; - return false; - } - if self.entries.len() >= MAX_ENTRIES { - return false; + return (false, None); } let group = self.group_of(&addr); if self.new_count_in_group(&group) >= MAX_NEW_PER_GROUP { - return false; + return (false, None); + } + let mut evicted = None; + if self.entries.len() >= MAX_ENTRIES { + let Some(victim) = self + .entries + .values() + .find(|e| !e.tried && e.is_terrible(now)) + .map(|e| e.addr) + else { + return (false, None); + }; + self.entries.remove(&victim); + evicted = Some(victim); } self.entries.insert( addr, @@ -278,7 +329,7 @@ impl AddrMan { group, }, ); - true + (true, evicted) } /// Promote an address to the *tried* table after a successful connect. diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index 9311dd86c..75ae70026 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -451,6 +451,13 @@ impl LearnedAddrs { true } + /// Drop `addr` from the list. + fn remove(&mut self, addr: &SocketAddr) { + if self.members.remove(addr) { + self.order.retain(|a| a != addr); + } + } + #[cfg(test)] fn contains(&self, addr: &SocketAddr) -> bool { self.members.contains(addr) @@ -1938,10 +1945,16 @@ impl PeerManager { tracing::debug!(%addr, "Ignoring a gossiped address the address book does not take"); return; } - if !book.add_from(addr, now_unix_secs(), services, source) && !book.contains(&addr) { + let (inserted, evicted) = book.add_from_replacing(addr, now_unix_secs(), services, source); + if !inserted && !book.contains(&addr) { tracing::debug!(%addr, "Not dialling a gossiped address the address book has no room for"); return; } + // A full book makes room by dropping a terrible entry; it + // leaves the dial list with it. + if let Some(gone) = evicted { + self.learned_addrs.write().remove(&gone); + } } self.push_learned(addr); } @@ -4155,6 +4168,11 @@ impl PeerManager { }; if let Err(e) = &result { tracing::debug!(%target, "Reconnect failed: {e}"); + // What makes an address terrible, and so replaceable once + // the book is full (Core's `Attempt`). + if let PeerAddr::Socket(sa) = &target { + pm.addrman.write().mark_attempt(*sa); + } } let connected = result.is_ok(); let update = |state: &mut ReconnectState| { diff --git a/node/src/net/manager_addrdials_tests.rs b/node/src/net/manager_addrdials_tests.rs index ca125da36..87d3f056b 100644 --- a/node/src/net/manager_addrdials_tests.rs +++ b/node/src/net/manager_addrdials_tests.rs @@ -602,3 +602,52 @@ fn setban_of_an_onion_host_disconnects_that_peer_only() { .unwrap(); assert!(pm.peers.read().contains_key(&2), "the other onion peer stays"); } + +/// A full address book makes room for a new address by dropping a terrible +/// entry (tried three times, never reached), as Core's does, and the dropped +/// address leaves the dial list. satd's book refused every new address once +/// full, for good. With nothing terrible to drop it still refuses. +#[test] +fn a_full_book_replaces_a_terrible_entry() { + let (pm, _dir) = mk_pm(); + let source = IpAddr::from([198, 51, 100, 1]); + let full = crate::net::addrman::MAX_ENTRIES; + // 64 per /16, across enough groups to fill the book. + let addr_at = |i: usize| SocketAddr::from(([11 + (i / 64 / 256) as u8, ((i / 64) % 256) as u8, 1, (i % 64) as u8 + 1], 8333)); + for i in 0..full { + pm.add_learned_addr_from(addr_at(i), 9, source); + } + assert_eq!(pm.addrman.read().len(), full, "fixture: the book is full"); + + let fresh = SocketAddr::from(([99, 1, 1, 1], 8333)); + pm.add_learned_addr_from(fresh, 9, source); + assert!(!pm.addrman.read().contains(&fresh), "nothing terrible: refused"); + + let stale = addr_at(0); + for _ in 0..3 { + pm.addrman.write().mark_attempt(stale); + } + pm.add_learned_addr_from(fresh, 9, source); + assert!(pm.addrman.read().contains(&fresh), "the new address takes the terrible one's place"); + assert!(!pm.addrman.read().contains(&stale)); + assert_eq!(pm.addrman.read().len(), full); + assert!(pm.learned_addrs.read().contains(&fresh), "and is dialled"); + assert!(!pm.learned_addrs.read().contains(&stale), "the dropped one is not"); +} + +/// A failed redial counts as an attempt in the address book: what makes an +/// unreachable address terrible. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_failed_redial_counts_as_an_attempt() { + let (pm, _dir) = mk_pm(); + leave_ibd(&pm); + let closed = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = closed.local_addr().unwrap(); + drop(closed); + pm.add_learned_addr_from(addr, 9, IpAddr::from([198, 51, 100, 1])); + pm.reconnect_tick(); + wait_for(&pm, || pm.reconnect_backoff.read().contains_key(&addr), "the dial fails").await; + settle(&pm).await; + let attempts = pm.addrman.read().iter().find(|e| e.addr == addr).map(|e| e.attempts); + assert_eq!(attempts, Some(1)); +} From 566eefbbe18e887a561ad639ce07c0963936b79a Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Tue, 13 Oct 2026 18:00:13 -0600 Subject: [PATCH 12/13] p2p: a full network group replaces a terrible entry; a replaced address's backoff goes Co-Authored-By: Claude Opus 5.5 --- node/src/net/addrman.rs | 22 +++++++++++++------ node/src/net/manager.rs | 6 +++++- node/src/net/manager_addrdials_tests.rs | 28 +++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/node/src/net/addrman.rs b/node/src/net/addrman.rs index c132b9fd8..aed342f3f 100644 --- a/node/src/net/addrman.rs +++ b/node/src/net/addrman.rs @@ -277,8 +277,9 @@ impl AddrMan { self.add_from_replacing(addr, now, services, source).0 } - /// [`add_from`](Self::add_from), and when the book is full, make room - /// for the new address by dropping one *new* entry that is terrible + /// [`add_from`](Self::add_from), and when the book or the address's + /// network group is full, make room for the new address by dropping one + /// *new* entry (of that group, when the group is full) that is terrible /// ([`AddrEntry::is_terrible`]), as Core's address manager replaces /// terrible entries. Returns whether `addr` was inserted, and the /// address dropped for it. @@ -300,11 +301,20 @@ impl AddrMan { return (false, None); } let group = self.group_of(&addr); - if self.new_count_in_group(&group) >= MAX_NEW_PER_GROUP { - return (false, None); - } let mut evicted = None; - if self.entries.len() >= MAX_ENTRIES { + if self.new_count_in_group(&group) >= MAX_NEW_PER_GROUP { + // The group is full: make room in it, or refuse. + let Some(victim) = self + .entries + .values() + .find(|e| !e.tried && e.group == group && e.is_terrible(now)) + .map(|e| e.addr) + else { + return (false, None); + }; + self.entries.remove(&victim); + evicted = Some(victim); + } else if self.entries.len() >= MAX_ENTRIES { let Some(victim) = self .entries .values() diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index 75ae70026..56f37f7ea 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -458,7 +458,6 @@ impl LearnedAddrs { } } - #[cfg(test)] fn contains(&self, addr: &SocketAddr) -> bool { self.members.contains(addr) } @@ -1954,6 +1953,7 @@ impl PeerManager { // leaves the dial list with it. if let Some(gone) = evicted { self.learned_addrs.write().remove(&gone); + self.reconnect_backoff.write().remove(&gone); } } self.push_learned(addr); @@ -4183,6 +4183,10 @@ impl PeerManager { } }; match &target { + // Not for an address that left both lists while the dial ran + // (the address book replaced it): its state would never go. + PeerAddr::Socket(sa) + if !pm.learned_addrs.read().contains(sa) && !pm.manual_addrs.read().contains(sa) => {} PeerAddr::Socket(sa) => { update(pm.reconnect_backoff.write().entry(*sa).or_insert_with(ReconnectState::new)) } diff --git a/node/src/net/manager_addrdials_tests.rs b/node/src/net/manager_addrdials_tests.rs index 87d3f056b..8d0664ea7 100644 --- a/node/src/net/manager_addrdials_tests.rs +++ b/node/src/net/manager_addrdials_tests.rs @@ -651,3 +651,31 @@ async fn a_failed_redial_counts_as_an_attempt() { let attempts = pm.addrman.read().iter().find(|e| e.addr == addr).map(|e| e.attempts); assert_eq!(attempts, Some(1)); } + +/// The same within a network group: a full group (64 new addresses) makes +/// room by dropping a terrible entry of that group, and the dropped +/// address's reconnect backoff goes with it. +#[test] +fn a_full_group_replaces_a_terrible_entry_and_its_backoff() { + let (pm, _dir) = mk_pm(); + let source = IpAddr::from([198, 51, 100, 1]); + let in_group = |i: u8| SocketAddr::from(([44, 1, 0, i + 1], 8333)); + for i in 0..64 { + pm.add_learned_addr_from(in_group(i), 9, source); + } + let fresh = in_group(100); + pm.add_learned_addr_from(fresh, 9, source); + assert!(!pm.addrman.read().contains(&fresh), "nothing terrible: refused"); + + let stale = in_group(0); + for _ in 0..3 { + pm.addrman.write().mark_attempt(stale); + } + pm.reconnect_backoff.write().insert(stale, ReconnectState::new()); + pm.add_learned_addr_from(fresh, 9, source); + assert!(pm.addrman.read().contains(&fresh)); + assert!(!pm.addrman.read().contains(&stale)); + assert!(pm.learned_addrs.read().contains(&fresh)); + assert!(!pm.learned_addrs.read().contains(&stale)); + assert!(!pm.reconnect_backoff.read().contains_key(&stale), "its backoff is dropped"); +} From c3b12e7f135517e4166cef06fa8497ef0063a230 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Tue, 13 Oct 2026 18:05:23 -0600 Subject: [PATCH 13/13] p2p: a full book of terrible tried entries takes new addresses; lock the backoff while checking membership Co-Authored-By: Claude Opus 5.5 --- node/src/net/addrman.rs | 19 ++++++++++++------- node/src/net/manager.rs | 11 ++++++++--- node/src/net/manager_addrdials_tests.rs | 23 +++++++++++++++++++++++ 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/node/src/net/addrman.rs b/node/src/net/addrman.rs index aed342f3f..0dd963226 100644 --- a/node/src/net/addrman.rs +++ b/node/src/net/addrman.rs @@ -279,7 +279,8 @@ impl AddrMan { /// [`add_from`](Self::add_from), and when the book or the address's /// network group is full, make room for the new address by dropping one - /// *new* entry (of that group, when the group is full) that is terrible + /// entry (of that group and new, when the group is full; new before + /// tried otherwise) that is terrible /// ([`AddrEntry::is_terrible`]), as Core's address manager replaces /// terrible entries. Returns whether `addr` was inserted, and the /// address dropped for it. @@ -315,12 +316,16 @@ impl AddrMan { self.entries.remove(&victim); evicted = Some(victim); } else if self.entries.len() >= MAX_ENTRIES { - let Some(victim) = self - .entries - .values() - .find(|e| !e.tried && e.is_terrible(now)) - .map(|e| e.addr) - else { + // A terrible new entry first, then a terrible tried one: Core + // keeps the two tables apart, so a full tried table never shuts + // new addresses out there; one shared cap must not either. + let terrible = |tried: bool| { + self.entries + .values() + .find(|e| e.tried == tried && e.is_terrible(now)) + .map(|e| e.addr) + }; + let Some(victim) = terrible(false).or_else(|| terrible(true)) else { return (false, None); }; self.entries.remove(&victim); diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index 56f37f7ea..42ba23abc 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -4185,10 +4185,15 @@ impl PeerManager { match &target { // Not for an address that left both lists while the dial ran // (the address book replaced it): its state would never go. - PeerAddr::Socket(sa) - if !pm.learned_addrs.read().contains(sa) && !pm.manual_addrs.read().contains(sa) => {} + // Checked with the backoff map locked, so a replacement that + // removes the address's state either sees this entry (and + // removes it after) or has already taken the address off the + // list (and this adds nothing). PeerAddr::Socket(sa) => { - update(pm.reconnect_backoff.write().entry(*sa).or_insert_with(ReconnectState::new)) + let mut backoff = pm.reconnect_backoff.write(); + if pm.learned_addrs.read().contains(sa) || pm.manual_addrs.read().contains(sa) { + update(backoff.entry(*sa).or_insert_with(ReconnectState::new)); + } } PeerAddr::Onion { .. } => update( pm.onion_reconnect_backoff diff --git a/node/src/net/manager_addrdials_tests.rs b/node/src/net/manager_addrdials_tests.rs index 8d0664ea7..ef7ddbc00 100644 --- a/node/src/net/manager_addrdials_tests.rs +++ b/node/src/net/manager_addrdials_tests.rs @@ -679,3 +679,26 @@ fn a_full_group_replaces_a_terrible_entry_and_its_backoff() { assert!(!pm.learned_addrs.read().contains(&stale)); assert!(!pm.reconnect_backoff.read().contains_key(&stale), "its backoff is dropped"); } + +/// A book full of tried addresses, all unseen for over 30 days, still makes +/// room for a new one: Core keeps its tried table apart from its new one, so +/// tried entries never shut new addresses out. +#[test] +fn a_book_full_of_stale_tried_entries_takes_a_new_address() { + let (pm, _dir) = mk_pm(); + let full = crate::net::addrman::MAX_ENTRIES; + let long_ago = crate::time::now_secs() - 31 * 24 * 60 * 60; + { + let mut book = pm.addrman.write(); + for i in 0..full { + let a = SocketAddr::from(([11 + (i / 65536) as u8, ((i / 256) % 256) as u8, (i % 256) as u8, 1], 8333)); + book.mark_good(a, long_ago); + } + assert_eq!(book.len(), full, "fixture: the book is full of tried entries"); + } + let fresh = SocketAddr::from(([99, 1, 1, 1], 8333)); + pm.add_learned_addr_from(fresh, 9, IpAddr::from([198, 51, 100, 1])); + assert!(pm.addrman.read().contains(&fresh)); + assert!(pm.learned_addrs.read().contains(&fresh)); + assert_eq!(pm.addrman.read().len(), full); +}