change(rpc): Update ZIP-317 transaction selection algorithm (#5776)

* Update ZIP-317 implementation for unpaid actions

* Split shared transaction choose and check into its own function

* Fix an incorrect address error message

* Simplify code, expand docs

* Require docs for getblocktemplate RPC types

* Account for the coinbase transaction in the transaction selection limits

* Fix a broken doc link, update comments, tidy imports

* Fix comment typos

* Use the actual block height rather than Height::MAX for the fake coinbase

* Use a 1 zat fee rather than 0, just in case someone gets clever and skips zero outputs

zebra-chain/src/transaction/builder.rs

@@ -63,7 +63,7 @@ impl Transaction {
             network_upgrade: NetworkUpgrade::current(network, height),
 
             // There is no documented consensus rule for the lock time field in coinbase transactions,
-            // so we just leave it unlocked.
+            // so we just leave it unlocked. (We could also set it to `height`.)
             lock_time: LockTime::unlocked(),
 
             // > The nExpiryHeight field of a coinbase transaction MUST be equal to its block height.

zebra-chain/src/transaction/lock_time.rs

@@ -1,12 +1,14 @@
 //! Transaction LockTime.
 
-use std::{convert::TryInto, io};
+use std::io;
 
 use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt};
 use chrono::{DateTime, TimeZone, Utc};
 
-use crate::block::{self, Height};
-use crate::serialization::{SerializationError, ZcashDeserialize, ZcashSerialize};
+use crate::{
+    block::{self, Height},
+    serialization::{SerializationError, ZcashDeserialize, ZcashSerialize},
+};
 
 /// A Bitcoin-style `locktime`, representing either a block height or an epoch
 /// time.

zebra-chain/src/transaction/unmined.rs

@@ -32,6 +32,10 @@ use UnminedTxId::*;
 #[cfg(any(test, feature = "proptest-impl"))]
 use proptest_derive::Arbitrary;
 
+// Documentation-only
+#[allow(unused_imports)]
+use crate::block::MAX_BLOCK_BYTES;
+
 mod zip317;
 
 /// The minimum cost value for a transaction in the mempool.
@@ -303,13 +307,21 @@ pub struct VerifiedUnminedTx {
     /// transparent inputs and outputs.
     pub legacy_sigop_count: u64,
 
-    /// The block production fee weight for `transaction`, as defined by [ZIP-317].
+    /// The number of unpaid actions for `transaction`,
+    /// as defined by [ZIP-317] for block production.
+    ///
+    /// The number of actions is limited by [`MAX_BLOCK_BYTES`], so it fits in a u32.
+    ///
+    /// [ZIP-317]: https://zips.z.cash/zip-0317#block-production
+    pub unpaid_actions: u32,
+
+    /// The fee weight ratio for `transaction`, as defined by [ZIP-317] for block production.
     ///
     /// This is not consensus-critical, so we use `f32` for efficient calculations
     /// when the mempool holds a large number of transactions.
     ///
     /// [ZIP-317]: https://zips.z.cash/zip-0317#block-production
-    pub block_production_fee_weight: f32,
+    pub fee_weight_ratio: f32,
 }
 
 impl fmt::Display for VerifiedUnminedTx {
@@ -318,6 +330,8 @@ impl fmt::Display for VerifiedUnminedTx {
             .field("transaction", &self.transaction)
             .field("miner_fee", &self.miner_fee)
             .field("legacy_sigop_count", &self.legacy_sigop_count)
+            .field("unpaid_actions", &self.unpaid_actions)
+            .field("fee_weight_ratio", &self.fee_weight_ratio)
             .finish()
     }
 }
@@ -330,14 +344,15 @@ impl VerifiedUnminedTx {
         miner_fee: Amount<NonNegative>,
         legacy_sigop_count: u64,
     ) -> Self {
-        let block_production_fee_weight =
-            zip317::block_production_fee_weight(&transaction, miner_fee);
+        let fee_weight_ratio = zip317::conventional_fee_weight_ratio(&transaction, miner_fee);
+        let unpaid_actions = zip317::unpaid_actions(&transaction, miner_fee);
 
         Self {
             transaction,
             miner_fee,
             legacy_sigop_count,
-            block_production_fee_weight,
+            fee_weight_ratio,
+            unpaid_actions,
         }
     }
 

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

@@ -6,43 +6,34 @@ use std::cmp::max;
 
 use crate::{
     amount::{Amount, NonNegative},
+    block::MAX_BLOCK_BYTES,
     serialization::ZcashSerialize,
     transaction::{Transaction, UnminedTx},
 };
 
 /// The marginal fee for the ZIP-317 fee calculation, in zatoshis per logical action.
 //
 // TODO: allow Amount<NonNegative> in constants
-const MARGINAL_FEE: i64 = 5_000;
+const MARGINAL_FEE: u64 = 5_000;
 
 /// The number of grace logical actions allowed by the ZIP-317 fee calculation.
-const GRACE_ACTIONS: u64 = 2;
+const GRACE_ACTIONS: u32 = 2;
 
 /// The standard size of p2pkh inputs for the ZIP-317 fee calculation, in bytes.
 const P2PKH_STANDARD_INPUT_SIZE: usize = 150;
 
 /// The standard size of p2pkh outputs for the ZIP-317 fee calculation, in bytes.
 const P2PKH_STANDARD_OUTPUT_SIZE: usize = 34;
 
-/// The recommended weight cap for ZIP-317 block production.
-const MAX_BLOCK_PRODUCTION_WEIGHT: f32 = 4.0;
+/// The recommended weight ratio cap for ZIP-317 block production.
+/// `weight_ratio_cap` in ZIP-317.
+const BLOCK_PRODUCTION_WEIGHT_RATIO_CAP: f32 = 4.0;
 
-/// Zebra's custom minimum weight for ZIP-317 block production,
-/// based on half the [ZIP-203] recommended transaction expiry height of 40 blocks.
+/// The minimum fee for the block production weight ratio calculation, in zatoshis.
+/// If a transaction has a lower fee, this value is used instead.
 ///
-/// This ensures all transactions have a non-zero probability of being mined,
-/// which simplifies our implementation.
-///
-/// If blocks are full, this makes it likely that very low fee transactions
-/// will be mined:
-/// - after approximately 20 blocks delay,
-/// - but before they expire.
-///
-/// Note: Small transactions that pay the legacy ZIP-313 conventional fee have twice this weight.
-/// If blocks are full, they will be mined after approximately 10 blocks delay.
-///
-/// [ZIP-203]: https://zips.z.cash/zip-0203#changes-for-blossom>
-const MIN_BLOCK_PRODUCTION_WEIGHT: f32 = 1.0 / 20.0;
+/// This avoids special handling for transactions with zero weight.
+const MIN_BLOCK_PRODUCTION_SUBSTITUTE_FEE: i64 = 1;
 
 /// Returns the conventional fee for `transaction`, as defined by [ZIP-317].
 ///
@@ -56,6 +47,66 @@ pub fn conventional_fee(transaction: &Transaction) -> Amount<NonNegative> {
 
     let marginal_fee: Amount<NonNegative> = MARGINAL_FEE.try_into().expect("fits in amount");
 
+    // marginal_fee * max(logical_actions, GRACE_ACTIONS)
+    let conventional_fee = marginal_fee * conventional_actions(transaction).into();
+
+    conventional_fee.expect("conventional fee is positive and limited by serialized size limit")
+}
+
+/// Returns the number of unpaid actions for `transaction`, as defined by [ZIP-317].
+///
+/// [ZIP-317]: https://zips.z.cash/zip-0317#block-production
+pub fn unpaid_actions(transaction: &UnminedTx, miner_fee: Amount<NonNegative>) -> u32 {
+    // max(logical_actions, GRACE_ACTIONS)
+    let conventional_actions = conventional_actions(&transaction.transaction);
+
+    // floor(tx.fee / marginal_fee)
+    let marginal_fee_weight_ratio = miner_fee / MARGINAL_FEE;
+    let marginal_fee_weight_ratio: i64 = marginal_fee_weight_ratio
+        .expect("marginal fee is not zero")
+        .into();
+
+    // max(0, conventional_actions - marginal_fee_weight_ratio)
+    //
+    // Subtracting MAX_MONEY/5000 from a u32 can't go above i64::MAX.
+    let unpaid_actions = i64::from(conventional_actions) - marginal_fee_weight_ratio;
+
+    unpaid_actions.try_into().unwrap_or_default()
+}
+
+/// Returns the block production fee weight ratio for `transaction`, as defined by [ZIP-317].
+///
+/// This calculation will always return a positive, non-zero value.
+///
+/// [ZIP-317]: https://zips.z.cash/zip-0317#block-production
+pub fn conventional_fee_weight_ratio(
+    transaction: &UnminedTx,
+    miner_fee: Amount<NonNegative>,
+) -> f32 {
+    // Check that this function will always return a positive, non-zero value.
+    //
+    // The maximum number of logical actions in a block is actually
+    // MAX_BLOCK_BYTES / MIN_ACTION_BYTES. MIN_ACTION_BYTES is currently
+    // the minimum transparent output size, but future transaction versions could change this.
+    assert!(
+        MIN_BLOCK_PRODUCTION_SUBSTITUTE_FEE as f32 / MAX_BLOCK_BYTES as f32 > 0.0,
+        "invalid block production constants: the minumum fee ratio must not be zero"
+    );
+
+    let miner_fee = max(miner_fee.into(), MIN_BLOCK_PRODUCTION_SUBSTITUTE_FEE) as f32;
+
+    let conventional_fee = i64::from(transaction.conventional_fee) as f32;
+
+    let uncapped_weight = miner_fee / conventional_fee;
+
+    uncapped_weight.min(BLOCK_PRODUCTION_WEIGHT_RATIO_CAP)
+}
+
+/// Returns the conventional actions for `transaction`, `max(logical_actions, GRACE_ACTIONS)`,
+/// as defined by [ZIP-317].
+///
+/// [ZIP-317]: https://zips.z.cash/zip-0317#fee-calculation
+fn conventional_actions(transaction: &Transaction) -> u32 {
     let tx_in_total_size: usize = transaction
         .inputs()
         .iter()
@@ -80,25 +131,11 @@ pub fn conventional_fee(transaction: &Transaction) -> Amount<NonNegative> {
         + 2 * n_join_split
         + max(n_spends_sapling, n_outputs_sapling)
         + n_actions_orchard;
-    let logical_actions: u64 = logical_actions
+    let logical_actions: u32 = logical_actions
         .try_into()
         .expect("transaction items are limited by serialized size limit");
 
-    let conventional_fee = marginal_fee * max(GRACE_ACTIONS, logical_actions);
-
-    conventional_fee.expect("conventional fee is positive and limited by serialized size limit")
-}
-
-/// Returns the block production fee weight for `transaction`, as defined by [ZIP-317].
-///
-/// [ZIP-317]: https://zips.z.cash/zip-0317#block-production
-pub fn block_production_fee_weight(transaction: &UnminedTx, miner_fee: Amount<NonNegative>) -> f32 {
-    let miner_fee = i64::from(miner_fee) as f32;
-    let conventional_fee = i64::from(transaction.conventional_fee) as f32;
-
-    let uncapped_weight = miner_fee / conventional_fee;
-
-    uncapped_weight.clamp(MIN_BLOCK_PRODUCTION_WEIGHT, MAX_BLOCK_PRODUCTION_WEIGHT)
+    max(GRACE_ACTIONS, logical_actions)
 }
 
 /// Divide `quotient` by `divisor`, rounding the result up to the nearest integer.

zebra-rpc/src/methods/get_block_template_rpcs.rs

@@ -8,7 +8,7 @@ use jsonrpc_derive::rpc;
 use tower::{buffer::Buffer, Service, ServiceExt};
 
 use zebra_chain::{
-    amount::{self, Amount, NonNegative},
+    amount::{self, Amount, NegativeOrZero, NonNegative},
     block::{
         self,
         merkle::{self, AuthDataRoot},
@@ -41,9 +41,8 @@ use crate::methods::{
 
 pub mod config;
 pub mod constants;
-
 pub mod types;
-pub(crate) mod zip317;
+pub mod zip317;
 
 /// The max estimated distance to the chain tip for the getblocktemplate method.
 ///
@@ -330,7 +329,7 @@ where
             let miner_address = miner_address.ok_or_else(|| Error {
                 code: ErrorCode::ServerError(0),
                 message: "configure mining.miner_address in zebrad.toml \
-                          with a transparent P2SH single signature address"
+                          with a transparent P2SH address"
                     .to_string(),
                 data: None,
             })?;
@@ -359,10 +358,6 @@ where
                 });
             }
 
-            let mempool_txs = zip317::select_mempool_transactions(mempool).await?;
-
-            let miner_fee = miner_fee(&mempool_txs);
-
             // Calling state with `ChainInfo` request for relevant chain data
             let request = ReadRequest::ChainInfo;
             let response = state
@@ -383,6 +378,13 @@ where
             // Get the tip data from the state call
             let block_height = (chain_info.tip_height + 1).expect("tip is far below Height::MAX");
 
+            // Use a fake coinbase transaction to break the dependency between transaction
+            // selection, the miner fee, and the fee payment in the coinbase transaction.
+            let fake_coinbase_tx = fake_coinbase_transaction(network, block_height, miner_address);
+            let mempool_txs = zip317::select_mempool_transactions(fake_coinbase_tx, mempool).await?;
+
+            let miner_fee = miner_fee(&mempool_txs);
+
             let outputs =
                 standard_coinbase_outputs(network, block_height, miner_address, miner_fee);
             let coinbase_tx = Transaction::new_v5_coinbase(network, block_height, outputs).into();
@@ -580,6 +582,35 @@ pub fn standard_coinbase_outputs(
         .collect()
 }
 
+/// Returns a fake coinbase transaction that can be used during transaction selection.
+///
+/// This avoids a data dependency loop involving the selected transactions, the miner fee,
+/// and the coinbase transaction.
+///
+/// This transaction's serialized size and sigops must be at least as large as the real coinbase
+/// transaction with the correct height and fee.
+fn fake_coinbase_transaction(
+    network: Network,
+    block_height: Height,
+    miner_address: transparent::Address,
+) -> TransactionTemplate<NegativeOrZero> {
+    // Block heights are encoded as variable-length (script) and `u32` (lock time, expiry height).
+    // They can also change the `u32` consensus branch id.
+    // We use the template height here, which has the correct byte length.
+    // https://zips.z.cash/protocol/protocol.pdf#txnconsensus
+    // https://github.com/zcash/zips/blob/main/zip-0203.rst#changes-for-nu5
+    //
+    // Transparent amounts are encoded as `i64`,
+    // so one zat has the same size as the real amount:
+    // https://developer.bitcoin.org/reference/transactions.html#txout-a-transaction-output
+    let miner_fee = 1.try_into().expect("amount is valid and non-negative");
+
+    let outputs = standard_coinbase_outputs(network, block_height, miner_address, miner_fee);
+    let coinbase_tx = Transaction::new_v5_coinbase(network, block_height, outputs).into();
+
+    TransactionTemplate::from_coinbase(&coinbase_tx, miner_fee)
+}
+
 /// Returns the transaction effecting and authorizing roots
 /// for `coinbase_tx` and `mempool_txs`.
 //