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

only add active validators to knownValidators at startup #4793

Merged
merged 1 commit into from
Apr 6, 2023
Merged

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Apr 6, 2023

Fix #4787

@tersec tersec enabled auto-merge (squash) April 6, 2023 08:28
@github-actions
Copy link

github-actions bot commented Apr 6, 2023

Unit Test Results

         6 files   -        3       712 suites   - 356   25m 13s ⏱️ - 9m 11s
  3 343 tests  -    310    3 322 ✔️  -      52  21 💤  - 258  0 ±0 
10 392 runs   - 5 192  10 350 ✔️  - 4 929  42 💤  - 263  0 ±0 

Results for commit 558ca85. ± Comparison against base commit b59f9f5.

@tersec tersec merged commit 742de13 into unstable Apr 6, 2023
@tersec tersec deleted the 8kl branch April 6, 2023 13:53
template v: auto = forkyState.data.validators.item(idx)
if is_active_validator(v, wallSlot.epoch) or
is_active_validator(v, wallSlot.epoch + 1):
node.consensusManager[].actionTracker.knownValidators[idx] = wallSlot
Copy link
Member

Choose a reason for hiding this comment

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

we should log a notice / warning here that the validator is exited

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might not be exited but not yet active, too. The window there is smaller because most of the time it's not active on that side of the timeline, it won't have a validator index yet, but there can be some window after the deposit contract picks it up but before the churn rate limits let it become active.

The check here is deliberately broad. The actual Nimbus mechanism cares about active-or-not, not why it might not be active, which is a bit irrelevant in determining whether it should be asking the action tracker to schedule actions.

Copy link
Member

Choose a reason for hiding this comment

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

The actual Nimbus mechanism cares about active-or-not,

the user cares about knowing about the state of their validator - ie the logging can differentiate between reasons, once it has figured out it's not active

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

2 participants