Skip to content

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

Open
oxarbitrage wants to merge 5 commits into
mainfrom
fix/coinbase-cache-eviction
Open

fix(rpc): two-slot coinbase cache prevents eviction between fake and real coinbase#10954
oxarbitrage wants to merge 5 commits into
mainfrom
fix/coinbase-cache-eviction

Conversation

@oxarbitrage

Copy link
Copy Markdown
Contributor

Summary

  • Each getblocktemplate call needs two cached coinbases at the same height: a zero-fee fake one for ZIP-317 weight estimation and the real one with actual fees
  • The single-slot CoinbaseCache evicts one when storing the other, so zebrad rebuilds both shielded proofs (Sapling/Orchard) on every short-poll when the mempool has fee-paying transactions
  • Replaces the single Option slot with a HashMap keyed on (height, fee) so both entries coexist, eliminating redundant proof work
  • Entries from previous heights are evicted on insert, and the map is capped at 4 entries to prevent unbounded growth from fee churn

Closes #10907

@Zk-nd3r could you review/test this when you get a chance?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the RPC coinbase cache to retain fake and real-fee coinbases simultaneously, avoiding repeated shielded proof generation.

Changes:

  • Replaces the single cache slot with a bounded HashMap.
  • Adds coexistence and height-eviction tests.
  • No consensus or serialization impact.

The cache-cap logic has an off-by-one eviction issue, and test descriptions remain outdated. The PR description also lacks test-execution evidence.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
zebra-rpc/src/methods/types/get_block_template.rs Implements the multi-entry coinbase cache.
zebra-rpc/src/methods/types/get_block_template/tests.rs Tests dual entries and height eviction.

Comment thread zebra-rpc/src/methods/types/get_block_template.rs Outdated
Comment thread zebra-rpc/src/methods/types/get_block_template/tests.rs Outdated
Comment thread zebra-rpc/src/methods/types/get_block_template/tests.rs Outdated
@oxarbitrage oxarbitrage added C-bug Category: This is a bug A-rpc Area: Remote Procedure Call interfaces P-Medium ⚡ labels Jul 14, 2026
@Zk-nd3r

Zk-nd3r commented Jul 14, 2026

Copy link
Copy Markdown

Reviewed and tested head 1c551e8146852ce62537829802ba4ec7ff40b06c:

cargo test -p zebra-rpc coinbase_cache_evicts_fake_coinbase_when_real_fee_stored --lib
1 passed; 0 failed; 79 filtered out

The (height, fee) map fixes #10907's core fake/real eviction path.

One edge remains: map.len() > 4 is checked before insertion, so the fifth distinct fee is retained and the sixth clears the entire map, including the zero-fee sizing coinbase. That temporarily recreates the churn.

Suggested adjustment:

  • when inserting a new key at len() >= 4, preserve the zero-fee entry and evict a nonzero-fee entry;
  • cover fifth/sixth-fee churn and replacement of an existing key at capacity;
  • rename the new test and stale BUG comments to describe post-fix coexistence.

Hermetic unit test only. No funds, mainnet activity, or live miner calls. I'll rerun against the updated head.

@oxarbitrage

Copy link
Copy Markdown
Contributor Author

Thanks for the review and test run! All three suggestions addressed in 9b11aa6:

  • Cap logic now uses >= 4 with a contains_key guard — when evicting, we remove a nonzero-fee entry instead of clearing the map, so the zero-fee sizing coinbase always survives.
  • Added coinbase_cache_preserves_zero_fee_entry_at_capacity covering the churn edge case (5 distinct fees) and in-place key replacement at capacity.
  • Renamed the test and removed the stale BUG comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-rpc Area: Remote Procedure Call interfaces C-bug Category: This is a bug P-Medium ⚡

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants