ethereum / staking-deposit-cli

Secure key generation for deposits
Creative Commons Zero v1.0 Universal
524 stars 321 forks source link

The `SignedBLSToExecutionChange` JSON file schema #312

Open hwwhww opened 1 year ago

hwwhww commented 1 year ago

Proposal

staking-deposit-cli will generate a bls_to_execution_change-{timestamp}.json file which includes the SignedBLSToExecutionChange fields and the meta for debugging. The idea is to allow the stakers to import this JSON file to the SignedBLSToExecutionChange pool of their beacon node (BN) with BN commands.

Seek client teams' feedback on the process and format design. 🙏 2023/01/27 updated: addressed https://github.com/ethereum/staking-deposit-cli/issues/312#issuecomment-1403764550 2023/01/17 updated: fixed JSON schema 2023/01/16 updated: removed fork_version, added '0x'-prefix to hex strings, and add "metadata" field.

JSON schema

The current schema is:

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "type": "array",
  "items": [
    {
      "type": "object",
      "properties": {
        "message": {
          "type": "object",
          "properties": {
            "validator_index": {
              "type": "string"
            },
            "from_bls_pubkey": {
              "type": "string"
            },
            "to_execution_address": {
              "type": "string"
            }
          },
          "required": [
            "validator_index",
            "from_bls_pubkey",
            "to_execution_address"
          ]
        },
        "signature": {
          "type": "string"
        },
        "metadata": {
          "type": "object",
          "properties": {
            "network_name": {
              "type": "string"
            },
            "genesis_validators_root": {
              "type": "string"
            },
            "deposit_cli_version": {
              "type": "string"
            }
          },
          "required": []
        }
      },
      "required": [
        "message",
        "signature"
      ]
    }
  ]
}

For example (it's not the real testable mainnet sig):

[
    {
        "message": {
            "validator_index": "1",
            "from_bls_pubkey": "0x86248e64705987236ec3c41f6a81d96f98e7b85e842a1d71405b216fa75a9917512f3c94c85779a9729c927ea2aa9ed1",
            "to_execution_address": "0x3434343434343434343434343434343434343434"
        },
        "signature": "0x8cf4219884b326a04f6664b680cd9a99ad70b5280745af1147477aa9f8b4a2b2b38b8688c6a74a06f275ad4e14c5c0c70e2ed37a15ece5bf7c0724a376ad4c03c79e14dd9f633a3d54abc1ce4e73bec3524a789ab9a69d4d06686a8a67c9e4dc",
        "metadata": {
            "network_name": "mainnet",
            "genesis_validators_root": "0x4b363db94e286120d76eb905340fdd4e54bfe9f06bf33ff6cf5ad27f511bfe95",
            "deposit_cli_version": "2.3.0"
        }
    }
]

NOTE:

  1. No 0x-prefix for the bytes fields here since the deposit_data-* file doesn't have it as well.
  2. A file MAY include one or multiple items.
mcdee commented 1 year ago

The hex values will need to be prefixed with 0x to be considered valid by the API as per https://github.com/ethereum/beacon-APIs/blob/master/types/primitive.yaml

The metadata fields may or may not be accepted by the consenus nodes, depending on if they are being strict on the JSON they accept. Would need to be checked with each of them individually to be sure.

paulhauner commented 1 year ago

The format generally looks good to me. I haven't been following the deposit specs intimately, though.

I agree with @mcdee that the lack of 0x prefixes is unfortunate. We parse the deposit JSON files from staking-deposit-cli and it uses the same non-consensus-standard formatting for hex values, so it wouldn't be too hard for us to parse this as-is. I understand that staking-deposit-cli is in a tough spot where it needs to decide if it needs to be consistent with itself or the rest of the consensus layer. I'd probably lean towards being consistent with the rest of the consensus layer, since this is designed specifically to go into consensus clients (whereas the deposit JSON wasn't). Especially if we want to be able to pipe this into a curl command. I wouldn't die on that hill, though.

FYI there's a typo in the schema: valdiator_index

lucassaldanha commented 1 year ago

The format looks ok. Although I would prefer if we had "0x" prefixes matching the API definition (https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Beacon/submitPoolBLSToExecutionChange).

The metadata fields may or may not be accepted by the consensus nodes, depending on if they are being strict on the JSON they accept.

In Teku we are lenient as long as the metadata fields are not inside the message object (which will be serialized into the BlsToExecutionChange container), so it would work fine. However, it would be great if the tool had the option to include/remove the metadata fields and the user could decide if they would use them or not.

potuz commented 1 year ago

We already have a feature in place that parses ethdo's output. Pinging @jameshe for necessary changes to parse this

mcdee commented 1 year ago

Note that the ethdo output is the same as the input to /eth/v1/beacon/pools/bls_to_execution_changes so sending these messages can be a simple effort of POSTing the data to the endpoint, reducing the burden on client teams.

(ethdo carries out additional checks and verifications on the input before posting it, but that's very much optional.)

james-prysm commented 1 year ago

Anyone else have any thoughts on if the fork_version, network_name, genesis_validators_root, and deposit_cli_version be part of the messages in the array or have this array wrapped by something with that metadata 🤔

hwwhww commented 1 year ago

Thanks for the feedback!

@mcdee @paulhauner @lucassaldanha: understand, I added 0x prefix to the hex strings. 👍

@james-prysm good point 👍 I think it will be more clear when reading the file and for clients to integrate with it. Updated with this change.

Also, with https://github.com/ethereum/consensus-specs/pull/3206 change, I removed fork_version from the JSON file.

james-prysm commented 1 year ago

I think Ethdo has validator index as a string instead of an integer, can this also be string? or should we support both? https://github.com/attestantio/go-eth2-client/blob/master/spec/capella/blstoexecutionchange.go

tersec commented 1 year ago

Not support both -- it should be specified which. Pointless flexibility is pointless and costly.

mcdee commented 1 year ago

Ah yes, good spot.

This should be a string as the beacon REST API expects all integers to be quoted strings.

hwwhww commented 1 year ago

@james-prysm @tersec @mcdee Thanks for the feedback! I updated the schema, and now valdiator_index is a string field.