From a74d30209f5c0b1f8a7251f68bb8080e4d168e07 Mon Sep 17 00:00:00 2001 From: arya2 Date: Thu, 13 Apr 2023 18:02:34 -0400 Subject: [PATCH 1/8] Adds `maturity_height` to VerifiedUnminedTx Filters out transactions that are invalid at next_block_height in getblocktemplate method --- zebra-chain/src/transaction/unmined.rs | 14 +++ zebra-consensus/src/error.rs | 30 ++++- zebra-consensus/src/transaction.rs | 24 +++- zebra-consensus/src/transaction/check.rs | 111 +++++++++++++++++- .../src/methods/get_block_template_rpcs.rs | 9 +- .../get_block_template.rs | 16 ++- zebra-rpc/src/methods/tests/vectors.rs | 1 + .../components/inbound/tests/fake_peer_set.rs | 4 + .../src/components/mempool/storage/tests.rs | 2 +- .../components/mempool/storage/tests/prop.rs | 8 +- .../mempool/storage/tests/vectors.rs | 2 +- zebrad/src/components/mempool/tests/vector.rs | 2 + 12 files changed, 206 insertions(+), 17 deletions(-) diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index 14390ec0934..4b130c3afbf 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -19,6 +19,7 @@ use std::{fmt, sync::Arc}; use crate::{ amount::{Amount, NonNegative}, + block::Height, serialization::ZcashSerialize, transaction::{ AuthDigest, Hash, @@ -322,6 +323,10 @@ pub struct VerifiedUnminedTx { /// /// [ZIP-317]: https://zips.z.cash/zip-0317#block-production pub fee_weight_ratio: f32, + + /// The earliest/lowest block height at which this transaction's transparent coinbase + /// spends will be valid, if this transaction contains any transparent coinbase spends. + pub maturity_height: Option, } impl fmt::Display for VerifiedUnminedTx { @@ -343,6 +348,7 @@ impl VerifiedUnminedTx { transaction: UnminedTx, miner_fee: Amount, legacy_sigop_count: u64, + maturity_height: Option, ) -> Self { let fee_weight_ratio = zip317::conventional_fee_weight_ratio(&transaction, miner_fee); let unpaid_actions = zip317::unpaid_actions(&transaction, miner_fee); @@ -353,6 +359,7 @@ impl VerifiedUnminedTx { legacy_sigop_count, fee_weight_ratio, unpaid_actions, + maturity_height, } } @@ -363,6 +370,13 @@ impl VerifiedUnminedTx { self.miner_fee >= self.transaction.conventional_fee } + /// Returns `true` if the transaction will be mature at the given block height. + pub fn is_mature_at(&self, height: Height) -> bool { + self.maturity_height + .map(|maturity_height| maturity_height <= height) + .unwrap_or(true) + } + /// The cost in bytes of the transaction, as defined in [ZIP-401]. /// /// A reflection of the work done by the network in processing them (proof diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index 578f706cb0f..080c7cede46 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}; @@ -193,6 +196,31 @@ pub enum TransactionError { #[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 { diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 30516887de1..84e00df3f46 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -224,9 +224,22 @@ impl Request { /// Returns true if the request is a mempool request. pub fn is_mempool(&self) -> bool { + matches!(self, Request::Mempool { .. }) + } + + /// Returns lowest height at which all transparent coinbase spends will be valid, + /// or None if this transaction has no transparent coinbase spends. + pub fn maturity_height(&self, spent_utxos: &HashMap) -> Option { match self { - Request::Block { .. } => false, - Request::Mempool { .. } => true, + // TODO: return an error for Request::Block to replace this check in the state (#2336) + Request::Block { .. } => None, + + Request::Mempool { transaction, height } => check::tx_transparent_coinbase_spends_maturity( + transaction.transaction.clone(), + *height, + Default::default(), + spent_utxos + ), } } } @@ -420,7 +433,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(()) } ); @@ -453,6 +469,7 @@ where } let legacy_sigop_count = cached_ffi_transaction.legacy_sigop_count()?; + let maturity_height = req.maturity_height(&spent_utxos); let rsp = match req { Request::Block { .. } => Response::Block { @@ -467,6 +484,7 @@ where "unexpected mempool coinbase transaction: should have already rejected", ), legacy_sigop_count, + maturity_height, ), }, }; diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 30a7943d509..395196d1636 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -2,7 +2,13 @@ //! //! 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}, + convert::TryFrom, + hash::Hash, + sync::Arc, +}; use chrono::{DateTime, Utc}; @@ -13,6 +19,11 @@ use zebra_chain::{ parameters::{Network, NetworkUpgrade}, primitives::zcash_note_encryption, transaction::{LockTime, Transaction}, + transparent::{self, MIN_TRANSPARENT_COINBASE_MATURITY}, +}; + +use zebra_chain::transparent::CoinbaseSpendRestriction::{ + OnlyShieldedOutputs, SomeTransparentOutputs, }; use crate::error::TransactionError; @@ -462,3 +473,101 @@ fn validate_expiry_height_mined( Ok(()) } + +/// Check that `utxo` is spendable, based on the coinbase `spend_restriction`. +/// +/// # Consensus +/// +/// > A transaction with one or more transparent inputs from coinbase transactions +/// > MUST have no transparent outputs (i.e. tx_out_count MUST be 0). +/// > Inputs from coinbase transactions include Founders’ Reward outputs and +/// > funding stream outputs. +/// +/// > A transaction MUST NOT spend a transparent output of a coinbase transaction +/// > from a block less than 100 blocks prior to the spend. +/// > Note that transparent outputs of coinbase transactions include +/// > Founders’ Reward outputs and transparent funding stream outputs. +/// +/// +fn transparent_coinbase_spend_maturity( + outpoint: transparent::OutPoint, + spend_restriction: transparent::CoinbaseSpendRestriction, + utxo: transparent::Utxo, +) -> Result<(), TransactionError> { + if !utxo.from_coinbase { + return Ok(()); + } + + let min_spend_height = 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"); + + match spend_restriction { + OnlyShieldedOutputs { spend_height } => { + if spend_height >= min_spend_height { + Ok(()) + } else { + Err(TransactionError::ImmatureTransparentCoinbaseSpend { + outpoint, + spend_height, + min_spend_height, + created_height: utxo.height, + }) + } + } + SomeTransparentOutputs => Err(TransactionError::UnshieldedTransparentCoinbaseSpend { + outpoint, + min_spend_height, + }), + } +} + +/// Accepts a transaction, UTXOs in the same block, and spent UTXOs from the chain. +/// +/// Returns the lowest/earlier height at which every transparent coinbase spend +/// in the provided [`Transaction`] will be mature, +/// +/// Returns None if the transaction has no transparent coinbase spends. +pub fn tx_transparent_coinbase_spends_maturity( + tx: Arc, + height: Height, + block_new_outputs: Arc>, + spent_utxos: &HashMap, +) -> Option { + let mut maturity_height = None; + + 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); + + let check_coinbase_spend_result = + transparent_coinbase_spend_maturity(spend, spend_restriction, utxo); + + match check_coinbase_spend_result { + Err(TransactionError::ImmatureTransparentCoinbaseSpend { + min_spend_height, .. + }) + | Err(TransactionError::UnshieldedTransparentCoinbaseSpend { + min_spend_height, .. + }) if maturity_height + .map(|h| h < min_spend_height) + .unwrap_or(true) => + { + maturity_height = Some(min_spend_height); + } + + Err(TransactionError::ImmatureTransparentCoinbaseSpend { .. }) + | Err(TransactionError::UnshieldedTransparentCoinbaseSpend { .. }) + | Ok(()) => {} + + Err(_other) => panic!("unexpected error from transparent_coinbase_spend_maturity"), + }; + } + + maturity_height +} diff --git a/zebra-rpc/src/methods/get_block_template_rpcs.rs b/zebra-rpc/src/methods/get_block_template_rpcs.rs index 34d8cb2f596..6629ab9e46f 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs.rs @@ -498,8 +498,11 @@ 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, + .. + } = 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 +515,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, tip_height)) .await? // If the mempool and state responses are out of sync: // - if we are not long polling, omit mempool transactions from the template, diff --git a/zebra-rpc/src/methods/get_block_template_rpcs/get_block_template.rs b/zebra-rpc/src/methods/get_block_template_rpcs/get_block_template.rs index 8439808fe70..794f77a9eab 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs/get_block_template.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs/get_block_template.rs @@ -245,7 +245,7 @@ where /// If the mempool is inactive because Zebra is not synced to the tip, returns no transactions. pub async fn fetch_mempool_transactions( mempool: Mempool, - chain_tip_hash: block::Hash, + (chain_tip_hash, chain_tip_height): (block::Hash, block::Height), ) -> Result>> where Mempool: Service< @@ -265,14 +265,24 @@ where })?; let mempool::Response::FullTransactions { - transactions, + mut transactions, last_seen_tip_hash, } = response else { unreachable!("unmatched response to a mempool::FullTransactions request") }; // Check that the mempool and state were in sync when we made the requests - Ok((last_seen_tip_hash == chain_tip_hash).then_some(transactions)) + let valid_transactions = if last_seen_tip_hash == chain_tip_hash { + let next_block_height = (chain_tip_height + 1).expect("tip is far below Height::MAX"); + + // Filter out that mempool transactions' transparent coinbase spends will be mature at the next block height + transactions.retain(|tx| tx.is_mature_at(next_block_height)); + Some(transactions) + } else { + None + }; + + Ok(valid_transactions) } // - Response processing diff --git a/zebra-rpc/src/methods/tests/vectors.rs b/zebra-rpc/src/methods/tests/vectors.rs index c2f2c0c258d..0f9b1d18a95 100644 --- a/zebra-rpc/src/methods/tests/vectors.rs +++ b/zebra-rpc/src/methods/tests/vectors.rs @@ -1428,6 +1428,7 @@ async fn rpc_getblocktemplate_mining_address(use_p2pkh: bool) { legacy_sigop_count: 0, unpaid_actions: 0, fee_weight_ratio: 1.0, + maturity_height: None, }; let next_fake_tip_hash = diff --git a/zebrad/src/components/inbound/tests/fake_peer_set.rs b/zebrad/src/components/inbound/tests/fake_peer_set.rs index 9a9b797c027..c349c07654c 100644 --- a/zebrad/src/components/inbound/tests/fake_peer_set.rs +++ b/zebrad/src/components/inbound/tests/fake_peer_set.rs @@ -168,6 +168,7 @@ async fn mempool_push_transaction() -> Result<(), crate::BoxError> { transaction, Amount::zero(), 0, + None, ))); }); @@ -270,6 +271,7 @@ async fn mempool_advertise_transaction_ids() -> Result<(), crate::BoxError> { transaction, Amount::zero(), 0, + None, ))); }); @@ -369,6 +371,7 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> { transaction, Amount::zero(), 0, + None, ))); }); @@ -503,6 +506,7 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> { transaction, Amount::zero(), 0, + None, ))); }); diff --git a/zebrad/src/components/mempool/storage/tests.rs b/zebrad/src/components/mempool/storage/tests.rs index 8fdf5431a39..b4e4a32f374 100644 --- a/zebrad/src/components/mempool/storage/tests.rs +++ b/zebrad/src/components/mempool/storage/tests.rs @@ -37,5 +37,5 @@ pub fn unmined_transactions_in_blocks( selected_blocks .flat_map(|block| block.transactions) .map(UnminedTx::from) - .map(|transaction| VerifiedUnminedTx::new(transaction, Amount::zero(), 0)) + .map(|transaction| VerifiedUnminedTx::new(transaction, Amount::zero(), 0, None)) } diff --git a/zebrad/src/components/mempool/storage/tests/prop.rs b/zebrad/src/components/mempool/storage/tests/prop.rs index 9b3a447c9e0..e089442783b 100644 --- a/zebrad/src/components/mempool/storage/tests/prop.rs +++ b/zebrad/src/components/mempool/storage/tests/prop.rs @@ -475,8 +475,8 @@ impl SpendConflictTestInput { }; ( - VerifiedUnminedTx::new(first.0.into(), Amount::zero(), 0), - VerifiedUnminedTx::new(second.0.into(), Amount::zero(), 0), + VerifiedUnminedTx::new(first.0.into(), Amount::zero(), 0, None), + VerifiedUnminedTx::new(second.0.into(), Amount::zero(), 0, None), ) } @@ -493,8 +493,8 @@ impl SpendConflictTestInput { Self::remove_orchard_conflicts(&mut first, &mut second); ( - VerifiedUnminedTx::new(first.0.into(), Amount::zero(), 0), - VerifiedUnminedTx::new(second.0.into(), Amount::zero(), 0), + VerifiedUnminedTx::new(first.0.into(), Amount::zero(), 0, None), + VerifiedUnminedTx::new(second.0.into(), Amount::zero(), 0, None), ) } diff --git a/zebrad/src/components/mempool/storage/tests/vectors.rs b/zebrad/src/components/mempool/storage/tests/vectors.rs index 5977662f19b..7ce39e5b608 100644 --- a/zebrad/src/components/mempool/storage/tests/vectors.rs +++ b/zebrad/src/components/mempool/storage/tests/vectors.rs @@ -271,7 +271,7 @@ fn mempool_expired_basic_for_network(network: Network) -> Result<()> { let tx_id = tx.unmined_id(); // Insert the transaction into the mempool, with a fake zero miner fee and sigops - storage.insert(VerifiedUnminedTx::new(tx.into(), Amount::zero(), 0))?; + storage.insert(VerifiedUnminedTx::new(tx.into(), Amount::zero(), 0, None))?; assert_eq!(storage.transaction_count(), 1); diff --git a/zebrad/src/components/mempool/tests/vector.rs b/zebrad/src/components/mempool/tests/vector.rs index 3dad0f1a466..afaa2a04ca0 100644 --- a/zebrad/src/components/mempool/tests/vector.rs +++ b/zebrad/src/components/mempool/tests/vector.rs @@ -809,6 +809,7 @@ async fn mempool_reverifies_after_tip_change() -> Result<(), Report> { transaction, Amount::zero(), 0, + None, ))); }) .await; @@ -866,6 +867,7 @@ async fn mempool_reverifies_after_tip_change() -> Result<(), Report> { transaction, Amount::zero(), 0, + None, ))); }) .await; From 3d258dddb5e92cf5d24bcb02f9524741b47aa51b Mon Sep 17 00:00:00 2001 From: arya2 Date: Thu, 13 Apr 2023 19:48:51 -0400 Subject: [PATCH 2/8] Adds unit testing --- zebra-consensus/src/transaction/check.rs | 2 + zebra-consensus/src/transaction/tests.rs | 83 ++++++++++++- zebra-rpc/src/methods/tests/vectors.rs | 143 ++++++++++++++--------- 3 files changed, 170 insertions(+), 58 deletions(-) diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 395196d1636..30bf46c99d0 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -489,6 +489,8 @@ fn validate_expiry_height_mined( /// > Founders’ Reward outputs and transparent funding stream outputs. /// /// +// TODO: Move tests for `zebra_state::service::check::utxo::transparent_coinbase_spend()` to this crate +// once the transaction verifier returns an error for block transactions that fail this check. fn transparent_coinbase_spend_maturity( outpoint: transparent::OutPoint, spend_restriction: transparent::CoinbaseSpendRestriction, diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 039adc132db..4998e97517d 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -25,7 +25,7 @@ use zebra_chain::{ }, Hash, HashType, JoinSplitData, LockTime, Transaction, }, - transparent::{self, CoinbaseData}, + transparent::{self, CoinbaseData, MIN_TRANSPARENT_COINBASE_MATURITY}, }; use zebra_test::mock_service::MockService; @@ -564,6 +564,87 @@ async fn mempool_request_with_past_lock_time_is_accepted() { ); } +#[tokio::test] +async fn mempool_response_maturity_height_correct() { + 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 coinbase_spend_height = Height(5); + + 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; + + let Ok(super::Response::Mempool { transaction }) = verifier_response else { + panic!("expected successful verification, got: {verifier_response:?}"); + }; + + let expected_maturity_height = + coinbase_spend_height + MIN_TRANSPARENT_COINBASE_MATURITY.try_into().unwrap(); + + assert_eq!(transaction.maturity_height, expected_maturity_height, "maturity_height should be MIN_TRANSPARENT_COINBASE_MATURITY ahead the spent coinbase tx height") +} + #[test] fn v5_transaction_with_no_outputs_fails_validation() { let transaction = fake_v5_transactions_for_network( diff --git a/zebra-rpc/src/methods/tests/vectors.rs b/zebra-rpc/src/methods/tests/vectors.rs index 0f9b1d18a95..b119ac03e1b 100644 --- a/zebra-rpc/src/methods/tests/vectors.rs +++ b/zebra-rpc/src/methods/tests/vectors.rs @@ -1271,66 +1271,95 @@ async fn rpc_getblocktemplate_mining_address(use_p2pkh: bool) { } }; - let get_block_template_fut = get_block_template_rpc.get_block_template(None); - let (get_block_template, ..) = tokio::join!( - get_block_template_fut, - make_mock_mempool_request_handler(vec![], fake_tip_hash), - make_mock_read_state_request_handler(), - ); - - let get_block_template::Response::TemplateMode(get_block_template) = get_block_template - .expect("unexpected error in getblocktemplate RPC call") else { - panic!("this getblocktemplate call without parameters should return the `TemplateMode` variant of the response") - }; + // get a block with a non-coinbase transaction + let block: Block = zebra_test::vectors::BLOCK_MAINNET_982681_BYTES + .zcash_deserialize_into() + .unwrap(); - assert_eq!( - get_block_template.capabilities, - GET_BLOCK_TEMPLATE_CAPABILITIES_FIELD.to_vec() - ); - assert_eq!(get_block_template.version, ZCASH_BLOCK_VERSION); - assert!(get_block_template.transactions.is_empty()); - assert_eq!( - get_block_template.target, - ExpandedDifficulty::from_hex( - "0000000000000000000000000000000000000000000000000000000000000001" + // use first non-coinbase transaction + let unmined_transaction: UnminedTx = block + .transactions + .into_iter() + .find(|tx| !tx.is_coinbase()) + .expect("at least one non-coinbase transaction") + .into(); + + let make_fake_mempool_tx = |maturity_height| { + VerifiedUnminedTx::new( + unmined_transaction.clone(), + Amount::zero(), + 0, + maturity_height, ) - .expect("test vector is valid") - ); - assert_eq!(get_block_template.min_time, fake_min_time); - assert_eq!( - get_block_template.mutable, - GET_BLOCK_TEMPLATE_MUTABLE_FIELD.to_vec() - ); - assert_eq!( - get_block_template.nonce_range, - GET_BLOCK_TEMPLATE_NONCE_RANGE_FIELD - ); - assert_eq!(get_block_template.sigop_limit, MAX_BLOCK_SIGOPS); - assert_eq!(get_block_template.size_limit, MAX_BLOCK_BYTES); - assert_eq!(get_block_template.cur_time, fake_cur_time); - assert_eq!( - get_block_template.bits, - CompactDifficulty::from_hex("01010000").expect("test vector is valid") - ); - assert_eq!(get_block_template.height, 1687105); // nu5 height - assert_eq!(get_block_template.max_time, fake_max_time); - - // Coinbase transaction checks. - assert!(get_block_template.coinbase_txn.required); - assert!(!get_block_template.coinbase_txn.data.as_ref().is_empty()); - assert_eq!(get_block_template.coinbase_txn.depends.len(), 0); - if use_p2pkh { - // there is one sig operation if miner address is p2pkh. - assert_eq!(get_block_template.coinbase_txn.sigops, 1); - } else { - // everything in the coinbase is p2sh. - assert_eq!(get_block_template.coinbase_txn.sigops, 0); + }; + + let fake_mempool_txs = vec![ + make_fake_mempool_tx(None), + make_fake_mempool_tx(Some(Height::MAX)), + ]; + + for (num_expected_txs, mempool_txs) in [(0, vec![]), (1, fake_mempool_txs)] { + let get_block_template_fut = get_block_template_rpc.get_block_template(None); + let (get_block_template, ..) = tokio::join!( + get_block_template_fut, + make_mock_mempool_request_handler(mempool_txs, fake_tip_hash), + make_mock_read_state_request_handler(), + ); + + let get_block_template::Response::TemplateMode(get_block_template) = get_block_template + .expect("unexpected error in getblocktemplate RPC call") else { + panic!("this getblocktemplate call without parameters should return the `TemplateMode` variant of the response") + }; + + assert_eq!( + get_block_template.capabilities, + GET_BLOCK_TEMPLATE_CAPABILITIES_FIELD.to_vec() + ); + assert_eq!(get_block_template.version, ZCASH_BLOCK_VERSION); + assert_eq!(get_block_template.transactions.len(), num_expected_txs); + assert_eq!( + get_block_template.target, + ExpandedDifficulty::from_hex( + "0000000000000000000000000000000000000000000000000000000000000001" + ) + .expect("test vector is valid") + ); + assert_eq!(get_block_template.min_time, fake_min_time); + assert_eq!( + get_block_template.mutable, + GET_BLOCK_TEMPLATE_MUTABLE_FIELD.to_vec() + ); + assert_eq!( + get_block_template.nonce_range, + GET_BLOCK_TEMPLATE_NONCE_RANGE_FIELD + ); + assert_eq!(get_block_template.sigop_limit, MAX_BLOCK_SIGOPS); + assert_eq!(get_block_template.size_limit, MAX_BLOCK_BYTES); + assert_eq!(get_block_template.cur_time, fake_cur_time); + assert_eq!( + get_block_template.bits, + CompactDifficulty::from_hex("01010000").expect("test vector is valid") + ); + assert_eq!(get_block_template.height, 1687105); // nu5 height + assert_eq!(get_block_template.max_time, fake_max_time); + + // Coinbase transaction checks. + assert!(get_block_template.coinbase_txn.required); + assert!(!get_block_template.coinbase_txn.data.as_ref().is_empty()); + assert_eq!(get_block_template.coinbase_txn.depends.len(), 0); + if use_p2pkh { + // there is one sig operation if miner address is p2pkh. + assert_eq!(get_block_template.coinbase_txn.sigops, 1); + } else { + // everything in the coinbase is p2sh. + assert_eq!(get_block_template.coinbase_txn.sigops, 0); + } + // Coinbase transaction checks for empty blocks. + assert_eq!( + get_block_template.coinbase_txn.fee, + Amount::::zero() + ); } - // Coinbase transaction checks for empty blocks. - assert_eq!( - get_block_template.coinbase_txn.fee, - Amount::::zero() - ); mock_chain_tip_sender.send_estimated_distance_to_network_chain_tip(Some(200)); let get_block_template_sync_error = get_block_template_rpc From 9b340a24277cd3c6c3701cea4dd823acc06e9915 Mon Sep 17 00:00:00 2001 From: arya2 Date: Thu, 13 Apr 2023 20:10:17 -0400 Subject: [PATCH 3/8] rustfmt --- zebra-consensus/src/transaction.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 84e00df3f46..2b89d26fd39 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -229,16 +229,22 @@ impl Request { /// Returns lowest height at which all transparent coinbase spends will be valid, /// or None if this transaction has no transparent coinbase spends. - pub fn maturity_height(&self, spent_utxos: &HashMap) -> Option { + pub fn maturity_height( + &self, + spent_utxos: &HashMap, + ) -> Option { match self { // TODO: return an error for Request::Block to replace this check in the state (#2336) Request::Block { .. } => None, - Request::Mempool { transaction, height } => check::tx_transparent_coinbase_spends_maturity( + Request::Mempool { + transaction, + height, + } => check::tx_transparent_coinbase_spends_maturity( transaction.transaction.clone(), *height, Default::default(), - spent_utxos + spent_utxos, ), } } @@ -434,7 +440,7 @@ where )) .map(|res| { assert!( - res? == zs::Response::ValidBestChainTipNullifiersAndAnchors, + res? == zs::Response::ValidBestChainTipNullifiersAndAnchors, "unexpected response to CheckBestChainTipNullifiersAndAnchors request" ); Ok(()) From d8c8160d24b9765f6c45246655723b8bae1ef8da Mon Sep 17 00:00:00 2001 From: arya2 Date: Fri, 14 Apr 2023 14:17:06 -0400 Subject: [PATCH 4/8] rejects txs with immature coinbase spends from mempool --- zebra-chain/src/transaction/unmined.rs | 14 -- zebra-consensus/src/transaction.rs | 32 ++-- zebra-consensus/src/transaction/check.rs | 29 +--- zebra-consensus/src/transaction/tests.rs | 14 +- .../src/methods/get_block_template_rpcs.rs | 30 ++-- .../get_block_template.rs | 17 +-- zebra-rpc/src/methods/tests/vectors.rs | 144 +++++++----------- 7 files changed, 97 insertions(+), 183 deletions(-) diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index 4b130c3afbf..14390ec0934 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -19,7 +19,6 @@ use std::{fmt, sync::Arc}; use crate::{ amount::{Amount, NonNegative}, - block::Height, serialization::ZcashSerialize, transaction::{ AuthDigest, Hash, @@ -323,10 +322,6 @@ pub struct VerifiedUnminedTx { /// /// [ZIP-317]: https://zips.z.cash/zip-0317#block-production pub fee_weight_ratio: f32, - - /// The earliest/lowest block height at which this transaction's transparent coinbase - /// spends will be valid, if this transaction contains any transparent coinbase spends. - pub maturity_height: Option, } impl fmt::Display for VerifiedUnminedTx { @@ -348,7 +343,6 @@ impl VerifiedUnminedTx { transaction: UnminedTx, miner_fee: Amount, legacy_sigop_count: u64, - maturity_height: Option, ) -> Self { let fee_weight_ratio = zip317::conventional_fee_weight_ratio(&transaction, miner_fee); let unpaid_actions = zip317::unpaid_actions(&transaction, miner_fee); @@ -359,7 +353,6 @@ impl VerifiedUnminedTx { legacy_sigop_count, fee_weight_ratio, unpaid_actions, - maturity_height, } } @@ -370,13 +363,6 @@ impl VerifiedUnminedTx { self.miner_fee >= self.transaction.conventional_fee } - /// Returns `true` if the transaction will be mature at the given block height. - pub fn is_mature_at(&self, height: Height) -> bool { - self.maturity_height - .map(|maturity_height| maturity_height <= height) - .unwrap_or(true) - } - /// The cost in bytes of the transaction, as defined in [ZIP-401]. /// /// A reflection of the work done by the network in processing them (proof diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 2b89d26fd39..aaf63c304d2 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -229,24 +229,16 @@ impl Request { /// Returns lowest height at which all transparent coinbase spends will be valid, /// or None if this transaction has no transparent coinbase spends. - pub fn maturity_height( + pub fn check_maturity_height( &self, spent_utxos: &HashMap, - ) -> Option { - match self { - // TODO: return an error for Request::Block to replace this check in the state (#2336) - Request::Block { .. } => None, - - Request::Mempool { - transaction, - height, - } => check::tx_transparent_coinbase_spends_maturity( - transaction.transaction.clone(), - *height, - Default::default(), - spent_utxos, - ), - } + ) -> Result<(), TransactionError> { + check::tx_transparent_coinbase_spends_maturity( + self.transaction(), + self.height(), + self.known_utxos(), + spent_utxos, + ) } } @@ -475,7 +467,12 @@ where } let legacy_sigop_count = cached_ffi_transaction.legacy_sigop_count()?; - let maturity_height = req.maturity_height(&spent_utxos); + + // TODO: Return an error for Request::Block as well to replace this check in + // the state once #2336 has been implemented? + if req.is_mempool() { + req.check_maturity_height(&spent_utxos)?; + } let rsp = match req { Request::Block { .. } => Response::Block { @@ -490,7 +487,6 @@ where "unexpected mempool coinbase transaction: should have already rejected", ), legacy_sigop_count, - maturity_height, ), }, }; diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 30bf46c99d0..a55863a73b6 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -535,9 +535,7 @@ pub fn tx_transparent_coinbase_spends_maturity( height: Height, block_new_outputs: Arc>, spent_utxos: &HashMap, -) -> Option { - let mut maturity_height = None; - +) -> Result<(), TransactionError> { for spend in tx.spent_outpoints() { let utxo = block_new_outputs .get(&spend) @@ -547,29 +545,8 @@ pub fn tx_transparent_coinbase_spends_maturity( let spend_restriction = tx.coinbase_spend_restriction(height); - let check_coinbase_spend_result = - transparent_coinbase_spend_maturity(spend, spend_restriction, utxo); - - match check_coinbase_spend_result { - Err(TransactionError::ImmatureTransparentCoinbaseSpend { - min_spend_height, .. - }) - | Err(TransactionError::UnshieldedTransparentCoinbaseSpend { - min_spend_height, .. - }) if maturity_height - .map(|h| h < min_spend_height) - .unwrap_or(true) => - { - maturity_height = Some(min_spend_height); - } - - Err(TransactionError::ImmatureTransparentCoinbaseSpend { .. }) - | Err(TransactionError::UnshieldedTransparentCoinbaseSpend { .. }) - | Ok(()) => {} - - Err(_other) => panic!("unexpected error from transparent_coinbase_spend_maturity"), - }; + transparent_coinbase_spend_maturity(spend, spend_restriction, utxo)?; } - maturity_height + Ok(()) } diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 4998e97517d..7181174cba9 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -25,7 +25,7 @@ use zebra_chain::{ }, Hash, HashType, JoinSplitData, LockTime, Transaction, }, - transparent::{self, CoinbaseData, MIN_TRANSPARENT_COINBASE_MATURITY}, + transparent::{self, CoinbaseData}, }; use zebra_test::mock_service::MockService; @@ -635,14 +635,10 @@ async fn mempool_response_maturity_height_correct() { }) .await; - let Ok(super::Response::Mempool { transaction }) = verifier_response else { - panic!("expected successful verification, got: {verifier_response:?}"); - }; - - let expected_maturity_height = - coinbase_spend_height + MIN_TRANSPARENT_COINBASE_MATURITY.try_into().unwrap(); - - assert_eq!(transaction.maturity_height, expected_maturity_height, "maturity_height should be MIN_TRANSPARENT_COINBASE_MATURITY ahead the spent coinbase tx height") + assert!( + verifier_response.is_err(), + "expected to fail verification, got: {verifier_response:?}" + ); } #[test] diff --git a/zebra-rpc/src/methods/get_block_template_rpcs.rs b/zebra-rpc/src/methods/get_block_template_rpcs.rs index 6629ab9e46f..5a94be6ceb0 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs.rs @@ -501,6 +501,8 @@ where 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?; @@ -515,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(), (tip_hash, tip_height)) + 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, @@ -526,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(); @@ -585,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 { @@ -610,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, @@ -624,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" @@ -637,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.\ @@ -660,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-rpc/src/methods/get_block_template_rpcs/get_block_template.rs b/zebra-rpc/src/methods/get_block_template_rpcs/get_block_template.rs index 794f77a9eab..5e4a95df5a5 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs/get_block_template.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs/get_block_template.rs @@ -245,7 +245,7 @@ where /// If the mempool is inactive because Zebra is not synced to the tip, returns no transactions. pub async fn fetch_mempool_transactions( mempool: Mempool, - (chain_tip_hash, chain_tip_height): (block::Hash, block::Height), + chain_tip_hash: block::Hash, ) -> Result>> where Mempool: Service< @@ -265,24 +265,13 @@ where })?; let mempool::Response::FullTransactions { - mut transactions, + transactions, last_seen_tip_hash, } = response else { unreachable!("unmatched response to a mempool::FullTransactions request") }; - // Check that the mempool and state were in sync when we made the requests - let valid_transactions = if last_seen_tip_hash == chain_tip_hash { - let next_block_height = (chain_tip_height + 1).expect("tip is far below Height::MAX"); - - // Filter out that mempool transactions' transparent coinbase spends will be mature at the next block height - transactions.retain(|tx| tx.is_mature_at(next_block_height)); - Some(transactions) - } else { - None - }; - - Ok(valid_transactions) + Ok((last_seen_tip_hash == chain_tip_hash).then_some(transactions)) } // - Response processing diff --git a/zebra-rpc/src/methods/tests/vectors.rs b/zebra-rpc/src/methods/tests/vectors.rs index b119ac03e1b..c2f2c0c258d 100644 --- a/zebra-rpc/src/methods/tests/vectors.rs +++ b/zebra-rpc/src/methods/tests/vectors.rs @@ -1271,95 +1271,66 @@ async fn rpc_getblocktemplate_mining_address(use_p2pkh: bool) { } }; - // get a block with a non-coinbase transaction - let block: Block = zebra_test::vectors::BLOCK_MAINNET_982681_BYTES - .zcash_deserialize_into() - .unwrap(); - - // use first non-coinbase transaction - let unmined_transaction: UnminedTx = block - .transactions - .into_iter() - .find(|tx| !tx.is_coinbase()) - .expect("at least one non-coinbase transaction") - .into(); - - let make_fake_mempool_tx = |maturity_height| { - VerifiedUnminedTx::new( - unmined_transaction.clone(), - Amount::zero(), - 0, - maturity_height, - ) - }; - - let fake_mempool_txs = vec![ - make_fake_mempool_tx(None), - make_fake_mempool_tx(Some(Height::MAX)), - ]; - - for (num_expected_txs, mempool_txs) in [(0, vec![]), (1, fake_mempool_txs)] { - let get_block_template_fut = get_block_template_rpc.get_block_template(None); - let (get_block_template, ..) = tokio::join!( - get_block_template_fut, - make_mock_mempool_request_handler(mempool_txs, fake_tip_hash), - make_mock_read_state_request_handler(), - ); + let get_block_template_fut = get_block_template_rpc.get_block_template(None); + let (get_block_template, ..) = tokio::join!( + get_block_template_fut, + make_mock_mempool_request_handler(vec![], fake_tip_hash), + make_mock_read_state_request_handler(), + ); - let get_block_template::Response::TemplateMode(get_block_template) = get_block_template - .expect("unexpected error in getblocktemplate RPC call") else { - panic!("this getblocktemplate call without parameters should return the `TemplateMode` variant of the response") - }; + let get_block_template::Response::TemplateMode(get_block_template) = get_block_template + .expect("unexpected error in getblocktemplate RPC call") else { + panic!("this getblocktemplate call without parameters should return the `TemplateMode` variant of the response") + }; - assert_eq!( - get_block_template.capabilities, - GET_BLOCK_TEMPLATE_CAPABILITIES_FIELD.to_vec() - ); - assert_eq!(get_block_template.version, ZCASH_BLOCK_VERSION); - assert_eq!(get_block_template.transactions.len(), num_expected_txs); - assert_eq!( - get_block_template.target, - ExpandedDifficulty::from_hex( - "0000000000000000000000000000000000000000000000000000000000000001" - ) - .expect("test vector is valid") - ); - assert_eq!(get_block_template.min_time, fake_min_time); - assert_eq!( - get_block_template.mutable, - GET_BLOCK_TEMPLATE_MUTABLE_FIELD.to_vec() - ); - assert_eq!( - get_block_template.nonce_range, - GET_BLOCK_TEMPLATE_NONCE_RANGE_FIELD - ); - assert_eq!(get_block_template.sigop_limit, MAX_BLOCK_SIGOPS); - assert_eq!(get_block_template.size_limit, MAX_BLOCK_BYTES); - assert_eq!(get_block_template.cur_time, fake_cur_time); - assert_eq!( - get_block_template.bits, - CompactDifficulty::from_hex("01010000").expect("test vector is valid") - ); - assert_eq!(get_block_template.height, 1687105); // nu5 height - assert_eq!(get_block_template.max_time, fake_max_time); - - // Coinbase transaction checks. - assert!(get_block_template.coinbase_txn.required); - assert!(!get_block_template.coinbase_txn.data.as_ref().is_empty()); - assert_eq!(get_block_template.coinbase_txn.depends.len(), 0); - if use_p2pkh { - // there is one sig operation if miner address is p2pkh. - assert_eq!(get_block_template.coinbase_txn.sigops, 1); - } else { - // everything in the coinbase is p2sh. - assert_eq!(get_block_template.coinbase_txn.sigops, 0); - } - // Coinbase transaction checks for empty blocks. - assert_eq!( - get_block_template.coinbase_txn.fee, - Amount::::zero() - ); + assert_eq!( + get_block_template.capabilities, + GET_BLOCK_TEMPLATE_CAPABILITIES_FIELD.to_vec() + ); + assert_eq!(get_block_template.version, ZCASH_BLOCK_VERSION); + assert!(get_block_template.transactions.is_empty()); + assert_eq!( + get_block_template.target, + ExpandedDifficulty::from_hex( + "0000000000000000000000000000000000000000000000000000000000000001" + ) + .expect("test vector is valid") + ); + assert_eq!(get_block_template.min_time, fake_min_time); + assert_eq!( + get_block_template.mutable, + GET_BLOCK_TEMPLATE_MUTABLE_FIELD.to_vec() + ); + assert_eq!( + get_block_template.nonce_range, + GET_BLOCK_TEMPLATE_NONCE_RANGE_FIELD + ); + assert_eq!(get_block_template.sigop_limit, MAX_BLOCK_SIGOPS); + assert_eq!(get_block_template.size_limit, MAX_BLOCK_BYTES); + assert_eq!(get_block_template.cur_time, fake_cur_time); + assert_eq!( + get_block_template.bits, + CompactDifficulty::from_hex("01010000").expect("test vector is valid") + ); + assert_eq!(get_block_template.height, 1687105); // nu5 height + assert_eq!(get_block_template.max_time, fake_max_time); + + // Coinbase transaction checks. + assert!(get_block_template.coinbase_txn.required); + assert!(!get_block_template.coinbase_txn.data.as_ref().is_empty()); + assert_eq!(get_block_template.coinbase_txn.depends.len(), 0); + if use_p2pkh { + // there is one sig operation if miner address is p2pkh. + assert_eq!(get_block_template.coinbase_txn.sigops, 1); + } else { + // everything in the coinbase is p2sh. + assert_eq!(get_block_template.coinbase_txn.sigops, 0); } + // Coinbase transaction checks for empty blocks. + assert_eq!( + get_block_template.coinbase_txn.fee, + Amount::::zero() + ); mock_chain_tip_sender.send_estimated_distance_to_network_chain_tip(Some(200)); let get_block_template_sync_error = get_block_template_rpc @@ -1457,7 +1428,6 @@ async fn rpc_getblocktemplate_mining_address(use_p2pkh: bool) { legacy_sigop_count: 0, unpaid_actions: 0, fee_weight_ratio: 1.0, - maturity_height: None, }; let next_fake_tip_hash = From a45a6f78c849069622c7f0bdba4a01e2d1940219 Mon Sep 17 00:00:00 2001 From: arya2 Date: Fri, 14 Apr 2023 15:15:40 -0400 Subject: [PATCH 5/8] Condenses fns for transparent coinbase spend check --- zebra-chain/src/block/arbitrary.rs | 24 ++++----- zebra-chain/src/transparent/utxo.rs | 18 +++++++ zebra-consensus/src/transaction/check.rs | 58 +-------------------- 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 | 14 +++-- zebra-state/src/service/check/utxo.rs | 16 +++--- 8 files changed, 55 insertions(+), 81 deletions(-) diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs index cc68df2c77e..9d6eb1867fb 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 f331619ae92..b3c3294cee0 100644 --- a/zebra-chain/src/transparent/utxo.rs +++ b/zebra-chain/src/transparent/utxo.rs @@ -54,6 +54,24 @@ pub struct OrderedUtxo { pub tx_index_in_block: usize, } +impl<'a> From<&'a OrderedUtxo> for &'a Utxo { + fn from(ordered_utxo: &'a OrderedUtxo) -> Self { + &ordered_utxo.utxo + } +} + +impl From for Utxo { + fn from(ordered_utxo: OrderedUtxo) -> Self { + ordered_utxo.utxo + } +} + +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/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index a55863a73b6..f37641cc368 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -19,11 +19,7 @@ use zebra_chain::{ parameters::{Network, NetworkUpgrade}, primitives::zcash_note_encryption, transaction::{LockTime, Transaction}, - transparent::{self, MIN_TRANSPARENT_COINBASE_MATURITY}, -}; - -use zebra_chain::transparent::CoinbaseSpendRestriction::{ - OnlyShieldedOutputs, SomeTransparentOutputs, + transparent, }; use crate::error::TransactionError; @@ -474,56 +470,6 @@ fn validate_expiry_height_mined( Ok(()) } -/// Check that `utxo` is spendable, based on the coinbase `spend_restriction`. -/// -/// # Consensus -/// -/// > A transaction with one or more transparent inputs from coinbase transactions -/// > MUST have no transparent outputs (i.e. tx_out_count MUST be 0). -/// > Inputs from coinbase transactions include Founders’ Reward outputs and -/// > funding stream outputs. -/// -/// > A transaction MUST NOT spend a transparent output of a coinbase transaction -/// > from a block less than 100 blocks prior to the spend. -/// > Note that transparent outputs of coinbase transactions include -/// > Founders’ Reward outputs and transparent funding stream outputs. -/// -/// -// TODO: Move tests for `zebra_state::service::check::utxo::transparent_coinbase_spend()` to this crate -// once the transaction verifier returns an error for block transactions that fail this check. -fn transparent_coinbase_spend_maturity( - outpoint: transparent::OutPoint, - spend_restriction: transparent::CoinbaseSpendRestriction, - utxo: transparent::Utxo, -) -> Result<(), TransactionError> { - if !utxo.from_coinbase { - return Ok(()); - } - - let min_spend_height = 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"); - - match spend_restriction { - OnlyShieldedOutputs { spend_height } => { - if spend_height >= min_spend_height { - Ok(()) - } else { - Err(TransactionError::ImmatureTransparentCoinbaseSpend { - outpoint, - spend_height, - min_spend_height, - created_height: utxo.height, - }) - } - } - SomeTransparentOutputs => Err(TransactionError::UnshieldedTransparentCoinbaseSpend { - outpoint, - min_spend_height, - }), - } -} - /// Accepts a transaction, UTXOs in the same block, and spent UTXOs from the chain. /// /// Returns the lowest/earlier height at which every transparent coinbase spend @@ -545,7 +491,7 @@ pub fn tx_transparent_coinbase_spends_maturity( let spend_restriction = tx.coinbase_spend_restriction(height); - transparent_coinbase_spend_maturity(spend, spend_restriction, utxo)?; + zebra_state::transparent_coinbase_spend(spend, spend_restriction, &utxo)?; } Ok(()) diff --git a/zebra-state/src/lib.rs b/zebra-state/src/lib.rs index 57472e4484e..18a7c7b5bcd 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, + init, spawn_init, transparent_coinbase_spend, watch_receiver::WatchReceiver, OutputIndex, OutputLocation, TransactionLocation, }; diff --git a/zebra-state/src/service.rs b/zebra-state/src/service.rs index ea4a63b6aa5..8a969677ecc 100644 --- a/zebra-state/src/service.rs +++ b/zebra-state/src/service.rs @@ -71,6 +71,8 @@ pub mod watch_receiver; pub(crate) mod check; +pub use check::transparent_coinbase_spend; + pub(crate) mod finalized_state; pub(crate) mod non_finalized_state; mod pending_utxos; diff --git a/zebra-state/src/service/check.rs b/zebra-state/src/service/check.rs index d8e747964e4..d9db02c154b 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 364bb0dc360..81bc8c4f2d6 100644 --- a/zebra-state/src/service/check/tests/utxo.rs +++ b/zebra-state/src/service/check/tests/utxo.rs @@ -53,8 +53,12 @@ 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!( + result.is_ok(), + "transparent coinbase spend check should return Ok" + ); } /// Check that non-shielded spends of coinbase transparent outputs fail. @@ -75,7 +79,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 +105,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 d29e5bca078..f3ef290acc5 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. @@ -191,26 +191,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, }) } } From ef8db4e516323c24706e42d2b5c6f41e5978c6b0 Mon Sep 17 00:00:00 2001 From: arya2 Date: Fri, 14 Apr 2023 15:21:50 -0400 Subject: [PATCH 6/8] Updates calls to VerifiedUnminedTx::new() --- zebrad/src/components/inbound/tests/fake_peer_set.rs | 4 ---- zebrad/src/components/mempool/storage/tests.rs | 2 +- zebrad/src/components/mempool/storage/tests/prop.rs | 8 ++++---- zebrad/src/components/mempool/storage/tests/vectors.rs | 2 +- zebrad/src/components/mempool/tests/vector.rs | 2 -- 5 files changed, 6 insertions(+), 12 deletions(-) diff --git a/zebrad/src/components/inbound/tests/fake_peer_set.rs b/zebrad/src/components/inbound/tests/fake_peer_set.rs index c349c07654c..9a9b797c027 100644 --- a/zebrad/src/components/inbound/tests/fake_peer_set.rs +++ b/zebrad/src/components/inbound/tests/fake_peer_set.rs @@ -168,7 +168,6 @@ async fn mempool_push_transaction() -> Result<(), crate::BoxError> { transaction, Amount::zero(), 0, - None, ))); }); @@ -271,7 +270,6 @@ async fn mempool_advertise_transaction_ids() -> Result<(), crate::BoxError> { transaction, Amount::zero(), 0, - None, ))); }); @@ -371,7 +369,6 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> { transaction, Amount::zero(), 0, - None, ))); }); @@ -506,7 +503,6 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> { transaction, Amount::zero(), 0, - None, ))); }); diff --git a/zebrad/src/components/mempool/storage/tests.rs b/zebrad/src/components/mempool/storage/tests.rs index b4e4a32f374..8fdf5431a39 100644 --- a/zebrad/src/components/mempool/storage/tests.rs +++ b/zebrad/src/components/mempool/storage/tests.rs @@ -37,5 +37,5 @@ pub fn unmined_transactions_in_blocks( selected_blocks .flat_map(|block| block.transactions) .map(UnminedTx::from) - .map(|transaction| VerifiedUnminedTx::new(transaction, Amount::zero(), 0, None)) + .map(|transaction| VerifiedUnminedTx::new(transaction, Amount::zero(), 0)) } diff --git a/zebrad/src/components/mempool/storage/tests/prop.rs b/zebrad/src/components/mempool/storage/tests/prop.rs index e089442783b..9b3a447c9e0 100644 --- a/zebrad/src/components/mempool/storage/tests/prop.rs +++ b/zebrad/src/components/mempool/storage/tests/prop.rs @@ -475,8 +475,8 @@ impl SpendConflictTestInput { }; ( - VerifiedUnminedTx::new(first.0.into(), Amount::zero(), 0, None), - VerifiedUnminedTx::new(second.0.into(), Amount::zero(), 0, None), + VerifiedUnminedTx::new(first.0.into(), Amount::zero(), 0), + VerifiedUnminedTx::new(second.0.into(), Amount::zero(), 0), ) } @@ -493,8 +493,8 @@ impl SpendConflictTestInput { Self::remove_orchard_conflicts(&mut first, &mut second); ( - VerifiedUnminedTx::new(first.0.into(), Amount::zero(), 0, None), - VerifiedUnminedTx::new(second.0.into(), Amount::zero(), 0, None), + VerifiedUnminedTx::new(first.0.into(), Amount::zero(), 0), + VerifiedUnminedTx::new(second.0.into(), Amount::zero(), 0), ) } diff --git a/zebrad/src/components/mempool/storage/tests/vectors.rs b/zebrad/src/components/mempool/storage/tests/vectors.rs index 7ce39e5b608..5977662f19b 100644 --- a/zebrad/src/components/mempool/storage/tests/vectors.rs +++ b/zebrad/src/components/mempool/storage/tests/vectors.rs @@ -271,7 +271,7 @@ fn mempool_expired_basic_for_network(network: Network) -> Result<()> { let tx_id = tx.unmined_id(); // Insert the transaction into the mempool, with a fake zero miner fee and sigops - storage.insert(VerifiedUnminedTx::new(tx.into(), Amount::zero(), 0, None))?; + storage.insert(VerifiedUnminedTx::new(tx.into(), Amount::zero(), 0))?; assert_eq!(storage.transaction_count(), 1); diff --git a/zebrad/src/components/mempool/tests/vector.rs b/zebrad/src/components/mempool/tests/vector.rs index afaa2a04ca0..3dad0f1a466 100644 --- a/zebrad/src/components/mempool/tests/vector.rs +++ b/zebrad/src/components/mempool/tests/vector.rs @@ -809,7 +809,6 @@ async fn mempool_reverifies_after_tip_change() -> Result<(), Report> { transaction, Amount::zero(), 0, - None, ))); }) .await; @@ -867,7 +866,6 @@ async fn mempool_reverifies_after_tip_change() -> Result<(), Report> { transaction, Amount::zero(), 0, - None, ))); }) .await; From ea4d62a7540d0ccdcf65118c3b5de6acf975b1ea Mon Sep 17 00:00:00 2001 From: Arya Date: Sun, 16 Apr 2023 19:31:58 -0400 Subject: [PATCH 7/8] Update zebra-chain/src/transparent/utxo.rs --- zebra-chain/src/transparent/utxo.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/zebra-chain/src/transparent/utxo.rs b/zebra-chain/src/transparent/utxo.rs index b3c3294cee0..db626148ee4 100644 --- a/zebra-chain/src/transparent/utxo.rs +++ b/zebra-chain/src/transparent/utxo.rs @@ -54,18 +54,6 @@ pub struct OrderedUtxo { pub tx_index_in_block: usize, } -impl<'a> From<&'a OrderedUtxo> for &'a Utxo { - fn from(ordered_utxo: &'a OrderedUtxo) -> Self { - &ordered_utxo.utxo - } -} - -impl From for Utxo { - fn from(ordered_utxo: OrderedUtxo) -> Self { - ordered_utxo.utxo - } -} - impl AsRef for OrderedUtxo { fn as_ref(&self) -> &Utxo { &self.utxo From e6c6c0f33593095568ffef531379d445f2baa290 Mon Sep 17 00:00:00 2001 From: arya2 Date: Mon, 17 Apr 2023 19:54:40 -0400 Subject: [PATCH 8/8] Applies suggestions from code review --- zebra-consensus/src/error.rs | 4 +- zebra-consensus/src/transaction.rs | 48 +++++++++++-------- zebra-consensus/src/transaction/check.rs | 12 ++--- zebra-consensus/src/transaction/tests.rs | 29 +++++++++-- .../get_block_template.rs | 1 + zebra-state/src/lib.rs | 2 +- zebra-state/src/service.rs | 4 +- zebra-state/src/service/check/tests/utxo.rs | 7 +-- 8 files changed, 67 insertions(+), 40 deletions(-) diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index 080c7cede46..d089073eb29 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -190,7 +190,7 @@ 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))] @@ -225,7 +225,7 @@ pub enum TransactionError { 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 aaf63c304d2..28546485b0a 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -226,20 +226,6 @@ impl Request { pub fn is_mempool(&self) -> bool { matches!(self, Request::Mempool { .. }) } - - /// Returns lowest height at which all transparent coinbase spends will be valid, - /// or None if this transaction has no transparent coinbase spends. - pub fn check_maturity_height( - &self, - spent_utxos: &HashMap, - ) -> Result<(), TransactionError> { - check::tx_transparent_coinbase_spends_maturity( - self.transaction(), - self.height(), - self.known_utxos(), - spent_utxos, - ) - } } impl Response { @@ -388,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)); @@ -468,12 +460,6 @@ where let legacy_sigop_count = cached_ffi_transaction.legacy_sigop_count()?; - // TODO: Return an error for Request::Block as well to replace this check in - // the state once #2336 has been implemented? - if req.is_mempool() { - req.check_maturity_height(&spent_utxos)?; - } - let rsp = match req { Request::Block { .. } => Response::Block { tx_id, @@ -587,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 f37641cc368..e7ec37be610 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -5,7 +5,6 @@ use std::{ borrow::Cow, collections::{HashMap, HashSet}, - convert::TryFrom, hash::Hash, sync::Arc, }; @@ -470,12 +469,11 @@ fn validate_expiry_height_mined( Ok(()) } -/// Accepts a transaction, UTXOs in the same block, and spent UTXOs from the chain. +/// Accepts a transaction, block height, block UTXOs, and +/// the transaction's spent UTXOs from the chain. /// -/// Returns the lowest/earlier height at which every transparent coinbase spend -/// in the provided [`Transaction`] will be mature, -/// -/// Returns None if the transaction has no transparent coinbase spends. +/// 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, @@ -491,7 +489,7 @@ pub fn tx_transparent_coinbase_spends_maturity( let spend_restriction = tx.coinbase_spend_restriction(height); - zebra_state::transparent_coinbase_spend(spend, spend_restriction, &utxo)?; + 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 7181174cba9..89fd0276762 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -564,8 +564,10 @@ 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_response_maturity_height_correct() { +async fn mempool_request_with_immature_spent_is_rejected() { let _init_guard = zebra_test::init(); let mut state: MockService<_, _, _, _> = MockService::build().for_prop_tests(); @@ -592,8 +594,26 @@ async fn mempool_response_maturity_height_correct() { 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) @@ -633,10 +653,11 @@ async fn mempool_response_maturity_height_correct() { transaction: tx.into(), height, }) - .await; + .await + .expect_err("verification of transaction with immature spend should fail"); - assert!( - verifier_response.is_err(), + assert_eq!( + verifier_response, expected_error, "expected to fail verification, got: {verifier_response:?}" ); } diff --git a/zebra-rpc/src/methods/get_block_template_rpcs/get_block_template.rs b/zebra-rpc/src/methods/get_block_template_rpcs/get_block_template.rs index 5e4a95df5a5..8439808fe70 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs/get_block_template.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs/get_block_template.rs @@ -271,6 +271,7 @@ where unreachable!("unmatched response to a mempool::FullTransactions request") }; + // Check that the mempool and state were in sync when we made the requests Ok((last_seen_tip_hash == chain_tip_hash).then_some(transactions)) } diff --git a/zebra-state/src/lib.rs b/zebra-state/src/lib.rs index 18a7c7b5bcd..53ec2dc3e95 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, transparent_coinbase_spend, + 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 8a969677ecc..a7fec4fcb20 100644 --- a/zebra-state/src/service.rs +++ b/zebra-state/src/service.rs @@ -69,9 +69,7 @@ pub mod block_iter; pub mod chain_tip; pub mod watch_receiver; -pub(crate) mod check; - -pub use check::transparent_coinbase_spend; +pub mod check; pub(crate) mod finalized_state; pub(crate) mod non_finalized_state; diff --git a/zebra-state/src/service/check/tests/utxo.rs b/zebra-state/src/service/check/tests/utxo.rs index 81bc8c4f2d6..d35441381e5 100644 --- a/zebra-state/src/service/check/tests/utxo.rs +++ b/zebra-state/src/service/check/tests/utxo.rs @@ -55,9 +55,10 @@ fn accept_shielded_mature_coinbase_utxo_spend() { let result = check::utxo::transparent_coinbase_spend(outpoint, spend_restriction, ordered_utxo.as_ref()); - assert!( - result.is_ok(), - "transparent coinbase spend check should return Ok" + assert_eq!( + result, + Ok(()), + "mature transparent coinbase spend check should return Ok(())" ); }