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

Fix underflows from #1017 #1027

Merged
merged 2 commits into from
May 3, 2019
Merged

Fix underflows from #1017 #1027

merged 2 commits into from
May 3, 2019

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented May 1, 2019

prevent underflows in generate_seed, get_randao_mix, and get_active_index_root.

Note that for epochs less than MIN_SEED_LOOKAHEAD we use ZERO_HASH rather than just pulling from get_randao_mix(state, GENESIS_EPOCH) because the randao_mix at GENESIS_EPOCH is not static and would thus break lookahead.

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

Good catch on randao_mix at GENESIS_EPOCH! 👍

Some epoch/get_current_epoch(state) confusions in my comments.

scripts/phase0/build_spec.py Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
@JustinDrake
Copy link
Collaborator

A few initial comments:

  1. Lines such as min_epoch = get_current_epoch(state) + ACTIVATION_EXIT_DELAY - LATEST_ACTIVE_INDEX_ROOTS_LENGTH + 1 if epoch > LATEST_ACTIVE_INDEX_ROOTS_LENGTH - ACTIVATION_EXIT_DELAY else GENESIS_EPOCH do not fit within the 120 character limit.
  2. Readability for these helper functions has been significantly reduced.
  3. Looking forward to removing the untriggerable assert in get_block_root_at_slot, get_randao_mix, get_active_index_root, get_shuffled_index, integer_squareroot, get_matching_source_attestations 😂Keen to find alternative testing solutions that do not pollute the state transition spec (in a similar vein to Monkeypatch the minus operator #1029).

@JustinDrake JustinDrake added the general:bug Something isn't working label May 2, 2019
@djrtwo
Copy link
Contributor Author

djrtwo commented May 3, 2019

@JustinDrake I'm okay taking the asserts out as they are untriggerable in the context of the state transition function. I added notes in the comment on the expected range for implementers whom I know utilize in other contexts

@djrtwo djrtwo merged commit 8942fac into master May 3, 2019
@djrtwo djrtwo deleted the fix-underflows branch May 3, 2019 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants