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

[feature] Optimistic v2 submission flow #524

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

Conversation

michaelneuder
Copy link
Collaborator

@michaelneuder michaelneuder commented Sep 8, 2023

πŸ“ Summary

Spiritual successor to #466.

This follows up on the series: #479, #491, #494, #498, #513, #514, #518, which aim at reducing the diff and productionizing #466.

β›± Motivation and Context

This PR implements the handleSubmitNewBlockV2 and the surrounding test infrastructure. The general flow is:

  • use the SubmitBlockRequestV2Optimistic SSZ type from Create ssz type: SubmitBlockRequestV2OptimisticΒ #518 to decode the header only from the stream.
  • use checkSubmissionSlotDetails, checkBuilderEntry, checkSubmissionPayloadAttrs, and checkBidFloorValue to run prechecks against the submission. each of these functions was part of the refactor set of the original handleSubmitNewBlock function.
  • calls SaveBidAndUpdateTopBid with just the getHeaderResponse because the getPayloadResponse is not yet available.
  • calls optimisticV2SlowPath, which currently just does nothing. The logic from the slow path will come in a follow up PR.

In an effort to increase review-ability of this PR, I stopped here. the slow path logic is quite simple in that it just decodes the full block and simulates it before updating redis with the getPayloadResponse.

Also have nearly 100% coverage of the new lines!

πŸ“š References

https://notes.ethereum.org/@mikeneuder/optimistic-v2

https://github.com/michaelneuder/optimistic-relay-documentation/blob/main/towards-epbs.md#optimistic-relay-v2-header-only-parsing


βœ… I have run these commands

  • make lint
  • make test-race
  • go mod tidy
  • I have seen and agree to CONTRIBUTING.md

"value": bid.Value.String(),
})

// Verify the signature
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why the signature verification was moved above some of the other checks? (compared to handleSubmitNewBlock v1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

honestly it was just a gut feeling that the signature check should probably come first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

good rule of thumb to do inexpensive checks first, if they don't lead to DoS issues and can quickly determine if the request is bad or not

then ramp up in resource demand to signature verification

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep agree, signature checks are expensive and should come after the cheaper checks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done in 3e5f962 !

ty

@michaelneuder michaelneuder mentioned this pull request Sep 15, 2023
4 tasks
michaelneuder and others added 9 commits September 21, 2023 09:24
fix(redis): move WasBidSaved to after exec

We have several conditions where we don't execute the pipelined
commands. Only set the WasBidSaved flag after we execute the pipeline
successfully. Do note, if the pipeline fails we may or may not have
actually saved the bid.
Bumps [github.com/flashbots/go-utils](https://github.com/flashbots/go-utils) from 0.4.11 to 0.4.12.
- [Commits](flashbots/go-utils@v0.4.11...v0.4.12)

---
updated-dependencies:
- dependency-name: github.com/flashbots/go-utils
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Add feature flags for backwards compatibility

* improve readability of flags

* renaming flags

* remove fork schedule checks and flag
Edited descriptions for Validators stats
* Try fix log point in builder api

* Fix log reassignment in builder submission
Bumps [github.com/flashbots/go-utils](https://github.com/flashbots/go-utils) from 0.4.12 to 0.5.0.
- [Commits](flashbots/go-utils@v0.4.12...v0.5.0)

---
updated-dependencies:
- dependency-name: github.com/flashbots/go-utils
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github.com/ethereum/go-ethereum](https://github.com/ethereum/go-ethereum) from 1.12.2 to 1.13.1.
- [Release notes](https://github.com/ethereum/go-ethereum/releases)
- [Commits](ethereum/go-ethereum@v1.12.2...v1.13.1)

---
updated-dependencies:
- dependency-name: github.com/ethereum/go-ethereum
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2023

Codecov Report

Merging #524 (6bab6ac) into main (e0d1248) will increase coverage by 2.41%.
Report is 7 commits behind head on main.
The diff coverage is 76.13%.

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

@@            Coverage Diff             @@
##             main     #524      +/-   ##
==========================================
+ Coverage   33.23%   35.64%   +2.41%     
==========================================
  Files          24       24              
  Lines        5079     5397     +318     
==========================================
+ Hits         1688     1924     +236     
- Misses       3173     3248      +75     
- Partials      218      225       +7     
Flag Coverage Ξ”
unittests 35.64% <76.13%> (+2.41%) ⬆️

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

Files Changed Coverage Ξ”
common/test_utils.go 26.25% <0.00%> (-11.25%) ⬇️
common/types_spec.go 0.00% <0.00%> (ΓΈ)
datastore/redis.go 59.87% <62.50%> (+0.16%) ⬆️
services/api/service.go 46.34% <89.81%> (+6.58%) ⬆️

... and 2 files with indirect coverage changes

pipeliner redis.Pipeliner
}

func (api *RelayAPI) optimisticV2SlowPath(r io.Reader, v2Opts v2SlowPathOpts) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is slowPath supposed to be doing?

@metachris
Copy link
Collaborator

metachris commented Oct 19, 2023

A new API should be proposed and merged in the relay specs first: https://github.com/flashbots/relay-specs

Would it be good if v2 also supports non-optimistic submissions?

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

8 participants