Skip to content
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

Toggleable discard for failed revertible transaction hashes #81

Merged
merged 26 commits into from Aug 3, 2023

Conversation

Wazzymandias
Copy link
Contributor

@Wazzymandias Wazzymandias commented Jul 2, 2023

📝 Summary

  • Current behavior of bundles is that:

    1. All transactions in a bundle must be included in specified order, with no transaction inserted between them, OR
    2. If any transaction reverts, then no transaction from the bundle will be included EXCEPT
    3. When user specifies reverting transaction hashes in their request, the builder can still include the bundle with one or more of the transactions failing within it
  • This PR makes it so when users specify reverting transaction hashes in their request, the builder discards the transaction(s) on error

Changes

📚 References


builder/config.go Outdated Show resolved Hide resolved
miner/algo_common.go Outdated Show resolved Hide resolved
miner/algo_common.go Outdated Show resolved Hide resolved
miner/algo_common.go Outdated Show resolved Hide resolved
@Wazzymandias Wazzymandias changed the title Toggleable transaction discard for reverted transaction hashes Toggleable discard for failed revertible transaction hashes Jul 4, 2023
@Wazzymandias Wazzymandias requested review from Ruteri and dvush July 4, 2023 19:52
@Wazzymandias Wazzymandias marked this pull request as ready for review July 4, 2023 19:55
miner/algo_common.go Outdated Show resolved Hide resolved
miner/algo_common.go Outdated Show resolved Hide resolved
miner/algo_common.go Outdated Show resolved Hide resolved
miner/algo_common.go Outdated Show resolved Hide resolved
miner/algo_common.go Outdated Show resolved Hide resolved
miner/algo_common.go Outdated Show resolved Hide resolved
miner/algo_common.go Show resolved Hide resolved
miner/algo_common.go Show resolved Hide resolved
@Wazzymandias Wazzymandias requested a review from Ruteri July 5, 2023 14:33
Copy link
Collaborator

@Ruteri Ruteri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring decision on the conventions to @dvush and @avalonche since they frequent this codebase more than I do

miner/algo_greedy.go Show resolved Hide resolved
miner/algo_common.go Outdated Show resolved Hide resolved
@@ -1944,7 +1949,10 @@ func containsHash(arr []common.Hash, match common.Hash) bool {

// Compute the adjusted gas price for a whole bundle
// Done by calculating all gas spent, adding transfers to the coinbase, and then dividing by gas used
func (w *worker) computeBundleGas(env *environment, bundle types.MevBundle, state *state.StateDB, gasPool *core.GasPool, pendingTxs map[common.Address]types.Transactions, currentTxCount int) (simulatedBundle, error) {
func (w *worker) computeBundleGas(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can consider restructuring these args into a struct

miner/algo_common.go Outdated Show resolved Hide resolved
miner/algo_common.go Outdated Show resolved Hide resolved
miner/algo_common.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dvush dvush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks correct to me
re: styling - I think breaking lines looks better but no strong opinion this or that way

@Wazzymandias Wazzymandias merged commit 4a2b6dc into main Aug 3, 2023
3 checks passed
@Wazzymandias Wazzymandias deleted the bundle-tx-discard branch August 3, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants