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.

Sign up for GitHub

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

Jump to bottom

fix: Multiple network flags should prevent the BN to start #14169

Merged
merged 21 commits into from
Jul 10, 2024
Merged

fix: Multiple network flags should prevent the BN to start #14169

merged 21 commits into from
Jul 10, 2024

Conversation

shyam-patel-kira
Copy link
Contributor

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?
This will prevent BN from starting if there are multiple network flags

Which issues(s) does this PR fix?

Fixes #13801

Other notes for review
Let me know if I need to add comments to change any part of the code

@shyam-patel-kira shyam-patel-kira requested a review from a team as a code owner July 2, 2024 00:43
@CLAassistant
Copy link

CLAassistant commented Jul 2, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

nalepae
nalepae reviewed Jul 3, 2024
View reviewed changes
cmd/flags_test.go Outdated Show resolved Hide resolved
nalepae
nalepae reviewed Jul 3, 2024
View reviewed changes
cmd/flags.go Outdated Show resolved Hide resolved
nalepae
nalepae reviewed Jul 3, 2024
View reviewed changes
cmd/flags.go Outdated Show resolved Hide resolved
nalepae
nalepae reviewed Jul 3, 2024
View reviewed changes
cmd/flags.go Outdated Show resolved Hide resolved
@nalepae
Copy link
Contributor

nalepae commented Jul 3, 2024

I know the GH issue is only for the beacon node.
However, other binaries have the same issue (validator client and some other CLIs).

Could you handle them as well?

nalepae
nalepae reviewed Jul 3, 2024
View reviewed changes
cmd/flags.go Outdated Show resolved Hide resolved
@shyam-patel-kira
Copy link
Contributor Author

shyam-patel-kira commented Jul 4, 2024 •
edited
Loading

Other binaries have the same issue (validator client and some other CLIs).

I added it to the validator client, can you point out which other cli needs to be changed?

nalepae
nalepae reviewed Jul 9, 2024
View reviewed changes
config/features/config.go Outdated
@@ -337,3 +339,24 @@ func logDisabled(flag cli.DocGenerationFlag) {
}
log.WithField(name, flag.GetUsage()).Warn(disabledFeatureFlag)
}

// ValidateNetworkFlags validates and prevents beacon node
Copy link
Contributor

Choose a reason for hiding this comment

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

Not only beacon node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment

@nalepae
Copy link
Contributor

nalepae commented Jul 9, 2024

I added it to the validator client, can you point out which other cli needs to be changed?

The only other place I see these flags are:

prysmctl validator exit

@shyam-patel-kira
Copy link
Contributor Author 

prysmctl validator exit

added it here

@nalepae nalepae enabled auto-merge July 10, 2024 07:10
@nalepae
Copy link
Contributor

nalepae commented Jul 10, 2024

Thanks!

@nalepae nalepae added this pull request to the merge queue Jul 10, 2024
Merged via the queue into prysmaticlabs:develop with commit 7c81c7d Jul 10, 2024
16 of 17 checks passed
saolyn pushed a commit that referenced this pull request Jul 17, 2024
@shyam-patel-kira @nalepae @saolyn
fix: Multiple network flags should prevent the BN to start (#14169)
911f7f2 
* Implement Initial Logic

* Include check in main.go

* Add tests for multiple flags

* remove usage of append

* remove config/features dependency

* Move ValidateNetworkFlags to config/features

* Nit

* removed NetworkFlags from cmd

* remove usage of empty string literal

* add comment

* add flag validation to prysctl validator-exit

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
github-merge-queue bot pushed a commit that referenced this pull request Jul 19, 2024
@saolyn @prestonvanloon
@nisdas @shyam-patel-kira @nalepae @rkapka
HTTP endpoint GetIndividualVotes (#14198)
57ffc12 
* add http endpoint

* add tests

* Gaz

* Add pointers

* add endpoint to test

* Electra: EIP-7251 Update `process_voluntary_exit` (#14176)

* Electra: EIP-7251 Update `process_voluntary_exit`

* Add unit test for VerifyExitAndSignature EIP-7251

* @potuz peer feedback

* Avoid Cloning When Creating a New Gossip Message (#14201)

* Add Current Changes

* add back check

* Avoid a Panic

* fix: Multiple network flags should prevent the BN to start (#14169)

* Implement Initial Logic

* Include check in main.go

* Add tests for multiple flags

* remove usage of append

* remove config/features dependency

* Move ValidateNetworkFlags to config/features

* Nit

* removed NetworkFlags from cmd

* remove usage of empty string literal

* add comment

* add flag validation to prysctl validator-exit

---------

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>

* fix tests

* Radek' review + tests

* fix tests

* Radek' review

* forgot one

* almost forgot the tests

---------

Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>
Co-authored-by: Nishant Das <nishdas93@gmail.com>
Co-authored-by: kira <shyampkira@gmail.com>
Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nalepae nalepae nalepae approved these changes

@kasey kasey Awaiting requested review from kasey kasey is a code owner automatically assigned from prysmaticlabs/core-team

@rauljordan rauljordan Awaiting requested review from rauljordan rauljordan is a code owner automatically assigned from prysmaticlabs/core-team

@potuz potuz Awaiting requested review from potuz potuz is a code owner automatically assigned from prysmaticlabs/core-team

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Multiple network flags should prevent the BN to start.
3 participants
@shyam-patel-kira @CLAassistant @nalepae