-
Notifications
You must be signed in to change notification settings - Fork 715
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] - Register validator api #3194
Conversation
…on endpoint to update cache for `ProposerPreparationData`
…d at appropriate times
…gister-validator-api � Conflicts: � validator_client/src/preparation_service.rs
let validator_registration_fut = async move { | ||
loop { | ||
if self.should_publish_at_current_slot(&spec) { | ||
//TODO(sean) we should probably make it so validator registrations happen well before the merge |
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.
Should probably sign on startup, but an epoch before merge seems reasonable to start submitting.. although I guess we don't want to flood the relays at once..
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.
Since we should only publish unchanged signatures once per epoch, I think it's ok to remove this check. And I don't think people will use the builder flow until closer to the merge anyways. I think this is better than potentially flooding the relay an epoch before the merge
fee_recipient, | ||
//TODO(sean) this is geth's default, we should make this configurable and maybe have the default be dynamic. | ||
// Discussion here: https://github.com/ethereum/builder-specs/issues/17 | ||
gas_limit: 30_000_000, |
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 making this configurable should be done in a follow up PR in order to unblock the larger mev-boost compatibility PR
…ns proposals flag
Ok this one's ready for review, it will conflict with #3213 but the conflicts should be pretty easy to resolve, happy to merge that one first and fix the conflicts here |
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.
Looking good! I just have a few minor nits.
It might be nice to make some issues for the TODOs before this merges. I'm happy to do that, here's a list so I don't forget (feel free to add):
- Enable Web3Signer support.
- Add support for custom gas values.
- Use of timestamp value?
beacon_node/http_api/src/lib.rs
Outdated
"client" => client_addr | ||
.map(|a| a.to_string()) | ||
.unwrap_or_else(|| "unknown".to_string()), |
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'm interested in the motivation behind this. This would put validator IP info in our DEBG
logs, which I don't think we usually do. I'm not against it if it's going to be useful.
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 exists in the prepare_beacon_proposer
, not sure the reasoning behind it being put there, but I can remove it from both if we'd like?
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.
Oh interesting, I missed this on a review from an external contributor. Let's pull them both out if you don't mind.
}; | ||
|
||
// Determine the Web3Signer message type. | ||
let message_type = object.message_type(); | ||
|
||
// The `fork_info` field is not required for deposits since they sign across the | ||
// genesis fork version. | ||
let fork_info = if let Web3SignerObject::Deposit { .. } = &object { |
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'm wondering if we've lost this logic on purpose? I don't think we ever actually use Deposit
, but it might be nice to have these here for future protections.
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 split this method into two:
get_signature_from_root
-- can be used for deposits and is used for validator registrations (validator registrations also sign across the genesis fork version)get_signature
-- used for everything else
So we can still sign over deposits with this method but will have to use get_signature_from_root
. I added a check to this method to make sure no fork info is provided if those two types are being signed over.
match self { | ||
// This value is an application index of 0 with the bitmask applied (so it's equivalent to the bit mask). | ||
// Little endian hex: 0x00000001, Binary: 1000000000000000000000000 | ||
ApplicationDomain::Builder => 16777216, |
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 might be nice to have 16777216
as a constant so we don't repeat it in other places of the code. Might even be able to make this function a const fn
?
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.
Went with a constant here for now
Co-authored-by: Paul Hauner <paul@paulhauner.com>
Co-authored-by: Paul Hauner <paul@paulhauner.com>
…an/lighthouse into register-validator-api
Made issues for the other two, but not sure I understand this one |
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, feel free to bors after we pull out those IP addresses 🚀
Already pulled them out in this commit: a73bafe |
bors r+ |
## Issue Addressed Lays the groundwork for builder API changes by implementing the beacon-API's new `register_validator` endpoint ## Proposed Changes - Add a routine in the VC that runs on startup (re-try until success), once per epoch or whenever `suggested_fee_recipient` is updated, signing `ValidatorRegistrationData` and sending it to the BN. - TODO: `gas_limit` config options ethereum/builder-specs#17 - BN only sends VC registration data to builders on demand, but VC registration data *does update* the BN's prepare proposer cache and send an updated fcU to a local EE. This is necessary for fee recipient consistency between the blinded and full block flow in the event of fallback. Having the BN only send registration data to builders on demand gives feedback directly to the VC about relay status. Also, since the BN has no ability to sign these messages anyways (so couldn't refresh them if it wanted), and validator registration is independent of the BN head, I think this approach makes sense. - Adds upcoming consensus spec changes for this PR ethereum/consensus-specs#2884 - I initially applied the bit mask based on a configured application domain.. but I ended up just hard coding it here instead because that's how it's spec'd in the builder repo. - Should application mask appear in the api? Co-authored-by: realbigsean <sean@sigmaprime.io>
## Issue Addressed This PR is a subset of the changes in #3134. Unstable will still not function correctly with the new builder spec once this is merged, #3134 should be used on testnets ## Proposed Changes - Removes redundancy in "builders" (servers implementing the builder spec) - Renames `payload-builder` flag to `builder` - Moves from old builder RPC API to new HTTP API, but does not implement the validator registration API (implemented in #3194) Co-authored-by: sean <seananderson33@gmail.com> Co-authored-by: realbigsean <sean@sigmaprime.io>
Issue Addressed
Lays the groundwork for builder API changes by implementing the beacon-API's new
register_validator
endpointProposed Changes
suggested_fee_recipient
is updated, signingValidatorRegistrationData
and sending it to the BN.gas_limit
config options Clarify Gas Limit in Register Validator endpoint ethereum/builder-specs#17DomainType
for application usage with "internal" namespace ethereum/consensus-specs#2884