libs/clist: revert clear and detach changes while debugging (#6731)

CHANGELOG_PENDING.md

@@ -21,7 +21,6 @@ Friendly reminder: We have a [bug bounty program](https://hackerone.com/tendermi
   - [cli] \#6372 Introduce `BootstrapPeers` as part of the new p2p stack. Peers to be connected on  startup (@cmwaters)
   - [config] \#6462 Move `PrivValidator` configuration out of `BaseConfig` into its own section. (@tychoish)
   - [rpc] \#6610 Add MaxPeerBlockHeight into /status rpc call (@JayT106)
-  - [libs/CList] \#6626 Automatically detach the prev/next elements in Remove function (@JayT106)
   - [fastsync/rpc] \#6620 Add TotalSyncedTime & RemainingTime to SyncInfo in /status RPC  (@JayT106)
   - [rpc/grpc] \#6725 Mark gRPC in the RPC layer as deprecated.
   - [blockchain/v2] \#6730 Fast Sync v2 is deprecated, please use v0

internal/evidence/pool.go

@@ -489,14 +489,12 @@ func (evpool *Pool) batchExpiredPendingEvidence(batch dbm.Batch) (int64, time.Ti
 func (evpool *Pool) removeEvidenceFromList(
 	blockEvidenceMap map[string]struct{}) {
 
-	el := evpool.evidenceList
-	var nextE *clist.CElement
-	for e := el.Front(); e != nil; e = nextE {
-		nextE = e.Next()
+	for e := evpool.evidenceList.Front(); e != nil; e = e.Next() {
 		// Remove from clist
 		ev := e.Value.(types.Evidence)
 		if _, ok := blockEvidenceMap[evMapKey(ev)]; ok {
 			evpool.evidenceList.Remove(e)
+			e.DetachPrev()
 		}
 	}
 }

internal/libs/clist/clist.go

@@ -196,26 +196,21 @@ func (e *CElement) SetPrev(newPrev *CElement) {
 	e.mtx.Unlock()
 }
 
-// Detach is a shortcut to mark the given element remove and detach the prev/next elements.
-func (e *CElement) Detach() {
+func (e *CElement) SetRemoved() {
 	e.mtx.Lock()
-	defer e.mtx.Unlock()
 
 	e.removed = true
 
+	// This wakes up anyone waiting in either direction.
 	if e.prev == nil {
 		e.prevWg.Done()
 		close(e.prevWaitCh)
-	} else {
-		e.prev = nil
 	}
-
 	if e.next == nil {
 		e.nextWg.Done()
 		close(e.nextWaitCh)
-	} else {
-		e.next = nil
 	}
+	e.mtx.Unlock()
 }
 
 //--------------------------------------------------------------------------------
@@ -353,26 +348,24 @@ func (l *CList) PushBack(v interface{}) *CElement {
 	return e
 }
 
-// Remove removes the given element in the CList
+// CONTRACT: Caller must call e.DetachPrev() and/or e.DetachNext() to avoid memory leaks.
 // NOTE: As per the contract of CList, removed elements cannot be added back.
-// Because CList detachse the prev/next element when it removes the given element,
-// please do not use CElement.Next() in the for loop postcondition, uses
-// a variable ahead the for loop and then assigns the Next() element
-// to it in the loop as the postcondition.
 func (l *CList) Remove(e *CElement) interface{} {
 	l.mtx.Lock()
-	defer l.mtx.Unlock()
 
 	prev := e.Prev()
 	next := e.Next()
 
 	if l.head == nil || l.tail == nil {
+		l.mtx.Unlock()
 		panic("Remove(e) on empty CList")
 	}
 	if prev == nil && l.head != e {
+		l.mtx.Unlock()
 		panic("Remove(e) with false head")
 	}
 	if next == nil && l.tail != e {
+		l.mtx.Unlock()
 		panic("Remove(e) with false tail")
 	}
 
@@ -397,53 +390,13 @@ func (l *CList) Remove(e *CElement) interface{} {
 		next.SetPrev(prev)
 	}
 
-	e.Detach()
+	// Set .Done() on e, otherwise waiters will wait forever.
+	e.SetRemoved()
 
+	l.mtx.Unlock()
 	return e.Value
 }
 
-// Clear removes all the elements in the CList
-func (l *CList) Clear() {
-	l.mtx.Lock()
-	defer l.mtx.Unlock()
-
-	if l.head == nil || l.tail == nil {
-		return
-	}
-
-	for e := l.head; e != nil; e = l.head {
-		prevE := e.Prev()
-		nextE := e.Next()
-
-		if prevE == nil && e != l.head {
-			panic("CList.Clear failed due to nil prev element")
-		}
-
-		if nextE == nil && e != l.tail {
-			panic("CList.Clear failed due to nil next element")
-		}
-
-		if l.len == 1 {
-			l.wg = waitGroup1()
-			l.waitCh = make(chan struct{})
-		}
-		l.len--
-
-		if prevE == nil {
-			l.head = nextE
-		} else {
-			prevE.SetNext(nextE)
-		}
-		if nextE == nil {
-			l.tail = prevE
-		} else {
-			nextE.SetPrev(prevE)
-		}
-
-		e.Detach()
-	}
-}
-
 func waitGroup1() (wg *sync.WaitGroup) {
 	wg = &sync.WaitGroup{}
 	wg.Add(1)

internal/libs/clist/clist_test.go

@@ -252,16 +252,9 @@ func TestScanRightDeleteRandom(t *testing.T) {
 	// time.Sleep(time.Second * 1)
 
 	// And remove all the elements.
-	// we detach the prev/next element in CList.Remove, so just assign l.Front direcly.
-	halfElements := numElements / 2
-	for el := l.Front(); el != nil && halfElements > 0; el = l.Front() {
+	for el := l.Front(); el != nil; el = el.Next() {
 		l.Remove(el)
-		halfElements--
 	}
-
-	// remove the rest half elements in the CList
-	l.Clear()
-
 	if l.Len() != 0 {
 		t.Fatal("Failed to remove all elements from CList")
 	}

internal/mempool/v0/clist_mempool.go

@@ -164,7 +164,10 @@ func (mem *CListMempool) Flush() {
 	_ = atomic.SwapInt64(&mem.txsBytes, 0)
 	mem.cache.Reset()
 
-	mem.txs.Clear()
+	for e := mem.txs.Front(); e != nil; e = e.Next() {
+		mem.txs.Remove(e)
+		e.DetachPrev()
+	}
 
 	mem.txsMap.Range(func(key, _ interface{}) bool {
 		mem.txsMap.Delete(key)
@@ -335,6 +338,7 @@ func (mem *CListMempool) addTx(memTx *mempoolTx) {
 // 	- resCbRecheck (lock not held) if tx was invalidated
 func (mem *CListMempool) removeTx(tx types.Tx, elem *clist.CElement, removeFromCache bool) {
 	mem.txs.Remove(elem)
+	elem.DetachPrev()
 	mem.txsMap.Delete(mempool.TxKey(tx))
 	atomic.AddInt64(&mem.txsBytes, int64(-len(tx)))
 

internal/mempool/v1/mempool.go

@@ -305,6 +305,7 @@ func (txmp *TxMempool) Flush() {
 			txmp.txStore.RemoveTx(wtx)
 			txmp.priorityIndex.RemoveTx(wtx)
 			txmp.gossipIndex.Remove(wtx.gossipEl)
+			wtx.gossipEl.DetachPrev()
 		}
 	}
 
@@ -742,6 +743,7 @@ func (txmp *TxMempool) removeTx(wtx *WrappedTx, removeFromCache bool) {
 	// Remove the transaction from the gossip index and cleanup the linked-list
 	// element so it can be garbage collected.
 	txmp.gossipIndex.Remove(wtx.gossipEl)
+	wtx.gossipEl.DetachPrev()
 
 	atomic.AddInt64(&txmp.sizeBytes, int64(-wtx.Size()))