search

ethereum / keymanager-APIs

Collection of RESTful APIs provided by Ethereum consensus keymanagers
https://ethereum.github.io/keymanager-APIs/
Creative Commons Zero v1.0 Universal
37 stars 19 forks source link

GET `/eth/v1/validator/{pubkey}/graffiti` clarification #77

Open courtneyeh opened 5 months ago

courtneyeh commented 5 months ago

A couple of scenarios require clarification:

  1. The graffiti is described as "Arbitrary data to set in the graffiti field of BeaconBlockBody". Should we be returning the exact graffiti that will be used when creating a block including the node info appended to the graffiti? Or should we just be returning the graffiti that was set / system wide default depending on the situation?

For context, Teku appends consensus layer (CL) and execution layer (EL) clients' information to the validator graffiti. More details are linked here: validators-graffiti-client-append-format

This clarification relates to whether the graffiti returned from the GET should include this appended format, or just the set graffiti, without the appended information.

  1. In the response, the pubkey is described as an optional field. In what case would this be empty, as we will always have the value from the path parameter?
nflaig commented 5 months ago

Should we be returning the exact graffiti that will be used when creating a block including the node info appended to the graffiti?

Based on previous discussion on this, I would say just return the graffiti as set by the user, without appending any data that might be implementation specific.

In the response, the pubkey is described as an optional field

Regarding (2), I just added that field in graffiti API to be inline with other APIs. I would have to dig through old PRs to give you context why the pubkey is part of the response in the first place. Apparently there was some discussion on discord around this https://github.com/ethereum/keymanager-APIs/pull/28, maybe @rolfyone can shed some light on it

rolfyone commented 5 months ago

looks like we removed it as redundant and just made it optional rather than creating a new api... Makes enough sense, basically I'd pretend its not there, but don't fail if it is.

re. the first point, I guess one thing would be the engine api change to allow us to markup the graffiti with version data... If we return the graffiti without this markup, it sounds like thats fine in your opinion @nflaig ?

So if teku would use graffiti: "graffiti TKXXXXBUYYYY" the user has set just "graffiti", we'd just return "graffiti"... and into the block would appear the full string... That's not confusing? Then i guess as extension, if the user deletes that, and the system has a default, we'd just return the default without the markup, and be consistent... so if the default is "default", you'll just get that back. In both cases you won't really know if its been set for that specific key via keymanager, or if it's the system default... I guess its kind of academic...

nflaig commented 5 months ago

If we return the graffiti without this markup, it sounds like thats fine in your opinion @nflaig ?

Depends on how people are using the APIs and if they use GET at all, it is likely just informational anyways. For example, rocketpool allows to set custom graffiti and only shows the custom value in their UI, but will wrap it "on the fly" and you will end up with something like RP-<client_combo> <RP_version> (<custom_graffiti>) when proposing the block.

So if teku would use graffiti: "graffiti TKXXXXBUYYYY" the user has set just "graffiti", we'd just return "graffiti"... and into the block would appear the full string... That's not confusing?

Both cases could be potentially confusing depending on user expectations. I am not a fan of modifying the graffiti of the user if it's set explicitly, or at a minimum, if it's done it has to be opt-in so the user should be aware that the graffiti set via CLI (or API) is not what will be used during proposal.

I had this comment in the description previously (https://github.com/ethereum/keymanager-APIs/pull/63/commits/e0332ebc8bf1751a6c045ef9ae2664f73dfd43f8)

This graffiti is the one used by the validator when proposing blocks.

which would have made it clear that you should return "graffiti TKXXXXBUYYYY" but there was pushback to add any specifics about what graffiti will be used during block proposal.

nflaig commented 5 months ago

So if teku would use graffiti: "graffiti TKXXXXBUYYYY" the user has set just "graffiti", we'd just return "graffiti"... and into the block would appear the full string... That's not confusing?

Regarding returning the full string incl. client info on the keymanager API, I am wondering how we would even do this technically as the graffiti is configured on the validator client but the extra client info is appended by the beacon node "on the fly" during proposal. If the beacon node is allowed to modify the graffiti, the validator client will not know what graffiti will be used in the end, it can ask the beacon node, but this gets complicated pretty quickly if you have multiple beacon nodes with different ELs connected.

Considering this, it is likely the best to just return the graffiti as it was set by the user which might or might not end up as is in the next proposal depending on if the beacon nodes modifies it or not.

yksnyn commented 1 month ago

https://github.com/ethereum/keymanager-APIs/releases/download/v1.0.0/keymanager-oapi.yaml