core/txpool: remove locals-tracking from pools (part 2) #30559
Conversation
holiman
force-pushed
the
local_pool_pt2
branch
2 times, most recently
from
cffa982 to
165e892
Compare
holiman
requested review from
gballet,
lightclient,
karalabe and
rjl493456442
as code owners
core/txpool/tracking/tx_tracker.go
Outdated
| // along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| // Package legacypool implements the normal EVM execution transaction pool. | ||
| package tracking |
There was a problem hiding this comment.
maybe rename it to tracker or txtracker?
core/txpool/tracking/tx_tracker.go
Outdated
| ) | ||
| for sender, txs := range tracker.byAddr { | ||
| stales := txs.Forward(tracker.pool.Nonce(sender)) | ||
| // Wipe the stales |
There was a problem hiding this comment.
The stales refers to the transactions with nonce lower than tracker.pool.Nonce(sender).
However, tracker.pool.Nonce(sender) is the current pending nonce of the pool, including the executable txs applied.
Shouldn't we use statedb(latest_chain_head).Nonce(sender) instead?
Furthermore, do we care about the reorg here? should we care about the reorg here?
- If we don't care, some local transactions might be dropped first and reorg occurs later, they won't be able to be resubmitted;
- If we care, the "stale" local transactions should somehow be kept for a few (maybe a few blocks) and try to resubmit them if reorg occurs;
There was a problem hiding this comment.
Shouldn't we use
statedb(latest_chain_head).Nonce(sender)instead?
Yes, good observation!
Furthermore, do we care about the reorg here? should we care about the reorg here?
* If we don't care, some local transactions might be dropped first and reorg occurs later, they won't be able to be resubmitted; * If we care, the "stale" local transactions should somehow be kept for a few (maybe a few blocks) and try to resubmit them if reorg occurs;
Once it's been included in a block, I think it can be untracked. It will most likely be picked up again if it's reorged. 🤷
There was a problem hiding this comment.
Shouldn't we use
statedb(latest_chain_head).Nonce(sender)instead?Yes, good observation!
No wait. Our job here is to make the pool accept the tx. If the pool already has nonce=5 from this sender, then we can consider nonce=5 as stale, and not worry about it anymore. I guess the caveat here is that it might be a replacement... :/
core/txpool/legacypool/legacypool.go
Outdated
| type lookup struct { | ||
| slots int | ||
| lock sync.RWMutex | ||
| locals map[common.Hash]*types.Transaction | ||
| remotes map[common.Hash]*types.Transaction |
There was a problem hiding this comment.
we can do rename in a following pr, just want to mention here
|
I think the general idea is good and it's a good direction to "distribute the complexity" of txpool graduately. |
holiman
force-pushed
the
local_pool_pt2
branch
2 times, most recently
from
e61a7cf to
82c9789
Compare
|
|
holiman
force-pushed
the
local_pool_pt2
branch
from
82c9789 to
c1ace4d
Compare
holiman
requested review from
MariusVanDerWijden,
fjl and
jwasinger
as code owners
Fixed (crosses fingers) |
|
I'm wondering whether we should save users from shooting themselves in the foot by only allowing non-blob transactions in the tracker |
|
Overall this PR looks very good to me. I love the concept and implementing it as an additional service is genius. I added some minor comments where this PR changes the behavior of the transaction pool, they boil down to the following:
I'm not saying that all of those are issues that need to be fixed necessarily, I think we should discuss them though before merging this PR to make sure we don't rug users |
|
Thanks. Re the last two points, they are described fully in the PR description.
The first two points should probably be undone
|
…ocal txs core/txpool/legacypool: handle the case when journalling is disabled core/txpool: store txs to journal on insert
…ing of recheck core/txpool/tracking: add ability to trigger recheck externally core/txpool: remove local param from api eth/catalyst: update tests for new API miner, eth: update tests for changed txpool api eth/catalyst: remove trace-log-output from tests core/txpool: fix nits, also store on insert
holiman
force-pushed
the
local_pool_pt2
branch
from
84d51a6 to
8639d5c
Compare
|
Thanks @MariusVanDerWijden, I have addressed the points you raised. Also rebased on master |
holiman
force-pushed
the
local_pool_pt2
branch
from
df84e36 to
3c33b7b
Compare
| @@ -615,9 +544,6 @@ func (pool *LegacyPool) validateTxBasics(tx *types.Transaction, local bool) erro | |||
| MaxSize: txMaxSize, | |||
| MinTip: pool.gasTip.Load().ToBig(), | |||
| } | |||
| if local { | |||
| opts.MinTip = new(big.Int) | |||
There was a problem hiding this comment.
Do I understand correctly that txs from prioritized addresses are now also subject to the minimum tip threshold and there is no way to force accept certain txs with low tip? If so then are you sure this is the right behavior? I would think that we should still set MinTip to zero if it's signed by one of the prio address set since the user's intention there was to include those txs whenever possible regardless of low price.
There was a problem hiding this comment.
The transaction pool does not have a concept of local transactions (or prioritized addresses).
The mintip enforcement or exception is instead a concern for miner, during block building
There was a problem hiding this comment.
But to answer the question. See worker.go
miner.confMu.RLock()
tip := miner.config.GasPrice
prio := miner.prio
miner.confMu.RUnlock()
// Retrieve the pending transactions pre-filtered by the 1559/4844 dynamic fees
filter := txpool.PendingFilter{
MinTip: uint256.MustFromBig(tip),
}So yes, the filter which we use to pick them from the pool does use the mintip indiscriminately, i.e the miner.GasPrice. After the mintip-filtering, the prio set is included first.
There was a problem hiding this comment.
I think I understand the concern from Zsolt.
Originally, the local transactions can bypass the minTip restrictions. Now, they will always be rejected by txpool, regardless of it's local or not.
With the local tracker, the local transactions will be persisted and be resubmitted all the time, but none of the attempt will succeed.
While It's a breaking change, I think it's acceptable. The consequence is local transactions will be rejected and won't be propagated into the networks if minTip is not reached.
The semantic of "local" in txpool has been changed, it will try to best effort to resubmit the tx into pool, but with no guarantee, e.g. if tip is too low, txpool is full, etc, it will fail
| @@ -279,7 +279,10 @@ func (b *EthAPIBackend) SubscribeLogsEvent(ch chan<- []*types.Log) event.Subscri | |||
| } | |||
|
|
|||
| func (b *EthAPIBackend) SendTx(ctx context.Context, signedTx *types.Transaction) error { | |||
| return b.eth.txPool.Add([]*types.Transaction{signedTx}, true, false)[0] | |||
| if locals := b.eth.localTxTracker; locals != nil { | |||
There was a problem hiding this comment.
What do you mean -- it's the api backend, it's where we tag them as locals? Did I do something wrong?
| } | ||
| tracker.byAddr[addr].Put(tx) | ||
| if tracker.journal != nil { | ||
| _ = tracker.journal.insert(tx) |
There was a problem hiding this comment.
Just out of curiosity, why the _ =? Does it do anything?
There was a problem hiding this comment.
Ah no, it silences some linters which complain about ignored returnvalues. We don't use those linters though, so it's not how we usually write it. Don't know why it crept in
| @@ -234,6 +237,17 @@ func New(stack *node.Node, config *ethconfig.Config) (*Ethereum, error) { | |||
| legacyPool := legacypool.New(config.TxPool, eth.blockchain) | |||
|
|
|||
| eth.txPool, err = txpool.New(config.TxPool.PriceLimit, eth.blockchain, []txpool.SubPool{legacyPool, blobPool}) | |||
|
|
|||
| if !config.TxPool.NoLocals { | |||
There was a problem hiding this comment.
Is it possible to move the local tracker into the txpool.Pool?
I think this is an internal mechanism of the transaction pool and would be better “hidden” under the hood.
There was a problem hiding this comment.
Generally LGTM!
I think it's a good change and simplify the internal logic a lot!
I would prefer to deploy it on one of our bootnoode, ensuring nothing is broken.
Also, we must state the breaking change clearly, that for the transactions with low gas price, they won't be accepted/propagated regardless of whether they are local or not.
Replaces #29297, descendant from #27535
This PR removes
localsas a concept from transaction pools. Therefore, the pool now acts as very a good simulation/approximation of how our peers' pools behave. What this PR does instead, is implement a locals-tracker, which basically is a little thing which, from time to time, asks the pool "did you forget this transaction?". If it did, the tracker resubmits it.If the txpool had forgotten it, chances are that the peers had also forgotten it. It will be propagated again.
Doing this change means that we can simplify the pool internals, quite a lot.
The semantics of
localHistorically, there has been two features, or usecases, that has been combined into the concept of
locals.This PR splits these features up, let's call it
1: localand2: prio. Thepriois not actually individual transaction, but rather a set ofaddresses to prioritize.The attribute
localmeans it will be tracked, andpriomeans it will be prioritized by miner.For
local: anything transaction received via the RPC is marked aslocal, and tracked by the tracker.For
prio: any transactions from this sender is included first, when building a block. The existing commandline-flag--txpool.localssets the set ofprioaddresses.