-
Notifications
You must be signed in to change notification settings - Fork 37
feat: electra get_attesting_indices changes + helper functions from predicates/misc. #1419
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a comment that could cause issues in the future (in the get_attesting_indices
) and another one to show how comprehension could match closer the spec if needed.
committee_bits | ||
|> get_committee_indices() | ||
|> Enum.reduce({MapSet.new(), 0}, fn index, {old_set, offset} -> | ||
with {:ok, committee} <- get_beacon_committee(state, data.slot, index) do | ||
committee | ||
|> Stream.with_index() | ||
|> Stream.filter(fn {_value, index} -> | ||
participated?(aggregation_bits, offset + index) | ||
end) | ||
|> Stream.map(fn {value, _index} -> value end) | ||
|> MapSet.new() | ||
|> then(&{MapSet.union(&1, old_set), offset + length(committee)}) | ||
end | ||
end) | ||
|> then(fn {map, _offset} -> {:ok, map} end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may have an issue here, before when the get_beacon_committee/3
returned an error we just bubbled it up, now if this happens the reduce will fail because it will try to parse an {:error, error}
as the {old_set, offset}
tuple. We could go with a reduce_while, instead (BTW made some changes for readability):
committee_bits
|> get_committee_indices()
|> Enum.reduce_while({MapSet.new(), 0}, fn committee_index, {attesters, offset} ->
case get_beacon_committee(state, data.slot, committee_index) do
{:ok, committee} ->
# Process this committee's attesters
committee_attesters =
committee
|> Stream.with_index(offset)
|> Stream.filter(fn {_validator, pos} -> participated?(aggregation_bits, pos) end)
|> Stream.map(fn {validator, _} -> validator end)
|> MapSet.new()
{:cont, {MapSet.union(attesters, committee_attesters), offset + length(committee)}}
error ->
# Depending on the specs we might just want to skip this error with something like: `{attesters, offset}`
{:halt, error}
end
end)
|> case do
{:error, error} -> {:error, error}
{attesters, _offset} -> {:ok, attesters}
end
Also, I'm not 100% sure about the usefulness of Streams here, but given that it was already implemented that way I wouldn't change it to Enum for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops I didn't know with
behaved that way. I implemented the changes here. I also tried both {:halt, error} and {:cont, {attesters, offset} and the amount of tests passing is the same so for now I think we can leave it as {:halt, error}.
Motivation
Implement the changes for Electra for sections predicates, misc
This PR does not fix any spec-test but does not break any either to check this
should return same amount of tests as #1417
Description
Predicates
is_eligible_for_activation_queue
linkis_compounding_withdrawal_credential
linkhas_compounding_withdrawal_credential
linkhas_execution_withdrawal_credential
linkis_fully_withdrawable_validator
linkis_partially_withdrawable_validator
linkMisc
get_committee_indices
linkBeacon state accessors
get_attesting_indices
link