-
Notifications
You must be signed in to change notification settings - Fork 960
Gloas containers additions #8227
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
Gloas containers additions #8227
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.
This function isn't used so I would leave it out. We have other ways of doing these things if it is needed.
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.
yes, alternatively, we could match on fork_name, kind of like this:
SupportedProtocol::BlocksByRangeV2 => match fork_name {
Some(ForkName::Altair) => Ok(Some(RpcSuccessResponse::BlocksByRange(Arc::new(
SignedBeaconBlock::Altair(SignedBeaconBlockAltair::from_ssz_bytes(decoded_buffer)?),
)))),
Some(ForkName::Base) => Ok(Some(RpcSuccessResponse::BlocksByRange(Arc::new(
SignedBeaconBlock::Base(SignedBeaconBlockBase::from_ssz_bytes(decoded_buffer)?),
)))),
the idea was that it would be perhaps more extensible to use from_ssz_bytes_for_fork so you wouldn't have to add variants for new forks in various places across the repo. just add the fork variant once to SignedExecutionPayloadEnvelope::from_ssz_bytes_for_fork instead, so you can just do things like:
Ok(Some(RpcSuccessResponse::ExecutionPayloadEnvelopesByRange(
Arc::new(SignedExecutionPayloadEnvelope::from_ssz_bytes_for_fork(
decoded_buffer,
fork_name,
)?),
)))
i used the from_ssz_bytes_for_fork in a couple p2p boilerplate PR's for ExecutionPayloadEnvelopesByRange and ExecutionPayloadEnvelopesByRoot
- https://github.com/shane-moore/lighthouse/blob/2abfbdab4814278a8ca64aa8119ffcb42ba0d25d/beacon_node/lighthouse_network/src/rpc/codec.rs#L741-L749
- https://github.com/shane-moore/lighthouse/blob/b2de0c5e01b2ff3ce50dfaee3450ca461d6c1fbb/beacon_node/lighthouse_network/src/rpc/codec.rs#L778-L783
but can easily update them to just use match fork_name approach instead. is preferable to kill from_ssz_bytes_for_fork and go this route?
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.
it's rare that we need a method like this because usually when we decode from the network we have context bytes to tell us what we decode and then we would do something like:
ExecutionPayloadEnvelope::Gloas(<_>::from_ssz_bytes(bytes))when we do need a method for decoding by fork variants, we have traits for that. Since the future fork doesn't even exist yet, it's best to leave out 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.
i feel you on that! thanks for the context about context bytes ha
just removed SignedExecutionPayloadEnvelope::from_ssz_bytes_for_fork in latest commit
|
|
||
| impl<E: EthSpec> SignedExecutionPayloadEnvelope<E> { | ||
| /// Create a new `SignedExecutionPayloadEnvelope` from an `ExecutionPayloadEnvelope` and `Signature`. | ||
| pub fn from_envelope(envelope: ExecutionPayloadEnvelope<E>, signature: Signature) -> Self { |
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 function is also not used so I would leave it out.
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 think this helper is just used later in this VC block production PR here:
https://github.com/shane-moore/lighthouse/blob/4eac78c70d1b1cb046180d21b6f81ffa5d617d9a/validator_client/lighthouse_validator_store/src/lib.rs#L760-L784
perhaps I just add the from_envelope to that PR instead?
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.
no that's fine we can leave this in then
433ea22 to
fafc062
Compare
fafc062 to
f3e822b
Compare
Changes
builder_payment_threshold_numeratorandbuilder_payment_threshold_denominatorfromchain_specConfigsince are constants per the specPayloadAttestationMessage.signaturetype updated toSignaturesince is an unaggregated signature@ethDreamer