-
Notifications
You must be signed in to change notification settings - Fork 784
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
[Merged by Bors] - Standard beacon api updates #1831
Conversation
…ties once per epoch
…dard-beacon-api-updates � Conflicts: � beacon_node/http_api/src/lib.rs � common/eth2/src/lib.rs
…dard-beacon-api-updates � Conflicts: � beacon_node/http_api/tests/tests.rs
…dard-beacon-api-updates � Conflicts: � beacon_node/http_api/src/lib.rs
…lighthouse into standard-beacon-api-updates
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.
This is great, I'm sure users will be stoked to have this. Sorry for taking so long to do the review.
No functional issues, just some style/efficiency comments.
pubkey: PublicKey, | ||
) -> Result<ValidatorDuty, String> { | ||
let pubkey_bytes = PublicKeyBytes::from(&pubkey); | ||
mut known_pubkeys: Vec<(PublicKey, u64)>, |
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.
I have an alternate pattern, feel free to ignore if you like.
Instead of known_pubkeys
and unknown_pubkeys
, just have a pubkeys: &[(PublicKey, Option<u64>)]
. Then, before the for
loop on L64, define an indices = Vec::with_capacity(pubkeys.len())
and push into it whenever you discover a validator index.
This removes known_pubkeys
from this function and the caller, replacing it with indicies
which is in this function only. It also removes an iteration on L85.
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.
Ok, I added this in, but needed an additional way to track the pubkeys of the validator indices we query for. They become necessary when creating empty duties for validators we query for but get no results for. Let me know what you think.
vec![] | ||
}; | ||
.into_iter() | ||
.map(|data| (data.validator_index, data)) |
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.
I get the feeling that if we mapped into a ValidatorDuty
here, instead of on L114, we could avoid the proposal_slots_by_index
map.
It's really not a big deal at all, I just thought I'd mention it.
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.
I was able to get rid of the attester_duties_by_slot
map, but now need to use the pubkey returned by the query for the duty as opposed to the pubkey we have locally. Not that they should be different, but there's an extra deserialization.
I also mapped the result of this query to validator indices to compare with indices we queried for so we can create any remaining empty duties. Let me know if this is along the lines you were thinking.. the codes still a bit ugly, but I'm not really sure how to clean it up further
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.
Cool, thanks for addressing those comments!
I have a few more ones around log consistency and I also have a suggestion commit for the duties endpoint.
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.
Awesome! I've been running this on our Medalla nodes overnight and attestation participation is looking great!
bors r+
## Issue Addressed Resolves #1809 Resolves #1824 Resolves #1818 Resolves #1828 (hopefully) ## Proposed Changes - add `validator_index` to the proposer duties endpoint - add the ability to query for historical proposer duties - `StateId` deserialization now fails with a 400 warp rejection - add the `validator_balances` endpoint - update the `aggregate_and_proofs` endpoint to accept an array - updates the attester duties endpoint from a `GET` to a `POST` - reduces the number of times we query for proposer duties from once per slot per validator to only once per slot Co-authored-by: realbigsean <seananderson33@gmail.com> Co-authored-by: Paul Hauner <paul@paulhauner.com>
Pull request successfully merged into master. Build succeeded: |
Issue Addressed
Resolves #1809
Resolves #1824
Resolves #1818
Resolves #1828 (hopefully)
Proposed Changes
validator_index
to the proposer duties endpointStateId
deserialization now fails with a 400 warp rejectionvalidator_balances
endpointaggregate_and_proofs
endpoint to accept an arrayGET
to aPOST