diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index b0d878d37..2815751fc 100644 --- a/CHANGELOG_PENDING.md +++ b/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 diff --git a/internal/evidence/pool.go b/internal/evidence/pool.go index 643ff9e7d..8ca97fd17 100644 --- a/internal/evidence/pool.go +++ b/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() } } } diff --git a/internal/libs/clist/clist.go b/internal/libs/clist/clist.go index 64b5cecc9..6cf515706 100644 --- a/internal/libs/clist/clist.go +++ b/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) diff --git a/internal/libs/clist/clist_test.go b/internal/libs/clist/clist_test.go index 9a997e5dc..4b222e3d5 100644 --- a/internal/libs/clist/clist_test.go +++ b/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") } diff --git a/internal/mempool/v0/clist_mempool.go b/internal/mempool/v0/clist_mempool.go index 636426267..40e93cc13 100644 --- a/internal/mempool/v0/clist_mempool.go +++ b/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))) diff --git a/internal/mempool/v1/mempool.go b/internal/mempool/v1/mempool.go index afaa88322..9b06ee4a8 100644 --- a/internal/mempool/v1/mempool.go +++ b/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()))