Skip to content

Comments

MEV support#226

Merged
liorrutenberg merged 19 commits intossvlabs:mainfrom
nkryuchkov:mev-support-2
May 21, 2023
Merged

MEV support#226
liorrutenberg merged 19 commits intossvlabs:mainfrom
nkryuchkov:mev-support-2

Conversation

@nkryuchkov
Copy link
Contributor

Supersedes #177, #163

Spec part of ssvlabs/ssv#918

@nkryuchkov nkryuchkov requested review from GalRogozinski and y0sher May 9, 2023 14:37
This was referenced May 9, 2023
nkryuchkov added a commit to ssvlabs/ssv that referenced this pull request May 9, 2023
nkryuchkov added a commit to ssvlabs/ssv that referenced this pull request May 9, 2023
@GalRogozinski
Copy link
Contributor

Hey @nkryuchkov

For this to merge I have 2 requests:

  1. Start using state comparison feature on tests that you change/add. Here is a random example: State comparison - QBFT proposal #210. Feel free to contact me to understand better.
  2. We want to have small PRs. Can you create a branch on this repo and do maybe 2 PRs to this branch? Later we will PR the big branch to main

Thanks!

@nkryuchkov
Copy link
Contributor Author

nkryuchkov commented May 11, 2023

Hey @GalRogozinski

  1. I'll check State comparison - QBFT proposal #210
  2. I think I could extract just a few line changes that are not directly related to MEV, but the rest looks to me like something whole. Why do you think the PR is not small enough? It's 109 changed lines now

# Conflicts:
#	types/testingutils/beacon_node.go
@liorrutenberg liorrutenberg requested a review from moshe-blox May 14, 2023 09:50
Copy link
Contributor

@y0sher y0sher left a comment

Choose a reason for hiding this comment

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

Approved. some minor comments.

Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

Initial review

Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

Initial review

@GalRogozinski GalRogozinski changed the base branch from main to mev-base May 15, 2023 13:36
@nkryuchkov nkryuchkov requested a review from GalRogozinski May 15, 2023 14:57
Co-authored-by: Gal Rogozinski <galrogogit@gmail.com>
@GalRogozinski
Copy link
Contributor

@nkryuchkov
I think that for all tests in the preconsensus folder that are for "validator registration" we should add
BeaconBroadcastedRoots: []string{ testingutils.GetSSZRootNoError(testingutils.TestingValidatorRegistration), }

@GalRogozinski GalRogozinski changed the base branch from mev-base to main May 17, 2023 09:49
@GalRogozinski
Copy link
Contributor

@alonmuroch
If we change DefaultGasLimit to 1, the roots stay the same.

@GalRogozinski
Copy link
Contributor

@alonmuroch
Could you shed light on the purpose of preconsensus/unordered_expected_roots tests? It looks the same as valid_msg?

@alonmuroch
Copy link
Contributor

@alonmuroch
Could you shed light on the purpose of preconsensus/unordered_expected_roots tests? It looks the same as valid_msg?

just checking roots not ordered, should pass as a valid msg

@liorrutenberg liorrutenberg merged commit d549a20 into ssvlabs:main May 21, 2023
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.

6 participants