Make X-chain mempool safe for concurrent use #2520
Conversation
StephenButtolph
added
enhancement
| Has(txID ids.ID) bool | ||
| Get(txID ids.ID) *txs.Tx |
There was a problem hiding this comment.
Generally I think having Has and Get on the mempool isn't very async friendly because the mempool is now expected to change concurrently with interactions with it.
| // Iterate over transactions from oldest to newest until the function | ||
| // returns false or there are no more transactions. | ||
| Iterate(f func(tx *txs.Tx) bool) |
There was a problem hiding this comment.
This is needed to support pull gossip
| lock sync.RWMutex | ||
| unissuedTxs linkedhashmap.LinkedHashmap[ids.ID, *txs.Tx] | ||
| consumedUTXOs set.Set[ids.ID] | ||
| bytesAvailable int |
There was a problem hiding this comment.
I restructured the order of the fields so that all the fields protected by the lock could be cleanly put under it.
| tx, _ := m.unissuedTxs.Get(txID) | ||
| return tx | ||
| func (m *mempool) Get(txID ids.ID) (*txs.Tx, bool) { | ||
| tx, ok := m.unissuedTxs.Get(txID) |
There was a problem hiding this comment.
LinkedHashMaps are already async safe.
| m.lock.RLock() | ||
| defer m.lock.RUnlock() |
There was a problem hiding this comment.
The lock is needed here to honor the Iter invariant that the linked hash map isn't arbitrarily modified while iterating.
| } | ||
| } | ||
|
|
||
| func createTestTxs(count int) []*txs.Tx { |
There was a problem hiding this comment.
This function must have been a copy paste from somewhere... because it is massively overkill for the tests in this file
| if _, ok := m.unissuedTxs.Get(txID); ok { | ||
| return | ||
| } | ||
|
|
||
| m.droppedTxIDs.Put(txID, reason) |
There was a problem hiding this comment.
I felt like it made the most sense to enforce that a tx wasn't considered dropped if it was in the mempool. I feel like this was the original idea (notice how we evict from droppedTxIDs in Add) but we probably just forgot to have the corresponding check here as well.
Interesting, previously we would insert errDuplicateTx here... which is especially weird.
vms/avm/network/network.go
Outdated
There was a problem hiding this comment.
Can/should we clean this up as a result of this change?
// We need to grab the context lock here to avoid racy behavior with
// transaction verification + mempool modifications.
//
// Invariant: tx should not be referenced again without the context lock
// held to avoid any data races.
n.ctx.Lock.Lock()
err = n.issueTx(tx)
n.ctx.Lock.Unlock()There was a problem hiding this comment.
hmmmm I suppose we may be able to reduce the scope of the lock... We still need the lock held when verifying the tx.
There was a problem hiding this comment.
I think I'll leave any larger changes to the network struct to when we are introducing the p2p SDK.
| }) | ||
| if err := registerer.Register(numTxsMetric); err != nil { | ||
| return nil, err | ||
| m := &mempool{ |
There was a problem hiding this comment.
This is an aesthetically pleasing cleanup- nicely done
…anchego into concurrent-x-chain-mempool
Why this should be merged
The X-chain mempool currently requires the context lock to be held. This removes that requirement to allow easier integration with the p2p SDK.
How this works
A number of the structures used by the mempool are already safe for concurrent use. This PR adds locking where required to provide a consistent interface along with preventing racy behavior for existing structures that are not safe for concurrent use.
How this was tested