From 8061ef1ac2decc8630f3cf1e611ee24af5144699 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Wed, 7 Oct 2026 21:28:57 -0600 Subject: [PATCH] Bound the unknown-parent block buffer A block whose parent had no index entry was parked in the block processor's buffer keyed by its prev hash: up to 8192 entries with no byte bound, no per-peer bound and no expiry, whether or not this node had asked for the block, before any proof-of-work or size check. Each entry kept its sender's in-flight guard, so that peer's pongs waited for the 60-second bound until the block's parent connected, and the block went through the per-input fee pass before accept_block refused it. Bitcoin Core keeps no such block: AcceptBlockHeader runs CheckBlockHeader (validation.cpp:4206) and then refuses an unknown parent as prev-blk-not-found (validation.cpp:4216). satd answers a block inv with a getdata, so a block it asked for can arrive before the parent it is still learning about, and keeping that one saves a second download. Now: - Only a block this node asked for (Core's IsBlockRequested, read in handle_block before the request record is cleared) may wait. - A waiting block must pass check_proof_of_work_bounded and be at most MAX_BLOCK_SERIALIZED_SIZE (4,000,000) bytes. - At most 4 per peer, 64 blocks and 32 MB in all; the sender's oldest goes first when it is at its share, then the oldest of all. Each block expires after 5 minutes. - The in-flight guard is counted out when the block is set aside, as Core is done with it once refused. - A block whose parent is unknown skips the fee pass and accept_block. - The sender is asked for headers, as satd already does for a cmpctblock on an unknown parent. - A block on a parent marked invalid is not kept. - Two blocks on the same parent can both wait; when one fails to connect the next is tried. Co-Authored-By: Claude Opus 5.5 (1M context) --- CORE_DIFFERENCES.md | 1 + node/src/net/manager.rs | 121 +++++-- node/src/net/manager_blockaccept_tests.rs | 3 + node/src/net/manager_orphanbuf_tests.rs | 127 +++++++ node/src/net/mod.rs | 1 + node/src/net/orphan_blocks.rs | 376 ++++++++++++++++++++ satd/tests/compact_blocks.rs | 3 + satd/tests/compact_blocks/unknown_parent.rs | 80 +++++ 8 files changed, 688 insertions(+), 24 deletions(-) create mode 100644 node/src/net/manager_orphanbuf_tests.rs create mode 100644 node/src/net/orphan_blocks.rs create mode 100644 satd/tests/compact_blocks/unknown_parent.rs diff --git a/CORE_DIFFERENCES.md b/CORE_DIFFERENCES.md index 86f081400..1de20af07 100644 --- a/CORE_DIFFERENCES.md +++ b/CORE_DIFFERENCES.md @@ -834,6 +834,7 @@ default behavior set the corresponding flag. | Invalid header in a `cmpctblock` | penalised for most classes of header invalidity (`MaybePunishNodeForBlock` with `via_compact_block`) | ignored, never penalised | A compact block is pushed, not requested, and a header that fails validation leaves no state behind; the block is simply not reconstructed. A `headers` message is still penalised as in Core. | | `cmpctblock` prefill (`-cmpctblockprefill`) | not available (proposed in #35558) | off; opt in | An extension Core does not ship yet. With it on, a block is announced with the transactions this node lacked when it arrived prefilled, up to `-cmpctblockprefillbytes` (default 8192); receivers need nothing new, since BIP 152 already requires accepting any well-formed prefill. Off, satd prefills the coinbase only, as Core does. | | `sendheaders` to an outbound peer | once the peer has shown a block past the minimum chain work (`MaybeSendSendHeaders`) | in the handshake | satd answers a block `inv` with a `getdata` for the block before it has the header, where Core sends `getheaders`, so a peer that mines while satd syncs from it would hand it blocks out of order until header announcements start. Inbound peers are sent it on Core's condition, and every peer at most once. | +| A block whose parent is unknown | refused as `prev-blk-not-found`, nothing kept, and the sender discouraged (`MaybePunishNodeForBlock`) | kept until its parent connects if this node asked for it, otherwise dropped; the sender is asked for headers and not penalised | Because a block `inv` is answered with a `getdata` (row above), a block the node asked for can arrive before the parent it is still learning about, and keeping it saves fetching it again. A kept block must carry valid proof of work within the network's limit and be at most 4,000,000 bytes; at most 4 are kept per peer and 64 (32 MB) in all, the oldest going first, each for at most 5 minutes. | | Compact block reconstructions per block | at most three, unless we requested the block from the sender | at most three, always | satd's tip-following fetch asks every peer for a missing block, so Core's "requested from this peer" exemption would cover nearly every peer. The full block that request asked for still arrives; a sender we asked for a `cmpctblock` is asked for the full block instead. | | WebSocket upgrade by an `-rpcwhitelist`-restricted user | not applicable (Core serves no WebSocket) | refused with HTTP 403 | satd's JSON-RPC listeners accept WebSocket, and calls over an open WebSocket do not pass the layer that checks a user's method list, so a restricted user may not open one. Unrestricted users, and bearer tokens (scoped by their capabilities, not by `-rpcwhitelist`), are unaffected. `-rpcdisableauth` cannot be combined with an allowlist: that surface authenticates no user to restrict. | | `--listenonion` | on (no-op without Tor) | off (on if `-torcontrol` set) | Core defaults it on, but it's a silent no-op unless a Tor control port is reachable; satd defaults it off to avoid dialing the control port on every boot. When on, satd creates a v3 hidden service via `-torcontrol` (default `127.0.0.1:9051`). An explicit `-torcontrol` implies `-listenonion=1`; `-listenonion=0` forces it off. | diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index c460eb4eb..d5fae5477 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -414,9 +414,11 @@ pub enum NetEvent { } /// A block handed to the block processor: the peer it came from, that peer's -/// counters, and the block. +/// counters, the block, its in-flight guard, and whether this node asked for +/// it. Only a block the node asked for may wait for a parent it does not +/// know yet (see [`crate::net::orphan_blocks`]). type IncomingBlock = - (PeerId, Option>, bitcoin::Block, crate::net::flow::InFlight); + (PeerId, Option>, bitcoin::Block, crate::net::flow::InFlight, bool); /// BIP 152: at most this many peers are asked to announce blocks to us as /// `cmpctblock`s (high-bandwidth mode). @@ -5346,6 +5348,15 @@ impl PeerManager { self.peers.read().get(&id).map(|h| h.flow.clone()) } + /// Whether this node has asked any peer for `hash` recently enough to + /// still expect it: Core's `IsBlockRequested`. + fn block_is_requested(&self, hash: &bitcoin::BlockHash) -> bool { + self.in_flight_blocks + .read() + .values() + .any(|asked| asked.get(hash).is_some_and(|r| r.at.elapsed() < BLOCK_IN_FLIGHT_TTL)) + } + fn handle_block( &self, id: PeerId, @@ -5355,6 +5366,10 @@ impl PeerManager { if self.reject_if_mutated(id, &block) { return; } + // Read before `note_block_arrived` clears the record. Core reads + // `IsBlockRequested` at the same point (`net_processing.cpp:4800`); + // here it decides whether the block may wait for an unknown parent. + let requested = self.block_is_requested(&block.block_hash()); self.note_block_arrived(&block.block_hash()); self.note_peer_has_block(id, block.block_hash()); // A peer that pushes a block has it, so the same availability rule @@ -5437,7 +5452,7 @@ impl PeerManager { return; } // Normal mode - let _ = self.block_tx.send((id, self.peer_stats(id), block, in_flight)); + let _ = self.block_tx.send((id, self.peer_stats(id), block, in_flight, requested)); } /// While an AssumeUTXO background validator is attached, refuse to @@ -6027,15 +6042,14 @@ impl PeerManager { // Normal mode: process blocks from the channel. // Periodically check if the IBD scheduler was activated (header download completed // while we were in normal mode), and switch to the IBD connect loop if so. - // The in-flight guard rides along with the buffered block: a block - // waiting on a parent it has not seen is still work this peer sent - // that the node has not finished with, and a pong behind it must - // wait. Dropping the entry — connected, or evicted when the buffer - // fills — is what finally counts it out. - let mut block_buffer: HashMap< - bitcoin::BlockHash, - (bitcoin::Block, crate::net::flow::InFlight), - > = HashMap::new(); + // + // A block this node asked for whose parent it does not know yet waits + // here, within the bounds `OrphanBlocks` enforces, and connects when + // the parent becomes the tip. Its in-flight guard is not kept with it: + // Core is done with such a block once it has refused it as + // `prev-blk-not-found`, so a pong behind it is answered then, and not + // after a parent that may never come. + let mut orphan_blocks = crate::net::orphan_blocks::OrphanBlocks::new(); loop { // Shutdown: stop connecting. `join_connectors` is waiting for // this thread to exit before the shutdown flush. @@ -6082,10 +6096,20 @@ impl PeerManager { // maintenance (and park peer tasks in `handle_block_ibd`) for // the duration. drop(ready); + orphan_blocks.expire(Instant::now()); - // Drain all available blocks from the channel - while let Ok((sender, sender_stats, block, in_flight)) = rx.try_recv() { + // Drain all available blocks from the channel. Each block's + // in-flight guard is counted out at the end of its iteration. + while let Ok((sender, sender_stats, block, _in_flight, requested)) = rx.try_recv() { let hash = block.block_hash(); + // A block whose parent has no index entry cannot connect yet, + // and `accept_block` would refuse it as `PrevBlockNotFound`. + // It skips that and the fee pass below, which looks up a coin + // per input. + if chain_state.get_block_index(&block.header.prev_blockhash).is_none() { + Self::hold_for_parent(&mut orphan_blocks, sender, block, requested, network, &peer_manager); + continue; + } // Compute fees BEFORE accept_block — connect_block removes spent coins. let fees = Self::compute_block_fee_rates(&block, &chain_state); match chain_state.accept_block(&block) { @@ -6137,11 +6161,11 @@ impl PeerManager { pm.block_became_tip(sender, &block, height); } } - // Drain buffer + // Connect the blocks that were waiting for this one. loop { let tip = chain_state.tip_hash(); - match block_buffer.remove(&tip) { - Some((b, _in_flight)) => { + match orphan_blocks.take_child_of(&tip) { + Some(b) => { let b_fees = Self::compute_block_fee_rates(&b, &chain_state); match chain_state.accept_block(&b) { Ok(acc) => { @@ -6159,7 +6183,10 @@ impl PeerManager { mempool.remove_for_block(&b, h); reconsider_orphans_on_block(&orphanage, &mempool, &chain_state, &b); } - Err(_) => break, + // Another block waiting on the same + // parent may still connect. Each pass + // takes one block out, so this ends. + Err(_) => continue, } } None => break, @@ -6167,7 +6194,7 @@ impl PeerManager { } let height = chain_state.tip_height(); if height / 1000 > last_log_height / 1000 { - tracing::info!(height, buffered = block_buffer.len(), "IBD progress"); + tracing::info!(height, buffered = orphan_blocks.len(), "IBD progress"); last_log_height = height; } @@ -6187,11 +6214,14 @@ impl PeerManager { } } Err(crate::chain::state::ChainError::Duplicate) => {} - Err(crate::chain::state::ChainError::PrevBlockNotFound) - | Err(crate::chain::state::ChainError::BadPrevBlock) => { - if block_buffer.len() < 8192 { - block_buffer.insert(block.header.prev_blockhash, (block, in_flight)); - } + Err(crate::chain::state::ChainError::PrevBlockNotFound) => { + Self::hold_for_parent(&mut orphan_blocks, sender, block, requested, network, &peer_manager); + } + // Core's `bad-prevblk`: nothing connects on a parent + // marked invalid until `reconsiderblock` clears it, so + // the block is not kept. + Err(crate::chain::state::ChainError::BadPrevBlock) => { + tracing::debug!(%hash, "Block builds on a block marked invalid; not kept"); } Err(e) => { tracing::warn!(%hash, "Block rejected: {}", e); @@ -6227,6 +6257,39 @@ impl PeerManager { } } + /// A block whose parent has no index entry. Core refuses it as + /// `prev-blk-not-found` (`validation.cpp:4216`) and keeps nothing. Here it + /// waits for its parent, within the bounds of + /// [`crate::net::orphan_blocks::OrphanBlocks`], only if this node asked + /// for it. Either way the sender is asked for headers, which is how the + /// node learns the parent, as Core does for headers that do not connect + /// (`HandleUnconnectingHeaders`) and satd does for a `cmpctblock` on an + /// unknown parent. + fn hold_for_parent( + orphan_blocks: &mut crate::net::orphan_blocks::OrphanBlocks, + sender: PeerId, + block: bitcoin::Block, + requested: bool, + network: Network, + peer_manager: &std::sync::Weak, + ) { + let hash = block.block_hash(); + if requested && orphan_blocks.insert(sender, block, network, Instant::now()) { + tracing::debug!( + %hash, + peer = sender, + waiting = orphan_blocks.len(), + bytes = orphan_blocks.bytes(), + "Block waits for its parent" + ); + } else { + tracing::debug!(%hash, peer = sender, requested, "Dropped a block whose parent is unknown"); + } + if let Some(pm) = peer_manager.upgrade() { + pm.maybe_send_getheaders(sender); + } + } + /// Connect blocks that are already stored on disk and extend the tip, /// walking the best-header-chain frontier until data runs out, a /// block fails to connect, or the per-wakeup cap is hit. Returns the @@ -8613,6 +8676,9 @@ impl PeerManager { self.peer_stats(id), block, crate::net::flow::InFlight::new(self.peer_flow(id)), + // Its header connected before it was rebuilt, so it never + // waits for a parent. + false, )); } compact::Reconstruction::Partial { txs, missing_indices, stats } => { @@ -8751,6 +8817,9 @@ impl PeerManager { self.peer_stats(id), block, crate::net::flow::InFlight::new(self.peer_flow(id)), + // Its header connected before it was rebuilt, so it never + // waits for a parent. + false, )); } Err(compact::CompleteError::Invalid) => { @@ -10445,6 +10514,10 @@ fn historical_block_storable( #[path = "manager_blockaccept_tests.rs"] mod blockaccept_tests; +#[cfg(test)] +#[path = "manager_orphanbuf_tests.rs"] +mod orphanbuf_tests; + /// Reconsider orphans whose missing parent was just confirmed in `block`. /// /// Standalone so the `block_processor` thread — which doesn't have a diff --git a/node/src/net/manager_blockaccept_tests.rs b/node/src/net/manager_blockaccept_tests.rs index 2ca439855..7d94adbeb 100644 --- a/node/src/net/manager_blockaccept_tests.rs +++ b/node/src/net/manager_blockaccept_tests.rs @@ -50,6 +50,9 @@ fn an_empty_body_buffered_before_its_parent_does_not_bar_the_real_block() { assert!(pm.ibd.read().is_none(), "fixture: a synced node, not IBD"); let in_flight = || crate::net::flow::InFlight::new(None); + // Only a block the node asked for waits for an unknown parent, as one + // fetched after an `inv` does. + pm.note_blocks_requested(7, &[b5.block_hash()]); pm.handle_message(7, NetworkMessage::Block(empty5), in_flight()); pm.handle_message(7, NetworkMessage::Headers(vec![b4.header, b5.header]), in_flight()); pm.handle_message(7, NetworkMessage::Block(b4.clone()), in_flight()); diff --git a/node/src/net/manager_orphanbuf_tests.rs b/node/src/net/manager_orphanbuf_tests.rs new file mode 100644 index 000000000..6d6c83554 --- /dev/null +++ b/node/src/net/manager_orphanbuf_tests.rs @@ -0,0 +1,127 @@ +//! Blocks whose parent the node does not know, through `handle_message` and +//! the real block processor thread. +//! +//! Core keeps none of them: `AcceptBlockHeader` refuses such a block as +//! `prev-blk-not-found` (`validation.cpp:4216`). satd keeps one only if it +//! asked for it, within the bounds in [`crate::net::orphan_blocks`], so it +//! can connect without being fetched again once its parent does. + +use super::*; +use crate::chain::state::tests::{build_test_block, make_chain_state}; +use crate::net::flow::{InFlight, PeerFlow}; + +fn peer_manager_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); + // Leak the sender so the channel stays open for the manager's life. + std::mem::forget(shutdown_tx); + PeerManager::new(chain_state, mempool, fee_estimator, Network::Regtest, shutdown_rx) +} + +fn tip_reaches(cs: &ChainState, hash: bitcoin::BlockHash, within: Duration) -> bool { + let deadline = Instant::now() + within; + while cs.tip_hash() != hash && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(50)); + } + cs.tip_hash() == hash +} + +fn idle_within(flow: &PeerFlow, within: Duration) -> bool { + let deadline = Instant::now() + within; + while !flow.is_idle() && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(20)); + } + flow.is_idle() +} + +/// A synced node at height 3, with blocks 4 and 5 built on it but unknown to +/// the node. +fn fixture() -> (Arc, std::path::PathBuf, bitcoin::Block, bitcoin::Block) { + let (cs, dir) = make_chain_state(); + let genesis = bitcoin::constants::genesis_block(Network::Regtest).block_hash(); + let mut parent = genesis; + for h in 1..=3 { + let b = build_test_block(parent, h, 1_707_100_000 + h); + parent = cs.accept_block(&b).expect("connect fixture block").hash(); + } + let b4 = build_test_block(parent, 4, 1_707_100_004); + let b5 = build_test_block(b4.block_hash(), 5, 1_707_100_005); + (Arc::new(cs), dir, b4, b5) +} + +/// A block the node never asked for, whose parent it does not know, is not +/// kept: when the parent then connects, the block does not follow it. +/// +/// Fails without the fix: the block waited in the buffer, with no request +/// behind it, and connected as soon as block 4 did. +#[test] +fn an_unrequested_block_with_an_unknown_parent_is_not_kept() { + let (cs, dir, b4, b5) = fixture(); + let pm = peer_manager_over(cs.clone()); + assert!(pm.ibd.read().is_none(), "fixture: a synced node, not IBD"); + + pm.handle_message(7, NetworkMessage::Block(b5.clone()), InFlight::new(None)); + // Block 4's guard is counted out once the processor is done with it, + // which includes connecting whatever was waiting for it. + let flow = Arc::new(PeerFlow::new()); + pm.handle_message(7, NetworkMessage::Block(b4.clone()), InFlight::new(Some(flow.clone()))); + assert!(idle_within(&flow, Duration::from_secs(10)), "block 4 must be processed"); + assert_eq!(cs.tip_hash(), b4.block_hash(), "block 4 connects and block 5 was not kept"); + assert!(cs.get_block_index(&b5.block_hash()).is_none(), "block 5 is unknown to the node"); + + // Sent again now that its parent is known, it connects. + pm.handle_message(7, NetworkMessage::Block(b5.clone()), InFlight::new(None)); + assert!(tip_reaches(&cs, b5.block_hash(), Duration::from_secs(10)), "block 5 must connect"); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// A block the node asked for waits for its parent and connects right after +/// it. This is the counterpart to the test above: the buffer still does its +/// job for a block that was requested. +#[test] +fn a_requested_block_waits_for_its_parent_and_connects_after_it() { + let (cs, dir, b4, b5) = fixture(); + let pm = peer_manager_over(cs.clone()); + + // As `handle_inv` records a `getdata` for an announced block. + pm.note_blocks_requested(7, &[b5.block_hash()]); + pm.handle_message(7, NetworkMessage::Block(b5.clone()), InFlight::new(None)); + pm.handle_message(7, NetworkMessage::Block(b4.clone()), InFlight::new(None)); + assert!( + tip_reaches(&cs, b5.block_hash(), Duration::from_secs(10)), + "block 5 must connect from the buffer once block 4 does; tip is {}", + cs.tip_hash() + ); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// A block waiting for its parent no longer counts as work in flight for the +/// peer that sent it, so a `ping` behind it is answered. Core is done with +/// such a block once it has refused it as `prev-blk-not-found`. +/// +/// Fails without the fix: the waiting block held the sender's in-flight +/// guard until its parent connected, so every pong from that peer waited for +/// the 60-second bound in the meantime. +#[test] +fn a_block_waiting_for_its_parent_does_not_hold_the_senders_pong() { + let (cs, dir, b4, b5) = fixture(); + let pm = peer_manager_over(cs.clone()); + + pm.note_blocks_requested(7, &[b5.block_hash()]); + let flow = Arc::new(PeerFlow::new()); + pm.handle_message(7, NetworkMessage::Block(b5.clone()), InFlight::new(Some(flow.clone()))); + assert!( + idle_within(&flow, Duration::from_secs(10)), + "the waiting block must not hold the peer's in-flight count ({} in flight)", + flow.in_flight() + ); + + // It did wait rather than being dropped: it follows its parent in. + pm.handle_message(7, NetworkMessage::Block(b4.clone()), InFlight::new(None)); + assert!(tip_reaches(&cs, b5.block_hash(), Duration::from_secs(10)), "block 5 must connect"); + + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/node/src/net/mod.rs b/node/src/net/mod.rs index 5069ddd6e..4e7792a04 100644 --- a/node/src/net/mod.rs +++ b/node/src/net/mod.rs @@ -8,6 +8,7 @@ pub mod dns; pub mod flow; pub mod ibd; pub mod manager; +pub mod orphan_blocks; pub mod peer; pub mod permissions; pub mod proxy; diff --git a/node/src/net/orphan_blocks.rs b/node/src/net/orphan_blocks.rs new file mode 100644 index 000000000..056498aad --- /dev/null +++ b/node/src/net/orphan_blocks.rs @@ -0,0 +1,376 @@ +//! Blocks that arrived before their parent, kept until the parent connects. +//! +//! Bitcoin Core keeps no such block. `AcceptBlockHeader` refuses a block +//! whose parent has no index entry as `prev-blk-not-found` +//! (`validation.cpp:4216`), and the block is gone. satd answers a block +//! `inv` with a `getdata` as well as a `getheaders` (`handle_inv`), so a +//! block it asked for can arrive before the parent it is still learning +//! about. Keeping that block saves fetching it a second time once the parent +//! connects. +//! +//! This holds the blocks and bounds them; the block processor decides what +//! to offer (only a block the node asked for) and takes a block back out when +//! its parent becomes the tip. A block is kept only if: +//! +//! - its header meets its own target and that target is within the +//! network's proof-of-work limit, Core's `CheckProofOfWork`, which Core's +//! `AcceptBlockHeader` runs before it looks for the parent +//! (`validation.cpp:4206`); +//! - its serialized size is at most Core's `MAX_BLOCK_SERIALIZED_SIZE` +//! (`consensus/consensus.h:13`), which no valid block exceeds. +//! +//! The buffer holds at most [`MAX_ORPHAN_BLOCKS_PER_PEER`] blocks from one +//! peer, [`MAX_ORPHAN_BLOCKS`] blocks and [`MAX_ORPHAN_BLOCK_BYTES`] +//! serialized bytes in all. When a new block needs room, the sender's own +//! oldest block goes first if the sender is at its share, then the oldest +//! block of all. A block is dropped after [`ORPHAN_BLOCK_EXPIRY`]. + +use std::collections::VecDeque; +use std::time::{Duration, Instant}; + +use bitcoin::{Block, BlockHash, Network}; + +use crate::net::peer::PeerId; + +/// Core's `MAX_BLOCK_SERIALIZED_SIZE` (`consensus/consensus.h:13`). A block's +/// serialized size never exceeds its weight, so a valid block is never +/// larger. +pub const MAX_BLOCK_SERIALIZED_SIZE: usize = 4_000_000; + +/// Blocks kept from one peer. +pub const MAX_ORPHAN_BLOCKS_PER_PEER: usize = 4; + +/// Blocks kept in all. +pub const MAX_ORPHAN_BLOCKS: usize = 64; + +/// Serialized bytes kept in all: eight blocks of the largest size. +pub const MAX_ORPHAN_BLOCK_BYTES: usize = 8 * MAX_BLOCK_SERIALIZED_SIZE; + +/// How long a block is kept. By then the headers the node asked for have +/// made the block's own header known, and the ordinary missing-block fetch +/// gets it again if the parent connects later. +pub const ORPHAN_BLOCK_EXPIRY: Duration = Duration::from_secs(5 * 60); + +/// The bounds a buffer enforces. [`OrphanBlocks::new`] uses the constants +/// above; tests use smaller ones. +#[derive(Debug, Clone, Copy)] +pub struct Limits { + pub per_peer: usize, + pub blocks: usize, + pub bytes: usize, + pub expiry: Duration, +} + +impl Default for Limits { + fn default() -> Self { + Self { + per_peer: MAX_ORPHAN_BLOCKS_PER_PEER, + blocks: MAX_ORPHAN_BLOCKS, + bytes: MAX_ORPHAN_BLOCK_BYTES, + expiry: ORPHAN_BLOCK_EXPIRY, + } + } +} + +struct Orphan { + hash: BlockHash, + block: Block, + from: PeerId, + size: usize, + since: Instant, +} + +/// Blocks waiting for a parent the node has no index entry for, oldest first. +pub struct OrphanBlocks { + waiting: VecDeque, + bytes: usize, + limits: Limits, +} + +impl Default for OrphanBlocks { + fn default() -> Self { + Self::new() + } +} + +impl OrphanBlocks { + pub fn new() -> Self { + Self::with_limits(Limits::default()) + } + + pub fn with_limits(limits: Limits) -> Self { + Self { waiting: VecDeque::new(), bytes: 0, limits } + } + + /// Blocks waiting. + pub fn len(&self) -> usize { + self.waiting.len() + } + + pub fn is_empty(&self) -> bool { + self.waiting.is_empty() + } + + /// Serialized bytes waiting. + pub fn bytes(&self) -> usize { + self.bytes + } + + /// Whether the block with this hash is waiting. + pub fn contains(&self, hash: &BlockHash) -> bool { + self.waiting.iter().any(|o| o.hash == *hash) + } + + /// Keep `block`, received from `from`, until its parent connects. + /// Returns whether it is waiting afterwards; a copy of a block that is + /// already waiting counts as waiting and is not kept twice. + pub fn insert(&mut self, from: PeerId, block: Block, network: Network, now: Instant) -> bool { + if crate::validation::pow::check_proof_of_work_bounded(&block.header, network).is_err() { + return false; + } + let size = block.total_size(); + if size > MAX_BLOCK_SERIALIZED_SIZE || size > self.limits.bytes { + return false; + } + let hash = block.block_hash(); + self.expire(now); + if self.contains(&hash) { + return true; + } + if self.limits.per_peer == 0 || self.limits.blocks == 0 { + return false; + } + if self.waiting.iter().filter(|o| o.from == from).count() >= self.limits.per_peer + && let Some(oldest) = self.waiting.iter().position(|o| o.from == from) + { + self.remove_at(oldest); + } + while !self.waiting.is_empty() + && (self.waiting.len() >= self.limits.blocks || self.bytes + size > self.limits.bytes) + { + self.remove_at(0); + } + self.bytes += size; + self.waiting.push_back(Orphan { hash, block, from, size, since: now }); + true + } + + /// Remove and return the oldest waiting block whose parent is `parent`. + pub fn take_child_of(&mut self, parent: &BlockHash) -> Option { + let at = self.waiting.iter().position(|o| o.block.header.prev_blockhash == *parent)?; + Some(self.remove_at(at).block) + } + + /// Drop every block that has waited [`Limits::expiry`] or longer. + pub fn expire(&mut self, now: Instant) { + let expiry = self.limits.expiry; + self.waiting.retain(|o| now.saturating_duration_since(o.since) < expiry); + self.bytes = self.waiting.iter().map(|o| o.size).sum(); + } + + fn remove_at(&mut self, at: usize) -> Orphan { + let orphan = self.waiting.remove(at).expect("index in range"); + self.bytes -= orphan.size; + orphan + } +} + +#[cfg(test)] +mod tests { + use super::*; + use bitcoin::hashes::Hash as _; + + /// A regtest block on `prev` whose coinbase carries `pad` extra bytes, + /// with its header ground to meet (or, without `pow`, to miss) the + /// regtest target. + fn block_on(prev: BlockHash, salt: u32, pad: usize, pow: bool) -> Block { + let coinbase = bitcoin::Transaction { + version: bitcoin::transaction::Version(2), + lock_time: bitcoin::absolute::LockTime::ZERO, + input: vec![bitcoin::TxIn { + previous_output: bitcoin::OutPoint::null(), + script_sig: bitcoin::script::Builder::new() + .push_int(1) + .push_int(i64::from(salt)) + .into_script(), + sequence: bitcoin::Sequence::MAX, + witness: bitcoin::Witness::new(), + }], + output: vec![bitcoin::TxOut { + value: bitcoin::Amount::ZERO, + script_pubkey: bitcoin::ScriptBuf::from_bytes(vec![0x6a; pad]), + }], + }; + let mut block = Block { + header: bitcoin::block::Header { + version: bitcoin::block::Version::from_consensus(0x2000_0000), + prev_blockhash: prev, + merkle_root: bitcoin::TxMerkleNode::all_zeros(), + time: 1_707_200_000 + salt, + bits: bitcoin::CompactTarget::from_consensus(0x207f_ffff), + nonce: 0, + }, + txdata: vec![coinbase], + }; + block.header.merkle_root = block.compute_merkle_root().expect("one transaction"); + let target = block.header.target(); + while block.header.validate_pow(target).is_ok() != pow { + block.header.nonce += 1; + } + block + } + + fn prev(n: u8) -> BlockHash { + BlockHash::from_byte_array([n; 32]) + } + + fn small(limits: Limits) -> OrphanBlocks { + OrphanBlocks::with_limits(limits) + } + + const ROOMY: Limits = Limits { + per_peer: 100, + blocks: 100, + bytes: 100 * MAX_BLOCK_SERIALIZED_SIZE, + expiry: Duration::from_secs(300), + }; + + #[test] + fn a_block_with_valid_proof_of_work_is_kept_and_returned_to_its_parent() { + let mut buf = OrphanBlocks::new(); + let b = block_on(prev(1), 1, 0, true); + let now = Instant::now(); + assert!(buf.insert(7, b.clone(), Network::Regtest, now)); + assert_eq!((buf.len(), buf.bytes()), (1, b.total_size())); + assert!(buf.take_child_of(&prev(2)).is_none(), "only to its own parent"); + assert_eq!(buf.take_child_of(&prev(1)).map(|b| b.block_hash()), Some(b.block_hash())); + assert_eq!((buf.len(), buf.bytes()), (0, 0), "its bytes are released"); + } + + /// Core's `CheckProofOfWork` (`pow.cpp`): the hash meets the header's + /// target, and the target is within powLimit. + #[test] + fn a_block_without_proof_of_work_within_the_limit_is_not_kept() { + let mut buf = OrphanBlocks::new(); + let now = Instant::now(); + let misses = block_on(prev(1), 2, 0, false); + assert!(!buf.insert(7, misses, Network::Regtest, now), "the hash misses its target"); + + // A target above regtest's powLimit (0x207fffff), which almost every + // hash meets. + let mut easy = block_on(prev(1), 3, 0, true); + easy.header.bits = bitcoin::CompactTarget::from_consensus(0x2100_ffff); + assert!(easy.header.validate_pow(easy.header.target()).is_ok(), "fixture: meets its own target"); + assert!(!buf.insert(7, easy, Network::Regtest, now), "the target is above powLimit"); + + // The same rule on mainnet, where regtest's work is far too little. + let regtest_work = block_on(prev(1), 4, 0, true); + assert!(!buf.insert(7, regtest_work.clone(), Network::Bitcoin, now)); + assert!(buf.is_empty()); + assert!(buf.insert(7, regtest_work, Network::Regtest, now), "but it is enough on regtest"); + } + + #[test] + fn a_block_over_the_serialized_size_limit_is_not_kept() { + let mut buf = OrphanBlocks::new(); + let now = Instant::now(); + let over = block_on(prev(1), 5, MAX_BLOCK_SERIALIZED_SIZE, true); + assert!(over.total_size() > MAX_BLOCK_SERIALIZED_SIZE, "fixture: over the limit"); + assert!(!buf.insert(7, over, Network::Regtest, now)); + assert!(buf.is_empty()); + + let at = block_on(prev(1), 6, MAX_BLOCK_SERIALIZED_SIZE - 200, true); + assert!(at.total_size() <= MAX_BLOCK_SERIALIZED_SIZE, "fixture: within the limit"); + assert!(buf.insert(7, at, Network::Regtest, now), "a block within the limit is kept"); + } + + /// One peer cannot take more than its share: past it, its own oldest + /// block goes, and blocks from other peers stay. + #[test] + fn one_peer_keeps_at_most_its_share_and_loses_its_oldest_first() { + let mut buf = small(Limits { per_peer: 2, ..ROOMY }); + let now = Instant::now(); + let other = block_on(prev(9), 9, 0, true); + assert!(buf.insert(8, other.clone(), Network::Regtest, now)); + let mine: Vec = (1..=3).map(|i| block_on(prev(i), 10 + u32::from(i), 0, true)).collect(); + for b in &mine { + assert!(buf.insert(7, b.clone(), Network::Regtest, now)); + } + assert_eq!(buf.len(), 3, "two from peer 7, one from peer 8"); + assert!(!buf.contains(&mine[0].block_hash()), "peer 7's oldest went"); + assert!(buf.contains(&mine[1].block_hash()) && buf.contains(&mine[2].block_hash())); + assert!(buf.contains(&other.block_hash()), "the other peer's block stays"); + } + + #[test] + fn the_byte_cap_drops_the_oldest_blocks_first() { + let probe = block_on(prev(1), 20, 1_000, true).total_size(); + let mut buf = small(Limits { bytes: 3 * probe, ..ROOMY }); + let now = Instant::now(); + let blocks: Vec = (1..=4).map(|i| block_on(prev(i), 20 + u32::from(i), 1_000, true)).collect(); + for (peer, b) in blocks.iter().enumerate() { + assert_eq!(b.total_size(), probe, "fixture: equal sizes"); + assert!(buf.insert(peer as PeerId, b.clone(), Network::Regtest, now)); + assert!(buf.bytes() <= 3 * probe, "never over the byte cap"); + } + assert_eq!(buf.len(), 3); + assert!(!buf.contains(&blocks[0].block_hash()), "the oldest went"); + assert!(blocks[1..].iter().all(|b| buf.contains(&b.block_hash()))); + } + + #[test] + fn the_block_cap_drops_the_oldest_blocks_first() { + let mut buf = small(Limits { blocks: 2, ..ROOMY }); + let now = Instant::now(); + let blocks: Vec = (1..=3).map(|i| block_on(prev(i), 30 + u32::from(i), 0, true)).collect(); + for (peer, b) in blocks.iter().enumerate() { + assert!(buf.insert(peer as PeerId, b.clone(), Network::Regtest, now)); + } + assert_eq!(buf.len(), 2); + assert!(!buf.contains(&blocks[0].block_hash()), "the oldest went"); + } + + #[test] + fn a_block_is_dropped_once_it_has_waited_the_expiry() { + let mut buf = small(Limits { expiry: Duration::from_secs(60), ..ROOMY }); + let start = Instant::now(); + let old = block_on(prev(1), 40, 0, true); + let young = block_on(prev(2), 41, 0, true); + assert!(buf.insert(7, old.clone(), Network::Regtest, start)); + assert!(buf.insert(8, young.clone(), Network::Regtest, start + Duration::from_secs(30))); + + buf.expire(start + Duration::from_secs(59)); + assert_eq!(buf.len(), 2, "nothing has waited a minute yet"); + buf.expire(start + Duration::from_secs(60)); + assert!(!buf.contains(&old.block_hash()), "the first block expired"); + assert!(buf.contains(&young.block_hash()), "the second has not"); + assert_eq!(buf.bytes(), young.total_size()); + } + + /// Two blocks on the same parent both wait; a copy of one that is already + /// waiting is not kept twice. + #[test] + fn siblings_both_wait_and_a_second_copy_is_not_kept_twice() { + let mut buf = OrphanBlocks::new(); + let now = Instant::now(); + let a = block_on(prev(1), 50, 0, true); + let b = block_on(prev(1), 51, 0, true); + assert!(buf.insert(7, a.clone(), Network::Regtest, now)); + assert!(buf.insert(8, b.clone(), Network::Regtest, now)); + assert!(buf.insert(9, a.clone(), Network::Regtest, now), "already waiting"); + assert_eq!((buf.len(), buf.bytes()), (2, a.total_size() + b.total_size())); + assert_eq!(buf.take_child_of(&prev(1)).map(|x| x.block_hash()), Some(a.block_hash()), "oldest first"); + assert_eq!(buf.take_child_of(&prev(1)).map(|x| x.block_hash()), Some(b.block_hash())); + assert!(buf.is_empty()); + } + + /// The default bounds always leave room for one block of the largest + /// valid size, and one peer can never fill the buffer on its own. + #[test] + fn the_default_bounds_fit_a_full_size_block_and_cap_each_peer_below_the_total() { + let l = Limits::default(); + assert!(l.bytes >= MAX_BLOCK_SERIALIZED_SIZE); + assert!(l.per_peer * MAX_BLOCK_SERIALIZED_SIZE < l.bytes); + assert!(l.per_peer < l.blocks); + } +} diff --git a/satd/tests/compact_blocks.rs b/satd/tests/compact_blocks.rs index eacc241a4..cd888bf31 100644 --- a/satd/tests/compact_blocks.rs +++ b/satd/tests/compact_blocks.rs @@ -21,6 +21,9 @@ use std::time::Duration; use p2p::RawPeer; +#[path = "compact_blocks/unknown_parent.rs"] +mod unknown_parent; + mod p2p { use bitcoin::consensus::{deserialize, serialize}; use bitcoin::p2p::message::{NetworkMessage, RawNetworkMessage}; diff --git a/satd/tests/compact_blocks/unknown_parent.rs b/satd/tests/compact_blocks/unknown_parent.rs new file mode 100644 index 000000000..8cfd39062 --- /dev/null +++ b/satd/tests/compact_blocks/unknown_parent.rs @@ -0,0 +1,80 @@ +//! Blocks whose parent the node does not know, over a raw P2P connection. +//! +//! Core refuses such a block as `prev-blk-not-found` and keeps nothing +//! (`validation.cpp:4216`). satd keeps one only if it asked for it, and asks +//! the sender for headers either way. + +use super::*; + +fn is_getheaders(m: &NetworkMessage) -> bool { + matches!(m, NetworkMessage::GetHeaders(_)) +} + +/// A pong for `nonce` within `within`. +fn pong_within(peer: &mut RawPeer, nonce: u64, within: Duration) -> bool { + peer.send(NetworkMessage::Ping(nonce)); + peer.recv_until(|m| matches!(m, NetworkMessage::Pong(n) if *n == nonce), within).is_some() +} + +/// A block the node did not ask for, on a parent it does not know, is +/// dropped, and the sender is asked for headers: that is how the node learns +/// the parent. The block does not connect when its parent does. +/// +/// Fails without the fix: no `getheaders` followed the block, and the block +/// waited in the buffer and connected right after its parent. +#[test] +fn an_unrequested_block_on_an_unknown_parent_is_dropped_and_headers_are_asked_for() { + let (node, _) = started_node(1); + let mut peer = RawPeer::connect(node.p2p_port.unwrap()); + poll_until(|| peer_count(&node) == 1, test_timeout(20), "peer must connect"); + let tip = block_at(&node, &best_hash(&node)); + let parent = build_block(&tip, height(&node) + 1, vec![], true, 81); + let child = build_block(&parent, height(&node) + 2, vec![], true, 82); + + // The node also sends every peer a `getheaders` every ten seconds. Start + // right after one, so the next one cannot land in the window below. + peer.recv_until(is_getheaders, test_timeout(30)).expect("the opening getheaders"); + peer.recv_until(is_getheaders, test_timeout(30)).expect("a periodic getheaders"); + + peer.send(NetworkMessage::Block(child.clone())); + assert!( + peer.recv_until(is_getheaders, Duration::from_secs(4)).is_some(), + "a block on an unknown parent must be answered with getheaders" + ); + + peer.send(NetworkMessage::Block(parent.clone())); + assert!(pong_within(&mut peer, 0x0b0b_0001, test_timeout(20)), "pong after the parent"); + poll_until(|| best_hash(&node) == parent.block_hash(), test_timeout(20), "the parent must connect"); + // The pong came after the parent's processing, which is where a waiting + // child would have been connected. + assert_eq!(best_hash(&node), parent.block_hash(), "the unrequested child must not have been kept"); + assert!(!peer.is_closed()); + assert_eq!(banned_count(&node), 0); +} + +/// A block the node asked for after an `inv` waits for its parent and +/// connects after it, and a ping sent behind it is answered while it waits. +/// +/// Fails without the fix: the waiting block held the peer's pong until its +/// parent connected, so the pong came only after the 60-second bound. +#[test] +fn a_requested_block_waits_for_its_parent_without_holding_the_pong() { + let (node, _) = started_node(1); + let mut peer = RawPeer::connect(node.p2p_port.unwrap()); + poll_until(|| peer_count(&node) == 1, test_timeout(20), "peer must connect"); + let tip = block_at(&node, &best_hash(&node)); + let parent = build_block(&tip, height(&node) + 1, vec![], true, 83); + let child = build_block(&parent, height(&node) + 2, vec![], true, 84); + + peer.send(NetworkMessage::Inv(vec![Inventory::Block(child.block_hash())])); + collect_until(&mut peer, is_getdata_for(child.block_hash()), test_timeout(20), "a getdata for the child"); + peer.send(NetworkMessage::Block(child.clone())); + assert!( + pong_within(&mut peer, 0x0b0b_0002, Duration::from_secs(10)), + "the pong must not wait for the child's parent" + ); + assert_eq!(height(&node), 1, "fixture: the child cannot connect yet"); + + peer.send(NetworkMessage::Block(parent.clone())); + poll_until(|| best_hash(&node) == child.block_hash(), test_timeout(20), "the child must follow its parent"); +}