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

Eth2api: GetBlockSSZ supports bellatrix block #10077

Merged
merged 6 commits into from
Jan 17, 2022

Conversation

terencechain
Copy link
Member

This PR adds support for Bellatrix block to GetBlockSSZ eth2api implementation.
The addition is from the kintsugi branch. The original author is @potuz, and I added the tests

@terencechain terencechain requested a review from a team as a code owner January 12, 2022 20:14
@terencechain terencechain self-assigned this Jan 12, 2022
@terencechain terencechain added Merge PRs related to the great milestone the merge Ready For Review labels Jan 12, 2022
Copy link
Contributor

@potuz potuz left a comment

Choose a reason for hiding this comment

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

The PR looks good to me, but it's hard if not impossible to review this without being the reviewer of #10044 and related PRs, I'd suggest @leolara and @rkapka to take a quick look.

@@ -312,6 +317,14 @@ func serializeV2Block(response interface{}) (apimiddleware.RunDefault, []byte, a
Signature: respContainer.Data.Signature,
},
}
} else if strings.EqualFold(respContainer.Version, strings.ToLower(ethpbv2.Version_MERGE.String())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the status of Version_MERGE vs Version_Bellatrix?

return &ethpbv2.BlockResponseV2{
Version: ethpbv2.Version_MERGE,
Data: &ethpbv2.SignedBeaconBlockContainerV2{
Message: &ethpbv2.SignedBeaconBlockContainerV2_MergeBlock{MergeBlock: v2Blk},
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@@ -108,6 +108,50 @@ func fillDBTestBlocksAltair(ctx context.Context, t *testing.T, beaconDB db.Datab
return genBlk, blkContainers
}

func fillDBTestBlocksBellatrix(ctx context.Context, t *testing.T, beaconDB db.Database) (*ethpbalpha.SignedBeaconBlockMerge, []*ethpbalpha.BeaconBlockContainer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

b.Block.Body.Attestations = []*ethpbalpha.Attestation{att1, att2}
root, err := b.Block.HashTreeRoot()
require.NoError(t, err)
signedB, err := wrapper.WrappedMergeSignedBeaconBlock(b)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same in lines 113, 117, 126, 138

@terencechain
Copy link
Member Author

The PR looks good to me, but it's hard if not impossible to review this without being the reviewer of #10044 and related PRs, I'd suggest @leolara and @rkapka to take a quick look.

Copy from Discord:

#10036 is still open. For now, review and approve with this disparity in mind. A simple renaming should not block off the rest of the progress. Regarding this PR, as long as CI/CD is green, we should have complete confidence that it is good to go. I will bump the priority for #10036, if there's no other attempt by the end of next week, I will personally take care of it

@leolara
Copy link
Contributor

leolara commented Jan 17, 2022

There will be conflics between this and the stuff I am doing for the name change. In my opinion I would merge this after we change the name for Bellatrix everywhere, untless this is more important to get merge ASAP.

@prylabs-bulldozer prylabs-bulldozer bot merged commit 33d1ae0 into develop Jan 17, 2022
@delete-merged-branch delete-merged-branch bot deleted the get-bellatrix-blk-rpc branch January 17, 2022 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge PRs related to the great milestone the merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants