Pull request overview

This PR refactors TxMetaCache to dispatch through a small internal backend interface and introduces an opt-in experimental pointer-backed cache backend (BucketType.Pointer) intended to reduce allocation churn on cache hits by storing *meta.Data directly. It also removes unused height-appending logic and drops the hitOldTx metric that depended on it.

Changes:

• Add cacheBackend interface and adapt the existing ImprovedCache via improvedCacheBackend; update TxMetaCache to use backend dispatch.
• Introduce PointerCache (sharded map + per-bucket FIFO ring eviction) plus tests/benchmarks, selectable via BucketType.Pointer.
• Update consumers/tests to use the new GetMetaCached(ctx, hash) (*meta.Data, bool) API; remove height-encoding + related metric.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Overview

Introduces an opt-in PointerCache backend storing *meta.Data directly in sharded map[uint64]*meta.Data shards with per-bucket FIFO eviction. Refactors TxMetaCache to dispatch through a new internal cacheBackend interface. Collapses GetMetaCached/GetMetaCachedWithBuffer into a single zero-buffer API returning (*meta.Data, bool). Drops the height-encoding/expiration dead code path (height was appended on every write but never read by any eviction logic).

Benchmarks show ~4.5× faster Set, ~11.5× faster Get, and zero GCs vs. the byte backend. The default BucketType is unchanged; Pointer mode is opt-in.

🚨 Critical Issues

1. Hash collisions in PointerCache silently corrupt data

stores/txmetacache/pointer_cache.go (the pointerBucket map and Get):

The map is keyed solely by xxhash.Sum64(hash[:]) — the original 32-byte hash is never stored or compared on read:

type pointerBucket struct {
    m map[uint64]*meta.Data   // uint64 key, no full-hash verification
    ...
}

func (c *PointerCache) Get(hash chainhash.Hash) (*meta.Data, bool) {
    k := xxhash.Sum64(hash[:])
    b := c.bucketFor(k)
    b.mu.RLock()
    d, ok := b.m[k]
    b.mu.RUnlock()
    return d, ok
}

Compare to ImprovedCache.bucketNative.Get (improved_cache.go:971), which verifies the full key on hit:

if string(key) == string(chunk[idx:idx+keyLen]) {

For a 64-bit hash with N cached entries, expected collisions are ~N² / 2⁶⁵. At ~10M live entries the per-pair probability multiplied by pair count is ~3×10⁻⁶ — across a fleet running for days this will happen. Impact: tx A's lookup returns tx B's metadata (Fee, SizeInBytes, IsCoinbase, TxInpoints), feeding directly into validation. This is a consensus-correctness risk if PointerCache is ever wired into a path that influences acceptance.

Fix options:

• Use map[chainhash.Hash]*meta.Data — [32]byte is comparable and works as a map key. Slightly slower lookups but eliminates the collision class entirely. Likely still beats the byte path by >10×.
• Or store (chainhash.Hash, *meta.Data) in a struct and compare the hash on read.

Either is a small change. Given the PR positions PointerCache as experimental and opt-in, this is fixable before any real adoption — but it MUST be fixed before anyone flips the switch.

⚠️ Moderate Issues

2. Silent error swallowing in improvedCacheBackend.Get

stores/txmetacache/improved_cache_meta.go (the new Get):

if err := b.cache.Get(&scratch, hash[:]); err != nil {
    if errors.Is(err, errors.ErrNotFound) {
        return nil, false
    }
    return nil, false   // any other error: silently treated as miss
}
...
if err := meta.NewMetaDataFromBytes(scratch, data); err != nil {
    return nil, false   // deserialise failure: silently treated as miss
}

The old GetMetaCachedWithBuffer returned non-NotFound errors to the caller, who logged them (e.g., processTxMetaUsingCache.go's Warnf("error retrieving txMeta…")). After this PR, real cache corruption or deserialiser failures become invisible misses. The cacheBackend.Get signature is (*meta.Data, bool) with no error channel.

Fix: Either log inside improvedCacheBackend.Get (e.g., debug-level — watch for log floods if corruption is repeated) or change the interface to surface errors. Quietly degrading correctness to a cache miss is exactly the kind of regression that's hard to detect in production.

3. GetMeta does shallow struct copy — aliases slices with the cache in Pointer mode

stores/txmetacache/txmetacache.go (post-PR GetMeta):

if cached, ok := t.cache.Get(*hash); ok {
    t.metrics.hits.Add(1)
    *data = *cached                       // shallow copy — slice headers aliased
    data.BlockIDs = make([]uint32, 0)
    return nil
}

*data = *cached copies the struct but slice headers in meta.Data (notably TxInpoints.ParentTxHashes) still point at the cache's backing arrays. In byte mode this is fine (each Get allocates a fresh *meta.Data with fresh slices). In Pointer mode, callers of GetMeta now silently share slice memory with every other reader and with the cache itself. The pointer-mode contract ("treat returned *meta.Data as read-only") is documented on cacheBackend.Get, but GetMeta's callers don't see that interface and have no signal that their data argument is now an alias.

This is a quiet semantic change that's easy to miss when reviewing future callers. Worth either deep-copying slices here, or documenting at the GetMeta doc comment that the result must be treated as read-only.

4. Del-then-Set leaks FIFO ring slots

stores/txmetacache/pointer_cache.go (Del and insert):

Del removes from the map but doesn't touch the ring:

func (c *PointerCache) Del(key []byte) {
    k := xxhash.Sum64(key)
    b := c.bucketFor(k)
    b.mu.Lock()
    delete(b.m, k)        // ring slot still references k
    b.mu.Unlock()
}

If the same key is later re-inserted, insert sees exists == false and allocates a fresh ring slot. The old ring slot still names the (now-deleted) key, so when its eviction turn arrives we delete(b.m, evict) — a no-op — but the ring has effectively wasted that slot for a full ring lap.

Worse, the pattern Set → Del → Set → Del → Set can fill a bucket's ring with duplicate references to the same key, silently reducing effective capacity. This matches a real workflow: SetCache → Delete (from SetMinedMulti eviction) → SetCache again on the same hash.

Fix: In Del, mark the ring slot stale (e.g., sentinel 0) so it's skipped on eviction; or accept and document the trade-off.

ℹ️ Minor Issues / Nits

5. Prometheus metric hit_old_tx deleted with no migration note

stores/txmetacache/metrics.go

Genuinely dead code internally — the height-based expiration was never wired to eviction — but the Prometheus gauge teranode_tx_meta_cache_hit_old_tx is a public surface. Any Grafana dashboard, alert, or recording rule referencing it will break on deploy. Worth a release-notes line.

6. getTxMetaCacheReadBuffer / putTxMetaCacheReadBuffer pool become dead code

stores/txmetacache/txmetacache.go

After this PR neither GetMetaCached nor GetMeta calls these helpers. The pool helpers and txMetaCacheReadBufferPool should be removed for clarity (along with txMetaCacheReadBufferMaxRetain if no longer needed by improvedCacheBackend), unless intentionally kept for future callers.

7. Removed warning log "txMetaCache empty for %s"

Previously emitted when a hit returned zero bytes. Corner-case, but it had operational value as a "corruption smoke" signal. Worth re-adding inside improvedCacheBackend.Get once it's logging non-NotFound errors (issue #2).

8. pointerCacheAvgEntryBytes = 300 is acknowledged guesswork

stores/txmetacache/pointer_cache.go

The PR description flags this. A tx with many parents (>5–6) overflows the estimate and the bucket holds fewer entries than the MB budget suggests. Consider deriving per-bucket entry count from observed runtime.MemStats at warmup, or at least logging the effective capacity at construction so operators can audit it.

9. Benchmark comment uses "Bitcoin transactions"

stores/txmetacache/pointer_cache_bench_test.go

"Two parents is a reasonable median for Bitcoin transactions…"

Prefer "BSV transactions" or just "transactions" in this codebase.

10. _ = entries[0].hash keepalive hack

stores/txmetacache/pointer_cache_bench_test.go (benchMemoryEfficiency)

Use runtime.KeepAlive(entries) for clarity.

✅ Strengths

• Compile-time interface checks (var _ cacheBackend = (*PointerCache)(nil)).
• Concurrent reader/writer test (TestPointerCache_ConcurrentReadersAndWriters) and deterministic FIFO eviction test (TestPointerCache_FIFOEvictionWhenRingFull).
• Benchmarks cover Set/Get/concurrent-read-heavy/GC-pause/memory-efficiency with both backends.
• The dead-code removal (height encoding, hitOldTx internals) is honestly dead.
• Interface adapter (improvedCacheBackend) preserves existing tests/benchmarks against ImprovedCache's native byte API.
• PR description is unusually thorough — caveats, alloc counts, and untouched production paths all called out.

Recommendation

Issue #1 (collision-safety) is non-negotiable before any candidacy for production use: a 64-bit hash with no full-key verification will produce silent wrong-data returns at fleet scale, and the cached fields feed validation. Issues #2, #3, and #4 are quiet correctness/observability regressions that should be addressed in the same pass. The rest is bookkeeping.

The architecture and benchmark results are genuinely compelling — the byte-mode allocation cost is real, and pointer-native storage is the right idea. Worth landing once the collision class is closed and the silent-error paths are loud again.

Request changes. Findings beyond ordishs's review.

1. setMinedInCacheParallel (txmetacache.go:709-738) silently drops BlockIDs in pointer mode. Path: retrieve txMeta → append blockID → SetCache → metadataOnly() strips BlockIDs. The append is a no-op. Anything gating on block membership returns stale answers.

2. PR body says appendHeightToValue was dropped — it wasn't. Still at line 770, called in all four SetCacheMulti* methods. Comment added but the description is factually wrong. In pointer mode trailing 4 bytes are tolerated by NewMetaDataFromBytes, but len+4 allocation per SetCacheFromBytes partially undermines the zero-alloc claim for byte ingestion.

3. metadataOnly() value-copies TxInpoints but shares the backing slice arrays — GetMeta's *data = *cached then propagates aliasing to all callers. Reinforces ordishs's #3: callers must treat result read-only, nothing enforces it. Detectable with -race.

4. GC bench at 64 MB / 100K entries — 12× below production (256 MB / ~800K). Pause numbers not representative at scale.

5. SetMultiSequentialWithHashes ignores supplied hashes in pointer mode (keys by embedded hash). Divergence from byte-backend keying — latent hazard for callers relying on consistent semantics across modes.

Overall

Pointer mode is well-designed, well-tested, and explicitly opt-in. Headline number (11× Get speedup, 0 GCs) is meaningful for a hot path. Approving with changes — one issue should be fixed before pointer mode is enabled in production.

Required before flipping pointer mode on

SetMultiSequential contract violation in PointerCache

stores/txmetacache/pointer_cache.go:295-301. The interface contract (cache_backend.go:30-35) says SetMultiSequential is "without goroutine fan-out" — the byte path's whole purpose is to skip errgroup spawning for partition-aligned receivers. The pointer implementation forwards to SetMultiFromBytes, which does fan out via errgroup per non-empty bucket.

Under pointer mode, the v2 Kafka receiver's "sequential" path will multiply goroutines: N partition workers × ~M buckets each. This defeats the design. Fix is to implement SetMultiSequential as a flat loop over keys/values (mirroring the byte path's small-batch branch) and skip the errgroup branch entirely.

Recommended

PR description is stale vs. the actual diff

Description claims appendHeightToValue, noOfBlocksToKeepInTxMetaCache, and hitOldTx are dropped. None are dropped:

• appendHeightToValue is still called on every byte-mode insert (txmetacache.go:293, 315, 342, 365)
• noOfBlocksToKeepInTxMetaCache is still computed and stored but never read (txmetacache.go:233, 246) — dead code
• hitOldTx is still in metrics struct and reported via Prometheus (metrics.go:131, 146, 165)

Either update the description to reflect what was reverted, or remove the unused pieces as originally intended. The claim "Saves a 4-byte alloc + per-Set GetBlockHeight() call" is no longer accurate.

GetMeta shallow-copies into caller's struct, aliasing the cache in pointer mode

stores/txmetacache/txmetacache.go:428-432:

if cached, ok := t.cache.Get(*hash); ok {
    t.metrics.hits.Add(1)
    *data = *cached
    data.BlockIDs = make([]uint32, 0)

*data = *cached is a struct copy — data.TxInpoints.ParentTxHashes now aliases the cache entry's backing slice. In byte mode this is benign (the returned *meta.Data is freshly allocated). In pointer mode the caller's data shares slices with the cache.

The read-only contract is documented on GetMetaCached, but GetMeta takes a caller-provided *meta.Data — the conventional "mine to mutate" pattern. A caller doing data.TxInpoints.ParentTxHashes = append(...) would corrupt the cache for every concurrent reader. Either document the aliasing rule on GetMeta or deep-clone TxInpoints on the GetMeta path under pointer mode.

metadataOnly fallback path leaks aliased TxInpoints

stores/txmetacache/pointer_cache.go:162-190. When Serialize() or NewTxInpointsFromBytes fails, the function falls through to cloned.TxInpoints = data.TxInpoints — a struct copy sharing ParentTxHashes with the caller. The comment says this is "preferable to dropping the entry entirely," but if the cause is real corruption, holding an entry whose slice is externally mutable is worse than dropping. Consider returning nil and bumping a counter so the rate is observable.

pointerCacheAvgEntryBytes = 320 is workload-dependent

Acknowledged in the PR caveats. 320 is calibrated against 2-parent synthetic txs; production txs with >5-6 parents will overflow and the cache holds fewer entries than budgeted. The MB sizing parameter has a hidden parent-count multiplier. Suggestions:

• Track actual entries-per-MB in UpdateStats and expose it
• Or, after the first ~1k inserts, recompute an average entry size and resize the per-bucket rings

Low-priority

• Ring walk under lock when entries are stale (pointer_cache.go:114-131): worst case after a SetMinedMulti burst is a linear walk of the ring under the bucket lock. Worth a metric (ring_walk_distance_p99 or similar) so the operator can see if this becomes a tail-latency contributor.
• Pointer mode still pays appendHeightToValue cost even though the deserializer ignores trailing bytes. Skip the append when bucketType == Pointer.
• Missing test for improvedCacheBackend.Get corruption path — the unmarshalErrors eviction is fail-safe code that should have a regression test.

Strengths

• Compile-time interface assertion catches contract drift at build time
• Defensive cloning in metadataOnly — TxInpoints round-trips through serialize/deserialize so the cache never aliases caller-owned backing slices. Exactly the right shape for a shared-pointer cache.
• Per-shard FIFO eviction is simple and predictable; lock granularity matches the byte path
• TestPointerCache_DelDoesNotLeakRingCapacity is exactly the right shape — pins a subtle invariant with a deterministic same-bucket collision setup
• Benchmark coverage is more thorough than most PRs

Re-review at d5e8118. Most prior findings addressed, but the BlockIDs blocker is still unfixed.

Still outstanding — must fix:

setMinedInCacheParallel (txmetacache.go:735-765) wasn't touched by any of the fixup commits. Path is still: append blockID → SetCache → cache.Set → metadataOnly() strips BlockIDs. Mined updates via this path don't persist in pointer mode. Two-option fix:

• (A) Replace SetCache with cache.Del to force a fresh store fetch on next read.
• (B) Skip the SetCache call when BlockIDs are already set, matching byte-mode semantics.

Resolved:

• TxInpoints aliasing — metadataOnly and GetMeta deep-clone via cloneTxInpoints (Serialize + NewTxInpointsFromBytes). TestPointerCache_SetDoesNotAliasCallerSlice pins it. Race tests green.
• SetMultiSequentialWithHashes ignored-hash divergence — documented as accepted (both backends produce identical end-state via re-derived bucket from key bytes).
• appendHeightToValue retention — documented with rationale in code, accepted.
• ordishs's SetMultiSequential fan-out — fixed, flat serial loop, test added.
• ordishs's Del orphan ring slot — fixed, stale-slot walk in insert(), test added.
• 64-bit hash collision class — keys switched to full chainhash.Hash.

Still worth noting (not blocking):

• GC bench at 64 MB / 100K entries — 12× below prod load (256 MB / 800K). The headline GC pause numbers in the PR body aren't representative.

Approve at cb606b158a. BlockIDs blocker correctly fixed.

ae4d4c4386 goes further than my suggested Option A. The whole Get+append+SetCache dance in setMinedInCacheParallel is gone, replaced with a single t.Delete(ctx, hash) per hash. blockID param is now _ uint32 (API symmetry only).

End-to-end in pointer mode: Delete evicts stale entry → next GetMeta misses cache → falls through to utxoStore.GetMeta → returns updated BlockIDs from store → len(data.BlockIDs) != 0 so SetCache is skipped and the entry stays out of cache. Matches SetMinedMulti semantics. Bug resolved.

SetMinedMultiParallel_EvictsCache test pins the eviction contract.

The 5 main merges (742793902, 958f99088, bb7e4f049, d2e5e09aa, 50822d938) are in unrelated areas (orphan pool, opcodeInvert, p2p fee policy, testcontainers deps, dashboards) with zero interaction with txmetacache.

Worth flagging for future maintainers (not a blocker)

SetMinedMultiParallel is now a cache-only operation — it never calls utxoStore.SetMinedMulti. The "store written first, then evict cache" invariant is the caller's responsibility. There are zero production callers today (tests only). When a real caller lands, it must follow the same pattern SetMinedMulti uses: write to store first, then call this to evict. Worth a one-line godoc on the function to lock that in.