Skip to content

MEV support - #226

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

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

Conversation

@nkryuchkov

Copy link
Copy Markdown
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
Copy Markdown
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

nkryuchkov commented May 11, 2023

Copy link
Copy Markdown
Contributor Author

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
Comment thread ssv/spectest/run_test.go Outdated
Comment thread ssv/value_check.go
Comment thread types/beacon_types.go

@y0sher y0sher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved. some minor comments.

@GalRogozinski GalRogozinski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Initial review

Comment thread ssv/types.go
Comment thread ssv/validator_registration.go Outdated
Comment thread ssv/validator_registration.go
Comment thread types/beacon_types.go
Comment thread types/beacon_types.go
Comment thread types/testingutils/beacon_node.go

@GalRogozinski GalRogozinski left a comment

Copy link
Copy Markdown
Contributor

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
Comment thread ssv/validator_registration.go
Comment thread ssv/value_check.go
Comment thread ssv/validator_registration.go Outdated
Comment thread ssv/validator_registration.go Outdated
Co-authored-by: Gal Rogozinski <galrogogit@gmail.com>
@GalRogozinski

Copy link
Copy Markdown
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), }

Comment thread ssv/types.go Outdated
Comment thread types/testingutils/runner.go
@GalRogozinski
GalRogozinski changed the base branch from mev-base to main May 17, 2023 09:49
@GalRogozinski

Copy link
Copy Markdown
Contributor

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

Comment thread ssv/spectest/tests/runner/consensus/valid_message.go
Comment thread ssv/value_check.go
Comment thread ssv/spectest/tests/runner/full_happy_flow.go
Comment thread ssv/spectest/tests/runner/postconsensus/invalid_msg_slot.go
Comment thread ssv/spectest/tests/runner/postconsensus/valid_msg.go
@GalRogozinski

Copy link
Copy Markdown
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
Copy Markdown
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