core/txpool: acquire write lock in "ContentFrom" (to be consistent with "Content") + documentation

[gyuho]

As far as I understand the underlying txList.Flatten mutates the list so we need write lock to prevent other racey updates to this list. So, adding documentation why write lock is needed, and change ContentFrom to acquire write lock same as Content.

[holiman]

At a glance, the existing code looks like it tries to protect the pending and queued maps themselves.

In the Content method, we do pending[addr] = ..., setting it. Whereas in the ContentFrom, we only do pool.pending[addr]: reading from it.

With that in mind, it looks correct that this method uses the full Lock and the latter uses RLock.

However. You are right that the Flatten is racy, so two ContentFrom called on the same addr could cause a race, where two routines wind up in core/txpool/list.go:

func (m *sortedMap) flatten() types.Transactions {
	// If the sorting was not cached yet, create and cache it
	if m.cache == nil {
		m.cache = make(types.Transactions, 0, len(m.items))
		for _, tx := range m.items {
			m.cache = append(m.cache, tx)
		}
		sort.Sort(types.TxByNonce(m.cache))
	}
	return m.cache
}

It could definitely cause a crash. Might be more theoretical than practical. In any case, I suspect it might be possible to fifx this differently, e.g. using atomic to set the m.cache.

Good catch though!

[holiman]

The txpool was moved into core/txpool/legacypool/. Please rebase this PR

[holiman]

I'll close this, I think the fix should be performed more 'locally' inside the sortedMap, as in #27898