Check MAX_BLOCK_SIGOPS in the block verifier (#3049)

* Cleanup a function that calls zcash_script

* Remove zebra_test::prelude macros that conflict with the Rust prelude

* Add sigops count support to zebra-script

* Check MAX_BLOCK_SIGOPS in the block verifier

* Test MAX_BLOCK_SIGOPS on generated and historic blocks

* Add SAFETY comments for all unsafe zebra-script code

* Explain where the consensus rule comes from

* Remove unused pretty_assertions dependency

* Allow large test block generation functions with the proptest-impl feature

* Replace `as` with `try_into` for integer conversions in unsafe code

* Expand SAFETY comments
This commit is contained in:
teor 2021-11-16 06:55:32 +10:00 committed by GitHub
parent 7457edcb86
commit 1df3bdb089
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 224 additions and 94 deletions

40
Cargo.lock generated
View File

@ -947,16 +947,6 @@ dependencies = [
"memchr",
]
[[package]]
name = "ctor"
version = "0.1.16"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "7fbaabec2c953050352311293be5c6aba8e141ba19d6811862b232d6fd020484"
dependencies = [
"quote 1.0.7",
"syn 1.0.60",
]
[[package]]
name = "curve25519-dalek"
version = "3.0.0"
@ -1026,12 +1016,6 @@ dependencies = [
"uuid",
]
[[package]]
name = "diff"
version = "0.1.12"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "0e25ea47919b1560c4e3b7fe0aaab9becf5b84a10325ddf7db0f0ba5e1026499"
[[package]]
name = "digest"
version = "0.9.0"
@ -2281,15 +2265,6 @@ dependencies = [
"num-traits",
]
[[package]]
name = "output_vt100"
version = "0.1.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "53cdc5b785b7a58c5aad8216b3dfa114df64b0b06ae6e1501cef91df2fbdf8f9"
dependencies = [
"winapi",
]
[[package]]
name = "owning_ref"
version = "0.4.1"
@ -2506,18 +2481,6 @@ version = "0.2.10"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ac74c624d6b2d21f425f752262f42188365d7b8ff1aff74c82e45136510a4857"
[[package]]
name = "pretty_assertions"
version = "1.0.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ec0cfe1b2403f172ba0f234e500906ee0a3e493fb81092dac23ebefe129301cc"
dependencies = [
"ansi_term 0.12.1",
"ctor",
"diff",
"output_vt100",
]
[[package]]
name = "proc-macro-error"
version = "1.0.4"
@ -4426,7 +4389,7 @@ dependencies = [
[[package]]
name = "zcash_script"
version = "0.1.6-alpha.0"
source = "git+https://github.com/ZcashFoundation/zcash_script.git?rev=1caa29e975405400983f679a376fb1c86527d5f2#1caa29e975405400983f679a376fb1c86527d5f2"
source = "git+https://github.com/ZcashFoundation/zcash_script.git?rev=0cb1aa5dd159cb205e5cae9683ca976df60b6b21#0cb1aa5dd159cb205e5cae9683ca976df60b6b21"
dependencies = [
"bindgen 0.58.1",
"blake2b_simd",
@ -4631,7 +4594,6 @@ dependencies = [
"lazy_static",
"once_cell",
"owo-colors 3.1.0",
"pretty_assertions",
"proptest",
"rand 0.8.4",
"regex",

View File

@ -11,7 +11,7 @@ pub mod merkle;
#[cfg(any(test, feature = "proptest-impl"))]
pub mod arbitrary;
#[cfg(any(test, feature = "bench"))]
#[cfg(any(test, feature = "bench", feature = "proptest-impl"))]
pub mod tests;
use std::{collections::HashMap, convert::TryInto, fmt, ops::Neg};

View File

@ -1,7 +1,7 @@
//! Tests for Zebra blocks
// XXX generate should be rewritten as strategies
#[cfg(any(test, feature = "bench"))]
#[cfg(any(test, feature = "bench", feature = "proptest-impl"))]
pub mod generate;
#[cfg(test)]
mod preallocate;

View File

@ -76,6 +76,15 @@ pub enum VerifyBlockError {
Transaction(#[from] TransactionError),
}
/// The maximum allowed number of legacy signature check operations in a block.
///
/// This consensus rule is not documented, so Zebra follows the `zcashd` implementation.
/// We re-use some `zcashd` C++ script code via `zebra-script` and `zcash_script`.
///
/// See:
/// https://github.com/zcash/zcash/blob/bad7f7eadbbb3466bebe3354266c7f69f607fcfd/src/consensus/consensus.h#L30
pub const MAX_BLOCK_SIGOPS: u64 = 20_000;
impl<S, V> BlockVerifier<S, V>
where
S: Service<zs::Request, Response = zs::Response, Error = BoxError> + Send + Clone + 'static,
@ -119,7 +128,6 @@ where
// We don't include the block hash, because it's likely already in a parent span
let span = tracing::debug_span!("block", height = ?block.coinbase_height());
// TODO(jlusby): Error = Report, handle errors from state_service.
async move {
let hash = block.hash();
// Check that this block is actually a new block.
@ -143,6 +151,9 @@ where
let height = block
.coinbase_height()
.ok_or(BlockError::MissingHeight(hash))?;
// TODO: support block heights up to u32::MAX (#1113)
// In practice, these blocks are invalid anyway, because their parent block doesn't exist.
if height > block::Height::MAX {
Err(BlockError::MaxHeight(height, hash, block::Height::MAX))?;
}
@ -197,12 +208,25 @@ where
}
tracing::trace!(len = async_checks.len(), "built async tx checks");
let mut legacy_sigop_count = 0;
use futures::StreamExt;
while let Some(result) = async_checks.next().await {
tracing::trace!(?result, remaining = async_checks.len());
result
let response = result
.map_err(Into::into)
.map_err(VerifyBlockError::Transaction)?;
legacy_sigop_count += response
.legacy_sigop_count()
.expect("block transaction responses must have a legacy sigop count");
}
if legacy_sigop_count > MAX_BLOCK_SIGOPS {
Err(BlockError::TooManyTransparentSignatureOperations {
height,
hash,
legacy_sigop_count,
})?;
}
let new_outputs = Arc::try_unwrap(known_utxos)

View File

@ -1,12 +1,5 @@
//! Tests for block verification
use crate::{
parameters::{SLOW_START_INTERVAL, SLOW_START_SHIFT},
script,
};
use super::*;
use std::sync::Arc;
use chrono::Utc;
@ -15,15 +8,25 @@ use once_cell::sync::Lazy;
use tower::{buffer::Buffer, util::BoxService};
use zebra_chain::{
block::{self, Block, Height},
block::{
self,
tests::generate::{large_multi_transaction_block, large_single_transaction_block},
Block, Height,
},
parameters::{Network, NetworkUpgrade},
serialization::{ZcashDeserialize, ZcashDeserializeInto},
transaction::{arbitrary::transaction_to_fake_v5, Transaction},
work::difficulty::{ExpandedDifficulty, INVALID_COMPACT_DIFFICULTY},
};
use zebra_script::CachedFfiTransaction;
use zebra_test::transcript::{ExpectedTranscriptError, Transcript};
use crate::transaction;
use crate::{
parameters::{SLOW_START_INTERVAL, SLOW_START_SHIFT},
script, transaction,
};
use super::*;
static VALID_BLOCK_TRANSCRIPT: Lazy<
Vec<(Arc<Block>, Result<block::Hash, ExpectedTranscriptError>)>,
@ -606,3 +609,55 @@ fn merkle_root_fake_v5_for_network(network: Network) -> Result<(), Report> {
Ok(())
}
#[test]
fn legacy_sigops_count_for_large_generated_blocks() {
zebra_test::init();
// We can't test sigops using the transaction verifier, because it looks up UTXOs.
let block = large_single_transaction_block();
let mut legacy_sigop_count = 0;
for transaction in block.transactions {
let cached_ffi_transaction = Arc::new(CachedFfiTransaction::new(transaction.clone()));
let tx_sigop_count = cached_ffi_transaction.legacy_sigop_count();
assert_eq!(tx_sigop_count, Ok(0));
legacy_sigop_count += tx_sigop_count.expect("unexpected invalid sigop count");
}
// We know this block has no sigops.
assert_eq!(legacy_sigop_count, 0);
let block = large_multi_transaction_block();
let mut legacy_sigop_count = 0;
for transaction in block.transactions {
let cached_ffi_transaction = Arc::new(CachedFfiTransaction::new(transaction.clone()));
let tx_sigop_count = cached_ffi_transaction.legacy_sigop_count();
assert_eq!(tx_sigop_count, Ok(1));
legacy_sigop_count += tx_sigop_count.expect("unexpected invalid sigop count");
}
// Test that large blocks can actually fail the sigops check.
assert!(legacy_sigop_count > MAX_BLOCK_SIGOPS);
}
#[test]
fn legacy_sigops_count_for_historic_blocks() {
zebra_test::init();
// We can't test sigops using the transaction verifier, because it looks up UTXOs.
for block in zebra_test::vectors::BLOCKS.iter() {
let mut legacy_sigop_count = 0;
let block: Block = block
.zcash_deserialize_into()
.expect("block test vector is valid");
for transaction in block.transactions {
let cached_ffi_transaction = Arc::new(CachedFfiTransaction::new(transaction.clone()));
legacy_sigop_count += cached_ffi_transaction
.legacy_sigop_count()
.expect("unexpected invalid sigop count");
}
// Test that historic blocks pass the sigops check.
assert!(legacy_sigop_count <= MAX_BLOCK_SIGOPS);
}
}

View File

@ -9,7 +9,7 @@ use thiserror::Error;
use zebra_chain::{orchard, sapling, sprout, transparent};
use crate::BoxError;
use crate::{block::MAX_BLOCK_SIGOPS, BoxError};
#[cfg(any(test, feature = "proptest-impl"))]
use proptest_derive::Arbitrary;
@ -208,4 +208,14 @@ pub enum BlockError {
#[error("transaction has wrong consensus branch id for block network upgrade")]
WrongTransactionConsensusBranchId,
#[error(
"block {height:?} {hash:?} has {legacy_sigop_count} legacy transparent signature operations, \
but the limit is {MAX_BLOCK_SIGOPS}"
)]
TooManyTransparentSignatureOperations {
height: zebra_chain::block::Height,
hash: zebra_chain::block::Hash,
legacy_sigop_count: u64,
},
}

View File

@ -118,6 +118,10 @@ pub enum Response {
///
/// https://zips.z.cash/protocol/protocol.pdf#transactions
miner_fee: Option<Amount<NonNegative>>,
/// The number of legacy signature operations in this transaction's
/// transparent inputs and outputs.
legacy_sigop_count: u64,
},
/// A response to a mempool transaction verification request.
@ -224,6 +228,20 @@ impl Response {
}
}
/// The number of legacy transparent signature operations in this transaction's
/// inputs and outputs.
///
/// Zebra does not check the legacy sigop count for mempool transactions,
/// because it is a standard rule (not a consensus rule).
pub fn legacy_sigop_count(&self) -> Option<u64> {
match self {
Response::Block {
legacy_sigop_count, ..
} => Some(*legacy_sigop_count),
Response::Mempool { .. } => None,
}
}
/// Returns true if the request is a mempool request.
pub fn is_mempool(&self) -> bool {
match self {
@ -289,16 +307,14 @@ where
// Note: this rule originally applied to Sapling, but we assume it also applies to Orchard.
//
// https://zips.z.cash/zip-0213#specification
let cached_ffi_transaction = Arc::new(CachedFfiTransaction::new(tx.clone()));
let async_checks = match tx.as_ref() {
Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => {
tracing::debug!(?tx, "got transaction with wrong version");
return Err(TransactionError::WrongVersion);
}
Transaction::V4 {
inputs,
// outputs,
// lock_time,
// expiry_height,
joinsplit_data,
sapling_shielded_data,
..
@ -306,13 +322,12 @@ where
&req,
network,
script_verifier,
inputs,
cached_ffi_transaction.clone(),
utxo_sender,
joinsplit_data,
sapling_shielded_data,
)?,
Transaction::V5 {
inputs,
sapling_shielded_data,
orchard_shielded_data,
..
@ -320,7 +335,7 @@ where
&req,
network,
script_verifier,
inputs,
cached_ffi_transaction.clone(),
utxo_sender,
sapling_shielded_data,
orchard_shielded_data,
@ -351,7 +366,11 @@ where
}
let rsp = match req {
Request::Block { .. } => Response::Block { tx_id, miner_fee },
Request::Block { .. } => Response::Block {
tx_id,
miner_fee,
legacy_sigop_count: cached_ffi_transaction.legacy_sigop_count()?,
},
Request::Mempool { transaction, .. } => Response::Mempool {
transaction: VerifiedUnminedTx::new(
transaction,
@ -389,14 +408,14 @@ where
/// for more information)
/// - the `network` to consider when verifying
/// - the `script_verifier` to use for verifying the transparent transfers
/// - the transparent `inputs` in the transaction
/// - the prepared `cached_ffi_transaction` used by the script verifier
/// - the Sprout `joinsplit_data` shielded data in the transaction
/// - the `sapling_shielded_data` in the transaction
fn verify_v4_transaction(
request: &Request,
network: Network,
script_verifier: script::Verifier<ZS>,
inputs: &[transparent::Input],
cached_ffi_transaction: Arc<CachedFfiTransaction>,
utxo_sender: mpsc::UnboundedSender<script::Response>,
joinsplit_data: &Option<transaction::JoinSplitData<Groth16Proof>>,
sapling_shielded_data: &Option<sapling::ShieldedData<sapling::PerSpendAnchor>>,
@ -412,7 +431,7 @@ where
request,
network,
script_verifier,
inputs,
cached_ffi_transaction,
utxo_sender,
)?
.and(Self::verify_sprout_shielded_data(
@ -471,14 +490,14 @@ where
/// for more information)
/// - the `network` to consider when verifying
/// - the `script_verifier` to use for verifying the transparent transfers
/// - the transparent `inputs` in the transaction
/// - the prepared `cached_ffi_transaction` used by the script verifier
/// - the sapling shielded data of the transaction, if any
/// - the orchard shielded data of the transaction, if any
fn verify_v5_transaction(
request: &Request,
network: Network,
script_verifier: script::Verifier<ZS>,
inputs: &[transparent::Input],
cached_ffi_transaction: Arc<CachedFfiTransaction>,
utxo_sender: mpsc::UnboundedSender<script::Response>,
sapling_shielded_data: &Option<sapling::ShieldedData<sapling::SharedAnchor>>,
orchard_shielded_data: &Option<orchard::ShieldedData>,
@ -494,7 +513,7 @@ where
request,
network,
script_verifier,
inputs,
cached_ffi_transaction,
utxo_sender,
)?
.and(Self::verify_sapling_shielded_data(
@ -508,9 +527,7 @@ where
// TODO:
// - verify orchard shielded pool (ZIP-224) (#2105)
// - ZIP-244 (#1874)
// - remaining consensus rules (#2379)
// - remove `should_panic` from tests
// - shielded input and output limits? (#2379)
}
/// Verifies if a V5 `transaction` is supported by `network_upgrade`.
@ -541,13 +558,15 @@ where
}
}
/// Verifies if a transaction's transparent `inputs` are valid using the provided
/// `script_verifier`.
/// Verifies if a transaction's transparent inputs are valid using the provided
/// `script_verifier` and `cached_ffi_transaction`.
///
/// Returns script verification responses via the `utxo_sender`.
fn verify_transparent_inputs_and_outputs(
request: &Request,
network: Network,
script_verifier: script::Verifier<ZS>,
inputs: &[transparent::Input],
cached_ffi_transaction: Arc<CachedFfiTransaction>,
utxo_sender: mpsc::UnboundedSender<script::Response>,
) -> Result<AsyncChecks, TransactionError> {
let transaction = request.transaction();
@ -557,9 +576,9 @@ where
// Coinbase transactions don't have any PrevOut inputs.
Ok(AsyncChecks::new())
} else {
// feed all of the inputs to the script and shielded verifiers
// feed all of the inputs to the script verifier
// the script_verifier also checks transparent sighashes, using its own implementation
let cached_ffi_transaction = Arc::new(CachedFfiTransaction::new(transaction));
let inputs = transaction.inputs();
let known_utxos = request.known_utxos();
let upgrade = request.upgrade(network);

View File

@ -8,8 +8,10 @@ edition = "2018"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[dependencies]
zcash_script = { git = "https://github.com/ZcashFoundation/zcash_script.git", rev = "1caa29e975405400983f679a376fb1c86527d5f2" }
zcash_script = { git = "https://github.com/ZcashFoundation/zcash_script.git", rev = "0cb1aa5dd159cb205e5cae9683ca976df60b6b21" }
zebra-chain = { path = "../zebra-chain" }
thiserror = "1.0.30"
displaydoc = "0.2.2"

View File

@ -6,19 +6,20 @@
#![warn(missing_docs)]
#![allow(clippy::try_err)]
#![deny(clippy::await_holding_lock)]
// we allow unsafe code to call zcash_script
// we allow unsafe code, so we can call zcash_script
use std::{convert::TryInto, sync::Arc};
use displaydoc::Display;
#[cfg(windows)]
use std::convert::TryInto;
use std::sync::Arc;
use thiserror::Error;
use zcash_script::{
zcash_script_error_t, zcash_script_error_t_zcash_script_ERR_OK,
zcash_script_error_t_zcash_script_ERR_TX_DESERIALIZE,
zcash_script_error_t_zcash_script_ERR_TX_INDEX,
zcash_script_error_t_zcash_script_ERR_TX_SIZE_MISMATCH,
};
use zebra_chain::{
parameters::ConsensusBranchId, serialization::ZcashSerialize, transaction::Transaction,
transparent,
@ -62,7 +63,16 @@ impl From<zcash_script_error_t> for Error {
/// Transaction.
#[derive(Debug)]
pub struct CachedFfiTransaction {
/// The deserialized Zebra transaction.
///
/// This field is private so that `transaction` and `precomputed` always match.
transaction: Arc<Transaction>,
/// The deserialized `zcash_script` transaction, as a C++ object.
///
/// SAFETY: this field must be private,
/// and `CachedFfiTransaction::new` must be the only method that modifies it,
/// so that it is [`Send`], [`Sync`], valid, and not NULL.
precomputed: *mut std::ffi::c_void,
}
@ -74,12 +84,17 @@ impl CachedFfiTransaction {
.expect("serialization into a vec is infallible");
let tx_to_ptr = tx_to.as_ptr();
let tx_to_len = tx_to.len() as u32;
let tx_to_len = tx_to
.len()
.try_into()
.expect("serialized transaction lengths are much less than u32::MAX");
let mut err = 0;
// SAFETY: the `tx_to_*` fields are created from a valid Rust `Vec`.
let precomputed = unsafe {
zcash_script::zcash_script_new_precomputed_tx(tx_to_ptr, tx_to_len, &mut err)
};
// SAFETY: the safety of other methods depends on `precomputed` being valid and not NULL.
assert!(
!precomputed.is_null(),
"zcash_script_new_precomputed_tx returned {} ({})",
@ -89,6 +104,8 @@ impl CachedFfiTransaction {
Self {
transaction,
// SAFETY: `precomputed` must not be modified after initialisation,
// so that it is `Send` and `Sync`.
precomputed,
}
}
@ -113,32 +130,42 @@ impl CachedFfiTransaction {
) -> Result<(), Error> {
let transparent::Output { value, lock_script } = previous_output;
let script_pub_key: &[u8] = lock_script.as_raw_bytes();
let n_in = input_index as _;
// This conversion is useful on some platforms, but not others.
#[allow(clippy::useless_conversion)]
let n_in = input_index
.try_into()
.expect("transaction indexes are much less than c_uint::MAX");
let script_ptr = script_pub_key.as_ptr();
let script_len = script_pub_key.len();
let amount = value.into();
let flags = zcash_script::zcash_script_SCRIPT_FLAGS_VERIFY_P2SH
| zcash_script::zcash_script_SCRIPT_FLAGS_VERIFY_CHECKLOCKTIMEVERIFY;
// This conversion is useful on some platforms, but not others.
#[allow(clippy::useless_conversion)]
let flags = flags
.try_into()
.expect("zcash_script_SCRIPT_FLAGS_VERIFY_* enum values fit in a c_uint");
let consensus_branch_id = branch_id.into();
let mut err = 0;
// SAFETY: `CachedFfiTransaction::new` makes sure `self.precomputed` is not NULL.
// The `script_*` fields are created from a valid Rust `slice`.
let ret = unsafe {
zcash_script::zcash_script_verify_precomputed(
self.precomputed,
n_in,
script_ptr,
script_len as u32,
amount,
#[cfg(not(windows))]
flags,
#[cfg(windows)]
flags
script_len
.try_into()
.expect("zcash_script_SCRIPT_FLAGS_VERIFY_* enum values fit in a c_uint"),
.expect("script lengths are much less than u32::MAX"),
amount,
flags,
consensus_branch_id,
&mut err,
)
@ -150,6 +177,24 @@ impl CachedFfiTransaction {
Err(Error::from(err))
}
}
/// Returns the number of transparent signature operations in the
/// transparent inputs and outputs of this transaction.
pub fn legacy_sigop_count(&self) -> Result<u64, Error> {
let mut err = 0;
// SAFETY: `CachedFfiTransaction::new` makes sure `self.precomputed` is not NULL.
let ret = unsafe {
zcash_script::zcash_script_legacy_sigop_count_precomputed(self.precomputed, &mut err)
};
if err == zcash_script_error_t_zcash_script_ERR_OK {
let ret = ret.try_into().expect("c_uint fits in a u64");
Ok(ret)
} else {
Err(Error::from(err))
}
}
}
// # SAFETY
@ -177,11 +222,16 @@ impl CachedFfiTransaction {
// `zcash_script::zcash_script_verify_precomputed` only reads from the
// precomputed context while verifying inputs, which makes it safe to treat this
// pointer like a shared reference (given that is how it is used).
//
// The function `zcash_script:zcash_script_legacy_sigop_count_precomputed` only reads
// from the precomputed context. Currently, these reads happen after all the concurrent
// async checks have finished.
unsafe impl Send for CachedFfiTransaction {}
unsafe impl Sync for CachedFfiTransaction {}
impl Drop for CachedFfiTransaction {
fn drop(&mut self) {
// SAFETY: `CachedFfiTransaction::new` makes sure `self.precomputed` is not NULL.
unsafe { zcash_script::zcash_script_free_precomputed_tx(self.precomputed) };
}
}
@ -226,6 +276,19 @@ mod tests {
Ok(())
}
#[test]
fn count_legacy_sigops() -> Result<()> {
zebra_test::init();
let transaction =
SCRIPT_TX.zcash_deserialize_into::<Arc<zebra_chain::transaction::Transaction>>()?;
let cached_tx = super::CachedFfiTransaction::new(transaction);
assert_eq!(cached_tx.legacy_sigop_count()?, 1);
Ok(())
}
#[test]
fn fail_invalid_script() -> Result<()> {
zebra_test::init();

View File

@ -180,7 +180,6 @@ mod tests {
use crate::{arbitrary::Prepare, tests::FakeChainHelper};
use self::assert_eq;
use super::*;
// Quick helper trait for making queued blocks with throw away channels

View File

@ -20,8 +20,6 @@ use crate::{
Config,
};
use self::assert_eq;
#[test]
fn construct_empty() {
zebra_test::init();

View File

@ -21,7 +21,6 @@ futures = "0.3.17"
color-eyre = "0.5.11"
owo-colors = "3.1.0"
pretty_assertions = "1.0.0"
spandoc = "0.2.0"
thiserror = "1.0.30"
tracing = "0.1.29"

View File

@ -6,5 +6,4 @@ pub use std::process::Stdio;
pub use color_eyre;
pub use color_eyre::eyre;
pub use eyre::Result;
pub use pretty_assertions::{assert_eq, assert_ne};
pub use proptest::prelude::*;