From 002c533ea8f720efeb94b62b3770deca0e6c2b6c Mon Sep 17 00:00:00 2001 From: Marek Date: Thu, 14 Oct 2021 23:15:10 +0200 Subject: [PATCH] Return transaction fee (#2876) * Get the transaction fee from utxos * Return the transaction fee from the verifier * Avoid calculating the fee for coinbase transactions Coinbase transactions don't have fees. In case of a coinbase transaction, the verifier returns a zero fee. * Update the result obtained by `Downloads` --- zebra-consensus/src/error.rs | 3 ++ zebra-consensus/src/transaction.rs | 45 +++++++++------------- zebra-consensus/src/transaction/tests.rs | 45 +++++++++++++++++----- zebra-test/src/mock_service.rs | 2 +- zebrad/src/components/inbound/tests.rs | 21 ++++++++-- zebrad/src/components/mempool/downloads.rs | 2 +- 6 files changed, 76 insertions(+), 42 deletions(-) diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index 4fdbaba9e69..fbbe06ddc62 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -99,6 +99,9 @@ pub enum TransactionError { #[error("adding to the sprout pool is disabled after Canopy")] DisabledAddToSproutPool, + + #[error("could not calculate the transaction fee")] + IncorrectFee, } impl From for TransactionError { diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 31242b2d7a2..cfbb5b594fe 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -18,6 +18,7 @@ use tower::{Service, ServiceExt}; use tracing::Instrument; use zebra_chain::{ + amount::{Amount, NonNegative}, block, orchard, parameters::{Network, NetworkUpgrade}, primitives::Groth16Proof, @@ -102,7 +103,7 @@ pub enum Request { /// /// [`Mempool`] requests are uniquely identified by the [`UnminedTxId`] /// variant for their transaction version. -pub type Response = zebra_chain::transaction::UnminedTxId; +pub type Response = (zebra_chain::transaction::UnminedTxId, Amount); impl Request { /// The transaction to verify that's in this request. @@ -251,34 +252,24 @@ where spent_utxos.insert(script_rsp.spent_outpoint, script_rsp.spent_utxo); } - // temporary assertions for testing ticket #2440 - // - // TODO: use spent_utxos to calculate the transaction fee (#2779) - // and remove these assertions - if tx.has_valid_coinbase_transaction_inputs() { - assert_eq!( - spent_utxos.len(), - 0, - "already checked that coinbase transactions don't spend UTXOs" - ); - } else if spent_utxos.len() < tx.inputs().len() { - // TODO: replace with double-spend check in PR #2843 - return Err(TransactionError::InternalDowncastError(format!( - "transparent double-spend within a transaction: \ - expected {} input UTXOs, got {} unique spent UTXOs", - tx.inputs().len(), - spent_utxos.len() - ))); - } else { - assert_eq!( - spent_utxos.len(), - tx.inputs().len(), - "unexpected excess looked-up spent UTXOs in transaction: \ - expected exactly one UTXO per verified transparent input" - ); + // Get the `value_balance` to calculate the transaction fee. + let value_balance = tx.value_balance(&spent_utxos); + + // Initialize the transaction fee to zero. + let mut tx_fee = Amount::::zero(); + + // Calculate the fee only for non-coinbase transactions. + if !tx.has_valid_coinbase_transaction_inputs() { + tx_fee = match value_balance { + Ok(vb) => match vb.remaining_transaction_value() { + Ok(tx_rtv) => tx_rtv, + Err(_) => return Err(TransactionError::IncorrectFee), + }, + Err(_) => return Err(TransactionError::IncorrectFee), + }; } - Ok(id) + Ok((id, tx_fee)) } .instrument(span) .boxed() diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index fd04838212a..7221aa9bbfe 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -274,7 +274,10 @@ async fn v5_transaction_is_accepted_after_nu5_activation_for_network(network: Ne }) .await; - assert_eq!(result, Ok(expected_hash)); + assert_eq!( + result.expect("expected a tx_id and tx_fee").0, + expected_hash + ); } /// Test if V4 transaction with transparent funds is accepted. @@ -320,7 +323,10 @@ async fn v4_transaction_with_transparent_transfer_is_accepted() { }) .await; - assert_eq!(result, Ok(transaction_hash)); + assert_eq!( + result.expect("expected a tx_id and tx_fee").0, + transaction_hash + ); } /// Test if V4 coinbase transaction is accepted. @@ -363,7 +369,10 @@ async fn v4_coinbase_transaction_is_accepted() { }) .await; - assert_eq!(result, Ok(transaction_hash)); + assert_eq!( + result.expect("expected a tx_id and tx_fee").0, + transaction_hash + ); } /// Test if V4 transaction with transparent funds is rejected if the source script prevents it. @@ -464,7 +473,10 @@ async fn v5_transaction_with_transparent_transfer_is_accepted() { }) .await; - assert_eq!(result, Ok(transaction_hash)); + assert_eq!( + result.expect("expected a tx_id and tx_fee").0, + transaction_hash + ); } /// Test if V5 coinbase transaction is accepted. @@ -510,7 +522,10 @@ async fn v5_coinbase_transaction_is_accepted() { }) .await; - assert_eq!(result, Ok(transaction_hash)); + assert_eq!( + result.expect("expected a tx_id and tx_fee").0, + transaction_hash + ); } /// Test if V5 transaction with transparent funds is rejected if the source script prevents it. @@ -627,7 +642,10 @@ fn v4_with_signed_sprout_transfer_is_accepted() { }) .await; - assert_eq!(result, Ok(expected_hash)); + assert_eq!( + result.expect("expected a tx_id and tx_fee").0, + expected_hash + ); }); } @@ -725,7 +743,10 @@ fn v4_with_sapling_spends() { }) .await; - assert_eq!(result, Ok(expected_hash)); + assert_eq!( + result.expect("expected a tx_id and tx_fee").0, + expected_hash + ); }); } @@ -766,7 +787,10 @@ fn v4_with_sapling_outputs_and_no_spends() { }) .await; - assert_eq!(result, Ok(expected_hash)); + assert_eq!( + result.expect("expected a tx_id and tx_fee").0, + expected_hash + ); }); } @@ -810,7 +834,10 @@ fn v5_with_sapling_spends() { }) .await; - assert_eq!(result, Ok(expected_hash)); + assert_eq!( + result.expect("expected a tx_id and tx_fee").0, + expected_hash + ); }); } diff --git a/zebra-test/src/mock_service.rs b/zebra-test/src/mock_service.rs index 30900eda26d..d255ef7f700 100644 --- a/zebra-test/src/mock_service.rs +++ b/zebra-test/src/mock_service.rs @@ -356,7 +356,7 @@ impl MockService 0).await.respond("response"); /// /// assert!(matches!(call.await, Ok(Ok("response")))); diff --git a/zebrad/src/components/inbound/tests.rs b/zebrad/src/components/inbound/tests.rs index 73efe4a00e2..25251d665d6 100644 --- a/zebrad/src/components/inbound/tests.rs +++ b/zebrad/src/components/inbound/tests.rs @@ -8,6 +8,7 @@ use tower::{buffer::Buffer, builder::ServiceBuilder, util::BoxService, Service, use tracing::Span; use zebra_chain::{ + amount::Amount, block::Block, parameters::Network, serialization::ZcashDeserializeInto, @@ -117,7 +118,10 @@ async fn mempool_push_transaction() -> Result<(), crate::BoxError> { // Simulate a successful transaction verification let verification = tx_verifier.expect_request_that(|_| true).map(|responder| { let txid = responder.request().tx_id(); - responder.respond(txid); + + // Set a dummy fee. + let tx_fee = Amount::zero(); + responder.respond((txid, tx_fee)); }); let (response, _) = futures::join!(request, verification); match response { @@ -205,7 +209,10 @@ async fn mempool_advertise_transaction_ids() -> Result<(), crate::BoxError> { // Simulate a successful transaction verification let verification = tx_verifier.expect_request_that(|_| true).map(|responder| { let txid = responder.request().tx_id(); - responder.respond(txid); + + // Set a dummy fee. + let tx_fee = Amount::zero(); + responder.respond((txid, tx_fee)); }); let (response, _, _) = futures::join!(request, peer_set_responder, verification); @@ -292,7 +299,10 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> { // Simulate a successful transaction verification let verification = tx_verifier.expect_request_that(|_| true).map(|responder| { tx1_id = responder.request().tx_id(); - responder.respond(tx1_id); + + // Set a dummy fee. + let tx_fee = Amount::zero(); + responder.respond((tx1_id, tx_fee)); }); let (response, _) = futures::join!(request, verification); match response { @@ -381,7 +391,10 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> { // Simulate a successful transaction verification let verification = tx_verifier.expect_request_that(|_| true).map(|responder| { tx2_id = responder.request().tx_id(); - responder.respond(tx2_id); + + // Set a dummy fee. + let tx_fee = Amount::zero(); + responder.respond((tx2_id, tx_fee)); }); let (response, _) = futures::join!(request, verification); match response { diff --git a/zebrad/src/components/mempool/downloads.rs b/zebrad/src/components/mempool/downloads.rs index 64b1f390768..180cbfcfbc4 100644 --- a/zebrad/src/components/mempool/downloads.rs +++ b/zebrad/src/components/mempool/downloads.rs @@ -300,7 +300,7 @@ where transaction: tx.clone(), height, }) - .map_ok(|_hash| tx) + .map_ok(|(_tx_id, _tx_fee)| tx) .await; tracing::debug!(?txid, ?result, "verified transaction for the mempool");