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

Add new DomainType for application usage with "internal" namespace #2884

Merged
merged 1 commit into from
May 16, 2022

Conversation

ralexstokes
Copy link
Member

@ralexstokes ralexstokes commented May 3, 2022

Opening in lieu of #2882.

Context

This PR adds a new DomainType for use by applications adjacent to the core protocol so they can leverage the signing machinery developed here when working with consensus types.

An example use case is the work-in-progress Builder API where a validator may want to outsource block construction to a network of builders external to their local execution client. See ethereum/execution-apis#209 for details.

Rationale

This PR creates a "reserved" signing namespace for applications by setting the top bit in the DomainType. Applications are still expected to set other bits in the remainder of the DomainType data to get domain separation from other applications using this scheme.

This responsibility is on the applications to self-organize who uses which concrete DomainTypes.

Examples:

DOMAIN_APPLICATION_MEV_BOOST = `0x10000001`
DOMAIN_APPLICATION_ANOTHER_APP = `0x2000001`

This approach is chosen over the approach in #2882 as there is less machinery to get right for users of the consensus spec.

@hwwhww
Copy link
Contributor

hwwhww commented May 4, 2022

I like this approach more than #2882.
Since it's not in use on the consensus specs side, maybe add some description to note that this particular domain type is reserved?

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

Looks good in general.

I might call it the DOMAIN_APPLICATION_MASK and say that DOMAIN_RANDOM_APPLICATION && DOMAIN_APPLICATION_MASK must be non-zero

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

Ah, just realized this was added to Bellatrix. Do you think it makes more sense in Phase 0? As it applies to all phases in gneral?

@ralexstokes
Copy link
Member Author

yeah makes sense, I'll update

@ralexstokes ralexstokes force-pushed the add-application-domain-type-by-bit branch 2 times, most recently from 5ba1812 to 71835c6 Compare May 13, 2022 16:16
@ralexstokes ralexstokes force-pushed the add-application-domain-type-by-bit branch from 71835c6 to 2a6d2e1 Compare May 13, 2022 16:17
@djrtwo djrtwo merged commit 20a90f1 into ethereum:dev May 16, 2022
@ralexstokes ralexstokes deleted the add-application-domain-type-by-bit branch May 16, 2022 14:18
bors bot pushed a commit to sigp/lighthouse that referenced this pull request Jun 30, 2022
## 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants