#10036 Replace codename Merge with Bellatrix (1st step) #10044

leolara · 2021-12-27T09:08:27Z

What type of PR is this?

Refactoring

What does this PR do? Why is it needed?

Replace codename Merge with Bellatrix

Which issues(s) does this PR fix?

Fixes #10036

Other notes for review

It does not complete 100% the task #10036 another PR will be necessary

CLAassistant · 2021-12-27T09:08:32Z

All committers have signed the CLA.

leolara · 2021-12-27T09:12:48Z

It is not passing in my local system the test TestEndToEnd_MinimalConfig_ValidatorAtCurrentRelease/chain_started but it does not pass it also without any changes, so I guess I have a local problem, and this is not breaking any tests.

The tests I am running is bazel test //testing/endtoend:go_default_test I am not sure if I need to run other tests.

beacon-chain/state/stateutil/state_hasher.go

rkapka · 2021-12-27T11:23:38Z

Following files need to be properly formatted:
config/params/mainnet_config.go
proto/prysm/v1alpha1/beacon_state.pb.go

rkapka · 2022-01-03T22:54:22Z

Hey @leolara , you still need to run gofmt on a few files:

Following files need to be properly formatted:
config/params/mainnet_config.go
proto/prysm/v1alpha1/beacon_state.pb.go
runtime/version/fork.go

leolara · 2022-01-04T08:09:27Z

@rkapka thank you for reminding me that. This PR is marked as WIP. The merge of develop into has caused me so git problems with a commit I did at the same time

leolara · 2022-01-04T08:11:48Z

There are lots of things to rename, I will let you know when I think I am finished, so we can review @rkapka . I will try to also rebase on develop after that so it is fresher.

Is there a bazel way of running gofmt? Or I just run it normally?

potuz · 2022-01-04T08:16:36Z

There are lots of things to rename, I will let you know when I think I am finished, so we can review @rkapka . I will try to also rebase on develop after that so it is fresher.

Is there a bazel way of running gofmt? Or I just run it normally?

You can run it normally or even as a pre commit hook. I recommend you mark the PR as draft, since then reviewers will get an automated email when you mark it requesting a review

PR is WIP

leolara · 2022-01-05T00:41:47Z

@potuz

There are lots of things to rename, I will let you know when I think I am finished, so we can review @rkapka . I will try to also rebase on develop after that so it is fresher.
Is there a bazel way of running gofmt? Or I just run it normally?

You can run it normally or even as a pre commit hook. I recommend you mark the PR as draft, since then reviewers will get an automated email when you mark it requesting a review

Thanks, I changed it to draft

leolara · 2022-01-05T02:34:03Z

I want to note that there is more than a thousand "merge" string instances in the code, and that some times it actually means the action of merge, as opposed to the hard-fork. So I have to read it 🤣

terencechain · 2022-01-05T02:52:05Z

I want to note that there is more than a thousand "merge" string instances in the code, and that some times it actually means the action of merge, as opposed to the hard-fork. So I have to read it 🤣

If it makes it easier, you don't have to replace everything at once. Open a PR package by package is perfectly fine

terencechain · 2022-01-05T02:53:10Z

beacon-chain/core/blocks/payload_test.go

@@ -12,7 +12,7 @@ import (
 	"github.com/prysmaticlabs/prysm/testing/util"
 )

-func Test_MergeComplete(t *testing.T) {
+func Test_BellatrixComplete(t *testing.T) {


This refers to the action of merge

Thanks @terencechain , this one slipped :-)

leolara · 2022-01-05T03:48:59Z

I want to note that there is more than a thousand "merge" string instances in the code, and that some times it actually means the action of merge, as opposed to the hard-fork. So I have to read it 🤣

If it makes it easier, you don't have to replace everything at once. Open a PR package by package is perfectly fine

I am doing it more subject by subject, like I am now changing comments and will send that as a commit. If you think the PR is going to get too long to review, then I could do it in several PRs. I think I am kind of half way

leolara · 2022-01-05T04:29:30Z

In order to do not make the review too tedious and complicated, I have decided to stop considering it WIP, get it reviewed and merged, and then I will continue with a follow up PR with the rest of the renaming.

I have rebased on develop, no need to merge develop now.

Please review @terencechain or other :-)

config/params/mainnet_config.go

terencechain · 2022-01-05T15:44:10Z

proto/prysm/v1alpha1/beacon_block.proto

@@ -33,7 +33,7 @@ message GenericSignedBeaconBlock {
        // Representing a signed, post-Altair fork beacon block.
        SignedBeaconBlockAltair altair = 2;

-        // Representing a signed, post-Merge fork beacon block.
+        // Representing a signed, post-Bellatrix fork beacon block.
        SignedBeaconBlockMerge merge = 3;


Suggested change

SignedBeaconBlockMerge merge = 3;

SignedBeaconBlockBellatrix bellatrix = 3;

Just a reminder here so we don't forget. We can do this in a follow-up PR

leolara · 2022-01-06T03:37:03Z

@terencechain I rebased to fix conflict, it seems it needs further action from mantainers to merge

beacon-chain/db/kv/schema.go

config/params/mainnet_config.go

prestonvanloon · 2022-01-06T17:06:25Z

proto/prysm/v1alpha1/beacon_state.pb.go

@@ -1073,7 +1073,7 @@ func (x *SyncAggregatorSelectionData) GetSubcommitteeIndex() uint64 {
 	return 0
 }

-type BeaconStateMerge struct {
+type BeaconStateBellatrix struct {


Renaming in the generated code is not ideal. Please re-run ./hack/update-go-pbs.sh after modifying proto files.

The reason is that file_proto_prysm_v1alpha1_beacon_state_proto_rawDesc attribute should be updated as well and it's some hex bytes so rebuilding the pb.go files is really the only way to do it. You may need to run gofmt or goimports to sort the imports for the linter.

Done, many files regenerated and pushed

…rBellatrix

leolara · 2022-01-10T11:05:56Z

@prestonvanloon @terencechain @rkapka I think all comments done, rebased to resolve conflicts.

Do you want me to squash when merging or merge normally?

Please, I think we need to move this, as this touches so many files many PRs will conflict this one

leolara · 2022-01-10T13:17:08Z

It seems the problems are not only conflicts with new additions, but also the use of symbols I have renamed in other PRs.

For example, I renamed version.Merge to version.Bellatrix, if someone uses version.Merge and merges a PR, then that symbol does not exist in my PR.

I think we should freeze merging other PRs until this is merged or it will be very difficult

prestonvanloon · 2022-01-10T14:59:16Z

@leolara You need to run gofmt and goimports after regenerating protofiles.

gofmt -w **/*.go
goimports -w **/*.go

Git/Github is not letting me push to your branch. Please fix and we can merge quickly.

leolara · 2022-01-10T15:09:58Z

Ok

leolara · 2022-01-10T15:21:36Z

@prestonvanloon I ran those commands, but there were no effect, it seems my branch is clean or the problem is other

leolara · 2022-01-10T15:23:17Z

@prestonvanloon I changed a setting try to push to my branch now

* Rename BeaconStateMerge to BeaconStateBellatrix * Rename version.Merge to version.Bellatrix * Rename ComputeFieldRootsWithHasherMerge to ComputeFieldRootsWithHasherBellatrix * Rename test names to Bellatrix * Rename comments and strings to Bellatrix * Fix formatting in a few files * Revert wrong renaming in test name * Revert renaming to Bellatrix in mainnet_config.go * Revert renaming of db key without migration * Regenerate from proto changes * Rename new use of already renamed symbols * gofmt and goimports after regenerating protofiles * revert weird imports Co-authored-by: prestonvanloon <preston@prysmaticlabs.com> (cherry picked from commit b1c2454) # Conflicts: # beacon-chain/cache/sync_committee_head_state.go # beacon-chain/core/execution/upgrade.go # beacon-chain/state-proto/v3/state_trie.go

leolara requested a review from a team as a code owner December 27, 2021 09:08

leolara requested review from rauljordan, prestonvanloon and potuz December 27, 2021 09:08

rkapka previously requested changes Dec 27, 2021

View reviewed changes

beacon-chain/state/stateutil/state_hasher.go Outdated Show resolved Hide resolved

beacon-chain/state/stateutil/state_hasher.go Outdated Show resolved Hide resolved

leolara force-pushed the refactor/I10036/rename-merge-to-bellatrix branch from 406f57d to d55d1c5 Compare January 4, 2022 08:09

leolara marked this pull request as draft January 5, 2022 00:40

terencechain reviewed Jan 5, 2022

View reviewed changes

leolara force-pushed the refactor/I10036/rename-merge-to-bellatrix branch from 69a793a to 30ff158 Compare January 5, 2022 04:26

leolara changed the title ~~[WIP] #10036 Replace codename Merge with Bellatrix~~ #10036 Replace codename Merge with Bellatrix (1st step) Jan 5, 2022

terencechain marked this pull request as ready for review January 5, 2022 04:30

terencechain added the Ready For Review label Jan 5, 2022

terencechain reviewed Jan 5, 2022

View reviewed changes

config/params/mainnet_config.go Outdated Show resolved Hide resolved

terencechain reviewed Jan 5, 2022

View reviewed changes

terencechain previously approved these changes Jan 5, 2022

View reviewed changes

leolara dismissed terencechain’s stale review via 74689bf January 6, 2022 03:02

leolara force-pushed the refactor/I10036/rename-merge-to-bellatrix branch from 30ff158 to 74689bf Compare January 6, 2022 03:02

prestonvanloon requested changes Jan 6, 2022

View reviewed changes

leolara added 10 commits January 10, 2022 11:01

Rename BeaconStateMerge to BeaconStateBellatrix

7518300

Rename version.Merge to version.Bellatrix

481a4cd

Rename ComputeFieldRootsWithHasherMerge to ComputeFieldRootsWithHashe…

bb1e0b8

…rBellatrix

Rename test names to Bellatrix

26d0b8c

Rename comments and strings to Bellatrix

363f168

Fix formatting in a few files

0c4f210

Revert wrong renaming in test name

54bab19

Revert renaming to Bellatrix in mainnet_config.go

dfe32b6

Revert renaming of db key without migration

9120557

Regenerate from proto changes

435c709

leolara force-pushed the refactor/I10036/rename-merge-to-bellatrix branch from e7c9fbe to 435c709 Compare January 10, 2022 11:03

Rename new use of already renamed symbols

978aba9

gofmt and goimports after regenerating protofiles

d9f0947

prestonvanloon previously approved these changes Jan 10, 2022

View reviewed changes

prestonvanloon added the OK to merge label Jan 10, 2022

revert weird imports

e5f532c

prestonvanloon dismissed their stale review via e5f532c January 10, 2022 15:35

prestonvanloon approved these changes Jan 10, 2022

View reviewed changes

Merge branch 'develop' into refactor/I10036/rename-merge-to-bellatrix

b15ed98

prylabs-bulldozer bot merged commit b1c2454 into prysmaticlabs:develop Jan 10, 2022

terencechain mentioned this pull request Jan 16, 2022

Eth2api: GetBlockSSZ supports bellatrix block #10077

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#10036 Replace codename Merge with Bellatrix (1st step) #10044

#10036 Replace codename Merge with Bellatrix (1st step) #10044

leolara commented Dec 27, 2021 •

edited

Loading

CLAassistant commented Dec 27, 2021 •

edited

Loading

leolara commented Dec 27, 2021

rkapka commented Dec 27, 2021

rkapka commented Jan 3, 2022

leolara commented Jan 4, 2022

leolara commented Jan 4, 2022

potuz commented Jan 4, 2022

leolara commented Jan 5, 2022

leolara commented Jan 5, 2022

terencechain commented Jan 5, 2022

terencechain Jan 5, 2022

leolara Jan 5, 2022

leolara commented Jan 5, 2022

leolara commented Jan 5, 2022

terencechain Jan 5, 2022

leolara commented Jan 6, 2022

prestonvanloon Jan 6, 2022

leolara Jan 7, 2022

leolara Jan 10, 2022

leolara commented Jan 10, 2022

leolara commented Jan 10, 2022

prestonvanloon commented Jan 10, 2022

leolara commented Jan 10, 2022

leolara commented Jan 10, 2022

leolara commented Jan 10, 2022

	SignedBeaconBlockMerge merge = 3;
	SignedBeaconBlockBellatrix bellatrix = 3;

#10036 Replace codename Merge with Bellatrix (1st step) #10044

#10036 Replace codename Merge with Bellatrix (1st step) #10044

Conversation

leolara commented Dec 27, 2021 • edited Loading

CLAassistant commented Dec 27, 2021 • edited Loading

leolara commented Dec 27, 2021

rkapka commented Dec 27, 2021

rkapka commented Jan 3, 2022

leolara commented Jan 4, 2022

leolara commented Jan 4, 2022

potuz commented Jan 4, 2022

leolara commented Jan 5, 2022

leolara commented Jan 5, 2022

terencechain commented Jan 5, 2022

terencechain Jan 5, 2022

Choose a reason for hiding this comment

leolara Jan 5, 2022

Choose a reason for hiding this comment

leolara commented Jan 5, 2022

leolara commented Jan 5, 2022

terencechain Jan 5, 2022

Choose a reason for hiding this comment

leolara commented Jan 6, 2022

prestonvanloon Jan 6, 2022

Choose a reason for hiding this comment

leolara Jan 7, 2022

Choose a reason for hiding this comment

leolara Jan 10, 2022

Choose a reason for hiding this comment

leolara commented Jan 10, 2022

leolara commented Jan 10, 2022

prestonvanloon commented Jan 10, 2022

leolara commented Jan 10, 2022

leolara commented Jan 10, 2022

leolara commented Jan 10, 2022

leolara commented Dec 27, 2021 •

edited

Loading

CLAassistant commented Dec 27, 2021 •

edited

Loading