From 6fdd02220ee952339eaa5011a9f4f2924673a919 Mon Sep 17 00:00:00 2001 From: Arya Date: Mon, 17 Apr 2023 23:43:39 -0400 Subject: [PATCH] fix(rpc): Omit transactions with transparent coinbase spends that are immature at the next block height from block templates (#6510) * Adds `maturity_height` to VerifiedUnminedTx Filters out transactions that are invalid at next_block_height in getblocktemplate method * Adds unit testing * rustfmt * rejects txs with immature coinbase spends from mempool * Condenses fns for transparent coinbase spend check * Updates calls to VerifiedUnminedTx::new() * Update zebra-chain/src/transparent/utxo.rs * Applies suggestions from code review --- zebra-chain/src/block/arbitrary.rs | 24 ++--- zebra-chain/src/transparent/utxo.rs | 6 ++ zebra-consensus/src/error.rs | 34 ++++++- zebra-consensus/src/transaction.rs | 38 ++++++- zebra-consensus/src/transaction/check.rs | 34 ++++++- zebra-consensus/src/transaction/tests.rs | 98 +++++++++++++++++++ .../src/methods/get_block_template_rpcs.rs | 37 +++---- zebra-state/src/lib.rs | 2 +- zebra-state/src/service.rs | 2 +- zebra-state/src/service/check.rs | 2 + zebra-state/src/service/check/tests/utxo.rs | 15 ++- zebra-state/src/service/check/utxo.rs | 16 +-- 12 files changed, 256 insertions(+), 52 deletions(-) diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs index cc68df2c7..9d6eb1867 100644 --- a/zebra-chain/src/block/arbitrary.rs +++ b/zebra-chain/src/block/arbitrary.rs @@ -373,7 +373,7 @@ impl Arbitrary for Block { pub fn allow_all_transparent_coinbase_spends( _: transparent::OutPoint, _: transparent::CoinbaseSpendRestriction, - _: transparent::OrderedUtxo, + _: &transparent::Utxo, ) -> Result<(), ()> { Ok(()) } @@ -390,7 +390,7 @@ impl Block { /// `generate_valid_commitments` specifies if the generated blocks /// should have valid commitments. This makes it much slower so it's better /// to enable only when needed. - pub fn partial_chain_strategy( + pub fn partial_chain_strategy( mut current: LedgerState, count: usize, check_transparent_coinbase_spend: F, @@ -400,8 +400,8 @@ impl Block { F: Fn( transparent::OutPoint, transparent::CoinbaseSpendRestriction, - transparent::OrderedUtxo, - ) -> Result + &transparent::Utxo, + ) -> Result<(), E> + Copy + 'static, { @@ -558,7 +558,7 @@ impl Block { /// Spends [`transparent::OutPoint`]s from `utxos`, and adds newly created outputs. /// /// If the transaction can't be fixed, returns `None`. -pub fn fix_generated_transaction( +pub fn fix_generated_transaction( mut transaction: Transaction, tx_index_in_block: usize, height: Height, @@ -570,8 +570,8 @@ where F: Fn( transparent::OutPoint, transparent::CoinbaseSpendRestriction, - transparent::OrderedUtxo, - ) -> Result + &transparent::Utxo, + ) -> Result<(), E> + Copy + 'static, { @@ -640,7 +640,7 @@ where /// Modifies `transaction` and updates `spend_restriction` if needed. /// /// If there is no valid output, or many search attempts have failed, returns `None`. -pub fn find_valid_utxo_for_spend( +pub fn find_valid_utxo_for_spend( transaction: &mut Transaction, spend_restriction: &mut CoinbaseSpendRestriction, spend_height: Height, @@ -651,8 +651,8 @@ where F: Fn( transparent::OutPoint, transparent::CoinbaseSpendRestriction, - transparent::OrderedUtxo, - ) -> Result + &transparent::Utxo, + ) -> Result<(), E> + Copy + 'static, { @@ -674,7 +674,7 @@ where if check_transparent_coinbase_spend( *candidate_outpoint, *spend_restriction, - candidate_utxo.clone(), + candidate_utxo.as_ref(), ) .is_ok() { @@ -683,7 +683,7 @@ where && check_transparent_coinbase_spend( *candidate_outpoint, delete_transparent_outputs, - candidate_utxo.clone(), + candidate_utxo.as_ref(), ) .is_ok() { diff --git a/zebra-chain/src/transparent/utxo.rs b/zebra-chain/src/transparent/utxo.rs index f331619ae..db626148e 100644 --- a/zebra-chain/src/transparent/utxo.rs +++ b/zebra-chain/src/transparent/utxo.rs @@ -54,6 +54,12 @@ pub struct OrderedUtxo { pub tx_index_in_block: usize, } +impl AsRef for OrderedUtxo { + fn as_ref(&self) -> &Utxo { + &self.utxo + } +} + impl Utxo { /// Create a new UTXO from its fields. pub fn new(output: transparent::Output, height: block::Height, from_coinbase: bool) -> Utxo { diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index 578f706cb..d089073eb 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -8,7 +8,10 @@ use chrono::{DateTime, Utc}; use thiserror::Error; -use zebra_chain::{amount, block, orchard, sapling, sprout, transparent}; +use zebra_chain::{ + amount, block, orchard, sapling, sprout, + transparent::{self, MIN_TRANSPARENT_COINBASE_MATURITY}, +}; use zebra_state::ValidateContextError; use crate::{block::MAX_BLOCK_SIGOPS, BoxError}; @@ -187,17 +190,42 @@ pub enum TransactionError { #[error("could not validate nullifiers and anchors on best chain")] #[cfg_attr(any(test, feature = "proptest-impl"), proptest(skip))] // This error variant is at least 128 bytes - ValidateNullifiersAndAnchorsError(Box), + ValidateContextError(Box), #[error("could not validate mempool transaction lock time on best chain")] #[cfg_attr(any(test, feature = "proptest-impl"), proptest(skip))] // TODO: turn this into a typed error ValidateMempoolLockTimeError(String), + + #[error( + "immature transparent coinbase spend: \ + attempt to spend {outpoint:?} at {spend_height:?}, \ + but spends are invalid before {min_spend_height:?}, \ + which is {MIN_TRANSPARENT_COINBASE_MATURITY:?} blocks \ + after it was created at {created_height:?}" + )] + #[non_exhaustive] + ImmatureTransparentCoinbaseSpend { + outpoint: transparent::OutPoint, + spend_height: block::Height, + min_spend_height: block::Height, + created_height: block::Height, + }, + + #[error( + "unshielded transparent coinbase spend: {outpoint:?} \ + must be spent in a transaction which only has shielded outputs" + )] + #[non_exhaustive] + UnshieldedTransparentCoinbaseSpend { + outpoint: transparent::OutPoint, + min_spend_height: block::Height, + }, } impl From for TransactionError { fn from(err: ValidateContextError) -> Self { - TransactionError::ValidateNullifiersAndAnchorsError(Box::new(err)) + TransactionError::ValidateContextError(Box::new(err)) } } diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 30516887d..28546485b 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -224,10 +224,7 @@ impl Request { /// Returns true if the request is a mempool request. pub fn is_mempool(&self) -> bool { - match self { - Request::Block { .. } => false, - Request::Mempool { .. } => true, - } + matches!(self, Request::Mempool { .. }) } } @@ -377,6 +374,12 @@ where Self::spent_utxos(tx.clone(), req.known_utxos(), req.is_mempool(), state.clone()); let (spent_utxos, spent_outputs) = load_spent_utxos_fut.await?; + // WONTFIX: Return an error for Request::Block as well to replace this check in + // the state once #2336 has been implemented? + if req.is_mempool() { + Self::check_maturity_height(&req, &spent_utxos)?; + } + let cached_ffi_transaction = Arc::new(CachedFfiTransaction::new(tx.clone(), spent_outputs)); @@ -420,7 +423,10 @@ where unmined_tx, )) .map(|res| { - assert!(res? == zs::Response::ValidBestChainTipNullifiersAndAnchors, "unexpected response to CheckBestChainTipNullifiersAndAnchors request"); + assert!( + res? == zs::Response::ValidBestChainTipNullifiersAndAnchors, + "unexpected response to CheckBestChainTipNullifiersAndAnchors request" + ); Ok(()) } ); @@ -567,6 +573,28 @@ where Ok((spent_utxos, spent_outputs)) } + /// Accepts `request`, a transaction verifier [`&Request`](Request), + /// and `spent_utxos`, a HashMap of UTXOs in the chain that are spent by this transaction. + /// + /// Gets the `transaction`, `height`, and `known_utxos` for the request and checks calls + /// [`check::tx_transparent_coinbase_spends_maturity`] to verify that every transparent + /// coinbase output spent by the transaction will have matured by `height`. + /// + /// Returns `Ok(())` if every transparent coinbase output spent by the transaction is + /// mature and valid for the request height, or a [`TransactionError`] if the transaction + /// spends transparent coinbase outputs that are immature and invalid for the request height. + pub fn check_maturity_height( + request: &Request, + spent_utxos: &HashMap, + ) -> Result<(), TransactionError> { + check::tx_transparent_coinbase_spends_maturity( + request.transaction(), + request.height(), + request.known_utxos(), + spent_utxos, + ) + } + /// Verify a V4 transaction. /// /// Returns a set of asynchronous checks that must all succeed for the transaction to be diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 30a7943d5..e7ec37be6 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -2,7 +2,12 @@ //! //! Code in this file can freely assume that no pre-V4 transactions are present. -use std::{borrow::Cow, collections::HashSet, convert::TryFrom, hash::Hash}; +use std::{ + borrow::Cow, + collections::{HashMap, HashSet}, + hash::Hash, + sync::Arc, +}; use chrono::{DateTime, Utc}; @@ -13,6 +18,7 @@ use zebra_chain::{ parameters::{Network, NetworkUpgrade}, primitives::zcash_note_encryption, transaction::{LockTime, Transaction}, + transparent, }; use crate::error::TransactionError; @@ -462,3 +468,29 @@ fn validate_expiry_height_mined( Ok(()) } + +/// Accepts a transaction, block height, block UTXOs, and +/// the transaction's spent UTXOs from the chain. +/// +/// Returns `Ok(())` if spent transparent coinbase outputs are +/// valid for the block height, or a [`Err(TransactionError)`](TransactionError) +pub fn tx_transparent_coinbase_spends_maturity( + tx: Arc, + height: Height, + block_new_outputs: Arc>, + spent_utxos: &HashMap, +) -> Result<(), TransactionError> { + for spend in tx.spent_outpoints() { + let utxo = block_new_outputs + .get(&spend) + .map(|ordered_utxo| ordered_utxo.utxo.clone()) + .or_else(|| spent_utxos.get(&spend).cloned()) + .expect("load_spent_utxos_fut.await should return an error if a utxo is missing"); + + let spend_restriction = tx.coinbase_spend_restriction(height); + + zebra_state::check::transparent_coinbase_spend(spend, spend_restriction, &utxo)?; + } + + Ok(()) +} diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 039adc132..89fd02767 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -564,6 +564,104 @@ async fn mempool_request_with_past_lock_time_is_accepted() { ); } +/// Tests that calls to the transaction verifier with a mempool request that spends +/// immature coinbase outputs will return an error. +#[tokio::test] +async fn mempool_request_with_immature_spent_is_rejected() { + let _init_guard = zebra_test::init(); + + let mut state: MockService<_, _, _, _> = MockService::build().for_prop_tests(); + let verifier = Verifier::new(Network::Mainnet, state.clone()); + + let height = NetworkUpgrade::Canopy + .activation_height(Network::Mainnet) + .expect("Canopy activation height is specified"); + let fund_height = (height - 1).expect("fake source fund block height is too small"); + let (input, output, known_utxos) = mock_transparent_transfer(fund_height, true, 0); + + // Create a non-coinbase V4 tx with the last valid expiry height. + let tx = Transaction::V4 { + inputs: vec![input], + outputs: vec![output], + lock_time: LockTime::min_lock_time_timestamp(), + expiry_height: height, + joinsplit_data: None, + sapling_shielded_data: None, + }; + + let input_outpoint = match tx.inputs()[0] { + transparent::Input::PrevOut { outpoint, .. } => outpoint, + transparent::Input::Coinbase { .. } => panic!("requires a non-coinbase transaction"), + }; + + let spend_restriction = tx.coinbase_spend_restriction(height); + + let coinbase_spend_height = Height(5); + + let utxo = known_utxos + .get(&input_outpoint) + .map(|utxo| { + let mut utxo = utxo.utxo.clone(); + utxo.height = coinbase_spend_height; + utxo.from_coinbase = true; + utxo + }) + .expect("known_utxos should contain the outpoint"); + + let expected_error = + zebra_state::check::transparent_coinbase_spend(input_outpoint, spend_restriction, &utxo) + .map_err(Box::new) + .map_err(TransactionError::ValidateContextError) + .expect_err("check should fail"); + + tokio::spawn(async move { + state + .expect_request(zebra_state::Request::BestChainNextMedianTimePast) + .await + .expect("verifier should call mock state service with correct request") + .respond(zebra_state::Response::BestChainNextMedianTimePast( + DateTime32::MAX, + )); + + state + .expect_request(zebra_state::Request::UnspentBestChainUtxo(input_outpoint)) + .await + .expect("verifier should call mock state service with correct request") + .respond(zebra_state::Response::UnspentBestChainUtxo( + known_utxos.get(&input_outpoint).map(|utxo| { + let mut utxo = utxo.utxo.clone(); + utxo.height = coinbase_spend_height; + utxo.from_coinbase = true; + utxo + }), + )); + + state + .expect_request_that(|req| { + matches!( + req, + zebra_state::Request::CheckBestChainTipNullifiersAndAnchors(_) + ) + }) + .await + .expect("verifier should call mock state service with correct request") + .respond(zebra_state::Response::ValidBestChainTipNullifiersAndAnchors); + }); + + let verifier_response = verifier + .oneshot(Request::Mempool { + transaction: tx.into(), + height, + }) + .await + .expect_err("verification of transaction with immature spend should fail"); + + assert_eq!( + verifier_response, expected_error, + "expected to fail verification, got: {verifier_response:?}" + ); +} + #[test] fn v5_transaction_with_no_outputs_fails_validation() { let transaction = fake_v5_transactions_for_network( diff --git a/zebra-rpc/src/methods/get_block_template_rpcs.rs b/zebra-rpc/src/methods/get_block_template_rpcs.rs index 34d8cb2f5..5a94be6ce 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs.rs @@ -498,8 +498,13 @@ where // // We always return after 90 minutes on mainnet, even if we have the same response, // because the max time has been reached. - let chain_tip_and_local_time = - fetch_state_tip_and_local_time(state.clone()).await?; + let chain_tip_and_local_time @ zebra_state::GetBlockTemplateChainInfo { + tip_hash, + tip_height, + max_time, + cur_time, + .. + } = fetch_state_tip_and_local_time(state.clone()).await?; // Fetch the mempool data for the block template: // - if the mempool transactions change, we might return from long polling. @@ -512,7 +517,7 @@ where // Optional TODO: // - add a `MempoolChange` type with an `async changed()` method (like `ChainTip`) let Some(mempool_txs) = - fetch_mempool_transactions(mempool.clone(), chain_tip_and_local_time.tip_hash) + fetch_mempool_transactions(mempool.clone(), tip_hash) .await? // If the mempool and state responses are out of sync: // - if we are not long polling, omit mempool transactions from the template, @@ -523,9 +528,9 @@ where // - Long poll ID calculation let server_long_poll_id = LongPollInput::new( - chain_tip_and_local_time.tip_height, - chain_tip_and_local_time.tip_hash, - chain_tip_and_local_time.max_time, + tip_height, + tip_hash, + max_time, mempool_txs.iter().map(|tx| tx.transaction.id), ) .generate_id(); @@ -582,9 +587,7 @@ where // It can also be zero if cur_time was clamped to max_time. In that case, // we want to wait for another change, and ignore this timeout. So we use an // `OptionFuture::None`. - let duration_until_max_time = chain_tip_and_local_time - .max_time - .saturating_duration_since(chain_tip_and_local_time.cur_time); + let duration_until_max_time = max_time.saturating_duration_since(cur_time); let wait_for_max_time: OptionFuture<_> = if duration_until_max_time.seconds() > 0 { Some(tokio::time::sleep(duration_until_max_time.to_std())) } else { @@ -607,8 +610,8 @@ where // This timer elapses every few seconds _elapsed = wait_for_mempool_request => { tracing::info!( - max_time = ?chain_tip_and_local_time.max_time, - cur_time = ?chain_tip_and_local_time.cur_time, + ?max_time, + ?cur_time, ?server_long_poll_id, ?client_long_poll_id, GET_BLOCK_TEMPLATE_MEMPOOL_LONG_POLL_INTERVAL, @@ -621,8 +624,8 @@ where match tip_changed_result { Ok(()) => { tracing::info!( - max_time = ?chain_tip_and_local_time.max_time, - cur_time = ?chain_tip_and_local_time.cur_time, + ?max_time, + ?cur_time, ?server_long_poll_id, ?client_long_poll_id, "returning from long poll because state has changed" @@ -634,8 +637,8 @@ where // it will help with debugging. tracing::info!( ?recv_error, - max_time = ?chain_tip_and_local_time.max_time, - cur_time = ?chain_tip_and_local_time.cur_time, + ?max_time, + ?cur_time, ?server_long_poll_id, ?client_long_poll_id, "returning from long poll due to a state error.\ @@ -657,8 +660,8 @@ where // This log should stay at info when the others go to debug, // it's very rare. tracing::info!( - max_time = ?chain_tip_and_local_time.max_time, - cur_time = ?chain_tip_and_local_time.cur_time, + ?max_time, + ?cur_time, ?server_long_poll_id, ?client_long_poll_id, "returning from long poll because max time was reached" diff --git a/zebra-state/src/lib.rs b/zebra-state/src/lib.rs index 57472e448..53ec2dc3e 100644 --- a/zebra-state/src/lib.rs +++ b/zebra-state/src/lib.rs @@ -36,7 +36,7 @@ pub use request::{FinalizedBlock, HashOrHeight, PreparedBlock, ReadRequest, Requ pub use response::{KnownBlock, MinedTx, ReadResponse, Response}; pub use service::{ chain_tip::{ChainTipChange, LatestChainTip, TipAction}, - init, spawn_init, + check, init, spawn_init, watch_receiver::WatchReceiver, OutputIndex, OutputLocation, TransactionLocation, }; diff --git a/zebra-state/src/service.rs b/zebra-state/src/service.rs index 2f072d8a5..48a841d0c 100644 --- a/zebra-state/src/service.rs +++ b/zebra-state/src/service.rs @@ -69,7 +69,7 @@ pub mod block_iter; pub mod chain_tip; pub mod watch_receiver; -pub(crate) mod check; +pub mod check; pub(crate) mod finalized_state; pub(crate) mod non_finalized_state; diff --git a/zebra-state/src/service/check.rs b/zebra-state/src/service/check.rs index d8e747964..d9db02c15 100644 --- a/zebra-state/src/service/check.rs +++ b/zebra-state/src/service/check.rs @@ -31,6 +31,8 @@ pub(crate) mod difficulty; pub(crate) mod nullifier; pub(crate) mod utxo; +pub use utxo::transparent_coinbase_spend; + #[cfg(test)] mod tests; diff --git a/zebra-state/src/service/check/tests/utxo.rs b/zebra-state/src/service/check/tests/utxo.rs index 364bb0dc3..d35441381 100644 --- a/zebra-state/src/service/check/tests/utxo.rs +++ b/zebra-state/src/service/check/tests/utxo.rs @@ -53,8 +53,13 @@ fn accept_shielded_mature_coinbase_utxo_spend() { }; let result = - check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo.clone()); - assert_eq!(result, Ok(ordered_utxo)); + check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo.as_ref()); + + assert_eq!( + result, + Ok(()), + "mature transparent coinbase spend check should return Ok(())" + ); } /// Check that non-shielded spends of coinbase transparent outputs fail. @@ -75,7 +80,8 @@ fn reject_unshielded_coinbase_utxo_spend() { let spend_restriction = transparent::CoinbaseSpendRestriction::SomeTransparentOutputs; - let result = check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo); + let result = + check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo.as_ref()); assert_eq!(result, Err(UnshieldedTransparentCoinbaseSpend { outpoint })); } @@ -100,7 +106,8 @@ fn reject_immature_coinbase_utxo_spend() { let spend_restriction = transparent::CoinbaseSpendRestriction::OnlyShieldedOutputs { spend_height }; - let result = check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo); + let result = + check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo.as_ref()); assert_eq!( result, Err(ImmatureTransparentCoinbaseSpend { diff --git a/zebra-state/src/service/check/utxo.rs b/zebra-state/src/service/check/utxo.rs index 73f36d05f..220019237 100644 --- a/zebra-state/src/service/check/utxo.rs +++ b/zebra-state/src/service/check/utxo.rs @@ -71,7 +71,7 @@ pub fn transparent_spend( // so we check transparent coinbase maturity and shielding // using known valid UTXOs during non-finalized chain validation. let spend_restriction = transaction.coinbase_spend_restriction(prepared.height); - let utxo = transparent_coinbase_spend(spend, spend_restriction, utxo)?; + transparent_coinbase_spend(spend, spend_restriction, utxo.as_ref())?; // We don't delete the UTXOs until the block is committed, // so we need to check for duplicate spends within the same block. @@ -185,26 +185,26 @@ fn transparent_spend_chain_order( pub fn transparent_coinbase_spend( outpoint: transparent::OutPoint, spend_restriction: transparent::CoinbaseSpendRestriction, - utxo: transparent::OrderedUtxo, -) -> Result { - if !utxo.utxo.from_coinbase { - return Ok(utxo); + utxo: &transparent::Utxo, +) -> Result<(), ValidateContextError> { + if !utxo.from_coinbase { + return Ok(()); } match spend_restriction { OnlyShieldedOutputs { spend_height } => { let min_spend_height = - utxo.utxo.height + MIN_TRANSPARENT_COINBASE_MATURITY.try_into().unwrap(); + utxo.height + MIN_TRANSPARENT_COINBASE_MATURITY.try_into().unwrap(); let min_spend_height = min_spend_height.expect("valid UTXOs have coinbase heights far below Height::MAX"); if spend_height >= min_spend_height { - Ok(utxo) + Ok(()) } else { Err(ImmatureTransparentCoinbaseSpend { outpoint, spend_height, min_spend_height, - created_height: utxo.utxo.height, + created_height: utxo.height, }) } }