From 5edeb7a35ebc9ee632aa3ce9de5b2432dd8eb347 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Wed, 7 Oct 2026 14:33:38 -0600 Subject: [PATCH 1/8] Check a block's merkle root first, and mark only what Core marks check_block tested for an empty transaction list before comparing the merkle root, so an empty body sent for a real header failed bad-blk-length rather than Core's bad-txnmrklroot. accept_block then marked the header's block Invalid for that and for every other non-mutation check_block failure. Core runs CheckMerkleRoot first (validation.cpp CheckBlock), and ProcessNewBlock never marks a block for a CheckBlock failure; of the rules in satd's check_block, Core marks only bad-blk-weight, from ContextualCheckBlock. A block that extends the tip and arrived without its header first had no index entry until it connected, so a failed connect marked nothing and each copy sent again was validated and written again. accept_block now writes the HeaderOnly row a header announcement would leave before connecting, as Core's AcceptBlockHeader creates the index entry before ConnectTip. Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/chain/state.rs | 67 ++++- node/src/chain/state_blockaccept_tests.rs | 303 ++++++++++++++++++++++ node/src/net/manager.rs | 4 + node/src/net/manager_blockaccept_tests.rs | 69 +++++ node/src/validation/block.rs | 62 +++-- node/tests/feature_block_consensus.rs | 13 + 6 files changed, 490 insertions(+), 28 deletions(-) create mode 100644 node/src/chain/state_blockaccept_tests.rs create mode 100644 node/src/net/manager_blockaccept_tests.rs diff --git a/node/src/chain/state.rs b/node/src/chain/state.rs index 6518f004..749d1620 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,37 @@ 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, so every outcome below matches the headers-first path: a + // connect writes the `Valid` entry over it, a verdict marks it + // `Invalid`, and any other failure leaves a known header whose block + // is not stored. The best-header pointer is not moved here; a + // successful connect moves it at the end. + 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 +9933,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 00000000..1d329596 --- /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 aea389ed..c460eb4e 100644 --- a/node/src/net/manager.rs +++ b/node/src/net/manager.rs @@ -10441,6 +10441,10 @@ fn historical_block_storable( canonical_at_height == Some(block_hash) } +#[cfg(test)] +#[path = "manager_blockaccept_tests.rs"] +mod blockaccept_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 00000000..2ca43985 --- /dev/null +++ b/node/src/net/manager_blockaccept_tests.rs @@ -0,0 +1,69 @@ +//! 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); + + 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/validation/block.rs b/node/src/validation/block.rs index dcb63763..f76126d6 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 550c19c6..6ec70af1 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 }, From 3051d8c10161df2878405092dfeefe0eed26d00a Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Wed, 7 Oct 2026 21:28:57 -0600 Subject: [PATCH 2/8] 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 86f08140..1de20af0 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 c460eb4e..d5fae547 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 2ca43985..7d94adbe 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 00000000..6d6c8355 --- /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 5069ddd6..4e7792a0 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 00000000..056498aa --- /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 eacc241a..cd888bf3 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 00000000..8cfd3906 --- /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"); +} From 960284b8ff54463ecbceae650009d0950bb2285a Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Wed, 7 Oct 2026 21:39:02 -0600 Subject: [PATCH 3/8] accept_block: say what a non-verdict failure leaves for an unannounced block The comment on the HeaderOnly row written before connecting said every outcome matched the headers-first path. A failure that is not a verdict on the block does not move the best-header pointer, which an announced header would have done. Say so, and say what reaches the block then. Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/chain/state.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/node/src/chain/state.rs b/node/src/chain/state.rs index 749d1620..afe51ff8 100644 --- a/node/src/chain/state.rs +++ b/node/src/chain/state.rs @@ -7380,11 +7380,13 @@ impl ChainState { // in full. // // The entry is the `HeaderOnly` row a header announcement would have - // left, so every outcome below matches the headers-first path: a - // connect writes the `Valid` entry over it, a verdict marks it - // `Invalid`, and any other failure leaves a known header whose block - // is not stored. The best-header pointer is not moved here; a - // successful connect moves it at the end. + // 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, From 3248cfeeb26918fc93802366d2ff1f49d780390e Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Wed, 7 Oct 2026 22:30:51 -0600 Subject: [PATCH 4/8] Stop on a coin read error instead of treating the coin as missing RocksDbStore turned a failed coin read into "no such coin": get_coin did `.ok()??`, get_coins_batch `.ok().flatten()` and has_coin matched only `Ok(Some(_))`. A row that did not decode was logged and returned as `None` too. connect_block takes a missing coin as a verdict on the block that spends it (bad-txns-inputs-missingorspent), and accept_block and the reorg path then record that block as invalid, a record that survives a restart. Bitcoin Core reads coins through CCoinsViewErrorCatcher, which logs "Error reading from database" and calls std::abort() on any read error (coins.cpp:398-412), because returning would be "interpreted as 'entry not found'". Each coin read now goes through one helper: a missing row is `None`, a read error or an undecodable row stops the node with abort(), as Core does and as satd's other fail-stops do. A test build panics instead so the path can be tested. The Operator Manual says what the stop looks like and what to do after it. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/manual/src/ibd.md | 15 ++ node/src/storage/rocksdb_store.rs | 103 +++++++--- .../storage/rocksdb_store_storefault_tests.rs | 182 ++++++++++++++++++ 3 files changed, 273 insertions(+), 27 deletions(-) create mode 100644 node/src/storage/rocksdb_store_storefault_tests.rs diff --git a/docs/manual/src/ibd.md b/docs/manual/src/ibd.md index 3f89c968..b628ce84 100644 --- a/docs/manual/src/ibd.md +++ b/docs/manual/src/ibd.md @@ -381,6 +381,21 @@ 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. 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 diff --git a/node/src/storage/rocksdb_store.rs b/node/src/storage/rocksdb_store.rs index f8680b8b..77926927 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 00000000..00ca8906 --- /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); +} From 430a730245c12d2ed124e89a365b0986949a85a9 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Thu, 8 Oct 2026 07:14:16 -0600 Subject: [PATCH 5/8] signet: rewrite the commitment section exactly as Core does A signet solution signs the block with the solution cut out of the coinbase witness commitment, so the bytes left behind are part of the signed message. satd rebuilt that script from rust-bitcoin's `instructions()`, which differs from Core's FetchAndClearCommitmentSection (signet.cpp:32-57) in two places: - A zero-length OP_PUSHDATA1/2/4 comes back as an empty push, which satd wrote as OP_0 (0x00). Core writes an op with no data back as its own opcode byte (`CScript << opcode`), 0x4c, 0x4d or 0x4e. - An op that does not parse made satd give up on the whole script and check the block as if it carried no solution. Core's GetOp loop stops at that op, keeps the section it found before it, and drops the rest from the rebuilt script. Either way the merkle root the solution signs differed from Core's, so satd refused blocks Core accepts and could accept, under a challenge an empty solution meets, a block whose solution Core refuses. The rewrite is now a port of Core's: GetScriptOp to read each op, the opcode byte for an op with no data, and AppendDataSize (the shortest length prefix) for a push with data, which Core also applies to pushes that used a longer prefix. The expected rewrites in the tests are Core's own output: the function from signet.cpp, compiled with Core's script.cpp, run over each input. Core's signet_parse_tests are ported too. Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/validation/signet.rs | 125 +++++-- .../src/validation/signet_consharden_tests.rs | 317 ++++++++++++++++++ 2 files changed, 414 insertions(+), 28 deletions(-) create mode 100644 node/src/validation/signet_consharden_tests.rs diff --git a/node/src/validation/signet.rs b/node/src/validation/signet.rs index 9c0a94e0..f67392eb 100644 --- a/node/src/validation/signet.rs +++ b/node/src/validation/signet.rs @@ -16,7 +16,7 @@ //! commitment), and time. use bitcoin::blockdata::opcodes::all::OP_RETURN; -use bitcoin::blockdata::script::{Builder, Instruction}; +use bitcoin::blockdata::script::Builder; use bitcoin::consensus::Decodable; use bitcoin::hashes::{sha256d, Hash}; use bitcoin::p2p::Magic; @@ -99,39 +99,103 @@ fn witness_commitment_index(coinbase: &Transaction) -> Option { /// [`SIGNET_HEADER`] and which carries data beyond the header). On the /// first match, return `(rebuilt_script, solution)` where `solution` is /// the bytes after the header and `rebuilt_script` is the script with -/// that push truncated to just the header — exactly Core's -/// `FetchAndClearCommitmentSection`. Returns `None` if no signet push is -/// present (Core allows this, e.g. an `OP_TRUE` trivial challenge). +/// that push truncated to just the header. Returns `None` if no signet push +/// is present (Core allows this, e.g. an `OP_TRUE` trivial challenge). +/// +/// A port of Core's `FetchAndClearCommitmentSection` (`signet.cpp:32-57`), +/// byte for byte, because the rebuilt script feeds the merkle root the +/// solution signs: +/// - every op is read with Core's `GetScriptOp` ([`get_script_op`]), and the +/// scan stops at the first op that does not parse. Ops already read are +/// kept, so a signet section found before that point is still used and the +/// unparseable tail is dropped from the rebuilt script. +/// - a push with data is written back as `CScript << data` writes it: the +/// shortest length prefix (`AppendDataSize`), whatever prefix it had. +/// - an op with no data is written back as its one opcode byte +/// (`CScript << opcode`). That includes a zero-length `OP_PUSHDATA1/2/4`, +/// which becomes the bare `0x4c`, `0x4d` or `0x4e`, not `OP_0`. fn fetch_and_clear_signet_section(script: &ScriptBuf) -> Option<(ScriptBuf, Vec)> { - let mut builder = Builder::new(); + let bytes = script.as_bytes(); + let mut replacement: Vec = Vec::with_capacity(bytes.len()); let mut solution: Option> = None; - for instr in script.instructions() { - // A malformed script can't carry a valid solution. - let instr = instr.ok()?; - match instr { - Instruction::Op(op) => { - builder = builder.push_opcode(op); - } - Instruction::PushBytes(push) => { - let bytes = push.as_bytes(); - if solution.is_none() - && bytes.len() > SIGNET_HEADER.len() - && bytes[..SIGNET_HEADER.len()] == SIGNET_HEADER - { - solution = Some(bytes[SIGNET_HEADER.len()..].to_vec()); - // Keep only the header in the rebuilt script. - builder = builder.push_slice(SIGNET_HEADER); - } else { - // push.as_bytes() is a valid pushable slice. - let pb: &bitcoin::script::PushBytes = push; - builder = builder.push_slice(pb); - } - } + let mut pc = 0; + while let Some((opcode, pushdata)) = get_script_op(bytes, &mut pc) { + if pushdata.is_empty() { + replacement.push(opcode); + continue; + } + let mut pushdata = pushdata; + if solution.is_none() + && pushdata.len() > SIGNET_HEADER.len() + && pushdata[..SIGNET_HEADER.len()] == SIGNET_HEADER + { + solution = Some(pushdata[SIGNET_HEADER.len()..].to_vec()); + // Keep only the header in the rebuilt script. + pushdata = &pushdata[..SIGNET_HEADER.len()]; } + append_push(&mut replacement, pushdata); } - solution.map(|s| (builder.into_script(), s)) + solution.map(|s| (ScriptBuf::from_bytes(replacement), s)) +} + +/// Core's `GetScriptOp` (`script/script.cpp:312-362`): read the op at `*pc`, +/// advance `*pc` past it, and return the opcode with its push data (empty +/// for an op that pushes nothing). `None` at the end of the script, or when +/// a push's length prefix or data runs past the end. +fn get_script_op<'a>(script: &'a [u8], pc: &mut usize) -> Option<(u8, &'a [u8])> { + const OP_PUSHDATA1: u8 = 0x4c; + const OP_PUSHDATA2: u8 = 0x4d; + const OP_PUSHDATA4: u8 = 0x4e; + + let opcode = *script.get(*pc)?; + let mut at = *pc + 1; + let size = match opcode { + 0..OP_PUSHDATA1 => usize::from(opcode), + OP_PUSHDATA1 => { + let n = *script.get(at)?; + at += 1; + usize::from(n) + } + OP_PUSHDATA2 => { + let n = script.get(at..at + 2)?; + at += 2; + usize::from(u16::from_le_bytes([n[0], n[1]])) + } + OP_PUSHDATA4 => { + let n = script.get(at..at + 4)?; + at += 4; + usize::try_from(u32::from_le_bytes([n[0], n[1], n[2], n[3]])).ok()? + } + _ => { + *pc = at; + return Some((opcode, &[])); + } + }; + let data = script.get(at..at.checked_add(size)?)?; + *pc = at + size; + Some((opcode, data)) +} + +/// Core's `CScript << data` for a non-empty push: `AppendDataSize` +/// (`script/script.h:407-425`), the shortest length prefix, then the bytes. +fn append_push(out: &mut Vec, data: &[u8]) { + const OP_PUSHDATA1: usize = 0x4c; + let len = data.len(); + if len < OP_PUSHDATA1 { + out.push(len as u8); + } else if len <= 0xff { + out.push(0x4c); + out.push(len as u8); + } else if len <= 0xffff { + out.push(0x4d); + out.extend_from_slice(&(len as u16).to_le_bytes()); + } else { + out.push(0x4e); + out.extend_from_slice(&(len as u32).to_le_bytes()); + } + out.extend_from_slice(data); } /// Double-SHA256 merkle root over `txids` (Bitcoin's odd-node-duplicates @@ -356,6 +420,7 @@ mod tests { } use bitcoin::blockdata::opcodes::all::OP_PUSHNUM_1; + use bitcoin::blockdata::script::Instruction; use bitcoin::script::PushBytes; use bitcoin::sighash::{EcdsaSighashType, SighashCache}; use bitcoin::{block, BlockHash, CompactTarget, TxMerkleNode}; @@ -634,3 +699,7 @@ mod tests { assert!(matches!(ins[1], Instruction::PushBytes(b) if b.len() == 72)); } } + +#[cfg(test)] +#[path = "signet_consharden_tests.rs"] +mod consharden_tests; diff --git a/node/src/validation/signet_consharden_tests.rs b/node/src/validation/signet_consharden_tests.rs new file mode 100644 index 00000000..e95f240d --- /dev/null +++ b/node/src/validation/signet_consharden_tests.rs @@ -0,0 +1,317 @@ +//! The signet commitment rewrite matches Bitcoin Core's byte for byte. +//! +//! A signet solution signs the block with the solution itself cut out of the +//! coinbase (`FetchAndClearCommitmentSection`, `signet.cpp:32-57`). The bytes +//! left behind change the coinbase txid and so the merkle root the signature +//! commits to: a rewrite that differs from Core's in one byte refuses a block +//! Core accepts. An op that does not parse ends Core's scan without losing the +//! section found before it, so the solution is still checked. +//! +//! Every expected rewrite below is Core's output, not a reading of it: the +//! function from `signet.cpp`, copied verbatim, compiled with Core's own +//! `script/script.cpp` (`GetScriptOp`, `CScript`) at commit 16613c9de9 and run +//! over each input. + +use super::*; +use bitcoin::hashes::hex::FromHex; +use bitcoin::script::PushBytes; +use bitcoin::sighash::{EcdsaSighashType, SighashCache}; +use bitcoin::{block, BlockHash, CompactTarget, Network, TxMerkleNode}; + +/// `OP_RETURN PUSH36[aa21a9ed || 32 zero bytes]`: a BIP 141 commitment. +const COMMITMENT: &str = "6a24aa21a9ed0000000000000000000000000000000000000000000000000000000000000000"; + +fn hex(s: &str) -> Vec { + Vec::::from_hex(s).expect("test hex") +} + +/// `COMMITMENT` followed by `rest` (hex). +fn commitment_then(rest: &str) -> Vec { + hex(&format!("{COMMITMENT}{rest}")) +} + +/// The rewrite of `script`, or `None` when no signet section is found (Core +/// then leaves the script as it is). +fn rewrite(script: &[u8]) -> Option<(Vec, Vec)> { + fetch_and_clear_signet_section(&ScriptBuf::from_bytes(script.to_vec())) + .map(|(cleared, solution)| (cleared.into_bytes(), solution)) +} + +/// `(name, commitment output script after COMMITMENT, Core's rewrite after +/// COMMITMENT, Core's solution)`. `06ecc7daa20000` is a signet section holding +/// an empty scriptSig and an empty witness stack. +const CORE_REWRITES: &[(&str, &str, &str, &str)] = &[ + ("section only", "06ecc7daa20000", "04ecc7daa2", "0000"), + ("zero-length PUSHDATA1 before", "4c0006ecc7daa20000", "4c04ecc7daa2", "0000"), + ("zero-length PUSHDATA1 after", "06ecc7daa200004c00", "04ecc7daa24c", "0000"), + ("zero-length PUSHDATA2 before", "4d000006ecc7daa20000", "4d04ecc7daa2", "0000"), + ("zero-length PUSHDATA2 after", "06ecc7daa200004d0000", "04ecc7daa24d", "0000"), + ("zero-length PUSHDATA4 before", "4e0000000006ecc7daa20000", "4e04ecc7daa2", "0000"), + ("zero-length PUSHDATA4 after", "06ecc7daa200004e00000000", "04ecc7daa24e", "0000"), + ("PUSHDATA1 with no length byte after", "06ecc7daa200004c", "04ecc7daa2", "0000"), + ("PUSHDATA1 data cut short after", "06ecc7daa200004c05aabb", "04ecc7daa2", "0000"), + ("direct push cut short after", "06ecc7daa2000005aabb", "04ecc7daa2", "0000"), + ("PUSHDATA2 length cut short after", "06ecc7daa200004d01", "04ecc7daa2", "0000"), + ("PUSHDATA4 length cut short after", "06ecc7daa200004e0100", "04ecc7daa2", "0000"), + ("OP_0 before", "0006ecc7daa20000", "0004ecc7daa2", "0000"), + ("non-push ops around", "0051ac06ecc7daa20000004f61", "0051ac04ecc7daa2004f61", "0000"), + ( + "long length prefixes are rewritten short", + "4c01014c06ecc7daa200004d0300aabbcc", + "010104ecc7daa203aabbcc", + "0000", + ), + ("a header with no data is not a section", "04ecc7daa206ecc7daa20000", "04ecc7daa204ecc7daa2", "0000"), + ("only the first section is cleared", "06ecc7daa2000006ecc7daa20101", "04ecc7daa206ecc7daa20101", "0000"), +]; + +#[test] +fn the_commitment_rewrite_is_cores_byte_for_byte() { + for (name, input, core_rewrite, core_solution) in CORE_REWRITES { + let (cleared, solution) = + rewrite(&commitment_then(input)).unwrap_or_else(|| panic!("{name}: Core finds the section")); + assert_eq!( + bitcoin::hex::DisplayHex::to_lower_hex_string(&cleared[..]), + format!("{COMMITMENT}{core_rewrite}"), + "{name}: rewritten commitment" + ); + assert_eq!(solution, hex(core_solution), "{name}: solution"); + } + + // A section long enough to need OP_PUSHDATA1 keeps only the header. + let data = "11".repeat(80); + let (cleared, solution) = + rewrite(&commitment_then(&format!("4c54ecc7daa2{data}"))).expect("Core finds the section"); + assert_eq!(cleared, commitment_then("04ecc7daa2")); + assert_eq!(solution, hex(&data)); +} + +/// Core finds no section in these and keeps the script as it is. +#[test] +fn a_commitment_without_a_section_is_left_alone() { + for (name, input) in [ + ("commitment only", ""), + ("zero-length PUSHDATA1", "4c00"), + ("PUSHDATA1 with no length byte", "4c"), + ("a header with no data", "04ecc7daa2"), + ("an unparseable op hides a later header", "4c0a04ecc7daa20000"), + ] { + assert_eq!(rewrite(&commitment_then(input)), None, "{name}"); + } +} + +/// Core's `signet_parse_tests` (`src/test/validation_tests.cpp:69-128`), with +/// its `OP_TRUE` challenge. +#[test] +fn cores_signet_parse_tests() { + let challenge = ScriptBuf::from_bytes(vec![0x51]); + let genesis = bitcoin::constants::genesis_block(Network::Signet).block_hash(); + let check = |block: &Block| { + let created = SignetTxs::create(block, &challenge).is_some(); + let checked = check_signet_block_solution(block, challenge.as_bytes(), genesis).is_ok(); + assert_eq!(created, checked, "Create and the check agree"); + checked + }; + let mut block = Block { + header: block::Header { + version: block::Version::from_consensus(0), + prev_blockhash: BlockHash::all_zeros(), + merkle_root: TxMerkleNode::all_zeros(), + time: 0, + bits: CompactTarget::from_consensus(0), + nonce: 0, + }, + txdata: vec![], + }; + + // empty block is invalid + assert!(!check(&block)); + + // no witness commitment + let mut cb = Transaction { + version: transaction::Version(2), + lock_time: absolute::LockTime::ZERO, + input: vec![], + output: vec![TxOut { value: Amount::ZERO, script_pubkey: ScriptBuf::new() }], + }; + block.txdata = vec![cb.clone(), cb.clone()]; + assert!(!check(&block)); + + let section_141: Vec = [0xaa, 0x21, 0xa9, 0xed].into_iter().chain([0xff; 32]).collect(); + let with_sections = |cb: &mut Transaction, block: &mut Block, sections: &[&[u8]]| { + let mut b = Builder::new().push_opcode(OP_RETURN); + for s in sections { + let pb: &PushBytes = (*s).try_into().unwrap(); + b = b.push_slice(pb); + } + cb.output[0].script_pubkey = b.into_script(); + block.txdata[0] = cb.clone(); + }; + + // no header is treated valid + with_sections(&mut cb, &mut block, &[§ion_141]); + assert!(check(&block)); + + // no data after header, valid + let mut section_325 = vec![0xec, 0xc7, 0xda, 0xa2]; + with_sections(&mut cb, &mut block, &[§ion_141, §ion_325]); + assert!(check(&block)); + + // Premature end of data, invalid + section_325.extend([0x01, 0x51]); + with_sections(&mut cb, &mut block, &[§ion_141, §ion_325]); + assert!(!check(&block)); + + // has data, valid + section_325.push(0x00); + with_sections(&mut cb, &mut block, &[§ion_141, §ion_325]); + assert!(check(&block)); + + // Extraneous data, invalid + section_325.push(0x00); + with_sections(&mut cb, &mut block, &[§ion_141, §ion_325]); + assert!(!check(&block)); +} + +// ---- Whole blocks, checked the way a block from the network is ---- + +fn block_with_commitment(script: Vec) -> Block { + let coinbase = Transaction { + version: transaction::Version(2), + lock_time: absolute::LockTime::ZERO, + input: vec![TxIn { + previous_output: OutPoint::null(), + script_sig: Builder::new().push_int(42).into_script(), + sequence: Sequence::MAX, + witness: Witness::new(), + }], + output: vec![TxOut { value: Amount::ZERO, script_pubkey: ScriptBuf::from_bytes(script) }], + }; + Block { + header: block::Header { + version: block::Version::from_consensus(0x20000000), + prev_blockhash: BlockHash::from_byte_array([0x11; 32]), + merkle_root: TxMerkleNode::from_raw_hash(coinbase.compute_txid().to_raw_hash()), + time: 1_600_000_000, + bits: CompactTarget::from_consensus(0x1e0377ae), + nonce: 0, + }, + txdata: vec![coinbase], + } +} + +fn genesis() -> BlockHash { + bitcoin::constants::genesis_block(Network::Signet).block_hash() +} + +/// A P2WPKH challenge and its key. +fn p2wpkh_challenge() -> (ScriptBuf, bitcoin::secp256k1::SecretKey, bitcoin::CompressedPublicKey) { + let secp = bitcoin::secp256k1::Secp256k1::new(); + let sk = bitcoin::secp256k1::SecretKey::from_slice(&[0x42u8; 32]).unwrap(); + let pk = bitcoin::CompressedPublicKey(sk.public_key(&secp)); + (ScriptBuf::new_p2wpkh(&pk.wpubkey_hash()), sk, pk) +} + +/// A solution (empty scriptSig, P2WPKH witness) signing the block whose +/// coinbase commitment is `signed_commitment` exactly. `signed_commitment` +/// must hold no signet section, so that the message is built over it as it +/// stands; the fixed header, prev hash and time are those of every block here. +fn solution_signing(signed_commitment: &[u8]) -> Vec { + assert_eq!(rewrite(signed_commitment), None, "the signed commitment carries no section"); + let (challenge, sk, pk) = p2wpkh_challenge(); + let txs = SignetTxs::create(&block_with_commitment(signed_commitment.to_vec()), &challenge) + .expect("signing transactions"); + let sighash = SighashCache::new(&txs.to_sign) + .p2wpkh_signature_hash(0, &challenge, Amount::ZERO, EcdsaSighashType::All) + .expect("sighash"); + let secp = bitcoin::secp256k1::Secp256k1::new(); + let msg = bitcoin::secp256k1::Message::from_digest(sighash.to_byte_array()); + let mut sig = secp.sign_ecdsa(&msg, &sk).serialize_der().to_vec(); + sig.push(EcdsaSighashType::All as u8); + let witness = Witness::from_slice(&[sig, pk.to_bytes().to_vec()]); + let mut solution = bitcoin::consensus::serialize(&ScriptBuf::new()); + solution.extend_from_slice(&bitcoin::consensus::serialize(&witness)); + solution +} + +/// `before || PUSH[signet header || solution] || after`, all after +/// `COMMITMENT`. +fn commitment_with_section(before: &str, solution: &[u8], after: &str) -> Vec { + let mut section = SIGNET_HEADER.to_vec(); + section.extend_from_slice(solution); + let mut script = commitment_then(before); + let mut pushed = Vec::new(); + append_push(&mut pushed, §ion); + script.extend_from_slice(&pushed); + script.extend_from_slice(&hex(after)); + script +} + +/// A zero-length `OP_PUSHDATA1/2/4` stays its own opcode in the signed +/// message. A block signed over Core's rewrite is accepted, and the same +/// block signed over the rewrite with `OP_0` in that place is refused. +#[test] +fn a_zero_length_pushdata_is_signed_as_its_own_opcode() { + let (challenge, _, _) = p2wpkh_challenge(); + // (before, after, Core's rewrite after COMMITMENT, the rewrite with OP_0) + for (before, after, core, with_op_0) in [ + ("4c00", "", "4c04ecc7daa2", "0004ecc7daa2"), + ("", "4c00", "04ecc7daa24c", "04ecc7daa200"), + ("", "4d0000", "04ecc7daa24d", "04ecc7daa200"), + ("", "4e00000000", "04ecc7daa24e", "04ecc7daa200"), + ] { + let signed_over_core = commitment_with_section(before, &solution_signing(&commitment_then(core)), after); + assert!( + check_signet_block_solution(&block_with_commitment(signed_over_core), challenge.as_bytes(), genesis()) + .is_ok(), + "{before}|section|{after}: a block signed over Core's rewrite {core} must verify" + ); + + let signed_over_op_0 = + commitment_with_section(before, &solution_signing(&commitment_then(with_op_0)), after); + assert!( + matches!( + check_signet_block_solution(&block_with_commitment(signed_over_op_0), challenge.as_bytes(), genesis()), + Err(ValidationError::BadSignetSolution) + ), + "{before}|section|{after}: a block signed over {with_op_0} is not one Core accepts" + ); + } +} + +/// Core keeps the section it found before an op that does not parse, and drops +/// the rest of the script from the signed message. A block signed that way is +/// accepted. +#[test] +fn a_section_before_an_unparseable_push_is_checked() { + let (challenge, _, _) = p2wpkh_challenge(); + for after in ["4c", "4c05aabb", "05aabb", "4d01", "4e0100"] { + let script = commitment_with_section("", &solution_signing(&commitment_then("04ecc7daa2")), after); + assert!( + check_signet_block_solution(&block_with_commitment(script), challenge.as_bytes(), genesis()).is_ok(), + "section then {after}: Core checks the section and accepts the block" + ); + } +} + +/// The other side of the same rule: the section is checked, so a solution that +/// does not parse is refused, even under a challenge any empty solution meets. +#[test] +fn a_bad_section_before_an_unparseable_push_is_refused() { + let op_true = [0x51]; + // A solution whose scriptSig claims one byte and has none. + let script = commitment_with_section("", &[0x01], "4c"); + assert!(matches!( + check_signet_block_solution(&block_with_commitment(script), &op_true, genesis()), + Err(ValidationError::BadSignetSolution) + )); + // The same block without the tail: refused too, for the same reason. + let script = commitment_with_section("", &[0x01], ""); + assert!(matches!( + check_signet_block_solution(&block_with_commitment(script), &op_true, genesis()), + Err(ValidationError::BadSignetSolution) + )); + // And with a well-formed solution and the tail, accepted. + let script = commitment_with_section("", &[0x00, 0x00], "4c"); + assert!(check_signet_block_solution(&block_with_commitment(script), &op_true, genesis()).is_ok()); +} From 11d5074b6cecd1e12e99c0d4e07e56bedc44d73f Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Thu, 8 Oct 2026 07:14:16 -0600 Subject: [PATCH 6/8] Take the median time past over a whole window, or fail the connect The connect paths read the median time past through the height index (or a replay plan) and skipped any height whose block they could not resolve, taking the median of the rest; with nothing resolved they used 0. Core's GetMedianTimePast (chain.h) walks the block and up to ten ancestors through pprev and has fewer only near genesis, so it never takes a median over a window with a hole. A shorter window moved BIP 113 and BIP 68 decisions, and an empty one met every BIP 68 time lock. A refusal it caused was then recorded against the block as invalid. A window with a missing block is now ConnectError::MedianTimeWindowGap, which is not a verdict on the block: the connect fails, nothing is marked, and the block connects once the index is whole. Every connect path uses it: accept_block and both reorg loops, the IBD connector, the reindex paths, the background chainstate and TestBlockValidity (through ChainState::median_time_past_for_connect, which still answers from the MTP cache first). The prefetcher hands such a block to the direct read. The mempool, templates and RPCs keep the old lenient get_median_time_past. BIP 68 now takes the coin's median as Core's CalculateSequenceLocks does (consensus/tx_verify.cpp:74), from the block before the coin's, GetAncestor(max(nCoinHeight - 1, 0)). That only differs for a coin at height 0, which the genesis coinbase never creates. Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/chain/background.rs | 2 +- node/src/chain/connect.rs | 72 ++++++-- node/src/chain/connect_consharden_tests.rs | 205 +++++++++++++++++++++ node/src/chain/prefetch.rs | 12 +- node/src/chain/state.rs | 98 ++++++---- node/src/chain/state_consharden_tests.rs | 99 ++++++++++ 6 files changed, 429 insertions(+), 59 deletions(-) create mode 100644 node/src/chain/connect_consharden_tests.rs create mode 100644 node/src/chain/state_consharden_tests.rs diff --git a/node/src/chain/background.rs b/node/src/chain/background.rs index 41124469..046c1cba 100644 --- a/node/src/chain/background.rs +++ b/node/src/chain/background.rs @@ -353,7 +353,7 @@ impl BackgroundChainState { } }; - let mtp = connect::get_median_time_past(store_ref, new_height); + let mtp = connect::get_median_time_past(store_ref, new_height)?; // The background runs with secondary indexes disabled. let addr_index = crate::index::address::AddressIndexConfig::default(); #[cfg(feature = "block-filter-index")] diff --git a/node/src/chain/connect.rs b/node/src/chain/connect.rs index 66694481..7bec274d 100644 --- a/node/src/chain/connect.rs +++ b/node/src/chain/connect.rs @@ -101,6 +101,13 @@ pub enum ConnectError { /// `-reindex-chainstate` rebuilds the series from genesis. #[error("cumulative transaction count missing for parent {parent}")] ChainTxGap { parent: BlockHash }, + /// The median-time-past window for the block at `height` has no block + /// at `missing`: the height index or the block index lacks a row for an + /// ancestor this node connected. Local storage damage, not a verdict + /// about the block. A median over the rows that are left would gate BIP + /// 113 and BIP 68 time locks on the wrong value. + #[error("median-time-past window for height {height} has no block at height {missing}")] + MedianTimeWindowGap { height: u32, missing: u32 }, /// The chain has more transactions than a 5-byte ordinal can name. /// Unreachable below ~1.1 trillion transactions; the variant exists /// so the encoder can never silently wrap two transactions onto one @@ -137,6 +144,7 @@ impl ConnectError { Self::FilterIndexEmit(_) => false, Self::SpIndexEmit(_) | Self::ChainTxGap { .. } + | Self::MedianTimeWindowGap { .. } | Self::TxSeqOverflow | Self::Interrupted => false, e => !e.is_mutation_class(), @@ -145,8 +153,9 @@ impl ConnectError { } /// Compute median time past (MTP) for a given height using the store directly. -/// MTP is the median of the timestamps of the previous 11 blocks. -pub(crate) fn get_median_time_past(store: &dyn Store, height: u32) -> u32 { +/// MTP is the median of the timestamps of the previous 11 blocks. See +/// [`median_time_past_with_plan`]. +pub(crate) fn get_median_time_past(store: &dyn Store, height: u32) -> Result { median_time_past_with_plan(store, None, height) } @@ -160,28 +169,45 @@ pub(crate) fn get_median_time_past(store: &dyn Store, height: u32) -> u32 { /// rewritten yet. MTP gates BIP113 locktimes and BIP68 time-based sequence /// locks, so resolving it against the wrong branch is a consensus decision made /// about a chain the node is not building. +/// +/// The value is Core's `GetMedianTimePast` for the block at `height - 1` +/// (`chain.h`): the median of that block's time and its ten ancestors' +/// (back to genesis, if it has fewer). Every block in the window must +/// resolve; a missing one is [`ConnectError::MedianTimeWindowGap`], never a +/// median over fewer blocks. No block precedes genesis, so the window for height 0 is +/// empty and the value is 0; nothing gates on it (BIP 113 starts at height 1, +/// [`bip113_activation_height`]). pub(crate) fn median_time_past_with_plan( store: &dyn Store, plan: Option<&crate::chain::replay_plan::ReplayPlan>, height: u32, -) -> u32 { - let start = height.saturating_sub(11); - let mut timestamps: Vec = Vec::new(); +) -> Result { + let start = height.saturating_sub(MEDIAN_TIME_SPAN); + let mut timestamps: Vec = Vec::with_capacity((height - start) as usize); for h in start..height { let hash = match plan { Some(p) => p.hash_at(h), None => store.get_block_hash_by_height(h), }; - if let Some(hash) = hash - && let Some(entry) = store.get_block_index(&hash) - { - timestamps.push(entry.header.time); - } + let entry = hash + .and_then(|hash| store.get_block_index(&hash)) + .ok_or(ConnectError::MedianTimeWindowGap { height, missing: h })?; + timestamps.push(entry.header.time); } + Ok(median_of(timestamps)) +} + +/// Core's `CBlockIndex::nMedianTimeSpan`. +pub(crate) const MEDIAN_TIME_SPAN: u32 = 11; + +/// The median of a median-time-past window, as Core's `GetMedianTimePast` +/// takes it: sorted, then the element at `len / 2`. 0 for the empty window, +/// which only height 0 has. +pub(crate) fn median_of(mut timestamps: Vec) -> u32 { if timestamps.is_empty() { return 0; } - timestamps.sort(); + timestamps.sort_unstable(); timestamps[timestamps.len() / 2] } @@ -730,8 +756,16 @@ fn connect_block_inner(params: &ConnectParams) -> Result Result InMemoryStore { + pub(super) fn test_store() -> InMemoryStore { let store = InMemoryStore::new(); let mut batch = StoreBatch::default(); batch.chain_tx_puts.push((BlockHash::all_zeros(), 0)); @@ -1540,7 +1578,7 @@ mod tests { } /// Create an InMemoryStore pre-loaded with a single coin. - fn make_test_store_with_coin(coin_height: u32, coinbase: bool) -> (InMemoryStore, OutPoint, Coin) { + pub(super) fn make_test_store_with_coin(coin_height: u32, coinbase: bool) -> (InMemoryStore, OutPoint, Coin) { let store = test_store(); let txid = bitcoin::Txid::from_raw_hash( bitcoin::hashes::sha256d::Hash::from_byte_array([0x42; 32]), @@ -1566,7 +1604,7 @@ mod tests { /// The coinbase encodes `height` via BIP 34 and pays exactly `block_subsidy(Network::Regtest, height)`. /// The spending tx consumes `outpoint` (expected to hold 50_000_000 sats) /// and produces a single output of the same value (zero fee). - fn make_block_spending( + pub(super) fn make_block_spending( outpoint: OutPoint, height: u32, tx_version: i32, @@ -1734,7 +1772,7 @@ mod tests { ); } - fn default_pos() -> FlatFilePos { + pub(super) fn default_pos() -> FlatFilePos { FlatFilePos { file_number: 0, data_pos: 0, diff --git a/node/src/chain/connect_consharden_tests.rs b/node/src/chain/connect_consharden_tests.rs new file mode 100644 index 00000000..e7e7e8ed --- /dev/null +++ b/node/src/chain/connect_consharden_tests.rs @@ -0,0 +1,205 @@ +//! The median time past is Core's `GetMedianTimePast`, over a whole window. +//! +//! Core walks a block and up to ten of its ancestors through `pprev` +//! (`chain.h`); fewer than eleven only when the walk reaches genesis. satd +//! reads the window through the height index (or a replay plan), where a row +//! can be missing. A median over the rows that are left gates BIP 113 and BIP +//! 68 on a value Core never computes, and an empty window gave 0, which meets +//! every time lock. A window with a missing block is now an error that says +//! nothing about the block being connected. + +use super::tests::{default_pos, make_block_spending, make_test_store_with_coin, test_store}; +use super::*; +use crate::storage::db::InMemoryStore; +use crate::validation::script::NoopVerifier; +use bitcoin::hashes::Hash; + +const BASE: u32 = 1_700_000_000; + +fn hash_for(height: u32) -> BlockHash { + let mut arr = [0u8; 32]; + arr[..4].copy_from_slice(&height.to_le_bytes()); + arr[4] = 0xC6; + BlockHash::from_byte_array(arr) +} + +fn entry_at(height: u32, time: u32) -> BlockIndexEntry { + let mut header = bitcoin::constants::genesis_block(Network::Regtest).header; + header.time = time; + BlockIndexEntry { + header, + height, + status: BlockStatus::Valid, + num_tx: 1, + file_number: 0, + data_pos: 0, + chainwork: [0u8; 32], + } +} + +/// Index a block at each height in `heights`, stamped `time_of(height)`, with +/// its height row. +fn index_blocks(store: &InMemoryStore, heights: impl IntoIterator, time_of: impl Fn(u32) -> u32) { + let mut batch = StoreBatch::default(); + for h in heights { + batch.block_index_puts.push((hash_for(h), entry_at(h, time_of(h)))); + batch.height_hash_puts.push((h, hash_for(h))); + } + store.write_batch(batch).unwrap(); +} + +/// Core's `GetMedianTimePast` for the block at `height - 1`, from the times +/// directly: up to eleven of them ending at that block, sorted, element n/2. +fn core_median(time_of: impl Fn(u32) -> u32, height: u32) -> u32 { + let mut times: Vec = (height.saturating_sub(11)..height).map(time_of).collect(); + times.sort_unstable(); + times[times.len() / 2] +} + +/// Out-of-order times, so the median is not simply the middle height's. +fn scrambled(h: u32) -> u32 { + BASE + (h * 7919) % 1000 +} + +#[test] +fn the_median_is_cores_over_a_full_window_and_near_genesis() { + let store = test_store(); + index_blocks(&store, 0..30, scrambled); + for height in 1..30 { + assert_eq!( + get_median_time_past(&store, height).unwrap(), + core_median(scrambled, height), + "height {height}" + ); + } + // Nothing precedes genesis. + assert_eq!(get_median_time_past(&store, 0).unwrap(), 0); +} + +#[test] +fn a_missing_height_row_is_an_error_not_a_shorter_median() { + let store = test_store(); + index_blocks(&store, (0..30).filter(|&h| h != 14), scrambled); + let err = get_median_time_past(&store, 20).unwrap_err(); + assert!( + matches!(err, ConnectError::MedianTimeWindowGap { height: 20, missing: 14 }), + "got {err:?}" + ); + assert!(!err.is_verdict_on_block(), "damage to the index is not a verdict on the block"); + // Windows that do not reach the hole are unaffected. + assert_eq!(get_median_time_past(&store, 14).unwrap(), core_median(scrambled, 14)); + assert_eq!(get_median_time_past(&store, 26).unwrap(), core_median(scrambled, 26)); +} + +#[test] +fn a_height_row_without_its_index_entry_is_an_error() { + let store = test_store(); + index_blocks(&store, 0..30, scrambled); + let mut batch = StoreBatch::default(); + batch.height_hash_puts.push((17, BlockHash::from_byte_array([0xEE; 32]))); + store.write_batch(batch).unwrap(); + assert!(matches!( + get_median_time_past(&store, 20), + Err(ConnectError::MedianTimeWindowGap { height: 20, missing: 17 }) + )); +} + +#[test] +fn a_replay_plan_that_does_not_reach_the_window_is_an_error() { + let store = test_store(); + index_blocks(&store, 0..30, scrambled); + let plan = crate::chain::replay_plan::ReplayPlan::from_hashes((0..15).map(hash_for).collect()); + assert_eq!( + median_time_past_with_plan(&store, Some(&plan), 15).unwrap(), + core_median(scrambled, 15) + ); + assert!(matches!( + median_time_past_with_plan(&store, Some(&plan), 20), + Err(ConnectError::MedianTimeWindowGap { height: 20, missing: 15 }) + )); +} + +/// Connect a block at `BLOCK_HEIGHT` spending a coin from `COIN_HEIGHT` with a +/// one-unit (512 s) BIP 68 time lock, against `store`. +fn connect_time_locked_spend(store: &InMemoryStore, outpoint: OutPoint, block_mtp: u32) -> Result { + const BLOCK_HEIGHT: u32 = 40; + let block = make_block_spending(outpoint, BLOCK_HEIGHT, 2, (1 << 22) | 1, 0); + let address_index = Default::default(); + let sp_index = Default::default(); + #[cfg(feature = "block-filter-index")] + let filter_index = Default::default(); + connect_block(&ConnectParams { + replay_plan: None, + store, + block: &block, + height: BLOCK_HEIGHT, + parent_chainwork: &[0u8; 32], + flat_pos: default_pos(), + script_verifier: &NoopVerifier, + median_time_past: block_mtp, + network: Network::Regtest, + pre_verified_txs: None, + num_threads: 1, + precomputed_txids: None, + address_index: &address_index, + sp_index: &sp_index, + #[cfg(feature = "block-filter-index")] + filter_index: &filter_index, + phase_tracker: None, + interrupt: None, + }) +} + +const COIN_HEIGHT: u32 = 30; + +/// Block times 100 s apart, so the coin's MTP (the window for height 30, +/// blocks 19..=29) is the time of block 24: `BASE + 2400`. +fn spaced(h: u32) -> u32 { + BASE + 100 * h +} + +#[test] +fn bip68_takes_the_coins_median_over_its_whole_window() { + let (store, outpoint, _) = make_test_store_with_coin(COIN_HEIGHT, false); + index_blocks(&store, 0..40, spaced); + let coin_mtp = core_median(spaced, COIN_HEIGHT); + assert_eq!(coin_mtp, BASE + 2400); + + // Exactly 512 s past the coin's MTP: the lock is met. + assert!(connect_time_locked_spend(&store, outpoint, coin_mtp + 512).is_ok()); + // One second short: not met, a verdict on the block. + let err = connect_time_locked_spend(&store, outpoint, coin_mtp + 511).err().expect("lock not met"); + assert!(matches!(err, ConnectError::SequenceLockNotMet), "got {err:?}"); + assert!(err.is_verdict_on_block()); +} + +/// With block 21's row gone, the median over the other ten is block 25's +/// time, 100 s later than Core's. A block that meets the lock by Core's +/// median would be refused with a verdict and marked invalid. +#[test] +fn bip68_with_a_hole_in_the_coins_window_is_not_a_verdict() { + let (store, outpoint, _) = make_test_store_with_coin(COIN_HEIGHT, false); + index_blocks(&store, (0..40).filter(|&h| h != 21), spaced); + let err = connect_time_locked_spend(&store, outpoint, core_median(spaced, COIN_HEIGHT) + 512) + .err() + .expect("the window has a hole"); + assert!( + matches!(err, ConnectError::MedianTimeWindowGap { height: COIN_HEIGHT, missing: 21 }), + "got {err:?}" + ); + assert!(!err.is_verdict_on_block()); +} + +/// With no rows at all in the coin's window, the median was 0 and every time +/// lock passed. +#[test] +fn an_empty_coin_window_no_longer_meets_every_time_lock() { + let (store, outpoint, _) = make_test_store_with_coin(COIN_HEIGHT, false); + index_blocks(&store, 0..19, spaced); + let err = connect_time_locked_spend(&store, outpoint, BASE).err().expect("the window is empty"); + assert!( + matches!(err, ConnectError::MedianTimeWindowGap { height: COIN_HEIGHT, missing: 19 }), + "got {err:?}" + ); + assert!(!err.is_verdict_on_block()); +} diff --git a/node/src/chain/prefetch.rs b/node/src/chain/prefetch.rs index af26145f..c878cc6e 100644 --- a/node/src/chain/prefetch.rs +++ b/node/src/chain/prefetch.rs @@ -131,8 +131,16 @@ pub fn prefetch_block( return None; } - // 5. Compute MTP (read-only store lookups) - let mtp = crate::chain::connect::median_time_past_with_plan(store, plan, height); + // 5. Compute MTP (read-only store lookups). A window with a missing block + // goes to the direct-read path too, which fails the connect with + // `MedianTimeWindowGap`. + let mtp = match crate::chain::connect::median_time_past_with_plan(store, plan, height) { + Ok(mtp) => mtp, + Err(e) => { + tracing::debug!(height, block = %hash, error = %e, "prefetch: deferring to the direct read"); + return None; + } + }; // 6. Context-free work: txids + check_transaction let mut txids = Vec::with_capacity(block.txdata.len()); diff --git a/node/src/chain/state.rs b/node/src/chain/state.rs index afe51ff8..4c130f87 100644 --- a/node/src/chain/state.rs +++ b/node/src/chain/state.rs @@ -3395,41 +3395,53 @@ impl ChainState { } } - /// Compute median time past (MTP) for a given height. - /// MTP is the median of the timestamps of the previous 11 blocks. + /// The median time past a block at `height` is validated against: the + /// median of the timestamps of the previous 11 blocks (Core's + /// `pindexPrev->GetMedianTimePast()`). For consensus. + /// + /// Served from the MTP cache when it holds the whole window, else from + /// the store through [`connect::get_median_time_past`], which refuses a + /// window with a missing block ([`connect::ConnectError::MedianTimeWindowGap`]) + /// rather than take the median of the blocks it found. + pub fn median_time_past_for_connect(&self, height: u32) -> Result { + match self.cached_mtp_window(height) { + Some(timestamps) => Ok(connect::median_of(timestamps)), + None => connect::get_median_time_past(&*self.store, height), + } + } + + /// [`median_time_past_for_connect`](Self::median_time_past_for_connect) + /// for the mempool, block templates and RPCs, which need a value even + /// from a damaged index: a window with a missing block gives the median + /// of the blocks found, and 0 when none are. Never use it for a block's + /// validity. pub fn get_median_time_past(&self, height: u32) -> u32 { - let start = height.saturating_sub(11); - let range_len = (height - start) as usize; - - // Try to satisfy entirely from cache - let cache = self.mtp_cache.lock(); - let mut timestamps: Vec = Vec::with_capacity(range_len); - for h in start..height { - if let Some((_, ts)) = cache.iter().find(|(ch, _)| *ch == h) { - timestamps.push(*ts); - } + if let Some(timestamps) = self.cached_mtp_window(height) { + return connect::median_of(timestamps); } - drop(cache); - - if timestamps.len() == range_len && !timestamps.is_empty() { - // Cache hit — all timestamps found - timestamps.sort(); - return timestamps[timestamps.len() / 2]; - } - - // Cache miss — fall back to store lookups - timestamps.clear(); + let start = height.saturating_sub(connect::MEDIAN_TIME_SPAN); + let mut timestamps: Vec = Vec::new(); for h in start..height { if let Some(hash) = self.store.get_block_hash_by_height(h) && let Some(entry) = self.store.get_block_index(&hash) { timestamps.push(entry.header.time); } } - if timestamps.is_empty() { - return 0; - } - timestamps.sort(); - timestamps[timestamps.len() / 2] + connect::median_of(timestamps) + } + + /// The MTP window for a block at `height` (the timestamps at heights + /// `height - 11 .. height`), if the MTP cache holds every one of them. + fn cached_mtp_window(&self, height: u32) -> Option> { + let start = height.saturating_sub(connect::MEDIAN_TIME_SPAN); + let cache = self.mtp_cache.lock(); + let timestamps: Vec = (start..height) + .map_while(|h| cache.iter().find(|(ch, _)| *ch == h).map(|(_, ts)| *ts)) + .collect(); + // The empty window (height 0) goes to the store path, as it always + // has; both give 0. + (!timestamps.is_empty() && timestamps.len() == (height - start) as usize) + .then_some(timestamps) } /// Authoritative active-chain lookup: the hash of the block at `height` on @@ -4393,7 +4405,7 @@ impl ChainState { // never read it, so for the bulk of IBD blocks "MTP comes from the // branch being connected" was simply not in force. Normally a cache // hit, since the previous block pushed its own entry. - let mtp = self.get_median_time_past(pre.height); + let mtp = self.median_time_past_for_connect(pre.height)?; let batch = connect::connect_block(&connect::ConnectParams { replay_plan: None, store: &*self.store, @@ -5187,7 +5199,7 @@ impl ChainState { let verifier: &dyn ScriptVerifier = if use_noop { &noop } else { &*self.script_verifier }; // Connect block - let mtp = self.get_median_time_past(entry.height); + let mtp = self.median_time_past_for_connect(entry.height)?; let batch = connect::connect_block(&connect::ConnectParams { replay_plan: None, store: &*self.store, @@ -5915,7 +5927,7 @@ impl ChainState { // locktimes, so that is a consensus input, and having the two replay // paths derive it from different sources made it depend on whether the // prefetcher happened to hit — which at replay startup it never does. - let mtp = connect::median_time_past_with_plan(&*self.store, Some(plan), height); + let mtp = connect::median_time_past_with_plan(&*self.store, Some(plan), height)?; let batch = connect::connect_block(&connect::ConnectParams { replay_plan: Some(plan), store: &*self.store, @@ -6578,7 +6590,7 @@ impl ChainState { let verifier: &dyn ScriptVerifier = if use_noop { &noop } else { &*self.script_verifier }; - let mtp = self.get_median_time_past(height); + let mtp = self.median_time_past_for_connect(height)?; let batch = match connect::connect_block(&connect::ConnectParams { replay_plan: None, store: &*self.store, @@ -7168,7 +7180,9 @@ impl ChainState { // building an address or filter index for a block that is not being // connected is pure cost. `flat_pos` is likewise only stamped into the // discarded batch. - let mtp = connect::get_median_time_past(store_ref, height); + // A window with a missing block is damage to this node's index, not a + // verdict on the block: an error, like the missing parent above. + let mtp = connect::get_median_time_past(store_ref, height)?; let no_address_index = crate::index::address::AddressIndexConfig::default(); let no_sp_index = crate::index::silent_payments::SpIndexConfig::default(); #[cfg(feature = "block-filter-index")] @@ -7632,7 +7646,7 @@ impl ChainState { let noop = NoopVerifier; let verifier: &dyn ScriptVerifier = if use_noop { &noop } else { &*self.script_verifier }; - let mtp = self.get_median_time_past(side_entry.height); + let mtp = self.median_time_past_for_connect(side_entry.height)?; let side_flat_pos = FlatFilePos { file_number: side_entry.file_number, data_pos: side_entry.data_pos, @@ -7795,9 +7809,11 @@ impl ChainState { // If the triggering block fails inside an in-progress reorg, roll // the chain back to the pre-reorg active chain before returning // the error — otherwise the failed candidate would leave the - // node permanently advanced onto a partial side-chain prefix. - let mtp = self.get_median_time_past(new_height); - let connect_attempt = connect::connect_block(&connect::ConnectParams { + // node permanently advanced onto a partial side-chain prefix. An MTP + // window this node cannot read fails the same way, as a connect + // failure that is not a verdict on the block. + let mtp = self.median_time_past_for_connect(new_height); + let connect_attempt = mtp.and_then(|mtp| connect::connect_block(&connect::ConnectParams { replay_plan: None, store: &*self.store, block, @@ -7816,7 +7832,7 @@ impl ChainState { filter_index: &self.filter_index, phase_tracker: None, interrupt: None, - }); + })); let batch = match connect_attempt { Ok(b) => b, Err(e) => { @@ -9500,7 +9516,7 @@ impl ChainState { let noop = NoopVerifier; let verifier: &dyn ScriptVerifier = if use_noop { &noop } else { &*self.script_verifier }; - let mtp = self.get_median_time_past(e.height); + let mtp = self.median_time_past_for_connect(e.height)?; let flat_pos = FlatFilePos { file_number: e.file_number, data_pos: e.data_pos, @@ -9939,6 +9955,10 @@ impl ChainState { #[path = "state_blockaccept_tests.rs"] mod blockaccept_tests; +#[cfg(test)] +#[path = "state_consharden_tests.rs"] +mod consharden_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 @@ -20104,7 +20124,7 @@ pub(crate) mod tests { // store lookups are what run. Warm entries here would mask that. cs.mtp_cache.lock().clear(); - let planned_mtp = connect::median_time_past_with_plan(&*cs.store, Some(&plan), 13); + let planned_mtp = connect::median_time_past_with_plan(&*cs.store, Some(&plan), 13).unwrap(); let indexed_mtp = cs.get_median_time_past(13); assert_eq!(planned_mtp, BASE + 700, "MTP of the branch being replayed"); assert_eq!(indexed_mtp, BASE + 650, "MTP the polluted rows produce"); diff --git a/node/src/chain/state_consharden_tests.rs b/node/src/chain/state_consharden_tests.rs new file mode 100644 index 00000000..02326c18 --- /dev/null +++ b/node/src/chain/state_consharden_tests.rs @@ -0,0 +1,99 @@ +//! The connect paths take a block's median time past whole, or not at all. +//! +//! `median_time_past_for_connect` is Core's `pindexPrev->GetMedianTimePast()` +//! (`chain.h`): the parent and up to ten of its ancestors. It answers from the +//! MTP cache when that holds the whole window and from the index otherwise, +//! and a window with a missing block is an error that is not a verdict on the +//! block, so it never marks one invalid. + +use super::tests::{build_test_block, make_chain_state}; +use super::*; +use crate::storage::StoreBatch; + +const BASE: u32 = 1_707_000_000; + +/// Connect `n` blocks on regtest genesis, 600 s apart. +fn connect_chain(cs: &ChainState, n: u32) -> Vec { + let mut parent = bitcoin::constants::genesis_block(Network::Regtest).block_hash(); + (1..=n) + .map(|h| { + let b = build_test_block(parent, h, BASE + 600 * h); + parent = cs.accept_block(&b).expect("connect fixture block").hash(); + b + }) + .collect() +} + +/// Core's `GetMedianTimePast` for the active block at `height - 1`, walking +/// parent links: its time and up to ten ancestors', sorted, element n/2. +fn core_median_time_past(cs: &ChainState, height: u32) -> u32 { + let mut hash = cs.active_chain_hash_at_height(height - 1).expect("active block"); + let mut times = Vec::new(); + for _ in 0..11 { + let entry = cs.get_block_index(&hash).expect("index entry"); + times.push(entry.header.time); + if entry.height == 0 { + break; + } + hash = entry.header.prev_blockhash; + } + times.sort_unstable(); + times[times.len() / 2] +} + +#[test] +fn the_connect_median_is_cores_from_the_cache_and_from_the_index() { + let (cs, _dir) = make_chain_state(); + connect_chain(&cs, 15); + for height in 1..=16 { + assert_eq!( + cs.median_time_past_for_connect(height).unwrap(), + core_median_time_past(&cs, height), + "height {height}, cache" + ); + } + cs.mtp_cache.lock().clear(); + for height in 1..=16 { + assert_eq!( + cs.median_time_past_for_connect(height).unwrap(), + core_median_time_past(&cs, height), + "height {height}, index" + ); + } +} + +/// Block 7's height row is lost. The next block's window (blocks 2..=12) +/// cannot be read whole, so it is not connected, and it is not marked: once +/// the row is back, the same block connects. +#[test] +fn a_hole_in_the_mtp_window_stops_the_connect_without_marking_the_block() { + let (cs, _dir) = make_chain_state(); + let blocks = connect_chain(&cs, 12); + let next = build_test_block(blocks[11].block_hash(), 13, BASE + 600 * 13); + + cs.store + .write_batch(StoreBatch { height_hash_removes: vec![7], ..Default::default() }) + .unwrap(); + cs.mtp_cache.lock().clear(); + + let err = cs.accept_block(&next).unwrap_err(); + assert!( + matches!( + err, + ChainError::Connect(connect::ConnectError::MedianTimeWindowGap { height: 13, missing: 7 }) + ), + "got {err:?}" + ); + assert_ne!( + cs.get_block_index(&next.block_hash()).map(|e| e.status), + Some(BlockStatus::Invalid), + "a window this node cannot read is not a verdict on the block" + ); + assert_eq!(cs.tip_height(), 12); + + cs.store + .write_batch(StoreBatch { height_hash_puts: vec![(7, blocks[6].block_hash())], ..Default::default() }) + .unwrap(); + cs.accept_block(&next).expect("the block connects once the window is whole"); + assert_eq!(cs.tip_hash(), next.block_hash()); +} From 1b055e9b4987a51f2f53955e2817f46bdf29dfce Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Thu, 8 Oct 2026 07:14:16 -0600 Subject: [PATCH 7/8] Count block work as Core's GetBitsProof does work_for_bits decoded nBits with target_from_compact, which ignores the sign bit and has no notion of overflow, and gave a zero target 2^256 - 1 work. Core's GetBitsProof (chain.cpp:121-134) gives no work to bits that SetCompact (arith_uint256.cpp:176-194) flags as negative or overflowing, or that decode to zero, and decodes exponents 33 and 34 differently from target_from_compact. work_for_bits now decodes as SetCompact does and returns zero work in those cases. For every target a header's proof of work can meet the value is unchanged, so stored chainwork stays valid; a test checks that over exponents 1 to 32. add_u256 wrapped on overflow. Chainwork is a sum of block work, and a sum that wrapped would rank the chain with the most work below every other, so it now saturates. Core's arith_uint256 wraps, but no reachable chain gets near 2^256. The expected values in the tests are Core's own output: GetBitsProof from chain.cpp, compiled with Core's arith_uint256.cpp, run over each nBits. Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/storage/blockindex.rs | 58 +++++++++++- .../storage/blockindex_consharden_tests.rs | 91 +++++++++++++++++++ 2 files changed, 145 insertions(+), 4 deletions(-) create mode 100644 node/src/storage/blockindex_consharden_tests.rs diff --git a/node/src/storage/blockindex.rs b/node/src/storage/blockindex.rs index 4f876b0f..f31ac6f9 100644 --- a/node/src/storage/blockindex.rs +++ b/node/src/storage/blockindex.rs @@ -27,8 +27,13 @@ pub struct BlockIndexEntry { /// Compute the work represented by a single block with the given target bits. /// Returns 2^256 / (target + 1) as a big-endian [u8; 32]. +/// +/// Core's `GetBitsProof` (`chain.cpp:121-134`): no work at all for bits that +/// encode a negative, overflowing or zero target. pub fn work_for_bits(bits: CompactTarget) -> [u8; 32] { - let target = target_from_compact(bits); + let Some(target) = target_with_work(bits) else { + return [0u8; 32]; + }; // work = (2^256 - target - 1) / (target + 1) + 1 // which equals floor(2^256 / (target + 1)) @@ -42,7 +47,42 @@ pub fn work_for_bits(bits: CompactTarget) -> [u8; 32] { div_2_256_by(&target_plus_one) } -/// Add two big-endian U256 values. +/// The target `bits` encodes, decoded as Core's `arith_uint256::SetCompact` +/// (`arith_uint256.cpp:176-194`) decodes it, or `None` where `GetBitsProof` +/// counts no work: a negative target, an overflowing one, or zero. +/// +/// For every exponent up to 32 with a positive mantissa (every target a +/// header can meet) this is [`target_from_compact`]'s value; they part only +/// on encodings no header's proof of work meets. +fn target_with_work(bits: CompactTarget) -> Option<[u8; 32]> { + let compact = bits.to_consensus(); + let size = compact >> 24; + let mut word = compact & 0x007f_ffff; + let mut target = [0u8; 32]; + if size <= 3 { + word >>= 8 * (3 - size); + target[28..].copy_from_slice(&word.to_be_bytes()); + } else { + // `word << 8 * (size - 3)`, keeping the low 256 bits: byte `i` of the + // big-endian word lands at big-endian position `31 - (size - i)`. + for (i, byte) in word.to_be_bytes().into_iter().enumerate() { + if let Some(pos) = (31 + i).checked_sub(size as usize) { + target[pos] = byte; + } + } + } + let negative = word != 0 && compact & 0x0080_0000 != 0; + let overflow = + word != 0 && (size > 34 || (word > 0xff && size > 33) || (word > 0xffff && size > 32)); + (!negative && !overflow && target != [0u8; 32]).then_some(target) +} + +/// Add two big-endian U256 values, saturating at 2^256 - 1. +/// +/// Chainwork is a sum of these, and a sum that wrapped would rank the chain +/// with the most work below every other. Core's `arith_uint256` wraps, but its +/// sums cannot get near 2^256: no block's proof of work meets a target small +/// enough to be worth that much. Saturating keeps the order if one ever did. pub fn add_u256(a: &[u8; 32], b: &[u8; 32]) -> [u8; 32] { let mut result = [0u8; 32]; let mut carry: u16 = 0; @@ -51,6 +91,9 @@ pub fn add_u256(a: &[u8; 32], b: &[u8; 32]) -> [u8; 32] { result[i] = sum as u8; carry = sum >> 8; } + if carry != 0 { + return [0xff; 32]; + } result } @@ -387,6 +430,10 @@ mod header_serde { } } +#[cfg(test)] +#[path = "blockindex_consharden_tests.rs"] +mod consharden_tests; + #[cfg(test)] mod tests { use super::*; @@ -462,8 +509,11 @@ mod tests { let mut b = [0u8; 32]; b[31] = 0x01; let result = add_u256(&a, &b); - // All-0xFF + 1 should wrap to all-0x00 (overflow wraps) - assert_eq!(result, [0u8; 32]); + // All-0xFF + 1 saturates: a chainwork sum never wraps below its parts. + assert_eq!(result, [0xFFu8; 32]); + let mut half = [0u8; 32]; + half[0] = 0x80; + assert_eq!(add_u256(&half, &half), [0xFFu8; 32]); } #[test] diff --git a/node/src/storage/blockindex_consharden_tests.rs b/node/src/storage/blockindex_consharden_tests.rs new file mode 100644 index 00000000..0f9cf829 --- /dev/null +++ b/node/src/storage/blockindex_consharden_tests.rs @@ -0,0 +1,91 @@ +//! A block's work is Core's `GetBitsProof`, including for bits no valid +//! header carries. +//! +//! Every expected value below is Core's: `GetBitsProof` from `chain.cpp`, +//! copied verbatim and compiled with Core's own `arith_uint256.cpp` at commit +//! 16613c9de9, run over each `bits`. + +use super::*; +use bitcoin::hex::DisplayHex; + +fn work_hex(bits: u32) -> String { + work_for_bits(CompactTarget::from_consensus(bits)).to_lower_hex_string() +} + +/// `(bits, GetBitsProof(bits))`, the work as 64 hex digits without leading +/// zeros. +const CORE_WORK: &[(u32, &str)] = &[ + // Zero targets: an exponent or a mantissa of zero, or a mantissa shifted + // out by a small exponent. + (0x0000_0000, "0"), + (0x0012_3456, "0"), + (0x0100_3456, "0"), + (0x0300_0000, "0"), + (0x0480_0000, "0"), + (0x1d00_0000, "0"), + (0x0180_0000, "0"), + (0x2100_0000, "0"), + (0x2300_0000, "0"), + // Negative: the sign bit with a nonzero mantissa. + (0x0492_3456, "0"), + (0x1d80_ffff, "0"), + (0x01fe_dcba, "0"), + // Overflowing: more than 256 bits. + (0x2200_0100, "0"), + (0x2200_ffff, "0"), + (0x2300_0001, "0"), + (0x2101_0000, "0"), + (0xff12_3456, "0"), + // Small exponents keep the mantissa's top bytes. + (0x0112_3456, "d79435e50d79435e50d79435e50d79435e50d79435e50d79435e50d79435e50"), + (0x0212_3456, "e0f7d0fc35d34ac057e0cda2850689332253d0537bf68d97f968bd609c6c4"), + (0x0312_3456, "e0fff97690309e2f96677e115e465ed2d49ebff2a34c636177dcdb14856"), + // Exponents 33 and 34 that do not overflow. + (0x2100_00ff, "101"), + (0x2100_0100, "ff"), + (0x2100_ffff, "1"), + (0x2200_00ff, "1"), + // Real targets: mainnet genesis, a mainnet retarget, regtest. + (0x1d00_ffff, "100010001"), + (0x1703_4219, "4e9235f043634662e0cb"), + (0x207f_ffff, "2"), +]; + +#[test] +fn block_work_is_cores_get_bits_proof() { + for (bits, core) in CORE_WORK { + assert_eq!(work_hex(*bits), format!("{core:0>64}"), "bits {bits:08x}"); + } +} + +/// Bits that carry no work add none: a zero target used to count as +/// 2^256 - 1, more than any chain. +#[test] +fn bits_without_work_add_nothing_to_chainwork() { + let parent = work_for_bits(CompactTarget::from_consensus(0x1d00_ffff)); + for bits in [0x0000_0000u32, 0x0492_3456, 0x2300_0001] { + assert_eq!(add_u256(&parent, &work_for_bits(CompactTarget::from_consensus(bits))), parent); + } +} + +/// Every target a header's proof of work can meet decodes as before, so no +/// chainwork already in an index changes. +#[test] +fn work_is_unchanged_for_every_target_a_header_can_meet() { + let mantissas = [ + 0x00_0001, 0x00_00ff, 0x00_0100, 0x00_ffff, 0x01_0000, 0x12_3456, 0x7f_ffff, 0x00_8000, + 0x7f_0000, 0x40_0000, 0x00_7fff, 0x03_77ae, + ]; + for exponent in 1u32..=32 { + for mantissa in mantissas { + let bits = CompactTarget::from_consensus((exponent << 24) | mantissa); + let old_target = target_from_compact(bits); + assert_eq!( + target_with_work(bits), + (old_target != [0u8; 32]).then_some(old_target), + "bits {:08x}", + bits.to_consensus() + ); + } + } +} From 10c5cecfb8df98c8feab5c225ffdbac54c93f066 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Thu, 8 Oct 2026 07:31:23 -0600 Subject: [PATCH 8/8] Manual: an unreadable coin stops the offline audit too satd-chainstate-audit looks coins up through the same store, so a coin the database cannot read now stops it with the outpoint and the RocksDB error instead of listing the coin as missing. Say so beside its exit statuses, and say where the stop applies. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/manual/src/ibd.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/docs/manual/src/ibd.md b/docs/manual/src/ibd.md index b628ce84..a98cf5ee 100644 --- a/docs/manual/src/ibd.md +++ b/docs/manual/src/ibd.md @@ -385,10 +385,12 @@ logged at `INFO`, not treated as damage. 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. 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. +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 @@ -426,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