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

Spec test: capella and update to v1.3.0-alpha.1 #11683

Merged
merged 15 commits into from
Nov 30, 2022
Merged

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Nov 24, 2022

This PR adds Capella spec tests and updates spec tests to the latest alpha release

Pre-req to merging this PR:

  • ProcessBLSToExecutionChanges changes in beacon-chain/core/blocks/withdrawals.go
  • State transition change in beacon-chain/core/transition/transition_no_verify_sig.go
  • Various bug fixes under beacon-chain/state

@terencechain terencechain marked this pull request as ready for review November 24, 2022 00:58
@terencechain terencechain requested a review from a team as a code owner November 24, 2022 00:58
@terencechain terencechain marked this pull request as draft November 24, 2022 00:58
@terencechain terencechain marked this pull request as ready for review November 28, 2022 15:25
Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Tests fail

Comment on lines -29 to -30
WithdrawalQueueLimit = 1099511627776 // WithdrawalQueueLimit defines the maximum number of withdrawals queued in the state.
ExecutionAddressLength = 20 // ExecutionAddressLength defines the length of an execution layer address.
Copy link
Member

Choose a reason for hiding this comment

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

Why are these deleted?

Copy link
Member Author

@terencechain terencechain Nov 28, 2022

Choose a reason for hiding this comment

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

They are not used, and they will never be used, so removed unused variables

MaxWithdrawalsPerPayload = 16 // MaxWithdrawalsPerPayloadLength defines the maximum number of withdrawals that can be included in a payload.
WithdrawalQueueLimit = 1099511627776 // WithdrawalQueueLimit defines the maximum number of withdrawals queued in the state.
MaxWithdrawalsPerPayload = 4 // MaxWithdrawalsPerPayloadLength defines the maximum number of withdrawals that can be included in a payload.
Copy link
Member

Choose a reason for hiding this comment

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

Same here, why these changes to presents?

Copy link
Member Author

Choose a reason for hiding this comment

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

The correct value for MaxWithdrawalsPerPayload is 4
This has to be fixed or else the minimal spec test will fail

"slashings_reset_test.go",
"slashings_test.go",
],
data = glob(["*.yaml"]) + [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data = glob(["*.yaml"]) + [
data = [

There are no yaml files here that would match that glob.

size = "medium",
timeout = "short",
srcs = ["finality_test.go"],
data = glob(["*.yaml"]) + [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data = glob(["*.yaml"]) + [
data = [

Same here, there are no local yaml files that would match this glob

data = glob(["*.yaml"]) + [
"@consensus_spec_tests_mainnet//:test_data",
],
shard_count = 4,
Copy link
Member

Choose a reason for hiding this comment

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

is this shard count appropriate? This looks like 1 test only.

Suggested change
shard_count = 4,
shard_count = 1,

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 here, we're using 1 on devnets but I wasn't aware it was 1 for mainnet

size = "enormous",
timeout = "short",
srcs = ["forkchoice_test.go"],
data = glob(["*.yaml"]) + [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data = glob(["*.yaml"]) + [
data = [

"voluntary_exit_test.go",
"withdrawals_test.go",
],
data = glob(["*.yaml"]) + [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data = glob(["*.yaml"]) + [
data = [

name = "go_default_test",
size = "small",
srcs = ["rewards_test.go"],
data = glob(["*.yaml"]) + [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data = glob(["*.yaml"]) + [
data = [

"blocks_test.go",
"slots_test.go",
],
data = glob(["*.yaml"]) + [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data = glob(["*.yaml"]) + [
data = [

name = "go_default_test",
size = "small",
srcs = ["ssz_static_test.go"],
data = glob(["*.yaml"]) + [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data = glob(["*.yaml"]) + [
data = [

Comment on lines -650 to -653
_, ok := data.Proto().(*enginev1.ExecutionPayloadCapella)
if ok {
return false, nil
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a bad check. data is an interface, and the underlying object could be bellatrix or capella

@terencechain terencechain added Ready For Review A pull request ready for code review Spec labels Nov 28, 2022
rauljordan
rauljordan previously approved these changes Nov 30, 2022
@terencechain terencechain merged commit 8cb07e0 into develop Nov 30, 2022
@delete-merged-branch delete-merged-branch bot deleted the capella-spec-tests branch November 30, 2022 20:08
Comment on lines 255 to +256
require.NoError(t, err)
require.Equal(t, true, got)
require.Equal(t, false, got)
Copy link
Member

Choose a reason for hiding this comment

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

Why is a Capella block not an execution block?

Comment on lines 358 to +360
got, err := blocks.IsExecutionEnabled(st, body)
require.NoError(t, err)
require.Equal(t, true, got)
require.Equal(t, false, got)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, why wouldn't this be enabled in capella?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review Spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants