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

Fork choice fixes 2 #1356

Merged
merged 2 commits into from
Jul 22, 2020
Merged

Fork choice fixes 2 #1356

merged 2 commits into from
Jul 22, 2020

Conversation

arnetheduck
Copy link
Member

No description provided.

* enable v2 pruning
* prefer `get_current_epoch`
* fix finalization check to use correct epoch
* add `count_active_validators`
* remove misleading logs
* fix justified checkpoint slot calculation in rpc
@arnetheduck arnetheduck requested review from mratsim and tersec July 22, 2020 18:12
@@ -56,6 +56,12 @@ func is_active_validator*(validator: Validator, epoch: Epoch): bool =
### Check if ``validator`` is active
validator.activation_epoch <= epoch and epoch < validator.exit_epoch

# https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/beacon-chain.md#get_active_validator_indices
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a fine abstraction, but still not something one would want to call that often, for the same reasons that get_active_validator_indices() is not great to call often.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, as an isolated function, just count_active_validators() captures the idea fine, as a name. There is, though, a case for the symmetry between {get,count}_active_validator_indices(). Subjective and to taste.

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, yeah, I suspect there may be tricks to get rid of it - until then, it's a trivial addition that simplifies some code

Copy link
Contributor

Choose a reason for hiding this comment

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

It also avoids memory allocations, which is probably much of what's left outside BLS cryptography.

func populateEpochCache(state: BeaconState, epoch: Epoch): EpochRef =
(EpochRef)(
epoch: state.slot.compute_epoch_at_slot,
func populateEpochCache(state: BeaconState): EpochRef =
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I noticed this when investigating possible causes for the recently found cache invalidation bug. it's conceptually plausible to have that flexibility of an epoch distinct from the state epoch, but in practice was unused.

@arnetheduck arnetheduck merged commit fb2f742 into devel Jul 22, 2020
@arnetheduck arnetheduck deleted the fork-choice-fixes-2 branch July 22, 2020 21:01
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