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

hardcoded Genesis validator root #14404

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

Redidacove
Copy link
Contributor

What does this PR do? Why is it needed ?
Replaced hardcoded value everywhere where fn was called to compute it from state calling st.GenesisValidatorRoot()
Which issues(s) does this PR fix ?
Fixes
Issue #14364

Other notes for review
Currently just replaced hardcoded value in
Rpc package
slasher package
P2p package
If asked can also do that for sync

beacon-chain/p2p/service.go Outdated Show resolved Hide resolved
@Redidacove
Copy link
Contributor Author

@nisdas Just asking if I can include sync package changes as well in this one or should I be opening it in future Pr

@nisdas
Copy link
Member

nisdas commented Sep 3, 2024

Lets do the rpc package first here

nisdas
nisdas previously approved these changes Sep 5, 2024
@james-prysm
Copy link
Contributor

There are definitely more places that are needlessly pulling genesis validators root that should probably be updated, not sure if it should be in this PR or separately i.e.

if !bytesutil.IsValidRoot(config.GenesisValidatorsRoot) {

@Redidacove
Copy link
Contributor Author

Redidacove commented Sep 12, 2024

There are definitely more places that are needlessly pulling genesis validators root that should probably be updated, not sure if it should be in this PR or separately i.e.

if !bytesutil.IsValidRoot(config.GenesisValidatorsRoot) {

I was asked to do for rpc only for now so removed those changes and that's why closed previous pr which had much more changes including yours and sync package and others.

nisdas
nisdas previously approved these changes Sep 17, 2024
@Redidacove
Copy link
Contributor Author

Redidacove commented Sep 17, 2024

@nisdas srry for lint fails, fixed it now

@nisdas
Copy link
Member

nisdas commented Sep 20, 2024

FAIL: TestNodeServer_GetGenesis (0.06s)
FAIL: TestWaitForChainStart_AlreadyStarted (0.00s)

These 2 tests fail now along with our E2E tests

@Redidacove
Copy link
Contributor Author

FAIL: TestNodeServer_GetGenesis (0.06s)
FAIL: TestWaitForChainStart_AlreadyStarted (0.00s)

These 2 tests fail now along with our E2E tests

Looking into it

@Redidacove
Copy link
Contributor Author

Saw it why failling coz i hardcoded few places in the code but test is expecting some functions to set root which is why they call the WaitForChainStart with stream and get hardcoded root as I replaced code where it was setting it instead hardcoded it.
things are too coupled here have too see where else needed to hardcode to make it work.Little unclear logs in failled statement difficult to understand where exactly is failing

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.

3 participants