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

Min bid #274

Merged
merged 6 commits into from
Oct 20, 2022
Merged

Min bid #274

merged 6 commits into from
Oct 20, 2022

Conversation

allboxes
Copy link
Contributor

@allboxes allboxes commented Aug 28, 2022

πŸ“ Summary

Allow users to set a minimum bid to accept a block from a relay

β›± Motivation and Context

#273


βœ… I have run these commands

  • [βœ… ] make lint
  • [βœ…] make test-race
  • [βœ…] go mod tidy

@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2022

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 54.76190% with 19 lines in your changes missing coverage. Please review.

Project coverage is 66.66%. Comparing base (5c49007) to head (f10d25c).

Files with missing lines Patch % Lines
cli/main.go 45.71% 18 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #274       +/-   ##
===========================================
- Coverage   82.12%   66.66%   -15.47%     
===========================================
  Files           5        7        +2     
  Lines         677      879      +202     
===========================================
+ Hits          556      586       +30     
- Misses         92      261      +169     
- Partials       29       32        +3     
Flag Coverage Ξ”
unittests 66.66% <54.76%> (-15.47%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

server/service.go Outdated Show resolved Hide resolved
cli/main.go Outdated Show resolved Hide resolved
@allboxes
Copy link
Contributor Author

@metachris do you need anything else from me on this?

cli/main.go Outdated Show resolved Hide resolved
cli/main.go Show resolved Hide resolved
server/service.go Outdated Show resolved Hide resolved
@metachris
Copy link
Collaborator

looks like a good feature to have. left a few comments, also please rebase πŸ™

server/service.go Outdated Show resolved Hide resolved
cli/main_test.go Outdated Show resolved Hide resolved
@allboxes
Copy link
Contributor Author

Thanks for the comments, they should all be resolved in the above commit

@metachris metachris added the in review πŸ” currently being reviewed label Oct 11, 2022
@jumboshrimp100
Copy link

@allboxes, I think this PR is step in the right direction given that many people are wary of running mev-boost especially since the mev-boost reward has the possibility of being less than the reward you would have earned from a locally produced block. I have 2 comments (although this may be beyond the scope of the PR)

  1. There are 2 types of min bid, absolute min bid and relative min bid. I take it this PR is an absolute mid bid, but it would also be useful to have the option to specify a relative min bid which would be the min bid above the reward you would receive from a locally produced block

  2. Is there a way of transmitting this information to the relay? Under this implementation if I have a min bid of .1 eth will the relay realize this and only submit bids of .1 eth or more? Will the bid immediately be rejected, giving the relay the opportunity to submit a higher bid? Or will the low bid be silently ignored? The last case seems sub-optimal if the relay is willing to bid higher but did not know of the min required bid of the validator.

@jtraglia
Copy link
Collaborator

Hey @jumboshrimp100 I appreciate the questions!

  1. There are 2 types of min bid, absolute min bid and relative min bid. I take it this PR is an absolute mid bid, but it would also be useful to have the option to specify a relative min bid which would be the min bid above the reward you would receive from a locally produced block

That's an interesting idea, but not in scope for mev-boost. Later, when the execution API adds the engine_getPayload endpoint (see this issue on GitHub), it will be possible for the validator client to compare the remote block's value to the local block's value. After this point, it's up the client to implement the feature (relative mid-bid) you mentioned.

  1. Is there a way of transmitting this information to the relay? Under this implementation if I have a min bid of .1 eth will the relay realize this and only submit bids of .1 eth or more? Will the bid immediately be rejected, giving the relay the opportunity to submit a higher bid? Or will the low bid be silently ignored? The last case seems sub-optimal if the relay is willing to bid higher but did not know of the min required bid of the validator.

Personally, if I were to use this I'd like to keep my min-bid preference private. And this doesn't really align with how things work. A relay only has one opportunity to send a bid to mev-boost. If mev-boost rejects that bid, there is no higher bid that the relay can send, it's already sent it's highest value bid.

Copy link
Collaborator

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Overall, this looks pretty good. I like this feature. See my suggestions.

cli/main_test.go Outdated Show resolved Hide resolved
cli/main.go Outdated Show resolved Hide resolved
cli/main_test.go Outdated Show resolved Hide resolved
jtraglia
jtraglia previously approved these changes Oct 18, 2022
Copy link
Collaborator

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing those things.

jtraglia
jtraglia previously approved these changes Oct 19, 2022
Copy link
Collaborator

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Thanks, I didn't see that typo.

cli/main.go Outdated
@@ -25,6 +27,7 @@ var (
defaultListenAddr = getEnv("BOOST_LISTEN_ADDR", "localhost:18550")
defaultRelayCheck = os.Getenv("RELAY_STARTUP_CHECK") != ""
defaultGenesisForkVersion = getEnv("GENESIS_FORK_VERSION", "")
defaultRelayMinBidEth = getEnvFloat64("MIN_BID_ETH", 0.001)
Copy link
Collaborator

Choose a reason for hiding this comment

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

One more thing, the default minimum bid should be zero. It's up to the user (operator) to set this value based on their preferences. I'm just concerned with the side effects of this. For example, a default value of 0.001 would affect testnets (like sepolia) where the average mev reward is most likely to be below that threshold. I understand the desire for a non-zero default: low value blocks will be built using the local execution engine. But when CL clients start to compare the value to the block it built locally, I think that would serve as a better "default" minimum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that makes sense.

Copy link
Collaborator

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks!

@metachris metachris merged commit dd4665d into flashbots:main Oct 20, 2022
@allboxes allboxes deleted the min-bid branch October 22, 2022 21:38
screwyprof pushed a commit to screwyprof/mev-boost that referenced this pull request Feb 3, 2023
* add min-bid feature

* Add Float to U256 test, other minor review changes

* Re-wrote floatEthTo256Wei + addressed review comments

* Apply suggestions from jtraglia's code review

Co-authored-by: Justin Traglia <[email protected]>

* fix typo

* Changed default min-bid to 0 and added logging

Co-authored-by: allboxes <[email protected]>
Co-authored-by: Justin Traglia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss in development πŸ› οΈ not yet ready to be merged in review πŸ” currently being reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants