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/docs/manual/src/ibd.md b/docs/manual/src/ibd.md index 3f89c9685..a98cf5eeb 100644 --- a/docs/manual/src/ibd.md +++ b/docs/manual/src/ibd.md @@ -381,6 +381,23 @@ On an AssumeUTXO node the history below the snapshot base is legitimately unvalidated until the background chainstate reaches it. That is recognised and logged at `INFO`, not treated as damage. +### A coin the database cannot read + +If RocksDB cannot read a coin from the chainstate (an I/O error or a checksum +mismatch), or a coin's row does not decode, satd stops at once, as Bitcoin Core +does. This holds wherever a coin is looked up: block connection, the mempool, +`gettxout`, the Electrum and Esplora lookups, and the offline +`satd-chainstate-audit` below. The log and stderr say `Error reading from +database, shutting down`, with the outpoint and the error. satd does not carry +on as if the coin were spent: that would refuse a valid block that spends it, +and record the block as invalid across restarts. + +The stop is an `abort()` (the process ends with `SIGABRT`), so no shutdown +flush runs. The next start replays the blocks after the last flush from the +block files, as after any crash. If the error comes back after a restart, the +disk or the datadir is damaged: check the disk, then rebuild the UTXO set with +`-reindex-chainstate`, or audit a copy of the datadir first (below). + ### Auditing a suspect datadir offline `satd-chainstate-audit` answers the question the startup checks cannot afford @@ -411,7 +428,10 @@ Note also that it is not included in the release tarballs or the Docker image; build it from source (`cargo build --release --bin satd-chainstate-audit`). Exit status is `0` when consistent, `1` when it could not run, `2` when it found -inconsistencies, so it scripts cleanly. +inconsistencies, so it scripts cleanly. A coin the database cannot read stops +the tool as it stops the node (see above): it prints `Error reading from +database` with the outpoint and the RocksDB error, which names the damaged +file, and ends with `SIGABRT` before any report. It diagnoses and does not repair. A missing coin is recoverable only by replaying the block that created it: `-reindex-chainstate`, or diff --git a/node/src/chain/state.rs b/node/src/chain/state.rs index 6518f0048..afe51ff88 100644 --- a/node/src/chain/state.rs +++ b/node/src/chain/state.rs @@ -7264,20 +7264,28 @@ impl ChainState { let new_height = parent.height + 1; - // Structural + witness block validation. If the block already has a - // `HeaderOnly` index entry (submitted via `submitheader`) and fails - // body validation, mark it — and every descendant in the index — as - // `Invalid` so that `getchaintips` reports the correct status. - // This matches Core's `InvalidBlockFound`. + // Structural + witness block validation: Core's `CheckBlock` plus the + // witness and weight rules of `ContextualCheckBlock`. // - // Except for mutation-class rejections, which Core deliberately - // excludes from that marking (CVE-2012-2459): the block hash does not - // commit to the data we just rejected, so the verdict may belong to a - // malleated copy of a block that is actually valid. Writing it down - // would bar the honest block — and, via the parent-status guard, its - // whole descendant chain — until an operator ran `reconsiderblock`. + // A failure here is not written down against the hash, with one + // exception. Core's `ProcessNewBlock` runs `CheckBlock` before + // `AcceptBlock` and returns on failure without marking anything + // (`validation.cpp`: "we will never mark a block as invalid if + // CheckBlock() fails"), so `bad-txnmrklroot`, `bad-txns-duplicate`, + // `bad-blk-length`, `bad-cb-missing`, `bad-cb-multiple` and + // `bad-blk-sigops` leave the index as it was. The witness rules are + // `BLOCK_MUTATED`, which `InvalidBlockFound` skips. That leaves + // `bad-blk-weight`, the one rule here that Core's `AcceptBlock` + // marks `BLOCK_FAILED_VALID`. `check_block` reaches it only after the + // merkle and witness checks have passed, so by then the hash commits + // to every byte that was weighed. + // + // Core marks it on the index entry `AcceptBlockHeader` made. Here that + // is the `HeaderOnly` row a header announcement left; a block with no + // row yet is refused unmarked, at the cost of re-running this check if + // it is sent again. if let Err(e) = validation::block::check_block(block, self.network, new_height) { - if !e.is_mutation_class() + if matches!(e, validation::ValidationError::OverweightBlock) && self .store .get_block_index(&block_hash) @@ -7361,6 +7369,39 @@ impl ChainState { let current_tip = self.tip_hash(); let new_chainwork = add_u256(&parent.chainwork, &work_for_bits(block.header.bits)); + // A block that extends the tip and arrived without its header first + // (`submitblock`, a pushed `block` message) has no index entry yet. + // Core's `AcceptBlock` creates one in `AcceptBlockHeader` before the + // block can reach `ConnectTip`, so a block that then fails to connect + // is marked `BLOCK_FAILED_VALID` and a copy sent again stops at + // `duplicate-invalid`. Without an entry the mark after a failed + // connect below had nothing to land on: the block was forgotten, and + // every copy sent again was written to disk again and validated again + // in full. + // + // The entry is the `HeaderOnly` row a header announcement would have + // left. A connect writes the `Valid` entry over it and a verdict marks + // it `Invalid`, as on the headers-first path. Any other failure is + // damage to this node's own storage and leaves a known header whose + // block is not stored. Unlike an announced header, that one has not + // moved the best-header pointer, which moves here only after a + // successful connect: a restart seeds the pointer from the index, and + // a descendant header or another copy of the block reaches it. + if prev_hash == current_tip && self.store.get_block_index(&block_hash).is_none() { + let entry = BlockIndexEntry { + header: block.header, + height: new_height, + status: BlockStatus::HeaderOnly, + num_tx: 0, + file_number: 0, + data_pos: 0, + chainwork: new_chainwork, + }; + let mut batch = crate::storage::StoreBatch::default(); + batch.block_index_puts.push((block_hash, entry)); + self.write_chain_batch(batch)?; + } + if prev_hash != current_tip { // Side chain block — store it first let entry = BlockIndexEntry { @@ -9894,6 +9935,10 @@ impl ChainState { } } +#[cfg(test)] +#[path = "state_blockaccept_tests.rs"] +mod blockaccept_tests; + /// Read-side surface for the silent-payment tweak index (the /// `getsilentpaymentblockdata` RPC, the streaming `tweaks` category replay, /// and the D4 rescan fast path). Backed by the same durable rows the connect diff --git a/node/src/chain/state_blockaccept_tests.rs b/node/src/chain/state_blockaccept_tests.rs new file mode 100644 index 000000000..1d3295969 --- /dev/null +++ b/node/src/chain/state_blockaccept_tests.rs @@ -0,0 +1,303 @@ +//! Which `accept_block` failures are written down against the block hash. +//! +//! Bitcoin Core marks a block `BLOCK_FAILED_VALID` only for a verdict the +//! block hash commits to, and only on an index entry that exists: +//! +//! - `ProcessNewBlock` runs `CheckBlock` first and, on failure, returns +//! without reaching `AcceptBlock`, so nothing is marked. +//! - `AcceptBlock` makes the index entry (`AcceptBlockHeader`) before +//! `ContextualCheckBlock` and `ConnectTip` can fail, and `InvalidBlockFound` +//! marks that entry unless the result is `BLOCK_MUTATED`. + +use super::tests::{build_test_block, build_test_block_spending, grind_test_pow, make_chain_state}; +use super::*; +use bitcoin::{ScriptBuf, Transaction, TxIn, TxOut}; + +/// Connect `n` blocks on regtest genesis through `accept_block`. +fn connect_chain(cs: &ChainState, n: u32) -> Vec { + let mut parent = bitcoin::constants::genesis_block(Network::Regtest).block_hash(); + let mut blocks = Vec::new(); + for h in 1..=n { + let b = build_test_block(parent, h, 1_707_000_000 + h); + parent = cs.accept_block(&b).expect("connect fixture block").hash(); + blocks.push(b); + } + blocks +} + +fn status(cs: &ChainState, hash: &BlockHash) -> Option { + cs.get_block_index(hash).map(|e| e.status) +} + +/// A regtest block on `parent` whose header commits to exactly `txdata`, with +/// its proof of work solved and its header accepted: the `HeaderOnly` row a +/// header announcement leaves. +fn announced_block(cs: &ChainState, parent: BlockHash, time: u32, txdata: Vec) -> Block { + let mut block = Block { + header: bitcoin::block::Header { + version: bitcoin::block::Version::from_consensus(0x2000_0000), + prev_blockhash: parent, + merkle_root: bitcoin::TxMerkleNode::all_zeros(), + time, + bits: bitcoin::CompactTarget::from_consensus(0x207f_ffff), + nonce: 0, + }, + txdata, + }; + block.header.merkle_root = block + .compute_merkle_root() + .unwrap_or_else(bitcoin::TxMerkleNode::all_zeros); + grind_test_pow(&mut block); + cs.accept_header(&block.header).expect("fixture header"); + assert_eq!(status(cs, &block.block_hash()), Some(BlockStatus::HeaderOnly)); + block +} + +fn coinbase(script_sig: Vec, outputs: Vec) -> Transaction { + Transaction { + version: bitcoin::transaction::Version(2), + lock_time: bitcoin::absolute::LockTime::ZERO, + input: vec![TxIn { + previous_output: OutPoint::null(), + script_sig: ScriptBuf::from(script_sig), + sequence: bitcoin::Sequence::MAX, + witness: bitcoin::Witness::new(), + }], + output: outputs, + } +} + +fn pay(sats: u64, script: Vec) -> TxOut { + TxOut { value: bitcoin::Amount::from_sat(sats), script_pubkey: ScriptBuf::from(script) } +} + +/// A non-coinbase transaction whose serialization without witness is exactly +/// 64 bytes: the shape that can be read as an inner merkle node. +fn sixty_four_byte_tx() -> Transaction { + let tx = Transaction { + version: bitcoin::transaction::Version(2), + lock_time: bitcoin::absolute::LockTime::ZERO, + input: vec![TxIn { + previous_output: OutPoint { txid: bitcoin::Txid::from_byte_array([0x11; 32]), vout: 0 }, + script_sig: ScriptBuf::new(), + sequence: bitcoin::Sequence::MAX, + witness: bitcoin::Witness::new(), + }], + output: vec![pay(1_000, vec![0x51; 4])], + }; + assert_eq!(tx.base_size(), 64, "fixture: a 64-byte transaction"); + tx +} + +/// An empty body sent for an announced header is a merkle mismatch, not a +/// verdict on the block. The honest body connects afterwards. +/// +/// This is the order the P2P path produces when the empty body arrives before +/// its parent is known: it waits in the unknown-parent buffer, the headers +/// arrive, the parent connects, and the buffer hands the empty body to +/// `accept_block` with the header now known. +/// +/// Fails without the fix: the empty body answered `bad-blk-length` +/// (`EmptyBlock`, tested before the merkle root), was marked `Invalid`, and +/// the honest block 5 then answered `duplicate`. +#[test] +fn an_empty_body_for_a_known_header_does_not_bar_the_real_block() { + let (cs, dir) = make_chain_state(); + let chain = connect_chain(&cs, 3); + let b4 = build_test_block(chain[2].block_hash(), 4, 1_707_000_004); + let b5 = build_test_block(b4.block_hash(), 5, 1_707_000_005); + let (accepted, err) = cs.accept_headers(&[b4.header, b5.header]); + assert_eq!(accepted, 2, "fixture: headers accepted ({err:?})"); + + let empty = Block { header: b5.header, txdata: Vec::new() }; + let err = cs.accept_block(&empty).expect_err("an empty body must be refused"); + assert_eq!(err.to_string(), "bad-txnmrklroot", "Core's CheckMerkleRoot runs first"); + assert_eq!( + status(&cs, &b5.block_hash()), + Some(BlockStatus::HeaderOnly), + "a body the header does not commit to must not mark the header invalid" + ); + + cs.accept_block(&b4).expect("block 4 connects"); + cs.accept_block(&b5).expect("the real block 5 connects"); + assert_eq!(cs.tip_hash(), b5.block_hash()); + assert_eq!(status(&cs, &b5.block_hash()), Some(BlockStatus::Valid)); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// `CheckBlock` failures are never marked, as in Core's `ProcessNewBlock`, +/// even when the header does commit to the failing body. Each case here is a +/// body whose merkle root matches its header. +/// +/// Fails without the fix: every one of these was marked `Invalid` on the +/// `HeaderOnly` row (only mutation-class reasons were exempt). +#[test] +fn a_check_block_failure_is_not_marked_on_a_known_header() { + let (cs, dir) = make_chain_state(); + let chain = connect_chain(&cs, 3); + let tip = chain[2].block_hash(); + let subsidy = crate::chain::connect::block_subsidy(Network::Regtest, 4); + + // `bad-cb-missing`, in the shape Core's `IsBlockMutated` singles out: no + // coinbase and one 64-byte transaction, which a header for a two- + // transaction block can be made to commit to. + let coinbase_less = announced_block(&cs, tip, 1_707_000_100, vec![sixty_four_byte_tx()]); + assert!( + crate::validation::block::is_block_mutated(&coinbase_less, true), + "fixture: the P2P gate would call this body mutated" + ); + // `bad-cb-multiple`. + let two_coinbases = announced_block( + &cs, + tip, + 1_707_000_101, + vec![ + coinbase(vec![0x54, 0x01], vec![pay(subsidy, vec![])]), + coinbase(vec![0x54, 0x02], vec![pay(1, vec![])]), + ], + ); + // `bad-blk-sigops`: Core's legacy-count ceiling in `CheckBlock`. + let sigops = announced_block( + &cs, + tip, + 1_707_000_102, + vec![coinbase(vec![0x54, 0x03], vec![pay(subsidy, vec![0xac; 20_001])])], + ); + + for (block, reason) in [ + (&coinbase_less, "bad-cb-missing"), + (&two_coinbases, "bad-cb-multiple"), + (&sigops, "bad-blk-sigops"), + ] { + let err = cs.accept_block(block).expect_err(reason); + assert_eq!(err.to_string(), reason); + assert_eq!( + status(&cs, &block.block_hash()), + Some(BlockStatus::HeaderOnly), + "{reason}: a CheckBlock failure must leave the header unmarked" + ); + } + + let _ = std::fs::remove_dir_all(&dir); +} + +/// `bad-blk-weight` is the one `check_block` rule Core marks: it comes from +/// `ContextualCheckBlock`, after the witness commitment has pinned every byte +/// that was weighed. The counterpart to the test above: marking still happens +/// where Core marks. +/// +/// Perturbation: drop the `OverweightBlock` arm from `accept_block`'s marking +/// and the status stays `HeaderOnly`. +#[test] +fn an_overweight_block_is_still_marked_invalid() { + let (cs, dir) = make_chain_state(); + let chain = connect_chain(&cs, 3); + let tip = chain[2].block_hash(); + + // A spend whose witness alone pushes the block past 4M weight units, under + // a valid BIP 141 commitment. + let mut witness = bitcoin::Witness::new(); + witness.push(vec![0u8; 4_000_000]); + let heavy = Transaction { + version: bitcoin::transaction::Version(2), + lock_time: bitcoin::absolute::LockTime::ZERO, + input: vec![TxIn { + previous_output: OutPoint { txid: bitcoin::Txid::from_byte_array([0x22; 32]), vout: 0 }, + script_sig: ScriptBuf::new(), + sequence: bitcoin::Sequence::MAX, + witness, + }], + output: vec![pay(1_000, vec![])], + }; + // BIP 141: the witness root over [0 (coinbase), wtxid(heavy)], then + // SHA256d(witness root || reserved value). + let sha256d = |a: &[u8; 32], b: &[u8; 32]| { + let mut preimage = [0u8; 64]; + preimage[..32].copy_from_slice(a); + preimage[32..].copy_from_slice(b); + bitcoin::hashes::sha256d::Hash::hash(&preimage).to_byte_array() + }; + let nonce = [0u8; 32]; + let witness_root = sha256d(&[0u8; 32], &heavy.compute_wtxid().to_raw_hash().to_byte_array()); + let commitment = sha256d(&witness_root, &nonce); + let mut commitment_script = vec![0x6a, 0x24, 0xaa, 0x21, 0xa9, 0xed]; + commitment_script.extend_from_slice(&commitment); + let mut cb = coinbase( + vec![0x54, 0x04], + vec![pay(crate::chain::connect::block_subsidy(Network::Regtest, 4), vec![]), pay(0, commitment_script)], + ); + cb.input[0].witness.push(nonce); + + let block = announced_block(&cs, tip, 1_707_000_200, vec![cb, heavy]); + let err = cs.accept_block(&block).expect_err("an overweight block must be refused"); + assert_eq!(err.to_string(), "bad-blk-weight"); + assert_eq!( + status(&cs, &block.block_hash()), + Some(BlockStatus::Invalid), + "Core's AcceptBlock marks bad-blk-weight BLOCK_FAILED_VALID" + ); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// A block that extends the tip, arrives without its header first and fails +/// to connect is remembered as invalid. A copy sent again is answered from +/// the index: not validated again, and not written to disk again. +/// +/// Fails without the fix: no index entry existed for the mark to land on, so +/// the second copy was validated in full, answered +/// `bad-txns-inputs-missingorspent` again, and grew the block files. +#[test] +fn a_tip_extending_block_that_fails_to_connect_is_remembered() { + let (cs, dir) = make_chain_state(); + let chain = connect_chain(&cs, 3); + let tip = chain[2].block_hash(); + + let missing = OutPoint { txid: bitcoin::Txid::from_byte_array([0x9e; 32]), vout: 0 }; + let bad = build_test_block_spending(tip, 4, 1_707_000_004, missing); + assert!(cs.get_block_index(&bad.block_hash()).is_none(), "fixture: no header announced"); + + let err = cs.accept_block(&bad).expect_err("a block spending a missing coin"); + assert_eq!(err.to_string(), "bad-txns-inputs-missingorspent"); + assert_eq!(status(&cs, &bad.block_hash()), Some(BlockStatus::Invalid)); + assert_eq!(cs.tip_hash(), tip, "the tip must not move"); + + let on_disk = cs.flat_files.lock().size_on_disk(); + let again = cs.accept_block(&bad).expect_err("the same block again"); + assert!(matches!(again, ChainError::Duplicate), "answered from the index, got: {again}"); + assert_eq!( + cs.flat_files.lock().size_on_disk(), + on_disk, + "a copy of a block already judged must not be written again" + ); + + // The tip's other children are untouched by the mark. + let good = build_test_block(tip, 4, 1_707_000_005); + cs.accept_block(&good).expect("a valid sibling connects"); + assert_eq!(cs.tip_hash(), good.block_hash()); + assert_eq!(status(&cs, &bad.block_hash()), Some(BlockStatus::Invalid)); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// The index entry made for an unannounced block is the one a header +/// announcement makes, so a block that connects ends exactly as it did before: +/// `Valid`, the tip, and the best header. +#[test] +fn an_unannounced_block_that_connects_ends_valid_and_best() { + let (cs, dir) = make_chain_state(); + let chain = connect_chain(&cs, 3); + let b4 = build_test_block(chain[2].block_hash(), 4, 1_707_000_004); + assert!(cs.get_block_index(&b4.block_hash()).is_none(), "fixture: no header announced"); + + cs.accept_block(&b4).expect("block 4 connects"); + let entry = cs.get_block_index(&b4.block_hash()).expect("indexed"); + assert_eq!(entry.status, BlockStatus::Valid); + assert_eq!(entry.num_tx, 1); + assert_eq!(cs.tip_hash(), b4.block_hash()); + assert_eq!(cs.best_header_hash(), b4.block_hash()); + assert_eq!(cs.check_block_index(None), Ok(4)); + + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/node/src/net/manager.rs b/node/src/net/manager.rs index aea389ed4..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) => { @@ -10441,6 +10510,14 @@ fn historical_block_storable( canonical_at_height == Some(block_hash) } +#[cfg(test)] +#[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 new file mode 100644 index 000000000..7d94adbeb --- /dev/null +++ b/node/src/net/manager_blockaccept_tests.rs @@ -0,0 +1,72 @@ +//! A block body that arrives before its parent is known skips the P2P +//! mutation gate (Core's `IsBlockMutated` needs the parent to decide the +//! witness rules) and waits in the unknown-parent buffer. When the parent +//! connects, the buffer hands it to `accept_block` with its header known by +//! then. A body the header does not commit to must not get the header marked +//! invalid on that route. + +use super::*; +use crate::chain::state::tests::{build_test_block, make_chain_state}; + +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 +} + +/// An empty body for block 5, sent before block 4 is known, then the headers, +/// then block 4, then the real block 5: the real block connects. +/// +/// Fails without the fix: the buffered empty body reached `accept_block` +/// after block 4 connected, failed `bad-blk-length` with block 5's header +/// known, and marked block 5 `Invalid`; the real block 5 was then refused as +/// a duplicate and the tip stayed at 4. +#[test] +fn an_empty_body_buffered_before_its_parent_does_not_bar_the_real_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_000_000 + h); + parent = cs.accept_block(&b).expect("connect fixture block").hash(); + } + let b4 = build_test_block(parent, 4, 1_707_000_004); + let b5 = build_test_block(b4.block_hash(), 5, 1_707_000_005); + let empty5 = bitcoin::Block { header: b5.header, txdata: Vec::new() }; + + let cs = Arc::new(cs); + let pm = peer_manager_over(cs.clone()); + 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()); + assert!( + tip_reaches(&cs, b4.block_hash(), Duration::from_secs(10)), + "block 4 must connect" + ); + + pm.handle_message(7, NetworkMessage::Block(b5.clone()), in_flight()); + assert!( + tip_reaches(&cs, b5.block_hash(), Duration::from_secs(10)), + "the real block 5 must connect; its status is {:?}", + cs.get_block_index(&b5.block_hash()).map(|e| e.status) + ); + + let _ = std::fs::remove_dir_all(&dir); +} 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/node/src/storage/rocksdb_store.rs b/node/src/storage/rocksdb_store.rs index f8680b8b4..779269278 100644 --- a/node/src/storage/rocksdb_store.rs +++ b/node/src/storage/rocksdb_store.rs @@ -457,6 +457,71 @@ fn backfill_temp_key(op: &OutPoint) -> [u8; 36] { type DB = DBWithThreadMode; +/// One coin read from the coins column family. +/// +/// `None` means the outpoint has no row: the coin was never created, or it +/// has been spent. `connect_block` takes that as a verdict on a block that +/// spends it (`bad-txns-inputs-missingorspent`), and `accept_block` and the +/// reorg path record the block as invalid. A read that failed, or a row that +/// does not decode, says nothing about the coin, so neither comes back as +/// `None`: the node stops ([`stop_on_unreadable_coin`]). +fn coin_from_read, E: std::fmt::Display>( + read: Result, E>, + outpoint: &OutPoint, +) -> Option { + let row = match read { + Ok(row) => row?, + Err(e) => stop_on_unreadable_coin(outpoint, &e), + }; + let row = row.as_ref(); + match Coin::deserialize_compact(row) { + Some(coin) => Some(coin), + // A row in a layout this binary does not read is refused by the + // schema check at open, so one that gets here is damage. + None => stop_on_unreadable_coin( + outpoint, + &format!("the {}-byte row does not decode as a coin", row.len()), + ), + } +} + +/// Stop the node on a coin the database could not read, as Bitcoin Core +/// does. +/// +/// Core reads its coins through `CCoinsViewErrorCatcher`, which logs +/// "Error reading from database" and calls `std::abort()` on any read error +/// (`coins.cpp:398-412`). Its reason: returning to the caller "would be +/// interpreted as 'entry not found' (as opposed to unable to read data), and +/// could lead to invalid interpretation". Here that interpretation is a +/// verdict: a valid block that spends the coin would be refused and recorded +/// as invalid, and the record outlives a restart. +/// +/// `abort()`, as in Core, and as satd's other fail-stops do: no shutdown +/// flush runs. The chainstate on disk is the one the last flush left, and +/// the next start replays the blocks after it from the block files, as after +/// any crash. +fn stop_on_unreadable_coin(outpoint: &OutPoint, error: &dyn std::fmt::Display) -> ! { + tracing::error!( + %outpoint, + %error, + "Error reading from database, shutting down: a coin in the chainstate \ + could not be read. Stopping without flushing. If this happens again \ + after a restart, check the disk; -reindex-chainstate rebuilds the \ + chainstate from the block files." + ); + eprintln!( + "Error reading from database, shutting down: coin {outpoint}: {error}. \ + If this happens again after a restart, check the disk; \ + -reindex-chainstate rebuilds the chainstate from the block files." + ); + // A test build panics instead, so a test can reach this without taking + // the harness down with it. + #[cfg(test)] + panic!("Error reading from database: coin {outpoint}: {error}"); + #[cfg(not(test))] + std::process::abort() +} + /// Where the recent-height window stands. Held under /// [`RocksDbStore::recent_window`]. enum RecentWindowState { @@ -2414,23 +2479,16 @@ impl Store for RocksDbStore { fn get_coin(&self, outpoint: &OutPoint) -> Option { let cf = self.cf(CF_COINS); let key = outpoint_to_key(outpoint); - let value = self.db.get_cf(&cf, key).ok()??; - let coin = Coin::deserialize_compact(&value); - if coin.is_none() { - tracing::error!( - "corrupt coin: failed to deserialize {} bytes for {}:{}", - value.len(), - outpoint.txid, - outpoint.vout - ); - } - coin + coin_from_read(self.db.get_cf(&cf, key), outpoint) } fn has_coin(&self, outpoint: &OutPoint) -> bool { let cf = self.cf(CF_COINS); let key = outpoint_to_key(outpoint); - matches!(self.db.get_pinned_cf(&cf, key), Ok(Some(_))) + match self.db.get_pinned_cf(&cf, key) { + Ok(row) => row.is_some(), + Err(e) => stop_on_unreadable_coin(outpoint, &e), + } } fn get_tip(&self) -> Option { @@ -2900,21 +2958,8 @@ impl Store for RocksDbStore { self.db .multi_get_cf(cf_keys) .into_iter() - .enumerate() - .map(|(i, result)| { - result.ok().flatten().and_then(|v| { - let coin = Coin::deserialize_compact(&v); - if coin.is_none() { - tracing::error!( - "corrupt coin: failed to deserialize {} bytes for {}:{}", - v.len(), - outpoints[i].txid, - outpoints[i].vout - ); - } - coin - }) - }) + .zip(outpoints) + .map(|(read, outpoint)| coin_from_read(read, outpoint)) .collect() } @@ -3700,6 +3745,10 @@ impl Store for RocksDbStore { } } +#[cfg(test)] +#[path = "rocksdb_store_storefault_tests.rs"] +mod storefault_tests; + #[cfg(test)] mod tests { use super::*; diff --git a/node/src/storage/rocksdb_store_storefault_tests.rs b/node/src/storage/rocksdb_store_storefault_tests.rs new file mode 100644 index 000000000..00ca89063 --- /dev/null +++ b/node/src/storage/rocksdb_store_storefault_tests.rs @@ -0,0 +1,182 @@ +//! A coin the chainstate database cannot read stops the node. It is never +//! reported as missing. +//! +//! "Missing" is an answer about the UTXO set: `connect_block` refuses a block +//! that spends a missing coin (`bad-txns-inputs-missingorspent`) and +//! `accept_block` records the block as invalid. A failed read, or a row that +//! does not decode, says nothing about the coin. Bitcoin Core stops on a +//! coins read error for this reason (`CCoinsViewErrorCatcher`, `coins.cpp`). +//! +//! The read errors here are real ones: the coin's SST file is damaged on +//! disk, and RocksDB's block checksum refuses the read. + +use super::*; +use crate::chain::state::tests::{build_test_block_spending, make_chain_state_with_store}; +use crate::storage::coin_cache::CoinCache; +use std::panic::{AssertUnwindSafe, catch_unwind}; +use std::path::PathBuf; + +fn spendable_outpoint() -> OutPoint { + OutPoint { + txid: Txid::from_raw_hash(bitcoin::hashes::sha256d::Hash::from_byte_array([0x5d; 32])), + vout: 0, + } +} + +fn spendable_coin() -> Coin { + Coin { + amount: 50_000, + script_pubkey: bitcoin::ScriptBuf::from_bytes(vec![0x51]), + height: 0, + coinbase: false, + txseq: 0, + } +} + +/// A store holding one coin, flushed to an SST file of its own. Returns the +/// store, its directory, and the path of that file. +fn store_with_a_coin_on_disk() -> (RocksDbStore, tempfile::TempDir, PathBuf) { + let dir = tempfile::tempdir().unwrap(); + let store = RocksDbStore::open(dir.path(), false, 16, false, -1).unwrap(); + let mut batch = StoreBatch::default(); + batch.coin_puts.push((spendable_outpoint(), spendable_coin())); + store.write_batch(batch).unwrap(); + + let cf = store.cf(CF_COINS); + let mut flush = rocksdb::FlushOptions::default(); + flush.set_wait(true); + store.db.flush_cfs_opt(&[&cf], &flush).unwrap(); + drop(cf); + + let ssts: Vec<_> = store + .db + .live_files() + .unwrap() + .into_iter() + .filter(|f| f.column_family_name == CF_COINS) + .collect(); + assert_eq!(ssts.len(), 1, "fixture: the coin is in one SST file"); + let sst = store.db.path().join(ssts[0].name.trim_start_matches('/')); + (store, dir, sst) +} + +/// Flip the first bytes of `sst`, inside its first data block, so that +/// RocksDB's block checksum fails every read of the coin. The file is +/// written in place, as a failing disk would leave it. +fn damage(sst: &std::path::Path) { + use std::io::{Read as _, Seek as _, SeekFrom, Write as _}; + let mut file = std::fs::OpenOptions::new().read(true).write(true).open(sst).unwrap(); + let mut head = [0u8; 16]; + file.read_exact(&mut head).unwrap(); + for b in &mut head { + *b ^= 0xff; + } + file.seek(SeekFrom::Start(0)).unwrap(); + file.write_all(&head).unwrap(); + file.sync_all().unwrap(); +} + +/// Run `f`. Returns the message it stopped the node with, or `Err` with +/// what it returned if it did not stop. +fn stopped_with(f: impl FnOnce() -> R) -> Result { + match catch_unwind(AssertUnwindSafe(f)) { + Ok(returned) => Err(format!("{returned:?}")), + Err(payload) => Ok(payload + .downcast_ref::() + .cloned() + .or_else(|| payload.downcast_ref::<&str>().map(|s| s.to_string())) + .unwrap_or_default()), + } +} + +fn assert_stops(what: &str, outcome: Result) { + match outcome { + Ok(message) => assert!( + message.contains("Error reading from database"), + "{what}: stopped with an unexpected message: {message}" + ), + Err(returned) => panic!("{what}: a coin that cannot be read must stop the node, but the read returned {returned}"), + } +} + +/// Every coin read, `get_coin`, `get_coins_batch` and `has_coin`, stops the +/// node when RocksDB cannot read the row. The same reads of an undamaged copy +/// find the coin, so the reads below do reach the damaged block. +/// +/// Fails without the fix: `get_coin` returned `None`, `get_coins_batch` +/// `[None]` and `has_coin` `false`, as if the coin had been spent. +#[test] +fn a_coin_read_error_stops_the_node_and_is_never_read_as_missing() { + let op = spendable_outpoint(); + + let (intact, _intact_dir, _) = store_with_a_coin_on_disk(); + assert_eq!(intact.get_coin(&op), Some(spendable_coin())); + assert_eq!(intact.get_coins_batch(&[op]), vec![Some(spendable_coin())]); + assert!(intact.has_coin(&op)); + + let (store, _dir, sst) = store_with_a_coin_on_disk(); + damage(&sst); + assert_stops("get_coin", stopped_with(|| store.get_coin(&op))); + assert_stops("get_coins_batch", stopped_with(|| store.get_coins_batch(&[op]))); + assert_stops("has_coin", stopped_with(|| store.has_coin(&op))); +} + +/// A row that is present but does not decode as a coin is not a missing coin +/// either. A row in a layout this binary does not read should have been +/// refused by the schema check at open; one that slips past it is damage. +/// +/// Fails without the fix: both reads logged "corrupt coin" and returned +/// `None`. +#[test] +fn an_undecodable_coin_row_stops_the_node_and_is_never_read_as_missing() { + let (store, _dir, _) = store_with_a_coin_on_disk(); + let op = OutPoint { vout: 1, ..spendable_outpoint() }; + let cf = store.cf(CF_COINS); + store.db.put_cf(&cf, outpoint_to_key(&op), b"").unwrap(); + drop(cf); + assert!(Coin::deserialize_compact(b"").is_none(), "fixture: the row does not decode"); + + assert_stops("get_coin", stopped_with(|| store.get_coin(&op))); + assert_stops("get_coins_batch", stopped_with(|| store.get_coins_batch(&[spendable_outpoint(), op]))); + // The coin beside it still reads. + assert_eq!(store.get_coin(&spendable_outpoint()), Some(spendable_coin())); +} + +/// A block that spends a coin the database cannot read is not refused and +/// not recorded as invalid: the node stops before it reaches a verdict, and +/// the block is judged again after the restart. The same block over an +/// undamaged copy of the database connects. +/// +/// The store is wrapped in the coin cache, as the node runs it. +/// +/// Fails without the fix: `accept_block` returned +/// `bad-txns-inputs-missingorspent` and the block's index entry was +/// `Invalid`, which survives a restart and keeps the node off any chain +/// that includes the block. +#[test] +fn a_block_spending_a_coin_that_cannot_be_read_is_not_marked_invalid() { + let genesis = bitcoin::constants::genesis_block(bitcoin::Network::Regtest).block_hash(); + let block = build_test_block_spending(genesis, 1, 1_707_300_001, spendable_outpoint()); + let hash = block.block_hash(); + + let (intact, _intact_db, _) = store_with_a_coin_on_disk(); + let (cs, dir) = make_chain_state_with_store(Box::new(CoinCache::new(Box::new(intact), 16))); + cs.accept_block(&block).expect("over an undamaged database the block connects"); + assert_eq!(cs.tip_hash(), hash); + drop(cs); + let _ = std::fs::remove_dir_all(&dir); + + let (store, _db, sst) = store_with_a_coin_on_disk(); + let (cs, dir) = make_chain_state_with_store(Box::new(CoinCache::new(Box::new(store), 16))); + damage(&sst); + let outcome = stopped_with(|| cs.accept_block(&block).map(|a| a.hash())); + let status = cs.get_block_index(&hash).map(|e| e.status); + assert_ne!( + status, + Some(BlockStatus::Invalid), + "a read error is not a verdict on the block (accept_block: {outcome:?})" + ); + assert_stops("accept_block", outcome); + assert_eq!(cs.tip_hash(), genesis, "the tip must not move"); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/node/src/validation/block.rs b/node/src/validation/block.rs index dcb637630..f76126d6f 100644 --- a/node/src/validation/block.rs +++ b/node/src/validation/block.rs @@ -67,11 +67,6 @@ pub fn check_block( network: Network, height: u32, ) -> Result<(), ValidationError> { - // Block must have at least one transaction - if block.txdata.is_empty() { - return Err(ValidationError::EmptyBlock); - } - // Check merkle root. Core runs `CheckMerkleRoot` FIRST, ahead of the size // limits and the coinbase-position tests (`validation.cpp`, `CheckBlock`), // because "all potential-corruption validation must be done before we do @@ -79,16 +74,17 @@ pub fn check_block( // a header must not get the header marked invalid. A block that is both // oversized and merkle-broken therefore answers `bad-txnmrklroot`, not // `bad-blk-length`. - let computed = block.compute_merkle_root(); - match computed { - Some(root) => { - if root != block.header.merkle_root { - return Err(ValidationError::BadMerkleRoot); - } - } - None => { - return Err(ValidationError::EmptyBlock); - } + // + // That includes a block with no transactions at all. Core's + // `ComputeMerkleRoot` returns all-zero for an empty list, so an empty body + // sent for a real header is `bad-txnmrklroot` (`BLOCK_MUTATED`): it says + // nothing about the block the header names. Only an empty body under an + // all-zero merkle root reaches the size limits below. + let computed = block + .compute_merkle_root() + .unwrap_or_else(bitcoin::TxMerkleNode::all_zeros); + if computed != block.header.merkle_root { + return Err(ValidationError::BadMerkleRoot); } // CVE-2012-2459: reject a merkle-mutated block. A tx list that duplicates a @@ -101,6 +97,12 @@ pub fn check_block( return Err(ValidationError::BadTxDuplicate); } + // Core's size-limits test opens with `block.vtx.empty()`, under the same + // `bad-blk-length` reason. + if block.txdata.is_empty() { + return Err(ValidationError::EmptyBlock); + } + // Size limits, split exactly as Core splits them (#548): // // 1. `CheckBlock`'s stripped-size test — tx count and witness-stripped @@ -536,11 +538,39 @@ mod tests { assert!(check_block(&genesis, Network::Regtest, 0).is_ok()); } + /// Core's `feature_block.py` b46: no transactions under an all-zero merkle + /// root reaches the size limits and is `bad-blk-length`. #[test] fn test_empty_block_rejected() { let mut block = bitcoin::constants::genesis_block(Network::Regtest); block.txdata.clear(); - assert!(matches!(check_block(&block, Network::Regtest, 0), Err(ValidationError::EmptyBlock))); + block.header.merkle_root = bitcoin::TxMerkleNode::all_zeros(); + let err = check_block(&block, Network::Regtest, 0).unwrap_err(); + assert!(matches!(err, ValidationError::EmptyBlock), "{err:?}"); + assert_eq!(err.to_string(), "bad-blk-length"); + assert!(!err.is_mutation_class()); + } + + /// An empty body sent for a real header: Core's `CheckMerkleRoot` runs + /// first and compares the header against the all-zero root of an empty + /// list, so the answer is `bad-txnmrklroot` (`BLOCK_MUTATED`), not + /// `bad-blk-length`. The difference decides whether the header's hash may + /// be marked invalid for it. + /// + /// Perturbation: move the `txdata.is_empty()` test back above the merkle + /// comparison and this returns `EmptyBlock`. + #[test] + fn an_empty_body_under_a_real_merkle_root_is_a_merkle_mismatch() { + let mut block = bitcoin::constants::genesis_block(Network::Regtest); + block.txdata.clear(); + let err = check_block(&block, Network::Regtest, 0).unwrap_err(); + assert!(matches!(err, ValidationError::BadMerkleRoot), "{err:?}"); + assert_eq!(err.to_string(), "bad-txnmrklroot"); + assert!(err.is_mutation_class()); + assert!( + is_block_mutated(&block, true), + "the P2P gate and check_block must agree on this body" + ); } #[test] diff --git a/node/tests/feature_block_consensus.rs b/node/tests/feature_block_consensus.rs index 550c19c61..6ec70af17 100644 --- a/node/tests/feature_block_consensus.rs +++ b/node/tests/feature_block_consensus.rs @@ -293,6 +293,18 @@ fn case_accept_genesis() -> Satd { } fn case_empty_block() -> Satd { + // feature_block.py b46: `vtx = []` with `hashMerkleRoot = 0`. The empty + // list's merkle root is all-zero, so the header matches it and the block + // reaches the size limits. Against any other root it is `bad-txnmrklroot`. + let mut b = bitcoin::constants::genesis_block(Network::Regtest); + b.txdata.clear(); + b.header.merkle_root = TxMerkleNode::all_zeros(); + cb(&b) +} + +fn case_empty_block_under_real_merkle_root() -> Satd { + // The same empty body sent for a real header: Core's `CheckMerkleRoot` + // runs before the size limits, so this is `bad-txnmrklroot`. let mut b = bitcoin::constants::genesis_block(Network::Regtest); b.txdata.clear(); cb(&b) @@ -720,6 +732,7 @@ fn cases() -> Vec { // context-free block structure Case { name: "accept_genesis", core: Accept, expect: Match, run: case_accept_genesis }, Case { name: "empty_block", core: Reject("bad-blk-length"), expect: Match, run: case_empty_block }, + Case { name: "empty_block_under_real_merkle_root", core: Reject("bad-txnmrklroot"), expect: Match, run: case_empty_block_under_real_merkle_root }, Case { name: "first_tx_not_coinbase", core: Reject("bad-cb-missing"), expect: Match, run: case_first_tx_not_coinbase }, Case { name: "multiple_coinbase", core: Reject("bad-cb-multiple"), expect: Match, run: case_multiple_coinbase }, Case { name: "bad_merkle_root", core: Reject("bad-txnmrklroot"), expect: Match, run: case_bad_merkle_root }, 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"); +}