zkvm: a full mempool gives up its cheapest transaction, not its best

Eviction popped the heap. The heap is ordered highest-fee-first, so a full pool
discarded its best payer — inverting the fee market and throwing away exactly
the transaction the next block would have taken first. It then removed that same
transaction a second time by ID. The variable was called lowest.

A full pool now gives up whatever pays least, and refuses an arrival that is
itself the cheapest, since accepting it would mean dropping a better transaction
for a worse one. The heap's root is the most expensive, so the cheapest is found
by walking it; the pool is bounded and this runs only when it is full.

MempoolTx.priority was written in three places and read in none. That would be
harmless dead weight except that GetPendingTransactions sorts a copy of the heap
which holds the same pointers, so sorting wrote through them while holding only
a read lock. Removing the field removes the race with it — the detector reports
it as a write in TxHeap.Swap once the field is put back.

Co-authored-by: Hanzo Dev <[email protected]>

zkvm/mempool.go

@@ -40,7 +40,6 @@ type MempoolTx struct {
 	tx         *Transaction
 	addedAt    time.Time
 	feePerByte uint64
-	priority   int // For heap ordering
 }
 
 // NewMempool creates a new mempool
@@ -75,19 +74,23 @@ func (mp *Mempool) AddTransaction(tx *Transaction) error {
 		}
 	}
 
-	// Check mempool size limit
-	if len(mp.txs) >= mp.maxSize {
-		// Remove lowest priority transaction
-		if mp.txHeap.Len() > 0 {
-			lowest := heap.Pop(&mp.txHeap).(*MempoolTx)
-			mp.removeTxNoLock(lowest.tx.ID)
-		}
-	}
-
 	// Fixed size estimate for fee calculation
 	txSize := uint64(256)
 	feePerByte := tx.Fee / txSize
 
+	// A full pool has to give something up, and it has to be whatever pays
+	// least. Dropping the best payer instead inverts the fee market and
+	// discards exactly the transaction a block would have taken first. If the
+	// arrival is itself the cheapest, the pool already holds better and there
+	// is nothing to gain by swapping.
+	if len(mp.txs) >= mp.maxSize {
+		cheapest := mp.cheapest()
+		if cheapest == nil || cheapest.feePerByte >= feePerByte {
+			return errors.New("mempool is full and the transaction pays less than what it would displace")
+		}
+		mp.removeTxNoLock(cheapest.tx.ID)
+	}
+
 	// Create mempool entry
 	mempoolTx := &MempoolTx{
 		tx:         tx,
@@ -130,6 +133,20 @@ func (mp *Mempool) RemoveTransaction(txID ids.ID) {
 	mp.removeTxNoLock(txID)
 }
 
+// cheapest returns the transaction paying least per byte, which is the one a
+// full pool gives up. The heap is ordered highest-first, so the cheapest sits
+// somewhere among its leaves rather than at its root; the pool is bounded and
+// this runs only when it is full, so walking it is the whole of the work.
+func (mp *Mempool) cheapest() *MempoolTx {
+	var out *MempoolTx
+	for _, candidate := range mp.txHeap {
+		if out == nil || candidate.feePerByte < out.feePerByte {
+			out = candidate
+		}
+	}
+	return out
+}
+
 // removeTxNoLock removes a transaction without locking (internal use)
 func (mp *Mempool) removeTxNoLock(txID ids.ID) {
 	mempoolTx, exists := mp.txs[txID]
@@ -249,15 +266,10 @@ func (h TxHeap) Less(i, j int) bool {
 
 func (h TxHeap) Swap(i, j int) {
 	h[i], h[j] = h[j], h[i]
-	h[i].priority = i
-	h[j].priority = j
 }
 
 func (h *TxHeap) Push(x interface{}) {
-	n := len(*h)
-	tx := x.(*MempoolTx)
-	tx.priority = n
-	*h = append(*h, tx)
+	*h = append(*h, x.(*MempoolTx))
 }
 
 func (h *TxHeap) Pop() interface{} {

zkvm/mempool_test.go

@@ -5,6 +5,7 @@ package zkvm
 
 import (
 	"context"
+	"sync"
 	"testing"
 	"time"
 
@@ -58,3 +59,62 @@ func TestMempoolTellsConsensusThereIsWork(t *testing.T) {
 		t.Fatal("a transaction was accepted and consensus was never told; this chain cannot produce a block")
 	}
 }
+
+// TestFullPoolGivesUpTheCheapest. Eviction used to pop the heap, which is
+// ordered highest-first, so a full pool discarded its best payer — inverting the
+// fee market and throwing away exactly the transaction a block would have taken
+// first. Fees are divided by a fixed 256-byte estimate, so they must differ by
+// more than that to differ in priority at all.
+func TestFullPoolGivesUpTheCheapest(t *testing.T) {
+	const unit = 256
+	mp := NewMempool(2, log.NoLog{})
+
+	best := shieldedTransfer(1000*unit, 1)
+	dust := shieldedTransfer(1*unit, 2)
+	middle := shieldedTransfer(100*unit, 3)
+
+	require.NoError(t, mp.AddTransaction(best))
+	require.NoError(t, mp.AddTransaction(dust))
+	require.NoError(t, mp.AddTransaction(middle), "the pool is full but the arrival outbids the dust")
+
+	require.True(t, mp.HasTransaction(best.ID), "the best payer must survive a full pool")
+	require.True(t, mp.HasTransaction(middle.ID))
+	require.False(t, mp.HasTransaction(dust.ID), "the cheapest is the one to give up")
+}
+
+// A full pool already holding better has nothing to gain by swapping, and
+// accepting the arrival would mean dropping a better transaction for a worse one.
+func TestFullPoolRefusesAnArrivalWorseThanItHolds(t *testing.T) {
+	const unit = 256
+	mp := NewMempool(2, log.NoLog{})
+	require.NoError(t, mp.AddTransaction(shieldedTransfer(1000*unit, 1)))
+	require.NoError(t, mp.AddTransaction(shieldedTransfer(900*unit, 2)))
+
+	pauper := shieldedTransfer(1*unit, 3)
+	require.Error(t, mp.AddTransaction(pauper))
+	require.False(t, mp.HasTransaction(pauper.ID))
+	require.Equal(t, 2, mp.Size(), "a refused arrival must not change what the pool holds")
+}
+
+// Two callers asking what to build at once must not write to each other's
+// transactions. GetPendingTransactions sorts a copy of the heap, but the copy
+// holds the same pointers, so anything the sort wrote through them was a write
+// under a read lock. Run with -race.
+func TestReadingThePoolDoesNotWriteToIt(t *testing.T) {
+	mp := NewMempool(64, log.NoLog{})
+	for i := 0; i < 16; i++ {
+		require.NoError(t, mp.AddTransaction(shieldedTransfer(uint64(i+1)*256, byte(i))))
+	}
+
+	var wg sync.WaitGroup
+	for i := 0; i < 8; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			if got := mp.GetPendingTransactions(8); len(got) != 8 {
+				t.Errorf("asked for 8 transactions, got %d", len(got))
+			}
+		}()
+	}
+	wg.Wait()
+}