Skip to content

Commit 6ff34e8

Browse files
committed
fix(blockvalidation): release pooled txMap on every Block.Valid exit path
Address PR bsv-blockchain#886 review (Copilot + claude-code-review confirmed Major): the pooled txMap allocated inside checkDuplicateTransactions was only returned to the size-class pool on the success path. Any error between the allocation and the final cleanup (a checkDuplicateTransactions worker error, the txMap.Flush failure, or any validOrderAndBlessed error) returned from Block.Valid with b.txMap still pointing at the multi-GB pooled instance. At 654M-tx scale each leaked map pins ~30 GB of backing storage on the Block until GC, which may not run for a long time if the Block is held by the lastValidatedBlocks cache. Extract the cleanup into a (b *Block).releaseTxMap helper (nil-safe, idempotent, handles disk-backed / pooled-in-memory / generic Closer) and call it via `defer` from Block.Valid immediately before invoking checkDuplicateTransactions. The defer fires on every exit path, so errors no longer leak the map. Also address Copilot Minor on batchKeysPool: document the workload assumption (uniform SetMinedMulti batch sizes within a single Store's lifetime) and explain when size-class bucketing would become necessary.
1 parent 63888c4 commit 6ff34e8

2 files changed

Lines changed: 35 additions & 5 deletions

File tree

model/Block.go

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -537,6 +537,15 @@ func (b *Block) Valid(ctx context.Context, logger ulogger.Logger, subtreeStore S
537537
// CVE-2012-2459 guard — runs unconditionally so future callers passing nil subtreeStore
538538
// don't silently skip dedup. checkDuplicateTransactions iterates SubtreeSlices in memory
539539
// and does not need the subtree store directly.
540+
//
541+
// b.txMap is allocated inside checkDuplicateTransactions (possibly mid-execution
542+
// when a worker fails). The deferred releaseTxMap below ensures the pooled
543+
// in-memory variant is returned to the pool on *every* exit path — including
544+
// errors from checkDuplicateTransactions, the flush below, and
545+
// validOrderAndBlessed. releaseTxMap is nil-safe, idempotent, and handles
546+
// all three b.txMap variants (disk-backed, pooled in-memory, generic Closer).
547+
defer b.releaseTxMap()
548+
540549
err = b.checkDuplicateTransactions(ctx, logger, settings.Block.CheckDuplicateTransactionsConcurrency, settings.Block.DiskMapDirs)
541550
if err != nil {
542551
return false, err
@@ -570,24 +579,36 @@ func (b *Block) Valid(ctx context.Context, logger ulogger.Logger, subtreeStore S
570579
}
571580
}
572581

573-
// close and release the txMap
582+
return true, nil
583+
}
584+
585+
// releaseTxMap returns b.txMap to the pool (in-memory variant) or closes the
586+
// disk-backed map, then nils b.txMap. Idempotent and nil-safe: safe to call
587+
// multiple times or before b.txMap is ever assigned. Invoked via defer from
588+
// Block.Valid so the pooled map is reclaimed on every exit path, including
589+
// errors during checkDuplicateTransactions or validOrderAndBlessed.
590+
func (b *Block) releaseTxMap() {
591+
if b.txMap == nil {
592+
return
593+
}
594+
574595
if diskMap, ok := b.txMap.(*DiskTxMapUint64); ok {
575596
ReportTxMapStats(diskMap.Stats())
576597
_ = diskMap.Close()
577598
ClearTxMapStats()
578599
} else if poolable, ok := b.txMap.(*txmap.SplitSwissMapUint64); ok {
579600
// Return the pooled in-memory map for reuse on the next block.
580-
// transactionCountUint32 was computed at Get time; recompute the
581-
// same way so the map lands in the matching size-class pool.
601+
// b.TransactionCount was set in GetAndValidateSubtrees before
602+
// checkDuplicateTransactions ran, so it matches the value used at
603+
// GetTxMap time and the map lands in the correct size-class pool.
582604
if n, err := safeconversion.Uint64ToUint32(b.TransactionCount); err == nil {
583605
PutTxMap(poolable, n)
584606
}
585607
} else if closer, ok := b.txMap.(io.Closer); ok {
586608
_ = closer.Close()
587609
}
588-
b.txMap = nil
589610

590-
return true, nil
611+
b.txMap = nil
591612
}
592613

593614
// https://en.bitcoin.it/wiki/BIP_0034

stores/utxo/aerospike/set_mined.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,15 @@ func putBatchRecordsSlice(s *[]aerospike.BatchRecordIfc) {
109109
// different (ns, set) — Key only exposes SetValue, which recomputes digest from
110110
// the existing setName.
111111
//
112+
// Workload assumption: SetMinedMulti batch sizes are relatively uniform within
113+
// a single Store's lifetime (Store lifetime == application lifetime, ~one Store
114+
// per deployment). Under that assumption the pool's per-Store scoping is the
115+
// right trade-off: a large batch seeds the pool with a large backing array
116+
// that subsequent batches reuse, amortizing allocation. sync.Pool's per-GC
117+
// drainage ages out the slice if the workload permanently shifts to small
118+
// batches. If multi-tenant or highly variable batch sizes become common,
119+
// switch to size-class bucketing analogous to model.GetTxMap.
120+
//
112121
// On return, the slice length equals `capacity`. Entries from a freshly allocated
113122
// slice are nil; the caller must initialize each via aerospike.NewKey before use.
114123
// Entries retained from a prior batch are non-nil and must be reset via SetValue.

0 commit comments

Comments
 (0)