Add per-validator builder configuration API - #9864
Conversation
|
This pull request has merge conflicts. Could you please resolve them @jimmygchen? 🙏 |
f0f272a to
c3f6c06
Compare
|
This pull request has merge conflicts. Could you please resolve them @jimmygchen? 🙏 |
c3f6c06 to
3c16568
Compare
3c16568 to
d4866c8
Compare
|
This pull request has merge conflicts. Could you please resolve them @jimmygchen? 🙏 |
chong-he
left a comment
There was a problem hiding this comment.
I have go through the PR and tested the endpoint manually and they look great. Just some minor comments and a bug found by Codex
| #[derive(Debug)] | ||
| pub struct CustomForbidden(pub String); | ||
|
|
||
| impl Reject for CustomForbidden {} | ||
|
|
||
| pub fn custom_forbidden(msg: String) -> warp::reject::Rejection { | ||
| warp::reject::custom(CustomForbidden(msg)) | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a new reject type for the http apis. It actually correct matches the specs 403 Forbidden, as from the spec:
A builder configuration was found, but cannot be removed. This may be because it was in configuration files that cannot be updated.
Given that this is new, so I look up existing Keymanager API, e.g., the fee recipient one:
lighthouse/validator_client/http_api/src/lib.rs
Lines 1013 to 1017 in 60f8759
and found that it is returning 500 for a deletion error, which isn't following the spec error code. I suspect this is the case for other existing Keymanager endpoints too. But then the error code has been incorrect for so long and it doesn't look like a big deal anyway.
Just pointing this out
There was a problem hiding this comment.
Yes this is according to spec. I've left the existing endpoint response scope outside of PR.
| } | ||
|
|
||
| impl ValidatorBuilderConfig { | ||
| pub(crate) fn validate(&self) -> Result<(), Error> { |
There was a problem hiding this comment.
Nit: there are a few function with name validate, maybe we can rename name for better clarity? For example, this can be renamed to validate_validator_builder_config or something like that
There was a problem hiding this comment.
i think this may look a bit too verbose, because you'd end up having something like
validator_builder_config.validate_validator_builder_config
| self.builders.push(definition); | ||
| } | ||
|
|
||
| pub fn validate(&self) -> Result<(), Error> { |
| } | ||
|
|
||
| /// Return the fully resolved configuration for a validator without signing builder auth data. | ||
| pub fn validator_config( |
There was a problem hiding this comment.
Maybe rename to get_validator_config? Sounds a bit clearer to me, but this is also a bit so feel free to ignore
There was a problem hiding this comment.
Changed this to get_validator_config, thanks
| fn global_max_execution_payment(&self, builder: &ValidatorBuilderDefinition) -> Option<u64> { | ||
| let auth_data = builder | ||
| .auth_data | ||
| .clone() | ||
| .unwrap_or_else(|| builder.url.to_default_auth_data()); | ||
| self.builders | ||
| .iter() | ||
| .filter(|global| global.enabled) | ||
| .find(|global| { | ||
| global.url == builder.url | ||
| && global | ||
| .auth_data | ||
| .clone() | ||
| .unwrap_or_else(|| global.url.to_default_auth_data()) | ||
| == auth_data | ||
| }) | ||
| .map(|global| global.max_execution_payment) | ||
| } |
There was a problem hiding this comment.
Is this intended that we want to find the max_execution_payment for the matching builder when it's not provided in the POST? I thought it could be simpler if we just default to 0 and no need to find the matching entries if the field is not provided
There was a problem hiding this comment.
Yes, the lookup is intentional. An omitted max_execution_payment takes the validator client's configured value, so defaulting directly to 0 would be incorrect. We match the enabled global entry by URL and effective auth_data. 0 is only used when no entry matches.
| builder.min_bid = Some(builder.min_bid.unwrap_or(min_bid)); | ||
| builder.builder_boost_factor = | ||
| Some(builder.builder_boost_factor.unwrap_or(builder_boost_factor)); | ||
| builder |
There was a problem hiding this comment.
Found by Codex:
Say we have a global builder enabled with min_bid: 100. And then we POST builder_config for a validator with request body:
{"min_bid": "500"}
When we GET the validator config, we don't get 500 min_bid for this validator, it uses the top level value:
{
"data": {
"min_bid": "1", # global top level
"builder_boost_factor": "100", # global top level
"builders": [
{
"url": "https://builder.example",
"auth_data": "0x68747470733a2f2f6275696c6465722e6578616d706c65",
"builder_pubkeys": [],
"max_execution_payment": "123",
"min_bid": "100", # should be 500 here
"builder_boost_factor": "100"
}
]
}
}
(althought the YAML file is showing per-validator min-bid is 500)
But if we POST with a builder url in the request body, then it works:
Request body:
{"min_bid": "500", "builders": [{"url": "https://example"}]}
Response of GET:
{
"data": {
"min_bid": "500",
"builder_boost_factor": "100",
"builders": [
{
"url": "https://another-builder.example",
"auth_data": "0x68747470733a2f2f616e6f746865722d6275696c6465722e6578616d706c65",
"builder_pubkeys": [],
"max_execution_payment": "0",
"min_bid": "500", # 500 min_bid shown correctly
"builder_boost_factor": "100"
}
]
}
}
There was a problem hiding this comment.
Great catch. Fixed in 7de5c200f. Validator level defaults now override inherited global builder values. The test uses conflicting values and checks that DELETE restores the global values.
d4866c8 to
023488e
Compare
Description
Adds the per-validator builder configuration endpoints from ethereum/keymanager-APIs#88.
GET /eth/v1/validator/{pubkey}/builder_configreturns the configuration in use for the validator.POST /eth/v1/validator/{pubkey}/builder_configreplaces and persists the full configuration on a per-validator basis. It does not merge with the previous entry.DELETE /eth/v1/validator/{pubkey}/builder_configremoves the stored entry, so the validator inherits the global configuration again.The API stores per-validator entries under
validator_configsinbuilder_definitions.yml.Changes made by
POSTandDELETEapply to subsequent builder preference publication and Gloas block production without a restart.Closes #9796
Additional Info
This PR is stacked on #9807 and should be reviewed after it.