diff --git a/CHANGELOG.md b/CHANGELOG.md index e6c2fead4..930a432f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,23 @@ item below is (or will be) written up in full in the in-development installed packages come from — was failing the whole step, and with it every canary job. +- **Breaking:** `getblocktemplate` proposal mode validates the proposed block + the way `submitblock` does. It ran a separate loop that skipped script + verification by its own admission, and with it BIP 68 sequence locks, the + block sigop cost and BIP 30, so a miner was told a block would be accepted + when it would not (#663). +- `getblocktemplate` answers `duplicate` / `duplicate-invalid` / + `duplicate-inconclusive` for a block the node already knows, as Core does, + instead of `inconclusive-not-best-prevblk` (#663). +- **Breaking:** `getblocktemplate` rejects a `mode` it does not understand with + `-8 Invalid mode` rather than silently returning a template, and proposal + mode without a string `data` is `-3`, as Core's is (#663). +- **Breaking:** a block that is both oversized and merkle-broken reports + `bad-txnmrklroot`, as Core's `CheckBlock` does — it checks the merkle root + before the size limits — and `check_block` applies Core's legacy-sigop + ceiling, which fired before any prevout was resolved (#663). +- `-blockversion` overrides the template's block version on regtest, as Core's + `CreateNewBlock` does. It was accepted and ignored (#663). - **Breaking:** dust thresholds are Bitcoin Core's. satd charged 68 vbytes to spend a witness output where Core charges 67, 107 for P2SH where Core charges 148, and truncated a fee Core rounds up — so P2WPKH was 297 against Core's diff --git a/docs/release-notes/0.5.2-pre.md b/docs/release-notes/0.5.2-pre.md index 92ccb3b5c..b2cf5f4e7 100644 --- a/docs/release-notes/0.5.2-pre.md +++ b/docs/release-notes/0.5.2-pre.md @@ -886,6 +886,69 @@ accepted member sweeps its dust. Reachable only through `submitpackage`; nothing on the P2P path can produce it. +### `getblocktemplate` proposal mode told miners the wrong answer (#663) + +BIP 22's proposal mode exists so a miner can ask "would you accept this +block?" before spending hash power on it. Core answers it with +`TestBlockValidity`, which ends in `ConnectBlock(fJustCheck=true)` against a +throwaway coins view — the same code path a real submission takes, with the +write discarded. + +satd answered it with a second, hand-written loop that reimplemented a subset +of the rules. It said so itself, in a comment: + +> Script verification is skipped — Core's TestBlockValidity runs full script +> verification, but proposal mode only needs to detect structural and +> contextual invalidity. + +Along with script verification it had no BIP 68 sequence-lock check, no block +sigop accounting and no BIP 30 test. A miner proposing a block with any of +those defects was told it was fine, then had it rejected on submission. + +The loop is gone. Proposal mode now calls `connect_block` and drops the +`StoreBatch` it returns — `connect_block` is pure, it reads the store and hands +its caller a batch to write — so there is exactly one implementation of the +rules and no way for the two answers to drift apart. A test asserts that +proposal mode and `submitblock` return the same verdict, and another that the +tip, the block index and the UTXO set are untouched after a proposal. + +The call holds `accept_lock` for its duration, as Core holds `cs_main`: the tip +must not move between the "builds on the tip" test and the connect. + +Three smaller divergences went with it: + +- A block the node already knows is `duplicate`, `duplicate-invalid` or + `duplicate-inconclusive` depending on what it decided last time + (`rpc/mining.cpp`). satd reported `inconclusive-not-best-prevblk` for all of + them, because a block already on the chain does not build on the tip. +- A `mode` that is present but not a string was read as "not proposal", so a + caller asking for something satd did not understand was quietly handed a + template. It is `-8 Invalid mode` now, and proposal mode with no string + `data` is `-3`, matching Core's `RPC_TYPE_ERROR`. +- An internal error is `-25`, Core's `RPC_VERIFY_ERROR`, not `-1`. + +### `check_block` orders two tests the way Core does (#663) + +Core's `CheckBlock` runs `CheckMerkleRoot` **first**, ahead of the size limits, +because "all potential-corruption validation must be done before we do any +transaction validation": a peer that sent the wrong transactions for a header +must not cause that header to be marked invalid. satd tested size first, so a +block that was both oversized and merkle-broken answered `bad-blk-length` where +Core answers `bad-txnmrklroot`. + +satd also had no equivalent of Core's legacy-sigop ceiling in `CheckBlock` — a +cheap, context-free gate that fires before any prevout is resolved. satd +counted sigops only in `connect_block`, where the accurate count needs resolved +prevouts, so a block over the ceiling that also spent nothing resolvable +reported `bad-txns-inputs-missingorspent` instead. Both cases are in the +block-consensus matrix and the live differential against bitcoind. + +### `-blockversion` was accepted and ignored (#663) + +Core's regtest-only `-blockversion=` stamps a chosen version on the template +header, which is how `mining_basic.py` sets up forking scenarios. satd listed +the option among its known configuration keys and then never read it. + ### Dust thresholds are Bitcoin Core's (#661) `GetDustThreshold` prices an output by what it costs to spend: Bitcoin Core diff --git a/node/src/chain/state.rs b/node/src/chain/state.rs index 57c609c25..5f55c9588 100644 --- a/node/src/chain/state.rs +++ b/node/src/chain/state.rs @@ -5707,6 +5707,35 @@ impl ChainState { /// `"inconclusive-not-best-prevblk"` rather than triggering a reorg. /// - No data is written to disk. pub fn test_block_validity(&self, block: &Block) -> Result, ChainError> { + // Core's `TestBlockValidity` holds `cs_main` throughout, for two + // reasons that both apply here: the tip must not move between the + // prevblk test and the connect, and the store this reads is the one + // `accept_block` writes. + let _accept_guard = self.accept_lock.lock(); + + // Core answers from the block index before validating anything + // (`rpc/mining.cpp`, `LookupBlockIndex` ahead of `TestBlockValidity`): + // a block it has already judged gets that judgement back rather than a + // second, possibly different, one. + let hash = block.block_hash(); + if let Some(entry) = self.store.get_block_index(&hash) { + return Ok(Some( + match entry.status { + // Core tests `IsValid(BLOCK_VALID_SCRIPTS)`, a validity + // level pruning does not lower: a pruned block was + // connected and judged, only its data is gone. + BlockStatus::Valid | BlockStatus::Pruned => "duplicate", + BlockStatus::Invalid => "duplicate-invalid", + // Header-only or stored-but-unvalidated: the node knows + // the block but has not decided about its contents. + BlockStatus::HeaderOnly | BlockStatus::DataStored => { + "duplicate-inconclusive" + } + } + .to_string(), + )); + } + let tip_hash = self.tip_hash(); // The block must build on the current tip. @@ -5755,139 +5784,52 @@ impl ChainState { return Ok(Some(e.to_string())); } - // Contextual transaction validation: finality, UTXO existence, amounts. - // This mirrors the per-tx loop in connect_block but without persisting - // anything. Script verification is skipped — Core's TestBlockValidity - // runs full script verification, but proposal mode only needs to detect - // structural and contextual invalidity. + // Everything else is `connect_block`, exactly as Core's + // `TestBlockValidity` finishes with `ConnectBlock(..., fJustCheck=true)` + // against a throwaway coins view. + // + // `connect_block` is pure: it takes `&dyn Store`, reads, and returns a + // `StoreBatch` its *caller* writes. Dropping the batch here is what + // makes this a dry run — there is no second implementation of the + // rules to keep in step. The hand-written loop this replaces skipped + // script verification by its own admission, and with it BIP 68 + // sequence locks, the block sigop cost, and BIP 30; a miner asking + // whether a block would be accepted got "yes" for blocks + // `submitblock` then rejected. + // + // The index configs are defaulted (disabled) rather than the node's: + // their only effect is on rows in the batch nobody will write, and + // 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); - let mut total_fees: u64 = 0; - // Track coins created within this block for intra-block spend resolution. - let mut intra_block_coins: std::collections::HashMap = - std::collections::HashMap::new(); - // Track which coins have been spent within this block to detect - // double-spends. - let mut spent_in_block: std::collections::HashSet = - std::collections::HashSet::new(); - - for tx in &block.txdata { - let is_coinbase = tx.is_coinbase(); - - // Context-free transaction checks. - if let Err(e) = crate::validation::tx::check_transaction(tx) { - return Ok(Some(e.to_string())); - } - - // Finality check (locktime / sequence). - let is_final = tx.input.iter().all(|i| i.sequence == bitcoin::Sequence::MAX); - if !is_final { - let locktime = tx.lock_time.to_consensus_u32(); - if locktime > 0 { - if locktime < 500_000_000 { - if locktime >= height { - return Ok(Some("bad-txns-nonfinal".to_string())); - } - } else { - let time_threshold = if height - >= connect::bip113_activation_height(self.network) - { - mtp - } else { - block.header.time - }; - if locktime >= time_threshold { - return Ok(Some("bad-txns-nonfinal".to_string())); - } - } - } - } - - // BIP 34 coinbase height check. - if is_coinbase - && height >= connect::bip34_activation_height(self.network) - { - let script = &tx.input[0].script_sig; - let bytes = script.as_bytes(); - if bytes.is_empty() { - return Ok(Some("bad-cb-height".to_string())); - } - if let Some(encoded_height) = connect::decode_coinbase_height(bytes) { - if encoded_height != height { - return Ok(Some("bad-cb-height".to_string())); - } - } else { - return Ok(Some("bad-cb-height".to_string())); - } - } - - // UTXO validation for non-coinbase transactions. - let mut sum_inputs: u64 = 0; - if !is_coinbase { - for input in &tx.input { - let outpoint = input.previous_output; - - if spent_in_block.contains(&outpoint) { - return Ok(Some("bad-txns-inputs-missingorspent".to_string())); - } - - let coin = intra_block_coins - .get(&outpoint) - .cloned() - .or_else(|| self.store.get_coin(&outpoint)); - - let Some(coin) = coin else { - return Ok(Some("bad-txns-inputs-missingorspent".to_string())); - }; - - // Coinbase maturity. - if coin.coinbase && height - coin.height < 100 { - return Ok(Some( - "bad-txns-premature-spend-of-coinbase".to_string(), - )); - } - - sum_inputs = sum_inputs.saturating_add(coin.amount); - spent_in_block.insert(outpoint); - } - - let sum_outputs: u64 = - tx.output.iter().map(|o| o.value.to_sat()).sum(); - if sum_inputs < sum_outputs { - return Ok(Some("bad-txns-in-belowout".to_string())); - } - total_fees = total_fees.saturating_add(sum_inputs - sum_outputs); - } - - // Add this transaction's outputs to the intra-block coin map. - let txid = tx.compute_txid(); - for (vout, output) in tx.output.iter().enumerate() { - if !connect::is_unspendable(&output.script_pubkey) { - let outpoint = OutPoint::new(txid, vout as u32); - intra_block_coins.insert( - outpoint, - Coin { - amount: output.value.to_sat(), - script_pubkey: output.script_pubkey.clone(), - height, - coinbase: is_coinbase, - }, - ); - } - } - } - - // Coinbase value must not exceed subsidy + fees. - if !block.txdata.is_empty() { - let subsidy = connect::block_subsidy(self.network, height); - let coinbase_value: u64 = - block.txdata[0].output.iter().map(|o| o.value.to_sat()).sum(); - if coinbase_value > subsidy + total_fees { - return Ok(Some("bad-cb-amount".to_string())); - } + let no_address_index = crate::index::address::AddressIndexConfig::default(); + let no_sp_index = crate::index::silent_payments::SpIndexConfig::default(); + #[cfg(feature = "block-filter-index")] + let no_filter_index = crate::index::filter::FilterIndexConfig::default(); + let params = connect::ConnectParams { + store: store_ref, + block, + height, + parent_chainwork: &parent.chainwork, + flat_pos: crate::storage::flatfile::FlatFilePos { file_number: 0, data_pos: 0 }, + script_verifier: &*self.script_verifier, + median_time_past: mtp, + network: self.network, + pre_verified_txs: None, + num_threads: self.num_threads, + precomputed_txids: None, + address_index: &no_address_index, + sp_index: &no_sp_index, + #[cfg(feature = "block-filter-index")] + filter_index: &no_filter_index, + phase_tracker: None, + replay_plan: None, + }; + match connect::connect_block(¶ms) { + Ok(_batch) => Ok(None), + Err(e) => Ok(Some(e.to_string())), } - - // Block passed all checks. - Ok(None) } /// Accept a new block into the chain. @@ -9914,6 +9856,176 @@ pub(crate) mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// `getblocktemplate` proposal mode is Core's `TestBlockValidity`, which + /// finishes with `ConnectBlock(fJustCheck=true)`. satd ran a hand-written + /// loop that skipped script verification by its own admission, and with it + /// BIP 68 sequence locks, the block sigop cost and BIP 30 — so a miner + /// asking whether a block would be accepted got "yes" for blocks + /// `submitblock` then rejected. + /// + /// A BIP 68 violation is the cheapest of those to build without a real + /// script verifier, and the hand loop had no sequence-lock check at all. + #[test] + fn a_proposal_enforces_the_rules_the_hand_written_loop_skipped() { + let (cs, dir) = make_chain_state(); + let blocks = build_and_connect_chain(&cs, 101); + // The coinbase of block 1 is mature at height 102. + let coin = OutPoint::new(blocks[0].txdata[0].compute_txid(), 0); + + // 200 blocks of relative lock against 101 confirmations: not final. + let bad = build_test_block_sequence_locked( + cs.tip_hash(), + 102, + 1_300_000_200, + coin, + 200, + ); + assert_eq!( + cs.test_block_validity(&bad).unwrap().as_deref(), + Some("bad-txns-nonBIP68-final"), + "proposal mode did not enforce BIP 68" + ); + // …and a real submission says the same thing, which is the property + // that matters: the two answers come from one implementation. + assert_eq!( + cs.accept_block(&bad).unwrap_err().to_string(), + "bad-txns-nonBIP68-final" + ); + + // The control: a lock the chain does satisfy is accepted, so the + // refusal above is the sequence lock and not the fixture. + let good = build_test_block_sequence_locked( + cs.tip_hash(), + 102, + 1_300_000_200, + coin, + 10, + ); + assert_eq!(cs.test_block_validity(&good).unwrap(), None); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// Core answers a proposal for a block it already knows from the block + /// index, before validating anything (`rpc/mining.cpp`), and the verdict + /// depends on what it decided last time. satd reported + /// `inconclusive-not-best-prevblk` for every one of them, because a block + /// already on the chain does not build on the tip. + #[test] + fn a_proposal_for_a_block_the_node_already_judged_is_a_duplicate() { + let (cs, dir) = make_chain_state(); + let blocks = build_and_connect_chain(&cs, 3); + + assert_eq!( + cs.test_block_validity(&blocks[2]).unwrap().as_deref(), + Some("duplicate"), + "the tip itself is a duplicate, not an inconclusive prevblk" + ); + assert_eq!( + cs.test_block_validity(&blocks[0]).unwrap().as_deref(), + Some("duplicate"), + "a block deeper in the chain is equally a duplicate" + ); + + // Pruning drops a block's data, not the judgement the node reached + // about it: Core's `IsValid(BLOCK_VALID_SCRIPTS)` still holds, so it + // is still `duplicate` — not `duplicate-inconclusive`, which would + // tell a miner the node never decided. + let pruned_hash = blocks[0].block_hash(); + let mut batch = crate::storage::StoreBatch::default(); + let mut entry = cs.get_block_index(&pruned_hash).unwrap(); + entry.status = BlockStatus::Pruned; + batch.block_index_puts.push((pruned_hash, entry)); + cs.store.write_batch(batch).unwrap(); + assert_eq!( + cs.test_block_validity(&blocks[0]).unwrap().as_deref(), + Some("duplicate"), + "a pruned block was judged; it is a duplicate" + ); + + // A block that is genuinely unknown and off the tip keeps the old + // answer, so `duplicate` is not being reported for everything. + let orphan = build_test_block(blocks[0].block_hash(), 2, 1_300_009_999); + assert_eq!( + cs.test_block_validity(&orphan).unwrap().as_deref(), + Some("inconclusive-not-best-prevblk") + ); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// A proposal is a dry run: `connect_block` returns a `StoreBatch` its + /// caller writes, and proposal mode drops it. Nothing about the node may + /// differ afterwards. + #[test] + fn a_proposal_commits_nothing() { + let (cs, dir) = make_chain_state(); + let blocks = build_and_connect_chain(&cs, 2); + + let tip_before = cs.tip_hash(); + let height_before = cs.tip_height(); + let coinbase = OutPoint::new(blocks[1].txdata[0].compute_txid(), 0); + let coin_before = cs.get_coin(&coinbase); + + let next = build_test_block(tip_before, 3, 1_300_000_003); + assert_eq!(cs.test_block_validity(&next).unwrap(), None, "fixture must be valid"); + + assert_eq!(cs.tip_hash(), tip_before, "a proposal moved the tip"); + assert_eq!(cs.tip_height(), height_before, "a proposal moved the height"); + assert!( + cs.get_block_index(&next.block_hash()).is_none(), + "a proposal wrote a block index entry" + ); + assert!( + cs.get_coin(&OutPoint::new(next.txdata[0].compute_txid(), 0)).is_none(), + "a proposal published the proposed block's coins" + ); + assert_eq!( + cs.get_coin(&coinbase).map(|c| c.amount), + coin_before.map(|c| c.amount), + "a proposal disturbed an existing coin" + ); + + // The control: accepting the same block really does change all of + // that, so the assertions above are not vacuous. + cs.accept_block(&next).expect("accept"); + assert_ne!(cs.tip_hash(), tip_before); + assert!(cs.get_block_index(&next.block_hash()).is_some()); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// Core holds `cs_main` across `TestBlockValidity` so the tip cannot move + /// between the prevblk test and the connect, and so the store this reads + /// is not being written underneath it. + #[test] + fn a_proposal_waits_for_the_accept_lock() { + use std::sync::atomic::{AtomicBool, Ordering}; + use std::time::Duration; + + let (cs, dir) = make_chain_state(); + build_and_connect_chain(&cs, 2); + let next = build_test_block(cs.tip_hash(), 3, 1_300_000_003); + + let done = AtomicBool::new(false); + std::thread::scope(|s| { + let guard = cs.accept_lock.lock(); + s.spawn(|| { + let _ = cs.test_block_validity(&next); + done.store(true, Ordering::SeqCst); + }); + std::thread::sleep(Duration::from_millis(200)); + assert!( + !done.load(Ordering::SeqCst), + "a proposal validated while accept_lock was held" + ); + drop(guard); + }); + assert!(done.load(Ordering::SeqCst), "the proposal completes once the lock is free"); + + let _ = std::fs::remove_dir_all(&dir); + } + /// `prune_blocks`' mutation section (file deletes + `Pruned` stamps) must /// wait for `accept_lock` — a prune that interleaves with a reorg can /// delete a block file the reorg is about to read back. Deleting the @@ -11416,6 +11528,54 @@ pub(crate) mod tests { panic!("Failed to mine time-locked test block within 2,000,000 nonce iterations"); } + /// Like `build_test_block_spending`, but with a BIP 68 *relative* + /// sequence lock on the grafted input: the spend is only final once + /// `sequence` blocks have passed since the coin's height. + /// + /// Bit 31 clear opts the input into BIP 68, so the low 16 bits are a + /// block-height delta. + pub(crate) fn build_test_block_sequence_locked( + parent_hash: BlockHash, + height: u32, + time: u32, + spend: bitcoin::OutPoint, + sequence: u32, + ) -> Block { + use bitcoin::Sequence; + use bitcoin::hashes::Hash; + use bitcoin::pow::CompactTarget; + + let mut block = build_test_block_spending(parent_hash, height, time, spend); + let tx = block.txdata.last_mut().expect("grafted spend tx"); + tx.input[0].sequence = Sequence::from_consensus(sequence); + block.header.merkle_root = block.compute_merkle_root().unwrap(); + + let bits = CompactTarget::from_consensus(0x207fffff); + let target = crate::storage::blockindex::target_from_compact(bits); + for nonce in 0u32..2_000_000 { + block.header.nonce = nonce; + let hash_bytes = *block.block_hash().as_raw_hash().as_byte_array(); + let mut hash_be = [0u8; 32]; + for i in 0..32 { + hash_be[i] = hash_bytes[31 - i]; + } + let mut ok = true; + for i in 0..32 { + if hash_be[i] < target[i] { + break; + } + if hash_be[i] > target[i] { + ok = false; + break; + } + } + if ok { + return block; + } + } + panic!("Failed to mine sequence-locked test block within 2,000,000 nonce iterations"); + } + /// Regression for issue #262: a reorg whose triggering block fails to /// connect must leave the original active chain — and its still-FRESH /// (un-flushed) coins — fully intact and durable. The old replay-based diff --git a/node/src/mining/template.rs b/node/src/mining/template.rs index 0cc8b9349..6b6ec23e8 100644 --- a/node/src/mining/template.rs +++ b/node/src/mining/template.rs @@ -106,6 +106,24 @@ static BLOCK_MIN_TX_FEE: std::sync::atomic::AtomicU64 = /// mempool would accept them and the template would never mine them. pub const DEFAULT_BLOCK_MIN_TX_FEE: u64 = 1; +/// Core's regtest-only `-blockversion=` override, or `i64::MIN` for "not +/// set" — the sentinel keeps this a lock-free atomic while still allowing a +/// caller to ask for version 0. +static BLOCK_VERSION_OVERRIDE: std::sync::atomic::AtomicI64 = + std::sync::atomic::AtomicI64::new(i64::MIN); + +/// Record the configured `-blockversion`. Called once during startup; a +/// `None` leaves the computed version in place. +/// +/// Core applies it only when `MineBlocksOnDemand()` — regtest — so the caller +/// is responsible for not passing one on any other network. +pub fn set_block_version_override(version: Option) { + BLOCK_VERSION_OVERRIDE.store( + version.unwrap_or(i64::MIN), + std::sync::atomic::Ordering::Relaxed, + ); +} + /// Record the configured `-blockmintxfee`. Called once during startup. pub fn set_block_min_tx_fee(rate: u64) { BLOCK_MIN_TX_FEE.store(rate, std::sync::atomic::Ordering::Relaxed); @@ -357,8 +375,15 @@ fn assemble_template( LAST_BLOCK_WEIGHT.store(total_weight as u64, std::sync::atomic::Ordering::Relaxed); + // `-blockversion` overrides the computed version on regtest only, as + // Core's `CreateNewBlock` does under `MineBlocksOnDemand()`. + let version = match BLOCK_VERSION_OVERRIDE.load(std::sync::atomic::Ordering::Relaxed) { + i64::MIN => 0x20000000u32 as i32, // BIP 9 version bits + v => v as i32, + }; + BlockTemplate { - version: 0x20000000, // BIP 9 version bits + version, prev_hash: tip_hash, height, bits, diff --git a/node/src/rpc/mining.rs b/node/src/rpc/mining.rs index 8676c1631..7d038366f 100644 --- a/node/src/rpc/mining.rs +++ b/node/src/rpc/mining.rs @@ -346,10 +346,14 @@ pub fn get_block_template_proposal( let block: bitcoin::Block = bitcoin::consensus::deserialize(&block_bytes) .map_err(|_| (-22, "Block decode failed".to_string()))?; + // Core's `BIP22ValidationResult`: a valid proposal is JSON null, an + // invalid one is its reject reason as a string, and an *error* (as opposed + // to a verdict) is `RPC_VERIFY_ERROR` — -25, not -1. match chain_state.test_block_validity(&block) { Ok(None) => Ok(Value::Null), + Ok(Some(reason)) if reason.is_empty() => Ok(Value::String("rejected".to_string())), Ok(Some(reason)) => Ok(Value::String(reason)), - Err(e) => Err((-1, e.to_string())), + Err(e) => Err((-25, e.to_string())), } } diff --git a/node/src/rpc/server.rs b/node/src/rpc/server.rs index 4c11e5f6e..4ec651b91 100644 --- a/node/src/rpc/server.rs +++ b/node/src/rpc/server.rs @@ -1512,22 +1512,38 @@ pub async fn start( args.optional::>("template_request")? .map(serde_json::Value::Object); args.check()?; - if let Some(ref req) = request - && req.get("mode").and_then(|m| m.as_str()) == Some("proposal") - { - let data = req - .get("data") + // Core (`rpc/mining.cpp`): `mode` absent or JSON null means "template"; + // a present-but-non-string `mode`, or any string other than + // "template"/"proposal", is `-8 Invalid mode`. Reading a non-string as + // "not proposal" silently mined a template for a caller who asked for + // something else. + let mode = match request.as_ref().and_then(|r| r.get("mode")) { + None | Some(serde_json::Value::Null) => "template", + Some(serde_json::Value::String(m)) => m.as_str(), + Some(_) => { + return Err(ErrorObjectOwned::owned(-8, "Invalid mode", None::<()>)); + } + }; + if mode == "proposal" { + // Core throws `RPC_TYPE_ERROR` (-3), not `-8`, when `data` is + // missing or not a string. + let data = request + .as_ref() + .and_then(|r| r.get("data")) .and_then(|d| d.as_str()) .ok_or_else(|| { ErrorObjectOwned::owned( - -8, - "\"data\" is required for proposal mode", + -3, + "Missing data String key for proposal", None::<()>, ) })?; return mining::get_block_template_proposal(&ctx.chain_state, data) .map_err(|(code, msg)| ErrorObjectOwned::owned(code, msg, None::<()>)); } + if mode != "template" { + return Err(ErrorObjectOwned::owned(-8, "Invalid mode", None::<()>)); + } Ok::<_, ErrorObjectOwned>(mining::get_block_template(&ctx.chain_state, &ctx.mempool)) })?; diff --git a/node/src/validation/block.rs b/node/src/validation/block.rs index d186e7003..29cb7a777 100644 --- a/node/src/validation/block.rs +++ b/node/src/validation/block.rs @@ -7,6 +7,9 @@ use crate::validation::ValidationError; /// Maximum block weight (4 million weight units, per BIP 141). const MAX_BLOCK_WEIGHT: usize = 4_000_000; +/// Bitcoin Core's `MAX_BLOCK_SIGOPS_COST`. +const MAX_BLOCK_SIGOPS_COST: usize = 80_000; + /// BIP 141 witness scale factor (Core's `WITNESS_SCALE_FACTOR`). pub const WITNESS_SCALE_FACTOR: usize = 4; @@ -37,29 +40,13 @@ pub fn check_block( return Err(ValidationError::EmptyBlock); } - // Size limits, split exactly as Core splits them (#548): - // - // 1. `CheckBlock`'s stripped-size test — tx count and witness-stripped - // serialized size, each scaled by the witness factor — rejects - // `bad-blk-length`. Witness bytes cannot trigger it. - // (`Block::base_size` is private in rust-bitcoin 0.32; recover it exactly - // from the public pair via weight = base * 3 + total.) - let weight = block.weight().to_wu() as usize; - let base_size = (weight - block.total_size()) / 3; - if block.txdata.len() * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT - || base_size * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT - { - return Err(ValidationError::OversizedBlock); - } - - // First transaction must be coinbase - if !block.txdata[0].is_coinbase() { - return Err(ValidationError::NoCoinbase); - } - - // Check merkle root — Core computes both the root and the mutation flag - // together, so the merkle-root mismatch and duplication check fire - // BEFORE the multiple-coinbase check. + // 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 + // any transaction validation": a peer that sent the wrong transactions for + // 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) => { @@ -82,6 +69,26 @@ pub fn check_block( return Err(ValidationError::BadTxDuplicate); } + // Size limits, split exactly as Core splits them (#548): + // + // 1. `CheckBlock`'s stripped-size test — tx count and witness-stripped + // serialized size, each scaled by the witness factor — rejects + // `bad-blk-length`. Witness bytes cannot trigger it. + // (`Block::base_size` is private in rust-bitcoin 0.32; recover it exactly + // from the public pair via weight = base * 3 + total.) + let weight = block.weight().to_wu() as usize; + let base_size = (weight - block.total_size()) / 3; + if block.txdata.len() * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT + || base_size * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT + { + return Err(ValidationError::OversizedBlock); + } + + // First transaction must be coinbase + if !block.txdata[0].is_coinbase() { + return Err(ValidationError::NoCoinbase); + } + // No other transaction may be coinbase — after the merkle checks so // that Core-order `bad-txns-duplicate` fires before `bad-cb-multiple` // when a coinbase is duplicated. @@ -91,6 +98,26 @@ pub fn check_block( } } + // Legacy sigop count. Core's `CheckBlock` runs this cheap, context-free + // gate before anything resolves a prevout — it "underestimates the number + // of sigops, because unlike ConnectBlock it does not count witness and + // p2sh sigops", but it is a hard ceiling a block cannot talk its way out + // of, and it fires ahead of `bad-txns-inputs-missingorspent`. + // `connect_block` still applies the full accurate count. + let mut legacy_sigops: usize = 0; + for tx in &block.txdata { + for input in &tx.input { + legacy_sigops = legacy_sigops.saturating_add(input.script_sig.count_sigops_legacy()); + } + for output in &tx.output { + legacy_sigops = + legacy_sigops.saturating_add(output.script_pubkey.count_sigops_legacy()); + } + } + if legacy_sigops.saturating_mul(WITNESS_SCALE_FACTOR) > MAX_BLOCK_SIGOPS_COST { + return Err(ValidationError::BadBlockSigops); + } + // Witness commitment (BIP 141), height-gated as Core gates it. check_witness_rules(block, segwit_active_at(network, height))?; diff --git a/node/src/validation/mod.rs b/node/src/validation/mod.rs index eff4177b5..cf3716865 100644 --- a/node/src/validation/mod.rs +++ b/node/src/validation/mod.rs @@ -30,6 +30,11 @@ pub enum ValidationError { // bytes push a length-legal block over the weight cap (#548). #[error("bad-blk-weight")] OverweightBlock, + // Core's `CheckBlock` applies a legacy-only sigop ceiling before any + // prevout is resolved; `connect_block` applies the full accurate count + // under the same reject reason. + #[error("bad-blk-sigops")] + BadBlockSigops, #[error("bad-diffbits")] BadDifficulty, // Core folds the empty-block case into its size-limits check, which emits diff --git a/node/tests/feature_block_consensus.rs b/node/tests/feature_block_consensus.rs index 10ac83bc4..d7b0def0c 100644 --- a/node/tests/feature_block_consensus.rs +++ b/node/tests/feature_block_consensus.rs @@ -306,6 +306,50 @@ fn case_oversize_block() -> Satd { cb(&block_of(vec![coinbase_with_outputs(1, outputs)])) } +/// Oversized *and* merkle-broken. Core runs `CheckMerkleRoot` before the size +/// limits — "all potential-corruption validation must be done before we do any +/// transaction validation" — so the answer is `bad-txnmrklroot`. satd tested +/// size first and answered `bad-blk-length`, which is the reason a peer would +/// get its header marked invalid over transactions it may not have sent. +/// Core's `CheckBlock` applies a legacy-only sigop ceiling — no prevouts, no +/// witness, no P2SH — before it looks at a single transaction's inputs. satd +/// counted sigops only in `connect_block`, where the accurate count needs +/// resolved prevouts, so a block that is over the ceiling *and* spends nothing +/// resolvable reported the wrong reason. +/// +/// `MAX_BLOCK_SIGOPS_COST` is 80_000 and each legacy sigop is scaled by 4, so +/// 20_001 bare `OP_CHECKSIG`s in an output script is one over. +fn case_legacy_sigops_over_limit() -> Satd { + let output = TxOut { + value: Amount::from_sat(0), + script_pubkey: bitcoin::ScriptBuf::from(vec![0xac; 20_001]), + }; + cb(&block_of(vec![coinbase_with_outputs(1, vec![output])])) +} + +/// One under the ceiling, so the refusal above is the ceiling and not the +/// shape of the fixture. +fn case_legacy_sigops_at_limit() -> Satd { + let output = TxOut { + value: Amount::from_sat(0), + script_pubkey: bitcoin::ScriptBuf::from(vec![0xac; 20_000]), + }; + cb(&block_of(vec![coinbase_with_outputs(1, vec![output])])) +} + +fn case_oversize_and_bad_merkle() -> Satd { + let mut outputs = Vec::new(); + for _ in 0..40 { + outputs.push(TxOut { + value: Amount::from_sat(1_000), + script_pubkey: bitcoin::ScriptBuf::from(vec![0x00; 30_000]), + }); + } + let mut block = block_of(vec![coinbase_with_outputs(1, outputs)]); + block.header.merkle_root = TxMerkleNode::from_byte_array([0xde; 32]); + cb(&block) +} + /// A spending tx carrying witness data, but the coinbase has no witness /// commitment output (BIP141). Core: `unexpected-witness` — with no /// commitment there is nothing to match against, so `CheckWitnessMalleation` @@ -635,6 +679,9 @@ fn cases() -> Vec { 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 }, Case { name: "oversize_block", core: Reject("bad-blk-length"), expect: Match, run: case_oversize_block }, + Case { name: "oversize_and_bad_merkle", core: Reject("bad-txnmrklroot"), expect: Match, run: case_oversize_and_bad_merkle }, + Case { name: "legacy_sigops_over_limit", core: Reject("bad-blk-sigops"), expect: Match, run: case_legacy_sigops_over_limit }, + Case { name: "legacy_sigops_at_limit", core: Accept, expect: Match, run: case_legacy_sigops_at_limit }, Case { name: "witness_commitment_missing", core: Reject("unexpected-witness"), expect: Match, run: case_witness_commitment_missing }, // context-free transaction Case { name: "tx_no_inputs", core: Reject("bad-txns-vin-empty"), expect: Match, run: case_tx_no_inputs }, diff --git a/satd/src/config.rs b/satd/src/config.rs index feebecf71..3d19955c7 100644 --- a/satd/src/config.rs +++ b/satd/src/config.rs @@ -914,6 +914,9 @@ pub struct Config { pub blockmaxweight: usize, #[allow(dead_code)] pub blockmintxfee: u64, + /// Core's regtest-only `-blockversion=`: the version stamped on a + /// template header, for testing forking scenarios. + pub blockversion: Option, // Misc pub pid: Option, // Cache — raw budget for the static partition; see `dbcache_mode` for @@ -3676,6 +3679,9 @@ impl Config { .or_else(|| file_get("blockmaxweight").and_then(|v| v.parse().ok())) .unwrap_or(4_000_000), blockmintxfee, + blockversion: cli + .blockversion + .or_else(|| file_get("blockversion").and_then(|v| v.parse().ok())), pid: cli.pid.or_else(|| file_get("pid")), mcp: cli.mcp.unwrap_or_else(|| { file_get("mcp").and_then(|v| parse_bool(&v)).unwrap_or(false) @@ -5446,6 +5452,13 @@ pub struct CliArgs { )] pub blockmintxfee: Option, + #[arg( + long, + value_name = "N", + help = "Override block version to test forking scenarios (regtest only)" + )] + pub blockversion: Option, + // Misc flags #[arg(long, value_name = "FILE", help = "Write PID to file")] pub pid: Option, @@ -6348,6 +6361,7 @@ pub fn normalize_args(args: Vec) -> Vec { "onlynet", "blockmaxweight", "blockmintxfee", + "blockversion", "pid", "mcp", "mcpport", @@ -9403,6 +9417,7 @@ testactivationheight=bip34@2 broadcastconfirmpeers: None, blockmaxweight: None, blockmintxfee: None, + blockversion: None, pid: None, server: Some(false), daemon: Some(false), @@ -9697,6 +9712,7 @@ testactivationheight=bip34@2 broadcastconfirmpeers: None, blockmaxweight: None, blockmintxfee: None, + blockversion: None, pid: None, server: Some(false), daemon: Some(false), diff --git a/satd/src/main.rs b/satd/src/main.rs index 0f0f5ca9a..2ab40c807 100644 --- a/satd/src/main.rs +++ b/satd/src/main.rs @@ -1214,6 +1214,12 @@ async fn main() { // value recorded once here rather than carrying it through every mining // entry point. node::mining::template::set_block_min_tx_fee(config.blockmintxfee); + // Core applies `-blockversion` only under `MineBlocksOnDemand()`, which is + // regtest alone: it exists to test forking scenarios, and honouring it on a + // live network would signal for deployments the node knows nothing about. + node::mining::template::set_block_version_override( + if config.network == bitcoin::Network::Regtest { config.blockversion } else { None }, + ); let mempool = Arc::new(Mempool::with_config(reload::mempool_config_from(&config))); diff --git a/satd/tests/core_block_differential.rs b/satd/tests/core_block_differential.rs index d5ab58d0e..5ea6a901c 100644 --- a/satd/tests/core_block_differential.rs +++ b/satd/tests/core_block_differential.rs @@ -462,6 +462,33 @@ fn c_bad_merkle_root(ctx: &Ctx, _u: &mut Vec) -> Submission { )) } +/// Oversized *and* merkle-broken. Core's `CheckBlock` runs `CheckMerkleRoot` +/// before the size limits, so it answers `bad-txnmrklroot`; satd tested size +/// first and answered `bad-blk-length`. The block has to be wrong in both ways +/// at once or the two orderings are indistinguishable. +fn c_oversize_and_bad_merkle(ctx: &Ctx, _u: &mut Vec) -> Submission { + let h = ctx.candidate_height(); + let outputs: Vec = (0..40) + .map(|_| TxOut { + value: Amount::from_sat(0), + script_pubkey: ScriptBuf::from(vec![0x00; 30_000]), + }) + .collect(); + let cb = { + let mut c = coinbase(h, block_subsidy(Network::Regtest, h), op_true()); + c.output = outputs; + c + }; + Submission::Block(assemble( + ctx.tip_hash, + ctx.candidate_time(), + POWLIMIT_BITS, + vec![cb], + Some(TxMerkleNode::from_byte_array([0xde; 32])), + true, + )) +} + fn c_oversize_block(ctx: &Ctx, _u: &mut Vec) -> Submission { let h = ctx.candidate_height(); let outputs: Vec = (0..40) @@ -854,6 +881,7 @@ fn cases() -> Vec { case("multiple_coinbase", "block-structure", Some("bad-cb-multiple"), c_multiple_coinbase), case("bad_merkle_root", "block-structure", Some("bad-txnmrklroot"), c_bad_merkle_root), case("oversize_block", "block-structure", Some("bad-blk-length"), c_oversize_block), + case("oversize_and_bad_merkle", "block-structure", Some("bad-txnmrklroot"), c_oversize_and_bad_merkle), case("overweight_block", "witness", Some("bad-blk-weight"), c_overweight_block), case("coinbase_scriptsig_too_short", "block-structure", Some("bad-cb-length"), c_coinbase_scriptsig_too_short), case("coinbase_scriptsig_too_long", "block-structure", Some("bad-cb-length"), c_coinbase_scriptsig_too_long), diff --git a/satd/tests/regtest.rs b/satd/tests/regtest.rs index 053438cfc..a29b3adb5 100644 --- a/satd/tests/regtest.rs +++ b/satd/tests/regtest.rs @@ -1283,6 +1283,251 @@ fn test_getblocktemplate_fields() { node.stop(); } +/// A transaction spending an outpoint that exists nowhere. Structurally valid, +/// contextually dead: the block carrying it fails at input resolution. +fn spend_of_a_nonexistent_output() -> bitcoin::Transaction { + use bitcoin::hashes::Hash as _; + bitcoin::Transaction { + version: bitcoin::transaction::Version(2), + lock_time: bitcoin::absolute::LockTime::ZERO, + input: vec![bitcoin::TxIn { + previous_output: bitcoin::OutPoint { + txid: bitcoin::Txid::from_byte_array([0x9e; 32]), + vout: 0, + }, + script_sig: bitcoin::ScriptBuf::new(), + sequence: bitcoin::Sequence::MAX, + witness: bitcoin::Witness::new(), + }], + output: vec![bitcoin::TxOut { + value: bitcoin::Amount::from_sat(1_000), + script_pubkey: bitcoin::ScriptBuf::new(), + }], + } +} + +/// Build a regtest block on `prev` at `height`, with a BIP 34 coinbase paying +/// exactly the subsidy, and grind its nonce to satisfy the regtest target. +fn build_regtest_block( + prev: bitcoin::BlockHash, + height: u32, + time: u32, + extra: Vec, +) -> bitcoin::Block { + use bitcoin::hashes::Hash as _; + + // Regtest subsidy: 50 BTC, halving every 150 blocks. + let subsidy = (50u64 * 100_000_000) >> (height / 150).min(63); + let script_sig = bitcoin::script::Builder::new() + .push_int(height as i64) + .push_int(time as i64) + .push_opcode(bitcoin::opcodes::OP_FALSE) + .into_script(); + 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, + sequence: bitcoin::Sequence::MAX, + witness: bitcoin::Witness::new(), + }], + output: vec![bitcoin::TxOut { + value: bitcoin::Amount::from_sat(subsidy), + script_pubkey: bitcoin::ScriptBuf::new(), + }], + }; + + let mut txdata = vec![coinbase]; + txdata.extend(extra); + let mut block = bitcoin::Block { + header: bitcoin::block::Header { + version: bitcoin::block::Version::from_consensus(0x2000_0000), + prev_blockhash: prev, + 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().expect("non-empty block"); + + // The regtest target has its top byte at 0x7f, so a hash whose leading + // byte is below 0x80 is under it — one comparison, no big-integer maths. + for nonce in 0u32..u32::MAX { + block.header.nonce = nonce; + let h = block.block_hash(); + if h.as_raw_hash().as_byte_array()[31] < 0x7f { + return block; + } + } + panic!("could not grind a regtest block header"); +} + +/// Core's `getblocktemplate` reads `mode` before anything else: absent or JSON +/// null means "template", a present-but-non-string value is `-8 Invalid mode`, +/// and any other string is too. satd read a non-string `mode` as "not +/// proposal" and quietly returned a template to a caller who had asked for +/// something else. +#[test] +fn getblocktemplate_rejects_a_mode_it_does_not_understand() { + let mut node = TestNode::start(&[]); + + for bad_mode in [serde_json::json!(1), serde_json::json!(true), serde_json::json!(["proposal"]), serde_json::json!("submit")] { + let resp = node + .rpc_call_with_params( + "getblocktemplate", + vec![serde_json::json!({ "mode": bad_mode })], + ) + .expect("rpc"); + assert_eq!( + resp["error"]["code"].as_i64(), + Some(-8), + "mode {bad_mode} was not refused: {resp}" + ); + assert_eq!(resp["error"]["message"].as_str(), Some("Invalid mode")); + } + + // An explicit null `mode` is Core's "do nothing" case: a template. + let resp = node + .rpc_call_with_params("getblocktemplate", vec![serde_json::json!({ "mode": null })]) + .expect("rpc"); + assert!(resp["error"].is_null(), "an explicit null mode is a template request: {resp}"); + assert!(resp["result"]["height"].is_number()); + + node.stop(); +} + +/// Proposal mode without usable `data` is `RPC_TYPE_ERROR` (-3) in Core, with +/// the message `Missing data String key for proposal` — not `-8`. +#[test] +fn getblocktemplate_proposal_without_data_is_a_type_error() { + let mut node = TestNode::start(&[]); + + for req in [ + serde_json::json!({ "mode": "proposal" }), + serde_json::json!({ "mode": "proposal", "data": 5 }), + serde_json::json!({ "mode": "proposal", "data": null }), + ] { + let resp = node + .rpc_call_with_params("getblocktemplate", vec![req.clone()]) + .expect("rpc"); + assert_eq!(resp["error"]["code"].as_i64(), Some(-3), "{req}: {resp}"); + assert_eq!( + resp["error"]["message"].as_str(), + Some("Missing data String key for proposal") + ); + } + + // Undecodable hex is Core's -22, and reaches that only because the `data` + // check above passed — so the two failures are distinguishable. + let resp = node + .rpc_call_with_params( + "getblocktemplate", + vec![serde_json::json!({ "mode": "proposal", "data": "zzzz" })], + ) + .expect("rpc"); + assert_eq!(resp["error"]["code"].as_i64(), Some(-22), "{resp}"); + + node.stop(); +} + +/// Proposal mode must give the same verdict as `submitblock`. satd's +/// hand-written validation loop skipped script verification by its own +/// admission, so a miner asking whether a block would be accepted was told +/// "yes" for a block the node then rejected. +#[test] +fn a_proposal_agrees_with_submitblock() { + let mut node = TestNode::start(&[]); + + // A template turned into a real block, with one transaction whose input + // does not exist — the block is structurally fine and contextually dead. + let template = node.rpc_call("getblocktemplate").unwrap(); + let t = &template["result"]; + let height = t["height"].as_u64().unwrap() as u32; + let prev: bitcoin::BlockHash = t["previousblockhash"].as_str().unwrap().parse().unwrap(); + let time = t["curtime"].as_u64().unwrap() as u32; + + let bad_block = build_regtest_block( + prev, + height, + time, + vec![spend_of_a_nonexistent_output()], + ); + let hex = hex::encode(bitcoin::consensus::serialize(&bad_block)); + + let proposal = node + .rpc_call_with_params( + "getblocktemplate", + vec![serde_json::json!({ "mode": "proposal", "data": hex.clone() })], + ) + .expect("rpc"); + assert!(proposal["error"].is_null(), "{proposal}"); + let proposal_verdict = proposal["result"].as_str().unwrap_or(""); + + let submit = node + .rpc_call_with_params("submitblock", vec![serde_json::json!(hex)]) + .expect("rpc"); + let submit_verdict = submit["result"].as_str().unwrap_or(""); + + assert_eq!( + proposal_verdict, submit_verdict, + "proposal mode and submitblock disagree: {proposal} vs {submit}" + ); + assert_eq!(proposal_verdict, "bad-txns-inputs-missingorspent"); + + // A well-formed block built on the same tip is accepted by both, so the + // agreement above is not two identical refusals of everything. + let good_block = build_regtest_block(prev, height, time, vec![]); + let good_hex = hex::encode(bitcoin::consensus::serialize(&good_block)); + let proposal = node + .rpc_call_with_params( + "getblocktemplate", + vec![serde_json::json!({ "mode": "proposal", "data": good_hex.clone() })], + ) + .expect("rpc"); + assert!( + proposal["result"].is_null() && proposal["error"].is_null(), + "a valid proposal must be null: {proposal}" + ); + + // …and once it is really submitted, proposing it again is a duplicate. + let submit = node + .rpc_call_with_params("submitblock", vec![serde_json::json!(good_hex.clone())]) + .expect("rpc"); + assert!(submit["result"].is_null(), "{submit}"); + let proposal = node + .rpc_call_with_params( + "getblocktemplate", + vec![serde_json::json!({ "mode": "proposal", "data": good_hex })], + ) + .expect("rpc"); + assert_eq!(proposal["result"].as_str(), Some("duplicate"), "{proposal}"); + + node.stop(); +} + +/// Core's regtest-only `-blockversion=` overrides the template's version, +/// which is how `mining_basic.py` tests forking scenarios. satd accepted the +/// option in `bitcoin.conf` and ignored it. +#[test] +fn blockversion_overrides_the_template_version() { + let mut node = TestNode::start(&["--blockversion=1337"]); + let response = node.rpc_call("getblocktemplate").unwrap(); + assert_eq!(response["result"]["version"].as_i64(), Some(1337), "{response}"); + node.stop(); + + let mut node = TestNode::start(&[]); + let response = node.rpc_call("getblocktemplate").unwrap(); + assert_eq!( + response["result"]["version"].as_i64(), + Some(0x2000_0000), + "without the override the computed version stands: {response}" + ); + node.stop(); +} + // --- P2P Integration Tests --- #[test]