getblocktemplate coinbase cache churns between fake and real-fee coinbases when mempool fees are nonzero

[Zk-nd3r]

Hi Zebra team - I think PR #10847 only fixes the shielded-coinbase re-proving cost for the empty-mempool case. With any fee-paying mempool transaction selected into the template, the new CoinbaseCache appears to churn between two keys inside every getblocktemplate call, so a short-polling miner still re-proves both the fake sizing coinbase and the final coinbase on each request.

This is not a consensus or security issue. It is a miner-facing performance regression / incomplete cache fix.

What I am seeing

The cache added in #10847 is a single slot:

Option<(block::Height, Amount<NonNegative>, TransactionTemplate<amount::NegativeOrZero>)>

It is keyed by (height, fee), but a normal non-empty block-template build uses two different fee keys:

1. select_mempool_transactions() asks for/builds a fake coinbase at Amount::zero() so ZIP-317 transaction selection can account for coinbase size and sigops.
2. BlockTemplateResponse::new_internal() then computes txs_fee from the selected transactions and asks for/builds the real coinbase at that nonzero fee.

Because the cache has only one slot, the zero-fee fake coinbase and the real-fee coinbase evict each other:

• call N ends with cache key (height, real_fee);
• call N+1 ZIP-317 sizing looks up (height, 0), misses, builds/proves the fake coinbase, and stores (height, 0);
• final template assembly looks up (height, real_fee), misses, builds/proves the real coinbase, and stores (height, real_fee);
• repeat on the next short-poll request.

So the cache only reliably helps when txs_fee == 0, because then the fake and real coinbase share the same key.

Why this matters

For a transparent mining address this is mostly hidden. For a shielded miner address, both coinbase builds can involve expensive Sapling/Orchard/Ironwood proving. Under a fee-paying mempool, a short-polling miner can still pay the same repeated proof cost that #10847 intended to remove.

This is especially relevant for mining pools and for the NU6.3/Ironwood testnet path, where shielded coinbase behavior is part of the readiness surface.

Relevant code

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

  • CoinbaseCache is a single Option<(height, fee, coinbase)>.
  • BlockTemplateResponse::new_internal() looks up/stores by (height, txs_fee).

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

  • select_mempool_transactions() looks up/stores the fake coinbase by (height, Amount::zero()).

• zebra-rpc/src/methods.rs

  • get_block_template() calls select_mempool_transactions(... Some(&coinbase_cache)), then calls BlockTemplateResponse::new_internal(... Some(self.gbt.coinbase_cache()), ...) for the same template.

Suggested reproduction

1. Configure Zebra mining to a shielded address.
2. Put at least one fee-paying transaction in the mempool so selected template fees are nonzero.
3. Short-poll getblocktemplate.
4. Instrument TransactionTemplate::new_coinbase() or cache hit/miss logging.

Expected: after the first request for a given (height, selected_fees), subsequent short-poll requests should not rebuild either coinbase unless the template inputs change.

Actual source-level behavior: each request alternates the single cache slot between (height, 0) and (height, selected_fees), causing repeated misses for both the fake and real coinbase.

Suggested fix

Make the cache hold at least two entries per active height, so the ZIP-317 fake coinbase and the final real-fee coinbase can coexist.

The smallest patch is probably a tiny two-entry cache keyed by (height, fee):

• on get(), return an entry whose (height, fee) matches;
• on store(), replace an existing matching key if present;
• otherwise insert into an empty slot;
• if both slots are full, evict a stale-height entry first, then fall back to evicting either entry.

A small HashMap<(Height, Amount<NonNegative>), TransactionTemplate<...>> bounded to the current height would also work, but two slots should cover the current fake-vs-real template path without widening memory behavior.

The existing randomize_coinbase_data() cache clear still needs to clear all entries, because miner params changed.

Test that would catch it

Add a unit test that uses one CoinbaseCache, stores/requests both:

• (height, Amount::zero()) for the ZIP-317 fake coinbase, and
• (height, nonzero_fee) for the final template coinbase.

The test should assert that both keys still hit after both have been stored. The current single-slot implementation cannot satisfy that.

Thanks for the #10847 work - this looks like a narrow cache-shape issue rather than a problem with the underlying template logic.

[Zk-nd3r]

One minimal regression test for this is to make the cache hold the two coinbases a single template build asks for when mempool fees are nonzero:

/// The template builder creates one zero-fee coinbase for size estimation, then creates the
/// real-fee coinbase for the returned template. Both variants need to remain cached for short
/// polling to avoid re-proving both coinbases on every request when the mempool has fees.
#[test]
fn coinbase_cache_keeps_fake_and_real_fee_coinbases() {
    use super::CoinbaseCache;

    let net = Network::Mainnet;
    let height = NetworkUpgrade::Nu5
        .activation_height(&net)
        .expect("Nu5 is active on Mainnet");
    let miner_params = MinerParams::from(
        Address::decode(
            &net,
            default_miner_address(net.kind(), &MinerAddressType::Sapling),
        )
        .expect("hard-coded Sapling address is valid"),
    );
    let fake_fee = Amount::zero();
    let real_fee = Amount::new(10_000);

    let fake_coinbase = TransactionTemplate::new_coinbase(&net, height, &miner_params, fake_fee)
        .expect("valid fake-fee coinbase tx");
    let real_coinbase = TransactionTemplate::new_coinbase(&net, height, &miner_params, real_fee)
        .expect("valid real-fee coinbase tx");

    let cache = CoinbaseCache::default();
    cache.store(height, fake_fee, fake_coinbase.clone());
    cache.store(height, real_fee, real_coinbase.clone());

    assert_eq!(
        cache.get(height, fake_fee),
        Some(fake_coinbase),
        "the zero-fee sizing coinbase remains cached after storing the real-fee coinbase",
    );
    assert_eq!(
        cache.get(height, real_fee),
        Some(real_coinbase),
        "the real-fee coinbase remains cached alongside the zero-fee sizing coinbase",
    );
}

Against the current single-slot CoinbaseCache, the first assertion is the one that fails: after store(height, real_fee, ...), get(height, fake_fee) no longer hits. A fix that keeps separate entries for (height, 0) and (height, real_fee) would make both assertions pass.

[natalieesk]

Thank you for this. We'll try and reproduce it and then have a look at your suggested fix. Will get back to you shortly.