eth: do not add failed tx to localTxTracker #31202
Conversation
|
WDYT about doing it like this: if err := b.eth.txPool.Add([]*types.Transaction{signedTx}, false)[0]; err != nil {
return err
}
if locals := b.eth.localTxTracker; locals != nil {
locals.Track(signedTx)
}
return nil |
|
Thanks for catching it. Unfortunately it's a bit more complicated. The error could occur in several different circumstances, We should prevent tracking invalid transactions, like with invalid gas limit. But for the transactions |
|
Afaicr, the localstracker doesn't track invalid txs, already.
I think this PR basically makes the localstracker totally moot/unused.
Seems wrong to me
|
|
Yep sorry, I checked it again and I agree with Martin. In the case of an underpriced transaction, the tx should still be added to the locals tracker. Maybe we should return a special error in that case to make the user aware that the transaction had a runtime error but is still being tracked |
I might be wrong but I cannot find localstracker checking if tx is invalid from below file |
As a web3 developer, speical state that a transaction returns error but it can be added to a block in the future is very confusing |
|
@MariusVanDerWijden PTAL |
core/txpool/locals/tx_tracker.go
Outdated
| @@ -88,6 +88,11 @@ func (tracker *TxTracker) TrackAll(txs []*types.Transaction) { | |||
| if tx.Type() == types.BlobTxType { | |||
| continue | |||
| } | |||
| // Ignore the transactions which are failed for fundamental | |||
| // validation such as invalid parameters. | |||
| if tracker.pool.ValidateTxBasics(tx) != nil { | |||
There was a problem hiding this comment.
We should maybe add tracing log here, so we can see why a transaction was not tracked
There was a problem hiding this comment.
What kind of log do you prefer? Info or Debug?
I am a bit concerned the Info logs might be overwhelming for certain types of node, e.g. rpc node.
eth/api_backend.go
Outdated
| // available slot), an error will be returned to the user. However, locally | ||
| // submitted transactions may be resubmitted later via the local tracker, which | ||
| // could cause confusion that a previously "rejected" transaction is later | ||
| // accepted. | ||
| return b.eth.txPool.Add([]*types.Transaction{signedTx}, false)[0] |
There was a problem hiding this comment.
So let's not return the error here!
We should instead return the error from locals.Track if there is one.
There was a problem hiding this comment.
Seems it's getting difficult to know a transaction has error or not...
There was a problem hiding this comment.
Okay. As localTxPool feature can be disabled by --txpool.nolocals flag
please return error like now when the flag is on
core/txpool/locals/tx_tracker.go
Outdated
| @@ -88,6 +88,11 @@ func (tracker *TxTracker) TrackAll(txs []*types.Transaction) { | |||
| if tx.Type() == types.BlobTxType { | |||
| continue | |||
| } | |||
| // Ignore the transactions which are failed for fundamental | |||
| // validation such as invalid parameters. | |||
| if tracker.pool.ValidateTxBasics(tx) != nil { | |||
There was a problem hiding this comment.
tx can stay local tx pool forever
seems it need more logic like retry count
There was a problem hiding this comment.
Local transactions won’t remain stuck in the transaction pool indefinitely.
If a transaction fails stateful checks—such as the pool being full or the gas price being too low to replace an existing transaction—it will be resubmitted. However, we know that the state of the tx pool will change over time, meaning the locally tracked transaction may or may not be accepted quickly.
If the transaction has a relatively low gas price (but still higher than the minimum enforced by the node operator), it may remain stuck. This aligns with the original behavior of retaining local transactions. The only way to unblock it is by replacing it with a transaction that has a higher gas price.
Additionally, resubmitting the transaction every minute is not CPU-intensive.
There was a problem hiding this comment.
What I was concerned is not cpu but memory.
It can cause memory leak as tx can reamin localTxTracker in some case
Attackers can use it to stop nodes by OOM killed
There was a problem hiding this comment.
The local tracker only takes care the transactions submitted via the RPC endpoints, not the ones from the p2p network.
The node operators are supposed to take care of the node by themselves and can disable the local notion with public endpoints, e.g. infura node.
rjl493456442
force-pushed
the
local-tx-tracker
branch
from
c5d04d6 to
bba05c6
Compare
rjl493456442
force-pushed
the
local-tx-tracker
branch
from
bba05c6 to
df06cba
Compare
check if tx is successful before add tx to localTxTracker.
As localTxTracker keep tx and tries to add the tx to txpool, it can make failed rpc tx to success in some case
which is confused for developer.
example
rpc failed due to insufficient funds for gas.
developer might think the tx was not accepted by node but the tx is in localTxTracker
after developer send eth to the address for gas, the tx in localTxTracker will be in block