change(chain): Drop transactions with unpaid actions (#8638)

* Set unpaid action limit to 0

* Update changelog

* Apply suggestions from code review

Co-authored-by: Arya <aryasolhi@gmail.com>

* Rephrase a note on mempool checks

---------

Co-authored-by: Arya <aryasolhi@gmail.com>

CHANGELOG.md

@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org).
 - Fixed a bug with trailing characters in the openapi spec method descriptions ([#8597](https://github.com/ZcashFoundation/zebra/pull/8597))
 - Added default constructions for several RPC method responses ([#8616](https://github.com/ZcashFoundation/zebra/pull/8616))
 - Added Windows to the list of supported platforms in Tier 2 ([#8637](https://github.com/ZcashFoundation/zebra/pull/8637))
+- Zebra now drops transactions with unpaid actions in block templates and from the mempool.
 
 ## [Zebra 1.7.0](https://github.com/ZcashFoundation/zebra/releases/tag/v1.7.0) - 2024-05-07
 

zebra-chain/src/transaction/unmined/zip317.rs

@@ -41,9 +41,13 @@ const BLOCK_PRODUCTION_WEIGHT_RATIO_CAP: f32 = 4.0;
 /// This avoids special handling for transactions with zero weight.
 const MIN_BLOCK_PRODUCTION_SUBSTITUTE_FEE: i64 = 1;
 
-/// The ZIP-317 recommended limit on the number of unpaid actions per block.
-/// `block_unpaid_action_limit` in ZIP-317.
-pub const BLOCK_PRODUCTION_UNPAID_ACTION_LIMIT: u32 = 50;
+/// If a tx has more than `BLOCK_UNPAID_ACTION_LIMIT` "unpaid actions", it will never be mined by
+/// the [_Recommended algorithm for block template construction_][alg-def], implemented in Zebra
+/// [here][alg-impl].
+///
+/// [alg-def]: https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction
+/// [alg-impl]: https://github.com/zcashfoundation/zebra/blob/95e4d0973caac075b47589f6a05f9d744acd3db3/zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs#L39
+pub const BLOCK_UNPAID_ACTION_LIMIT: u32 = 0;
 
 /// The minimum fee per kilobyte for Zebra mempool transactions.
 /// Also used as the minimum fee for a mempool transaction.
@@ -178,7 +182,7 @@ pub fn mempool_checks(
     // > Nodes MAY drop these transactions.
     //
     // <https://zips.z.cash/zip-0317#transaction-relaying>
-    if unpaid_actions > BLOCK_PRODUCTION_UNPAID_ACTION_LIMIT {
+    if unpaid_actions > BLOCK_UNPAID_ACTION_LIMIT {
         return Err(Error::UnpaidActions);
     }
 
@@ -198,6 +202,12 @@ pub fn mempool_checks(
     // In zcashd this is `DEFAULT_MIN_RELAY_TX_FEE` and `LEGACY_DEFAULT_FEE`:
     // <https://github.com/zcash/zcash/blob/f512291ff20098291442e83713de89bcddc07546/src/main.h#L71-L72>
     // <https://github.com/zcash/zcash/blob/9e856cfc5b81aa2607a16a23ff5584ea10014de6/src/amount.h#L35-L36>
+    //
+    // ## Note
+    //
+    // If the check above for the maximum number of unpaid actions passes with
+    // [`BLOCK_UNPAID_ACTION_LIMIT`] set to zero, then there is no way for the legacy check below to
+    // fail. This renders the legacy check redundant in that case.
 
     const KILOBYTE: usize = 1000;
 

zebra-chain/src/transaction/unmined/zip317/tests.rs

@@ -3,16 +3,21 @@
 use super::{mempool_checks, Amount, Error};
 #[test]
 fn zip317_unpaid_actions_err() {
-    let check = mempool_checks(51, Amount::try_from(1).unwrap(), 1);
+    let check = mempool_checks(1, Amount::try_from(1).unwrap(), 1);
 
     assert!(check.is_err());
     assert_eq!(check.err(), Some(Error::UnpaidActions));
 }
 
 #[test]
 fn zip317_minimum_rate_fee_err() {
-    let check = mempool_checks(50, Amount::try_from(1).unwrap(), 1000);
+    let check = mempool_checks(0, Amount::try_from(1).unwrap(), 1000);
 
     assert!(check.is_err());
     assert_eq!(check.err(), Some(Error::FeeBelowMinimumRate));
 }
+
+#[test]
+fn zip317_mempool_checks_ok() {
+    assert!(mempool_checks(0, Amount::try_from(100).unwrap(), 1000).is_ok())
+}

zebra-consensus/src/transaction/tests.rs

@@ -2945,7 +2945,7 @@ async fn mempool_zip317_error() {
         fund_height,
         true,
         0,
-        Amount::try_from(10).expect("invalid value"),
+        Amount::try_from(10).expect("valid amount"),
     );
 
     // Create a non-coinbase V5 tx.
@@ -2998,7 +2998,7 @@ async fn mempool_zip317_error() {
     assert!(verifier_response.is_err());
     assert_eq!(
         verifier_response.err(),
-        Some(TransactionError::Zip317(zip317::Error::FeeBelowMinimumRate))
+        Some(TransactionError::Zip317(zip317::Error::UnpaidActions))
     );
 }
 
@@ -3017,7 +3017,7 @@ async fn mempool_zip317_ok() {
         fund_height,
         true,
         0,
-        Amount::try_from(10001).expect("invalid value"),
+        Amount::try_from(10_001).expect("valid amount"),
     );
 
     // Create a non-coinbase V5 tx.

zebra-rpc/src/methods/get_block_template_rpcs/zip317.rs

@@ -15,7 +15,7 @@ use zebra_chain::{
     amount::NegativeOrZero,
     block::{Height, MAX_BLOCK_BYTES},
     parameters::Network,
-    transaction::{zip317::BLOCK_PRODUCTION_UNPAID_ACTION_LIMIT, VerifiedUnminedTx},
+    transaction::{zip317::BLOCK_UNPAID_ACTION_LIMIT, VerifiedUnminedTx},
     transparent,
 };
 use zebra_consensus::MAX_BLOCK_SIGOPS;
@@ -64,7 +64,7 @@ pub async fn select_mempool_transactions(
     // Set up limit tracking
     let mut remaining_block_bytes: usize = MAX_BLOCK_BYTES.try_into().expect("fits in memory");
     let mut remaining_block_sigops = MAX_BLOCK_SIGOPS;
-    let mut remaining_block_unpaid_actions: u32 = BLOCK_PRODUCTION_UNPAID_ACTION_LIMIT;
+    let mut remaining_block_unpaid_actions: u32 = BLOCK_UNPAID_ACTION_LIMIT;
 
     // Adjust the limits based on the coinbase transaction
     remaining_block_bytes -= fake_coinbase_tx.data.as_ref().len();