From 7c08c0c315abe65e88b33fc60e66c0c750fe95c5 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Tue, 24 Nov 2020 19:55:15 -0800 Subject: [PATCH] consensus: check Merkle roots As a side effect of computing Merkle roots, we build a list of transaction hashes. Instead of discarding these, add them to PreparedBlock and FinalizedBlock so that they can be reused rather than recomputed. This commit adds Merkle root validation to: 1. the block verifier; 2. the checkpoint verifier. In the first case, Bitcoin Merkle tree malleability has no effect, because only a single Merkle tree in each malleablity set is valid (the others have duplicate transactions). In the second case, we need to check that the Merkle tree does not contain any duplicate transactions. Closes #1385 Closes #906 --- zebra-consensus/src/block.rs | 36 +++++++++++++------ zebra-consensus/src/block/check.rs | 23 ++++++++++-- zebra-consensus/src/checkpoint.rs | 36 ++++++++++++++++++- zebra-consensus/src/error.rs | 8 ++++- zebra-state/src/request.rs | 20 +++++++++-- zebra-state/src/service/finalized_state.rs | 9 +++-- .../src/service/non_finalized_state/chain.rs | 28 +++++++++++---- zebra-state/src/tests.rs | 2 ++ 8 files changed, 136 insertions(+), 26 deletions(-) diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index f1623b789..b044f4f93 100644 --- a/zebra-consensus/src/block.rs +++ b/zebra-consensus/src/block.rs @@ -25,12 +25,12 @@ use tracing::Instrument; use zebra_chain::{ block::{self, Block}, parameters::Network, - transparent, + transaction, transparent, work::equihash, }; use zebra_state as zs; -use crate::{error::*, transaction}; +use crate::{error::*, transaction as tx}; use crate::{script, BoxError}; mod check; @@ -44,7 +44,7 @@ pub struct BlockVerifier { /// The network to be verified. network: Network, state_service: S, - transaction_verifier: transaction::Verifier, + transaction_verifier: tx::Verifier, } // TODO: dedupe with crate::error::BlockError @@ -83,7 +83,7 @@ where { pub fn new(network: Network, state_service: S) -> Self { let transaction_verifier = - transaction::Verifier::new(network, script::Verifier::new(state_service.clone())); + tx::Verifier::new(network, script::Verifier::new(state_service.clone())); Self { network, @@ -161,17 +161,24 @@ where check::coinbase_is_first(&block)?; check::subsidy_is_valid(&block, network)?; - // TODO: context-free header verification: merkle root + // Precomputing this avoids duplicating transaction hash computations. + let transaction_hashes = block + .transactions + .iter() + .map(|t| t.hash()) + .collect::>(); + + check::merkle_root_validity(&block, &transaction_hashes)?; let mut async_checks = FuturesUnordered::new(); - let known_utxos = new_outputs(&block); + let known_utxos = new_outputs(&block, &transaction_hashes); for transaction in &block.transactions { let rsp = transaction_verifier .ready_and() .await .expect("transaction verifier is always ready") - .call(transaction::Request::Block { + .call(tx::Request::Block { transaction: transaction.clone(), known_utxos: known_utxos.clone(), height, @@ -199,6 +206,7 @@ where hash, height, new_outputs, + transaction_hashes, }; match state_service .ready_and() @@ -220,11 +228,19 @@ where } } -fn new_outputs(block: &Block) -> Arc> { +/// Compute an index of newly created transparent outputs, given a block and a +/// list of precomputed transaction hashes. +fn new_outputs( + block: &Block, + transaction_hashes: &[transaction::Hash], +) -> Arc> { let mut new_outputs = HashMap::default(); let height = block.coinbase_height().expect("block has coinbase height"); - for transaction in &block.transactions { - let hash = transaction.hash(); + for (transaction, hash) in block + .transactions + .iter() + .zip(transaction_hashes.iter().cloned()) + { let from_coinbase = transaction.is_coinbase(); for (index, output) in transaction.outputs().iter().cloned().enumerate() { let index = index as u32; diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs index c06d99922..e59c24b3e 100644 --- a/zebra-consensus/src/block/check.rs +++ b/zebra-consensus/src/block/check.rs @@ -3,10 +3,9 @@ use chrono::{DateTime, Utc}; use zebra_chain::{ - block::Hash, - block::Height, - block::{Block, Header}, + block::{Block, Hash, Header, Height}, parameters::{Network, NetworkUpgrade}, + transaction, work::{difficulty::ExpandedDifficulty, equihash}, }; @@ -165,3 +164,21 @@ pub fn time_is_valid_at( ) -> Result<(), zebra_chain::block::BlockTimeError> { header.time_is_valid_at(now, height, hash) } + +/// Check Merkle root validity. +/// +/// `transaction_hashes` is a precomputed list of transaction hashes. +pub fn merkle_root_validity( + block: &Block, + transaction_hashes: &[transaction::Hash], +) -> Result<(), BlockError> { + let merkle_root = transaction_hashes.iter().cloned().collect(); + if block.header.merkle_root == merkle_root { + Ok(()) + } else { + Err(BlockError::BadMerkleRoot { + actual: merkle_root, + expected: block.header.merkle_root, + }) + } +} diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs index ec98e0ada..a74eea74b 100644 --- a/zebra-consensus/src/checkpoint.rs +++ b/zebra-consensus/src/checkpoint.rs @@ -14,7 +14,7 @@ //! block for the configured network. use std::{ - collections::BTreeMap, + collections::{BTreeMap, HashSet}, future::Future, ops::{Bound, Bound::*}, pin::Pin, @@ -459,6 +459,33 @@ where } }; + // Check for a valid Merkle root. To prevent malleability (CVE-2012-2459), + // we also need to check whether the transaction hashes are unique. + + let transaction_hashes = block + .transactions + .iter() + .map(|tx| tx.hash()) + .collect::>(); + let merkle_root = transaction_hashes.iter().cloned().collect(); + + if block.header.merkle_root != merkle_root { + tx.send(Err(VerifyCheckpointError::BadMerkleRoot { + expected: block.header.merkle_root, + actual: merkle_root, + })) + .expect("rx has not been dropped yet"); + return rx; + } + + // Collecting into a HashSet deduplicates, so this checks that there + // are no duplicate transaction hashes, preventing Merkle root malleability. + if transaction_hashes.len() != transaction_hashes.iter().collect::>().len() { + tx.send(Err(VerifyCheckpointError::DuplicateTransaction)) + .expect("rx has not been dropped yet"); + return rx; + } + // Since we're using Arc, each entry is a single pointer to the // Arc. But there are a lot of QueuedBlockLists in the queue, so we keep // allocations as small as possible. @@ -779,6 +806,13 @@ pub enum VerifyCheckpointError { }, #[error("the block {hash:?} does not have a coinbase height")] CoinbaseHeight { hash: block::Hash }, + #[error("merkle root {actual:?} does not match expected {expected:?}")] + BadMerkleRoot { + actual: block::merkle::Root, + expected: block::merkle::Root, + }, + #[error("duplicate transactions in block")] + DuplicateTransaction, #[error("checkpoint verifier was dropped")] Dropped, #[error(transparent)] diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index 419dc2e8d..d0fb1603e 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -7,7 +7,7 @@ use thiserror::Error; -use zebra_chain::primitives::ed25519; +use zebra_chain::{block, primitives::ed25519}; use crate::BoxError; @@ -99,6 +99,12 @@ pub enum BlockError { #[error("block has no transactions")] NoTransactions, + #[error("block has mismatched merkle root")] + BadMerkleRoot { + actual: block::merkle::Root, + expected: block::merkle::Root, + }, + #[error("block {0:?} is already in the chain at depth {1:?}")] AlreadyInChain(zebra_chain::block::Hash, u32), diff --git a/zebra-state/src/request.rs b/zebra-state/src/request.rs index 6bfb28aca..a47a271e6 100644 --- a/zebra-state/src/request.rs +++ b/zebra-state/src/request.rs @@ -74,6 +74,8 @@ pub struct PreparedBlock { /// be unspent, since a later transaction in a block can spend outputs of an /// earlier transaction. pub new_outputs: HashMap, + /// A precomputed list of the hashes of the transactions in this block. + pub transaction_hashes: Vec, // TODO: add these parameters when we can compute anchors. // sprout_anchor: sprout::tree::Root, // sapling_anchor: sapling::tree::Root, @@ -97,6 +99,8 @@ pub struct FinalizedBlock { /// be unspent, since a later transaction in a block can spend outputs of an /// earlier transaction. pub(crate) new_outputs: HashMap, + /// A precomputed list of the hashes of the transactions in this block. + pub(crate) transaction_hashes: Vec, } // Doing precomputation in this From impl means that it will be done in @@ -108,10 +112,19 @@ impl From> for FinalizedBlock { .coinbase_height() .expect("finalized blocks must have a valid coinbase height"); let hash = block.hash(); + let transaction_hashes = block + .transactions + .iter() + .map(|tx| tx.hash()) + .collect::>(); let mut new_outputs = HashMap::default(); - for transaction in &block.transactions { - let hash = transaction.hash(); + + for (transaction, hash) in block + .transactions + .iter() + .zip(transaction_hashes.iter().cloned()) + { let from_coinbase = transaction.is_coinbase(); for (index, output) in transaction.outputs().iter().cloned().enumerate() { let index = index as u32; @@ -131,6 +144,7 @@ impl From> for FinalizedBlock { height, hash, new_outputs, + transaction_hashes, } } } @@ -142,12 +156,14 @@ impl From for FinalizedBlock { height, hash, new_outputs, + transaction_hashes, } = prepared; Self { block, height, hash, new_outputs, + transaction_hashes, } } } diff --git a/zebra-state/src/service/finalized_state.rs b/zebra-state/src/service/finalized_state.rs index ba93f9c0b..4793aeef2 100644 --- a/zebra-state/src/service/finalized_state.rs +++ b/zebra-state/src/service/finalized_state.rs @@ -152,6 +152,7 @@ impl FinalizedState { hash, height, new_outputs, + transaction_hashes, } = finalized; let hash_by_height = self.db.cf_handle("hash_by_height").unwrap(); @@ -215,8 +216,12 @@ impl FinalizedState { // Index each transaction, spent inputs, nullifiers // TODO: move computation into FinalizedBlock as with transparent outputs - for (transaction_index, transaction) in block.transactions.iter().enumerate() { - let transaction_hash = transaction.hash(); + for (transaction_index, (transaction, transaction_hash)) in block + .transactions + .iter() + .zip(transaction_hashes.into_iter()) + .enumerate() + { let transaction_location = TransactionLocation { height, index: transaction_index diff --git a/zebra-state/src/service/non_finalized_state/chain.rs b/zebra-state/src/service/non_finalized_state/chain.rs index 39c0e26c8..82946e6ce 100644 --- a/zebra-state/src/service/non_finalized_state/chain.rs +++ b/zebra-state/src/service/non_finalized_state/chain.rs @@ -136,7 +136,12 @@ trait UpdateWith { impl UpdateWith for Chain { fn update_chain_state_with(&mut self, prepared: &PreparedBlock) { - let (block, hash, height) = (prepared.block.as_ref(), prepared.hash, prepared.height); + let (block, hash, height, transaction_hashes) = ( + prepared.block.as_ref(), + prepared.hash, + prepared.height, + &prepared.transaction_hashes, + ); // add hash to height_by_hash let prior_height = self.height_by_hash.insert(hash, height); @@ -154,7 +159,12 @@ impl UpdateWith for Chain { self.partial_cumulative_work += block_work; // for each transaction in block - for (transaction_index, transaction) in block.transactions.iter().enumerate() { + for (transaction_index, (transaction, transaction_hash)) in block + .transactions + .iter() + .zip(transaction_hashes.iter().cloned()) + .enumerate() + { let (inputs, shielded_data, joinsplit_data) = match transaction.deref() { transaction::Transaction::V4 { inputs, @@ -168,7 +178,6 @@ impl UpdateWith for Chain { }; // add key `transaction.hash` and value `(height, tx_index)` to `tx_by_hash` - let transaction_hash = transaction.hash(); let prior_pair = self .tx_by_hash .insert(transaction_hash, (height, transaction_index)); @@ -190,7 +199,11 @@ impl UpdateWith for Chain { #[instrument(skip(self, prepared), fields(block = %prepared.block))] fn revert_chain_state_with(&mut self, prepared: &PreparedBlock) { - let (block, hash) = (prepared.block.as_ref(), prepared.hash); + let (block, hash, transaction_hashes) = ( + prepared.block.as_ref(), + prepared.hash, + &prepared.transaction_hashes, + ); // remove the blocks hash from `height_by_hash` assert!( @@ -207,7 +220,9 @@ impl UpdateWith for Chain { self.partial_cumulative_work -= block_work; // for each transaction in block - for transaction in &block.transactions { + for (transaction, transaction_hash) in + block.transactions.iter().zip(transaction_hashes.iter()) + { let (inputs, shielded_data, joinsplit_data) = match transaction.deref() { transaction::Transaction::V4 { inputs, @@ -221,9 +236,8 @@ impl UpdateWith for Chain { }; // remove `transaction.hash` from `tx_by_hash` - let transaction_hash = transaction.hash(); assert!( - self.tx_by_hash.remove(&transaction_hash).is_some(), + self.tx_by_hash.remove(transaction_hash).is_some(), "transactions must be present if block was" ); diff --git a/zebra-state/src/tests.rs b/zebra-state/src/tests.rs index 3c7889673..8e17019e4 100644 --- a/zebra-state/src/tests.rs +++ b/zebra-state/src/tests.rs @@ -21,6 +21,7 @@ impl Prepare for Arc { let block = self; let hash = block.hash(); let height = block.coinbase_height().unwrap(); + let transaction_hashes = block.transactions.iter().map(|tx| tx.hash()).collect(); let new_outputs = crate::utxo::new_outputs(&block); PreparedBlock { @@ -28,6 +29,7 @@ impl Prepare for Arc { hash, height, new_outputs, + transaction_hashes, } } }