From bfc3e4a46cd8033869216951833750ddceccc285 Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 19 Jul 2021 23:52:32 +1000 Subject: [PATCH] Add an OrderedUtxo type for transparent spend validation (#2502) * Add an OrderedUtxo type for validation of spends within a block This change allows us to check that transparent spends use outputs from earlier in their block. (But we don't actually do that check yet.) We need to keep the order of UTXOs when we're contextually verifying each new block that is added to a chain. But the block order is irrelevant for UTXOs stored in the state. * Take ownership in utxos_from_ordered_utxos * Delete a confusing comment --- zebra-chain/src/transparent.rs | 2 +- zebra-chain/src/transparent/utxo.rs | 82 ++++++++++++++++--- zebra-consensus/src/block.rs | 5 +- zebra-consensus/src/script.rs | 4 +- zebra-consensus/src/transaction.rs | 4 +- zebra-consensus/src/transaction/tests.rs | 17 ++-- zebra-state/src/lib.rs | 2 + zebra-state/src/request.rs | 47 +++++++++-- zebra-state/src/service.rs | 6 +- .../src/service/finalized_state/tests/prop.rs | 3 +- .../src/service/non_finalized_state.rs | 4 +- .../src/service/non_finalized_state/chain.rs | 63 +++++++++----- .../non_finalized_state/queued_blocks.rs | 5 +- zebra-state/src/service/pending_utxos.rs | 17 ++-- zebra-state/src/tests.rs | 2 +- 15 files changed, 199 insertions(+), 64 deletions(-) diff --git a/zebra-chain/src/transparent.rs b/zebra-chain/src/transparent.rs index 7395a2ba..7534651e 100644 --- a/zebra-chain/src/transparent.rs +++ b/zebra-chain/src/transparent.rs @@ -9,7 +9,7 @@ mod utxo; pub use address::Address; pub use script::Script; -pub use utxo::{new_outputs, Utxo}; +pub use utxo::{new_ordered_outputs, new_outputs, utxos_from_ordered_utxos, OrderedUtxo, Utxo}; #[cfg(any(test, feature = "proptest-impl"))] use proptest_derive::Arbitrary; diff --git a/zebra-chain/src/transparent/utxo.rs b/zebra-chain/src/transparent/utxo.rs index dbab18c1..cb652d50 100644 --- a/zebra-chain/src/transparent/utxo.rs +++ b/zebra-chain/src/transparent/utxo.rs @@ -22,32 +22,94 @@ pub struct Utxo { pub from_coinbase: bool, } -/// Compute an index of newly created transparent outputs, given a block and a +/// A [`Utxo`], and the index of its transaction within its block. +/// +/// This extra index is used to check that spends come after outputs, +/// when a new output and its spend are both in the same block. +/// +/// The extra index is only used during block verification, +/// so it does not need to be sent to the state. +#[derive(Clone, Debug, PartialEq, Eq)] +#[cfg_attr( + any(test, feature = "proptest-impl"), + derive(proptest_derive::Arbitrary) +)] +pub struct OrderedUtxo { + /// An unspent transaction output. + pub utxo: Utxo, + /// The index of the transaction that created the output, in the block at `height`. + /// + /// Used to make sure that transaction can only spend outputs + /// that were created earlier in the chain. + /// + /// Note: this is different from `OutPoint.index`, + /// which is the index of the output in its transaction. + pub tx_index_in_block: usize, +} + +impl OrderedUtxo { + /// Create a new ordered UTXO from its fields. + pub fn new( + output: transparent::Output, + height: block::Height, + from_coinbase: bool, + tx_index_in_block: usize, + ) -> OrderedUtxo { + let utxo = Utxo { + output, + height, + from_coinbase, + }; + + OrderedUtxo { + utxo, + tx_index_in_block, + } + } +} + +/// Compute an index of [`Utxo`]s, given an index of [`OrderedUtxo`]s. +pub fn utxos_from_ordered_utxos( + ordered_utxos: HashMap, +) -> HashMap { + ordered_utxos + .into_iter() + .map(|(out_point, ordered_utxo)| (out_point, ordered_utxo.utxo)) + .collect() +} + +/// Compute an index of newly created [`Utxo`]s, given a block and a /// list of precomputed transaction hashes. pub fn new_outputs( block: &Block, transaction_hashes: &[transaction::Hash], ) -> HashMap { - let mut new_outputs = HashMap::default(); + utxos_from_ordered_utxos(new_ordered_outputs(block, transaction_hashes)) +} + +/// Compute an index of newly created [`OrderedUtxo`]s, given a block and a +/// list of precomputed transaction hashes. +pub fn new_ordered_outputs( + block: &Block, + transaction_hashes: &[transaction::Hash], +) -> HashMap { + let mut new_ordered_outputs = HashMap::default(); let height = block.coinbase_height().expect("block has coinbase height"); - for (transaction, hash) in block + for (tx_index_in_block, (transaction, hash)) in block .transactions .iter() .zip(transaction_hashes.iter().cloned()) + .enumerate() { let from_coinbase = transaction.is_coinbase(); for (index, output) in transaction.outputs().iter().cloned().enumerate() { let index = index as u32; - new_outputs.insert( + new_ordered_outputs.insert( transparent::OutPoint { hash, index }, - Utxo { - output, - height, - from_coinbase, - }, + OrderedUtxo::new(output, height, from_coinbase, tx_index_in_block), ); } } - new_outputs + new_ordered_outputs } diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index bca718ab..f8fc0fd4 100644 --- a/zebra-consensus/src/block.rs +++ b/zebra-consensus/src/block.rs @@ -174,7 +174,10 @@ where let mut async_checks = FuturesUnordered::new(); - let known_utxos = Arc::new(transparent::new_outputs(&block, &transaction_hashes)); + let known_utxos = Arc::new(transparent::new_ordered_outputs( + &block, + &transaction_hashes, + )); for transaction in &block.transactions { let rsp = transaction_verifier .ready_and() diff --git a/zebra-consensus/src/script.rs b/zebra-consensus/src/script.rs index d71c3b94..006f4ae3 100644 --- a/zebra-consensus/src/script.rs +++ b/zebra-consensus/src/script.rs @@ -58,7 +58,7 @@ pub struct Request { /// A set of additional UTXOs known in the context of this verification request. /// /// This allows specifying additional UTXOs that are not already known to the chain state. - pub known_utxos: Arc>, + pub known_utxos: Arc>, /// The network upgrade active in the context of this verification request. /// /// Because the consensus branch ID changes with each network upgrade, @@ -110,7 +110,7 @@ where tracing::trace!("awaiting outpoint lookup"); let utxo = if let Some(output) = known_utxos.get(&outpoint) { tracing::trace!("UXTO in known_utxos, discarding query"); - output.clone() + output.utxo.clone() } else if let zebra_state::Response::Utxo(utxo) = query.await? { utxo } else { diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 022e282b..f4d77221 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -70,7 +70,7 @@ pub enum Request { /// The transaction itself. transaction: Arc, /// Additional UTXOs which are known at the time of verification. - known_utxos: Arc>, + known_utxos: Arc>, /// The height of the block containing this transaction. height: block::Height, }, @@ -100,7 +100,7 @@ impl Request { } /// The set of additional known unspent transaction outputs that's in this request. - pub fn known_utxos(&self) -> Arc> { + pub fn known_utxos(&self) -> Arc> { match self { Request::Block { known_utxos, .. } => known_utxos.clone(), Request::Mempool { .. } => HashMap::new().into(), diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 0583fcbb..1b72f05f 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -822,23 +822,27 @@ fn v5_with_sapling_spends() { /// First, this creates a fake unspent transaction output from a fake transaction included in the /// specified `previous_utxo_height` block height. This fake [`Utxo`] also contains a simple script /// that can either accept or reject any spend attempt, depending on if `script_should_succeed` is -/// `true` or `false`. +/// `true` or `false`. Since the `tx_index_in_block` is irrelevant for blocks that have already +/// been verified, it is set to `1`. /// /// Then, a [`transparent::Input::PrevOut`] is created that attempts to spend the previously created fake -/// UTXO. A new UTXO is created with the [`transparent::Output`] resulting from the spend. +/// UTXO to a new [`transparent::Output`]. /// /// Finally, the initial fake UTXO is placed in a `known_utxos` [`HashMap`] so that it can be /// retrieved during verification. /// /// The function then returns the generated transparent input and output, as well as the /// `known_utxos` map. +/// +/// Note: `known_utxos` is only intended to be used for UTXOs within the same block, +/// so future verification changes might break this mocking function. fn mock_transparent_transfer( previous_utxo_height: block::Height, script_should_succeed: bool, ) -> ( transparent::Input, transparent::Output, - HashMap, + HashMap, ) { // A script with a single opcode that accepts the transaction (pushes true on the stack) let accepting_script = transparent::Script::new(&[1, 1]); @@ -862,11 +866,8 @@ fn mock_transparent_transfer( lock_script, }; - let previous_utxo = transparent::Utxo { - output: previous_output, - height: previous_utxo_height, - from_coinbase: false, - }; + let previous_utxo = + transparent::OrderedUtxo::new(previous_output, previous_utxo_height, false, 1); // Use the `previous_outpoint` as input let input = transparent::Input::PrevOut { diff --git a/zebra-state/src/lib.rs b/zebra-state/src/lib.rs index 499393a6..e3f08dab 100644 --- a/zebra-state/src/lib.rs +++ b/zebra-state/src/lib.rs @@ -35,3 +35,5 @@ pub use error::{BoxError, CloneError, CommitBlockError, ValidateContextError}; pub use request::{FinalizedBlock, HashOrHeight, PreparedBlock, Request}; pub use response::Response; pub use service::init; + +pub(crate) use request::ContextuallyValidBlock; diff --git a/zebra-state/src/request.rs b/zebra-state/src/request.rs index 0a0949cc..c66e5eb2 100644 --- a/zebra-state/src/request.rs +++ b/zebra-state/src/request.rs @@ -68,10 +68,14 @@ pub struct PreparedBlock { /// New transparent outputs created in this block, indexed by /// [`Outpoint`](transparent::Outpoint). /// + /// Each output is tagged with its transaction index in the block. + /// (The outputs of earlier transactions in a block can be spent by later + /// transactions.) + /// /// Note: although these transparent outputs are newly created, they may not /// be unspent, since a later transaction in a block can spend outputs of an /// earlier transaction. - pub new_outputs: HashMap, + 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. @@ -79,6 +83,19 @@ pub struct PreparedBlock { // sapling_anchor: sapling::tree::Root, } +/// A contextually validated block, ready to be committed directly to the finalized state with +/// no checks, if it becomes the root of the best non-finalized chain. +/// +/// Used by the state service and non-finalized [`Chain`]. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct ContextuallyValidBlock { + pub(crate) block: Arc, + pub(crate) hash: block::Hash, + pub(crate) height: block::Height, + pub(crate) new_outputs: HashMap, + pub(crate) transaction_hashes: Vec, +} + /// A finalized block, ready to be committed directly to the finalized state with /// no checks. /// @@ -90,14 +107,7 @@ pub struct FinalizedBlock { pub(crate) block: Arc, pub(crate) hash: block::Hash, pub(crate) height: block::Height, - /// New transparent outputs created in this block, indexed by - /// [`Outpoint`](transparent::Outpoint). - /// - /// Note: although these transparent outputs are newly created, they may not - /// 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, } @@ -127,7 +137,7 @@ impl From> for FinalizedBlock { } } -impl From for FinalizedBlock { +impl From for ContextuallyValidBlock { fn from(prepared: PreparedBlock) -> Self { let PreparedBlock { block, @@ -136,6 +146,25 @@ impl From for FinalizedBlock { new_outputs, transaction_hashes, } = prepared; + Self { + block, + hash, + height, + new_outputs: transparent::utxos_from_ordered_utxos(new_outputs), + transaction_hashes, + } + } +} + +impl From for FinalizedBlock { + fn from(contextually_valid: ContextuallyValidBlock) -> Self { + let ContextuallyValidBlock { + block, + hash, + height, + new_outputs, + transaction_hashes, + } = contextually_valid; Self { block, hash, diff --git a/zebra-state/src/service.rs b/zebra-state/src/service.rs index e975a623..2442884e 100644 --- a/zebra-state/src/service.rs +++ b/zebra-state/src/service.rs @@ -246,6 +246,9 @@ impl StateService { /// Check that the prepared block is contextually valid for the configured /// network, based on the committed finalized and non-finalized state. + /// + /// Note: some additional contextual validity checks are performed by the + /// non-finalized [`Chain`]. fn check_contextual_validity( &mut self, prepared: &PreparedBlock, @@ -631,7 +634,8 @@ impl Service for StateService { Request::CommitBlock(prepared) => { metrics::counter!("state.requests", 1, "type" => "commit_block"); - self.pending_utxos.check_against(&prepared.new_outputs); + self.pending_utxos + .check_against_ordered(&prepared.new_outputs); let rsp_rx = self.queue_and_commit_non_finalized(prepared); async move { diff --git a/zebra-state/src/service/finalized_state/tests/prop.rs b/zebra-state/src/service/finalized_state/tests/prop.rs index 821d4254..93209b51 100644 --- a/zebra-state/src/service/finalized_state/tests/prop.rs +++ b/zebra-state/src/service/finalized_state/tests/prop.rs @@ -9,6 +9,7 @@ use crate::{ arbitrary::PreparedChain, finalized_state::{FinalizedBlock, FinalizedState}, }, + ContextuallyValidBlock, }; const DEFAULT_PARTIAL_CHAIN_PROPTEST_CASES: u32 = 32; @@ -26,7 +27,7 @@ fn blocks_with_v5_transactions() -> Result<()> { // use `count` to minimize test failures, so they are easier to diagnose for block in chain.iter().take(count) { let hash = state.commit_finalized_direct( - FinalizedBlock::from(block.clone()), + FinalizedBlock::from(ContextuallyValidBlock::from(block.clone())), "blocks_with_v5_transactions test" ); prop_assert_eq!(Some(height), state.finalized_tip_height()); diff --git a/zebra-state/src/service/non_finalized_state.rs b/zebra-state/src/service/non_finalized_state.rs index eefa915a..68f7a67f 100644 --- a/zebra-state/src/service/non_finalized_state.rs +++ b/zebra-state/src/service/non_finalized_state.rs @@ -222,8 +222,8 @@ impl NonFinalizedState { /// `transparent::OutPoint` if it is present in any chain. pub fn any_utxo(&self, outpoint: &transparent::OutPoint) -> Option { for chain in self.chain_set.iter().rev() { - if let Some(output) = chain.created_utxos.get(outpoint) { - return Some(output.clone()); + if let Some(utxo) = chain.created_utxos.get(outpoint) { + return Some(utxo.clone()); } } diff --git a/zebra-state/src/service/non_finalized_state/chain.rs b/zebra-state/src/service/non_finalized_state/chain.rs index 0b62abc5..2a29f613 100644 --- a/zebra-state/src/service/non_finalized_state/chain.rs +++ b/zebra-state/src/service/non_finalized_state/chain.rs @@ -11,25 +11,44 @@ use zebra_chain::{ transaction::Transaction::*, transparent, work::difficulty::PartialCumulativeWork, }; -use crate::{service::check, PreparedBlock, ValidateContextError}; +use crate::{service::check, ContextuallyValidBlock, PreparedBlock, ValidateContextError}; #[derive(Debug, Default, Clone)] pub struct Chain { - pub blocks: BTreeMap, + /// The contextually valid blocks which form this non-finalized partial chain, in height order. + pub(crate) blocks: BTreeMap, + + /// An index of block heights for each block hash in `blocks`. pub height_by_hash: HashMap, + /// An index of block heights and transaction indexes for each transaction hash in `blocks`. pub tx_by_hash: HashMap, - pub created_utxos: HashMap, + /// The [`Utxo`]s created by `blocks`. + /// + /// Note that these UTXOs may not be unspent. + /// Outputs can be spent by later transactions or blocks in the chain. + pub(super) created_utxos: HashMap, + /// The [`OutPoint`]s spent by `blocks`, + /// including those created by earlier transactions or blocks in the chain. pub(super) spent_utxos: HashSet, + /// The sprout anchors created by `blocks`. + /// + /// TODO: does this include intersitial anchors? pub(super) sprout_anchors: HashSet, + /// The sapling anchors created by `blocks`. pub(super) sapling_anchors: HashSet, + /// The orchard anchors created by `blocks`. pub(super) orchard_anchors: HashSet, + /// The sprout nullifiers revealed by `blocks`. pub(super) sprout_nullifiers: HashSet, + /// The sapling nullifiers revealed by `blocks`. pub(super) sapling_nullifiers: HashSet, + /// The orchard nullifiers revealed by `blocks`. pub(super) orchard_nullifiers: HashSet, + /// The cumulative work represented by this partial non-finalized chain. pub(super) partial_cumulative_work: PartialCumulativeWork, } @@ -76,6 +95,9 @@ impl Chain { /// If the block is invalid, drop this chain and return an error. #[instrument(level = "debug", skip(self, block), fields(block = %block.block))] pub fn push(mut self, block: PreparedBlock) -> Result { + // the block isn't contextually valid until `update_chain_state_with` returns success + let block = ContextuallyValidBlock::from(block); + // update cumulative data members self.update_chain_state_with(&block)?; tracing::debug!(block = %block.block, "adding block to chain"); @@ -85,7 +107,7 @@ impl Chain { /// Remove the lowest height block of the non-finalized portion of a chain. #[instrument(level = "debug", skip(self))] - pub fn pop_root(&mut self) -> PreparedBlock { + pub(crate) fn pop_root(&mut self) -> ContextuallyValidBlock { let block_height = self.lowest_height(); // remove the lowest height block from self.blocks @@ -180,16 +202,18 @@ trait UpdateWith { fn revert_chain_state_with(&mut self, _: &T); } -impl UpdateWith for Chain { +impl UpdateWith for Chain { + #[instrument(skip(self, contextually_valid), fields(block = %contextually_valid.block))] fn update_chain_state_with( &mut self, - prepared: &PreparedBlock, + contextually_valid: &ContextuallyValidBlock, ) -> Result<(), ValidateContextError> { - let (block, hash, height, transaction_hashes) = ( - prepared.block.as_ref(), - prepared.hash, - prepared.height, - &prepared.transaction_hashes, + let (block, hash, height, new_outputs, transaction_hashes) = ( + contextually_valid.block.as_ref(), + contextually_valid.hash, + contextually_valid.height, + &contextually_valid.new_outputs, + &contextually_valid.transaction_hashes, ); // add hash to height_by_hash @@ -254,7 +278,7 @@ impl UpdateWith for Chain { ); // add the utxos this produced - self.update_chain_state_with(&prepared.new_outputs)?; + self.update_chain_state_with(new_outputs)?; // add the utxos this consumed self.update_chain_state_with(inputs)?; @@ -268,12 +292,13 @@ impl UpdateWith for Chain { Ok(()) } - #[instrument(skip(self, prepared), fields(block = %prepared.block))] - fn revert_chain_state_with(&mut self, prepared: &PreparedBlock) { - let (block, hash, transaction_hashes) = ( - prepared.block.as_ref(), - prepared.hash, - &prepared.transaction_hashes, + #[instrument(skip(self, contextually_valid), fields(block = %contextually_valid.block))] + fn revert_chain_state_with(&mut self, contextually_valid: &ContextuallyValidBlock) { + let (block, hash, new_outputs, transaction_hashes) = ( + contextually_valid.block.as_ref(), + contextually_valid.hash, + &contextually_valid.new_outputs, + &contextually_valid.transaction_hashes, ); // remove the blocks hash from `height_by_hash` @@ -331,7 +356,7 @@ impl UpdateWith for Chain { ); // remove the utxos this produced - self.revert_chain_state_with(&prepared.new_outputs); + self.revert_chain_state_with(new_outputs); // remove the utxos this consumed self.revert_chain_state_with(inputs); diff --git a/zebra-state/src/service/non_finalized_state/queued_blocks.rs b/zebra-state/src/service/non_finalized_state/queued_blocks.rs index 5fe2d4bd..b60a0d2e 100644 --- a/zebra-state/src/service/non_finalized_state/queued_blocks.rs +++ b/zebra-state/src/service/non_finalized_state/queued_blocks.rs @@ -33,8 +33,9 @@ impl QueuedBlocks { let parent_hash = new.0.block.header.previous_block_hash; // Track known UTXOs in queued blocks. - for (outpoint, output) in new.0.new_outputs.iter() { - self.known_utxos.insert(*outpoint, output.clone()); + for (outpoint, ordered_utxo) in new.0.new_outputs.iter() { + self.known_utxos + .insert(*outpoint, ordered_utxo.utxo.clone()); } let replaced = self.blocks.insert(new_hash, new); diff --git a/zebra-state/src/service/pending_utxos.rs b/zebra-state/src/service/pending_utxos.rs index 6fbb71cf..e7d38007 100644 --- a/zebra-state/src/service/pending_utxos.rs +++ b/zebra-state/src/service/pending_utxos.rs @@ -46,13 +46,20 @@ impl PendingUtxos { } } - /// Check the list of pending UTXO requests against the supplied UTXO index. + /// Check the list of pending UTXO requests against the supplied [`OrderedUtxo`] index. + pub fn check_against_ordered( + &mut self, + ordered_utxos: &HashMap, + ) { + for (outpoint, ordered_utxo) in ordered_utxos.iter() { + self.respond(outpoint, ordered_utxo.utxo.clone()) + } + } + + /// Check the list of pending UTXO requests against the supplied [`Utxo`] index. pub fn check_against(&mut self, utxos: &HashMap) { for (outpoint, utxo) in utxos.iter() { - if let Some(sender) = self.0.remove(outpoint) { - tracing::trace!(?outpoint, "found pending UTXO"); - let _ = sender.send(utxo.clone()); - } + self.respond(outpoint, utxo.clone()) } } diff --git a/zebra-state/src/tests.rs b/zebra-state/src/tests.rs index 224d454b..b2e78a4d 100644 --- a/zebra-state/src/tests.rs +++ b/zebra-state/src/tests.rs @@ -21,7 +21,7 @@ impl Prepare for Arc { let hash = block.hash(); let height = block.coinbase_height().unwrap(); let transaction_hashes: Vec<_> = block.transactions.iter().map(|tx| tx.hash()).collect(); - let new_outputs = transparent::new_outputs(&block, transaction_hashes.as_slice()); + let new_outputs = transparent::new_ordered_outputs(&block, transaction_hashes.as_slice()); PreparedBlock { block,