Skip to content
This repository has been archived by the owner on Oct 25, 2024. It is now read-only.

miner: fix mev gas price calc when bundle replaces mempool tx #41

Open
wants to merge 88 commits into
base: main
Choose a base branch
from

Conversation

bsh98
Copy link

@bsh98 bsh98 commented Jan 31, 2023

📝 Summary

Currently, if a bundle includes a transaction that has the same sender and nonce as a transaction in the mempool (i.e. attempting to replace a mempool transaction with a bundle transaction), the gas fees from the bundle transaction will not be counted towards the bundle's value. This PR compares transaction hashes instead of transaction nonces when checking if a bundle transaction is present in the mempool. This PR allows for replacing a mempool transaction with a bundle transaction (and having its value counted towards the bundle value) when the effective gas price of the bundle transaction is larger than the gas price of the mempool transaction.

Motivation

There are cases when a searcher may want to re-use a nonce from a mempool transaction in a bundle. For example, a searcher may want to privately replace a pending transaction or move a PGA to a bundle auction.

Variations

Another approach would be to only count the gas fees if the bundle transaction's effective gas price (coinbaseDiff + gasFees) / gasUsed is greater than N% (configured price bump) larger than the mempool transaction's gas price. This would closely follow the replacement logic of the mempool.

builder/core/tx_list.go

Lines 293 to 296 in 8caba1f

// thresholdTip = oldTip * (100 + priceBump) / 100
b := big.NewInt(100)
thresholdFeeCap := aFeeCap.Div(aFeeCap, b)
thresholdTip := aTip.Div(aTip, b)

This would avoid allowing "cheap" cancellations/replacements.

Implications

This could incentivize searchers to include replaced transactions in their bundles. Possible solution: 947f59a


Ruteri and others added 30 commits August 26, 2022 13:44
* Add remote relay connection for getting validator data
* Add block submission to remote relay
* Adjust readme
* Add block build trigger from beacon node

* remove empty block building

Co-authored-by: avalonche <avvalonche@protonmail.com>
* fix issue with geth not shutting down (flashbots#97)
* Add eth_callBundle rpc method (flashbots#14)
* flashbots: add eth_estimateGasBundle (flashbots#102)
* feat(ethash): flashbots_getWork RPC with profit (flashbots#106)
* Calculate megabundle as soon as it's received (flashbots#112)
* Add v0.5 specification link (flashbots#118)
flashbots#123)

* Discard reverting megabundle blocks and head change interrupted blocks

* Discard all blocks with incomplete bundles

* Run reverting megabundles regression test separately from bundle tests
avalonche and others added 17 commits December 6, 2022 04:04
* Fix duplicate relay registrations in cache

* Remove Timestamp from builder validation data as it should be ignored

* Adjust cfg.SecondaryRemoteRelayEndpoints for empty list

Co-authored-by: Mateusz Morusiewicz <11313015+Ruteri@users.noreply.github.com>
* Fix getting validators map for local relay

* pr comments

* add timer for updating known validators

* improvement to local validator map fetching

* lock for map updating

* properly lock updates

* get current slot if the mapping is empty

* remove onForkchoiceUpdate

* graceful shutdown

* Split initial proposer sync from the proposer fetch loop (flashbots#28)

Co-authored-by: Mateusz Morusiewicz <11313015+Ruteri@users.noreply.github.com>
* Always recommit when creating blocks

* Allow algo worker for local block creation
Refactor miner tests and how bundles are ordered to allow easier extensibility
* Add metrics for builder

* add metrics for individual simulations

* Add builder metrics to README (flashbots#34)
Introduces bundle replacement and cancellation via replacementUuid.
Since the replacement is tied to a specific sender, eth_sendBundle gets two additional optional fields: the replacement uuid and the signingAddress of the bundle submission.

The DB requests are done in the background, and cancellations are resolved while non-cancelable bundles are already being simulated to avoid waiting for DB to reply.
If anything goes wrong with the cancellations, the cancelable bundles are not considered.

Note: every block is now sent to the relay, as we can no longer rely on the highest-profit rule!
@bsh98 bsh98 changed the title miner: fix mev gas price calc when same nonce tx not in mempool miner: fix mev gas price calc when bundle replaces mempool tx Jan 31, 2023
@Ruteri
Copy link
Collaborator

Ruteri commented Jan 31, 2023

While I love the idea, correct me if I'm wrong in thinking that a transaction can overwrite a transaction with the same nonce, removing it from the pending list.
So you'd actually still count the old, replaced transactions as not having been in the pending. This would require keeping track of the dropped transactions.

@bsh98
Copy link
Author

bsh98 commented Feb 7, 2023

While I love the idea, correct me if I'm wrong in thinking that a transaction can overwrite a transaction with the same nonce, removing it from the pending list. So you'd actually still count the old, replaced transactions as not having been in the pending. This would require keeping track of the dropped transactions.

Apologies for the late reply and I appreciate the response.

Yes, you are correct in that just comparing transaction hashes could lead to a transaction not being counted as pending (I didn't catch this in my first commit). However, is it safe to assume that a transaction could only have been dropped and replaced if it had a lower gas price than the current pending transaction? I believe this would allow you to count a bundle transaction as known/seen if the gas price is less than or equal to the current pending transaction. This would avoid having to keep track of all dropped transactions. If a transaction was dropped for another reason (e.g. pool lifetime exceeded), it wouldn't be counted as pending anyways. In other words, if a bundle includes a transaction with the same account + nonce as a pending transaction, it is marked as known unless the effective gas price is higher (i.e. effectively replacing the pending transaction).

avalonche pushed a commit that referenced this pull request Feb 7, 2023
* Backport improvements to the builder from incremental improvements
* Make linter happy
@Ruteri
Copy link
Collaborator

Ruteri commented Feb 14, 2023

However, is it safe to assume that a transaction could only have been dropped and replaced if it had a lower gas price than the current pending transaction?

Yes, but we still don't want to count that gas, even if it's lower than what we have now in the mempool. This still allows stuffing as far as I can tell.

@bsh98
Copy link
Author

bsh98 commented Feb 14, 2023

Yes, but we still don't want to count that gas, even if it's lower than what we have now in the mempool.

Agreed! The goal is to not count the gas fees if the effective gas price is less than or equal to the transaction in the mempool. If it's safe to assume that a transaction could have only been dropped if it had a lower gas price, then we should safely be able to prevent stuffing a dropped transaction by marking all underpriced transactions as seen.

builder/miner/worker.go

Lines 1889 to 1901 in 660deaa

} else if accountTx.Nonce() == txNonce && accountTx.Hash() != txHash {
// Bundle is attempting to replace a transaction from the mempool.
txTipValue := new(big.Int).Add(gasFeesTx, coinbaseDelta)
txTipGasPrice := new(big.Int).Div(txTipValue, gasUsed)
accountTxTipGasPrice := accountTx.EffectiveGasTipValue(env.header.BaseFee)
if txTipGasPrice.Cmp(accountTxTipGasPrice) != 1 {
// Replacement underpriced, so don't count the gas fees towards the bundle value.
// This could mean that the transaction was already replaced in the mempool.
// This prevents incentivising bundles to include replaced transactions.
txInPendingPool = true
}
break
}

We should only count the gas fees if the effective gas price is strictly greater than the transaction in the mempool. Apologies if I'm still missing an edge case or something major here— would it possible to provide an example of how this could allow stuffing?

avalonche pushed a commit that referenced this pull request Mar 9, 2023
* Backport improvements to the builder from incremental improvements
* Make linter happy
avalonche pushed a commit that referenced this pull request Mar 15, 2023
* Backport improvements to the builder from incremental improvements
* Make linter happy
avalonche pushed a commit that referenced this pull request Mar 17, 2023
* Backport improvements to the builder from incremental improvements
* Make linter happy
avalonche pushed a commit that referenced this pull request Mar 22, 2023
* Backport improvements to the builder from incremental improvements
* Make linter happy
avalonche pushed a commit that referenced this pull request Jul 6, 2023
* Backport improvements to the builder from incremental improvements
* Make linter happy
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.