-
Notifications
You must be signed in to change notification settings - Fork 275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(miner): fix repeatedly processing dropped txs in a new block #491
Conversation
05ae5fb
to
3705fd6
Compare
9ab1d19
to
4edf645
Compare
A more performant way is to change |
f487083
to
50b02a6
Compare
50b02a6
to
7df1f93
Compare
* force drop * minor
Update: I've changed my mind. We should remove the tx from the txpool. Otherwise if a user uses the nonce with |
I wonder if the rpc node will remove the tx after the signer does so. Otherwise, users will get the incorrect nonce from the rpc node. |
I will run a local testnet to check. |
Co-authored-by: Péter Garamvölgyi <peter@scroll.io>
a8f6057
to
4f3725d
Compare
1. Purpose or design rationale of this PR
txs.Shift()
andtxs.Pop()
will only drop the tx from the currenttxs *types.TransactionsByPriceAndNonce
. However, the "dropped" tx is not really removed from the mempool, and will still be fed intocommitTransactions
when making a new block.(You can verify this statement using #492. Can try and play around using https://github.com/HAOYUatHZ/ccc-tester, changing https://github.com/HAOYUatHZ/ccc-tester/blob/ccc/docker/l2geth/Dockerfile to
scrolltech/l2geth:scroll-v4.3.56-PoC
).This PR removes the txs from the processing tx queue.(update: This PR now removes the txs from the txpool. See discussion below.) Besides, it makes no sense to re-trace the tx, if such a single tx is already knowncircuitcapacitychecker.ErrBlockRowConsumptionOverflow
orcircuitcapacitychecker.ErrUnknown
Note: L1msg is not affected by this because we read and maintain the queue in db instead of reading directly from mempool.
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.go
been updated?4. Breaking change label
Does this PR have the
breaking-change
label?