feat: remove txs which do not end up in a block out of the mempool #15
Conversation
bharath-123
force-pushed
the
bharath/remove-invalid-txs
branch
from
bbefcc6
to
e30e123
Compare
| @@ -190,7 +187,7 @@ func (w *worker) buildPayload(args *BuildPayloadArgs) (*Payload, error) { | |||
| noTxs: false, | |||
| } | |||
| start := time.Now() | |||
| full := w.getSealingBlock(emptyParams) | |||
| full := w.getSealingBlock(fullParams) | |||
There was a problem hiding this comment.
In vanilla geth, an empty block is created and then a full block is created. This is mainly to spend more time to optimize for gas fees and also to return at least an empty block if we were not able to create a full block on time.
We don't need this astria since we just blindly use the txs received from ExecuteBlock. so fullParams makes more sense than emptyParams over here.
bharath-123
force-pushed
the
bharath/remove-invalid-txs
branch
2 times, most recently
from
c8f6554
to
ad9cced
Compare
bharath-123
force-pushed
the
bharath/remove-invalid-txs
branch
from
ad9cced
to
fe45f4e
Compare
bharath-123
requested review from
joroshiba,
mycodecrafting and
noot
as code owners
|
We’ve had previous discussions about the management of txs between the geth/rollup mempool and the Astria sequencer mempool. Is the focus of this PR to remove transactions which were paid to be included in Astria but didn’t get executed in geth because we ran into the execution gas limit? |
miner/worker.go
Outdated
| @@ -855,6 +857,10 @@ func (w *worker) commitAstriaTransactions(env *environment, txs *types.Transacti | |||
| // nonce-too-high clause will prevent us from executing in vain). | |||
| log.Debug("Transaction failed, account skipped", "hash", tx.Hash(), "err", err) | |||
| } | |||
| if err != nil { | |||
| log.Trace("Marking transaction as invalid", "hash", tx.Hash(), "err", err) | |||
| w.eth.TxPool().UpdateAstriaInvalid(tx) | |||
There was a problem hiding this comment.
should we also remove if a tx is skipped because of a high nonce or low nonce? or only because it exceeded the gas limit?
There was a problem hiding this comment.
We ideally should to maintain 1:1 block determinism. So we should include all of these cases.
core/txpool/blobpool/blobpool.go
Outdated
| func (p *BlobPool) SetAstriaOrdered(types.Transactions) {} | ||
| func (p *BlobPool) ClearAstriaOrdered() {} | ||
| func (p *BlobPool) UpdateAstriaExcludedFromBlock(*types.Transaction) {} | ||
| func (p *BlobPool) AstriaExcludedFromBlock() *types.Transactions { return &types.Transactions{} } |
There was a problem hiding this comment.
Seems like this public function is added solely for test purposes? Is there another way to manage that?
There was a problem hiding this comment.
don't think there is, we can't attribute functions with a #cfg[test] like how we do in rust :( , we need to do hacks like https://github.com/golang/go/blob/master/src/reflect/export_test.go
There are some cases in the Geth codebase where they have written public functions only for testing like this one:
astria-geth/miner/payload_building.go
Line 144 in a9326e1
core/txpool/blobpool/blobpool.go
Outdated
| func (p *BlobPool) AstriaOrdered() *types.Transactions { return &types.Transactions{} } | ||
| func (p *BlobPool) SetAstriaOrdered(types.Transactions) {} | ||
| func (p *BlobPool) ClearAstriaOrdered() {} | ||
| func (p *BlobPool) UpdateAstriaExcludedFromBlock(*types.Transaction) {} |
There was a problem hiding this comment.
nit on naming... perhaps NewAstriaOrderedExcludedTx? naming here is always going to be awkward as it's actually a method on a subtype, its semantically kinda wrong.
Not for this PR, but the adding stuff to the mempool has always felt a bit awkward. Might makes more sense to add payload argument or something, or even create a new subpool.
bharath-123
force-pushed
the
bharath/remove-invalid-txs
branch
from
d2fc307
to
67d9f9e
Compare
|
Merging this to rebase on top of the PR to fix tests |
When the txs we receive from `ExecuteBlocks` call fail to end up in a block, we keep them lying around the geth mempool but clear it from the astria mempool. This PR attempts to remove such txs from the geth mempool.
When the txs we receive from
ExecuteBlockscall fail to end up in a block, we keep them lying around the geth mempool but clear it from the astria mempool.This PR attempts to remove such txs from the geth mempool.