fix(rpc): two-slot coinbase cache prevents eviction between fake and real coinbase (#10954)

Author: mergify[bot]

zebra-rpc/CHANGELOG.md

@@ -18,6 +18,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Clarified the error message returned by `getrawtransaction` for transactions
   that are not in the mempool or best chain
   ([#11014](https://github.com/ZcashFoundation/zebra/pull/11014)).
+- Fixed coinbase cache eviction that rebuilt shielded proofs on every
+  `getblocktemplate` poll when the mempool had fee-paying transactions
+  ([#10954](https://github.com/ZcashFoundation/zebra/pull/10954)).
 
 ## [15.0.0] - 2026-07-27
 

zebra-rpc/src/methods/types/get_block_template.rs

@@ -9,6 +9,7 @@ pub mod zip317;
 mod tests;
 
 use std::{
+    collections::HashMap,
     fmt::{self},
     sync::{Arc, Mutex},
 };
@@ -558,22 +559,24 @@ impl From<zcash_address::ConversionError<&'static str>> for MinerParamsError {
     }
 }
 
-/// Caches the most recently built coinbase transaction for the next block, keyed on its height and
-/// total transaction fees.
+/// Caches recently built coinbase transactions for the next block, keyed on `(height, fee)`.
 ///
 /// `getblocktemplate` clients commonly short-poll (re-request without long polling), and building
 /// the coinbase to a shielded address re-runs an expensive Sapling/Orchard proof. The coinbase only
 /// depends on `(height, fees)` for a given miner configuration, so repeated requests within the
 /// same block can reuse the cached transaction instead of re-proving it on every call.
+///
+/// Each `getblocktemplate` call needs two coinbase transactions at the same height: a zero-fee
+/// "fake" coinbase for ZIP-317 weight estimation, and the real coinbase with actual fees. Entries
+/// from previous heights are cleared on insert to bound memory.
 #[derive(Clone, Default)]
 pub(crate) struct CoinbaseCache(
     Arc<
         Mutex<
-            Option<(
-                block::Height,
-                Amount<NonNegative>,
+            HashMap<
+                (block::Height, Amount<NonNegative>),
                 TransactionTemplate<amount::NegativeOrZero>,
-            )>,
+            >,
         >,
     >,
 );
@@ -585,18 +588,11 @@ impl CoinbaseCache {
         height: block::Height,
         fee: Amount<NonNegative>,
     ) -> Option<TransactionTemplate<amount::NegativeOrZero>> {
-        let cache = self
-            .0
+        self.0
             .lock()
-            .unwrap_or_else(|poisoned| poisoned.into_inner());
-        match &*cache {
-            Some((cached_height, cached_fee, coinbase))
-                if *cached_height == height && *cached_fee == fee =>
-            {
-                Some(coinbase.clone())
-            }
-            _ => None,
-        }
+            .unwrap_or_else(|poisoned| poisoned.into_inner())
+            .get(&(height, fee))
+            .cloned()
     }
 
     /// Stores `coinbase` as the cached transaction for `height` and `fee`.
@@ -606,18 +602,35 @@ impl CoinbaseCache {
         fee: Amount<NonNegative>,
         coinbase: TransactionTemplate<amount::NegativeOrZero>,
     ) {
-        *self
+        let mut map = self
             .0
             .lock()
-            .unwrap_or_else(|poisoned| poisoned.into_inner()) = Some((height, fee, coinbase));
+            .unwrap_or_else(|poisoned| poisoned.into_inner());
+
+        // Evict entries from previous heights so the map stays bounded.
+        map.retain(|&(h, _), _| h == height);
+        // Only 2 entries are ever useful (zero-fee fake + current real-fee coinbase), but mempool
+        // fee churn can accumulate stale entries within a block. Cap at 4 to stay well above the
+        // useful set while preventing unbounded growth. When evicting, preserve the zero-fee sizing
+        // coinbase — losing it recreates the churn this cache exists to prevent.
+        if !map.contains_key(&(height, fee)) && map.len() >= 4 {
+            let evict_key = map
+                .keys()
+                .copied()
+                .find(|&(_, f)| f != Amount::<NonNegative>::zero());
+            if let Some(key) = evict_key {
+                map.remove(&key);
+            }
+        }
+        map.insert((height, fee), coinbase);
     }
 
-    /// Discards the cached coinbase, forcing the next request to rebuild it.
+    /// Discards all cached coinbases, forcing the next request to rebuild them.
     fn clear(&self) {
-        *self
-            .0
+        self.0
             .lock()
-            .unwrap_or_else(|poisoned| poisoned.into_inner()) = None;
+            .unwrap_or_else(|poisoned| poisoned.into_inner())
+            .clear();
     }
 }
 
@@ -680,6 +693,10 @@ where
     /// address on a cloned handler, without changing the configured default.
     pub fn set_miner_params(&mut self, miner_params: MinerParams) {
         self.miner_params = Some(miner_params);
+        // Cached coinbases pay the previous miner address, and this handler shares
+        // its cache with the handler it was cloned from. Detach to a fresh cache so
+        // neither handler can serve a coinbase built for the other's address.
+        self.coinbase_cache = CoinbaseCache::default();
     }
 
     /// Returns a handle to the coinbase transaction cache.

zebra-rpc/src/methods/types/get_block_template/tests.rs

@@ -201,6 +201,162 @@ fn coinbase_cache_reuses_built_coinbase() {
     assert!(cache.get(height, fee).is_none(), "a cleared cache misses");
 }
 
+/// Verifies the fix for #10907: the multi-entry coinbase cache retains both the zero-fee fake
+/// coinbase (used for ZIP-317 weight sizing) and the real-fee coinbase simultaneously, so
+/// `getblocktemplate` doesn't rebuild shielded proofs on every short-poll.
+#[test]
+fn coinbase_cache_retains_both_fake_and_real_fee_entries() {
+    use super::CoinbaseCache;
+
+    let height = Height(1_000_000);
+    let zero_fee = Amount::zero();
+    let real_fee: Amount<zebra_chain::amount::NonNegative> =
+        Amount::try_from(10_000).expect("valid amount");
+
+    let cache = CoinbaseCache::default();
+
+    // Simulate what getblocktemplate does: store a fake coinbase at zero fee (ZIP-317 sizing),
+    // then store the real coinbase at the actual fee.
+    let fake_coinbase = TransactionTemplate::new_coinbase(
+        &Network::Mainnet,
+        height,
+        &MinerParams::from(
+            Address::decode(
+                &Network::Mainnet,
+                default_miner_address(
+                    zebra_chain::parameters::NetworkKind::Mainnet,
+                    &MinerAddressType::Sapling,
+                ),
+            )
+            .unwrap(),
+        ),
+        zero_fee,
+    )
+    .unwrap();
+
+    let real_coinbase = TransactionTemplate::new_coinbase(
+        &Network::Mainnet,
+        height,
+        &MinerParams::from(
+            Address::decode(
+                &Network::Mainnet,
+                default_miner_address(
+                    zebra_chain::parameters::NetworkKind::Mainnet,
+                    &MinerAddressType::Sapling,
+                ),
+            )
+            .unwrap(),
+        ),
+        real_fee,
+    )
+    .unwrap();
+
+    cache.store(height, zero_fee, fake_coinbase.clone());
+    cache.store(height, real_fee, real_coinbase.clone());
+
+    // Both entries coexist — the zero-fee sizing coinbase survives the real-fee store.
+    assert_eq!(
+        cache.get(height, zero_fee),
+        Some(fake_coinbase),
+        "zero-fee fake coinbase should still be cached after storing real-fee coinbase"
+    );
+    assert_eq!(
+        cache.get(height, real_fee),
+        Some(real_coinbase),
+        "real-fee coinbase should be cached"
+    );
+
+    // Height transition: storing at a new height evicts the stale entries.
+    let next_height = Height(height.0 + 1);
+    let next_coinbase = TransactionTemplate::new_coinbase(
+        &Network::Mainnet,
+        next_height,
+        &MinerParams::from(
+            Address::decode(
+                &Network::Mainnet,
+                default_miner_address(
+                    zebra_chain::parameters::NetworkKind::Mainnet,
+                    &MinerAddressType::Sapling,
+                ),
+            )
+            .unwrap(),
+        ),
+        zero_fee,
+    )
+    .unwrap();
+
+    cache.store(next_height, zero_fee, next_coinbase.clone());
+    assert_eq!(
+        cache.get(next_height, zero_fee),
+        Some(next_coinbase),
+        "new-height entry should be cached"
+    );
+    assert!(
+        cache.get(height, zero_fee).is_none(),
+        "old-height entry should be evicted"
+    );
+}
+
+/// Verifies that fee churn beyond the cache cap (4 entries) evicts stale nonzero-fee entries
+/// while preserving the zero-fee sizing coinbase. Without this, the cap would clear the
+/// entire map — including the zero-fee entry — recreating the original #10907 churn.
+#[test]
+fn coinbase_cache_preserves_zero_fee_entry_at_capacity() {
+    use super::CoinbaseCache;
+
+    let height = Height(2_000_000);
+    let zero_fee = Amount::zero();
+    let cache = CoinbaseCache::default();
+
+    let miner_params = MinerParams::from(
+        Address::decode(
+            &Network::Mainnet,
+            default_miner_address(
+                zebra_chain::parameters::NetworkKind::Mainnet,
+                &MinerAddressType::Sapling,
+            ),
+        )
+        .unwrap(),
+    );
+
+    let make_coinbase = |fee: Amount<zebra_chain::amount::NonNegative>| {
+        TransactionTemplate::new_coinbase(&Network::Mainnet, height, &miner_params, fee).unwrap()
+    };
+
+    // Store the zero-fee sizing coinbase first.
+    let fake_coinbase = make_coinbase(zero_fee);
+    cache.store(height, zero_fee, fake_coinbase.clone());
+
+    // Fill to capacity with distinct fee values (simulating mempool fee churn).
+    for i in 1..=5u64 {
+        let fee = Amount::try_from(i * 1_000).expect("valid amount");
+        cache.store(height, fee, make_coinbase(fee));
+    }
+
+    // The zero-fee entry must survive eviction at capacity.
+    assert_eq!(
+        cache.get(height, zero_fee),
+        Some(fake_coinbase.clone()),
+        "zero-fee sizing coinbase must survive fee churn at capacity"
+    );
+
+    // Updating an existing key at capacity should not trigger eviction.
+    let fee_1k: Amount<zebra_chain::amount::NonNegative> =
+        Amount::try_from(1_000).expect("valid amount");
+    let updated_coinbase = make_coinbase(fee_1k);
+    cache.store(height, fee_1k, updated_coinbase.clone());
+    assert_eq!(
+        cache.get(height, fee_1k),
+        Some(updated_coinbase),
+        "updating an existing key should replace in place"
+    );
+    assert_eq!(
+        cache.get(height, zero_fee),
+        Some(fake_coinbase),
+        "zero-fee entry must still be present after in-place update"
+    );
+}
+
 /// From NU6.3 onward, a shielded coinbase paid to a Unified miner address with an Orchard
 /// receiver routes newly minted value into the Ironwood pool, not the Orchard pool, and remains
 /// recoverable with the consensus-required all-zero outgoing viewing key.