feat: BLS Key Separation and ERC2335 Implementation #396

wnjoon · 2025-01-10T12:12:31Z

Description

This PR was created from epic #336, separates BLS key from priv_validator_key.json, and applies ERC2335.

There are still issues in CI that are being resolved, but I'm uploading a PR to first discuss the changed structure and ideas.

ERC2335 structure

Use keystorev4 with prysm as reference
Add ERC2335 encryption/decryption and password generation functions

Change WrappedFilePV to include BLS structure and use cometbft's FilePV directly

type WrappedFilePVKey struct {
	CometPVKey privval.FilePVKey
	BlsPVKey   BlsPVKey
}

BLS separation

At the time of babylond init, only priv_validator_key.json is created.
babylond create-bls-key creates a BLS key with the password entered as the prompt and encrypts it into an ERC2335 structure.
The generated BLS key in ERC2335 format is saved as bls_key.json, and the password is saved as bls_password.txt.

An example of a bls_key.json is below:

{
  "crypto": {
    "checksum": {
      "function": "sha256",
      "message": "d291102754ece0a5ab9d4c3c75f452069d192df6c90d616e466d7e7884f41b3a",
      "params": {}
    },
    "cipher": {
      "function": "aes-128-ctr",
      "message": "e638d3f04b803c3ec934c31749ed20c1ffcd48ab481b4fad6b303aa2e7ff991c",
      "params": {
        "iv": "473e315746ec9997f0d0e423f73e2d3d"
      }
    },
    "kdf": {
      "function": "pbkdf2",
      "message": "",
      "params": {
        "c": 262144,
        "dklen": 32,
        "prf": "hmac-sha256",
        "salt": "3f1b139f3663082f582acc6d48fd9bf8caf4ea9c4e51c6ef75c9bc5b5c04ee9e"
      }
    }
  },
  "version": 4,
  "uuid": "",
  "path": "",
  "pubkey": "a28b189e76e8d61461811eec22bc8d5c926bf1276afde5344a3b9d3a2769a1a61ea42471a867d4811a85640dfde8fed702d81bcc9f56494ec65471151fac6e3c262541245f1b16c9e22000eef2142c43f6ad5ca8c31fc16081261cc54d3f034c",
  "description": "bbn196xcr99y5eehdg6lr38u8re0j6xeq0twup5rl4"
}

An example of a priv_validator_key.json with BLS separated is below:

{
  "address": "3141C6C8EF5FDAE49931AE2F9EB295F5C9E2212A",
  "pub_key": {
    "type": "tendermint/PubKeyEd25519",
    "value": "CwCehdhn8DmcVTKn4I1RHzlmur5LbEZxSEr+zvf7Ifo="
  },
  "priv_key": {
    "type": "tendermint/PrivKeyEd25519",
    "value": "HIHCMsU9+J3FjEU7gooSed5moKRjar6TfLa3Nb1CSCULAJ6F2GfwOZxVMqfgjVEfOWa6vktsRnFISv7O9/sh+g=="
  }
}

Change DelegatorAddress to be stored in the description field of ERC2335 structure instead of storing it in `priv_validator_key.json`.

DelegatorAddress is stored in the description field of the bls_key.json file in erc2335 format.

type BlsPVKey struct {
	PubKey  bls12381.PublicKey  `json:"bls_pub_key"`
	PrivKey bls12381.PrivateKey `json:"bls_priv_key"`
	DelegatorAddress string
        // ...
}

Test script

$ ./babylond init my-node --chain-id test-chain-1

$ ./babylond keys add validator1 --keyring-backend test

$ ./babylond add-genesis-account $(./babylond keys show validator1 -a --keyring-backend test) 2000000000000ubbn,2000000000000stake

$ ./babylond gentx validator1 1500000000000stake --chain-id test-chain-1 --keyring-backend test --fees 400ubbn

$ ./babylond collect-gentxs

$ ./babylond create-bls-key $(./babylond keys show validator1 -a --keyring-backend test)

$ ./babylond gen-helpers create-bls

After the create-bls command, json file starting with the prefix "gen-bls" will be created in {home}/.babylond/config.

example: gen-bls-bbnvaloper196xcr99y5eehdg6lr38u8re0j6xeq0twpr77n5.json

Copy data in the created file to the 'checkpointing.genesis_keys' field in genesis.json.

Here is an example after inserting data to 'checkpointing.genesis_keys' field in genesis.json

"checkpointing": {
      "genesis_keys": [
        {
          "validator_address": "bbnvaloper196xcr99y5eehdg6lr38u8re0j6xeq0twpr77n5",
          "bls_key": {
            "pubkey": "oosYnnbo1hRhgR7sIryNXJJr8Sdq/eU0SjudOidpoaYepCRxqGfUgRqFZA396P7XAtgbzJ9WSU7GVHEVH6xuPCYlQSRfGxbJ4iAA7vIULEP2rVyowx/BYIEmHMVNPwNM",
            "pop": {
              "ed25519_sig": "9YrVS1/KN0Fq6po0JbWirX+0enhxiShqxVw+lQFn22qam/2KaZXXzNMsOOOBoyQ9ZSD7ARc/TtohYBbhLIF/Cg==",
              "bls_sig": "pRAZUAH2ZDPZqgoiPTsFYTYndBctJXoY1lvN5GsWNNbPDYyIeXihaS28pqauHCcy"
            }
          },
          "val_pubkey": {
            "key": "CwCehdhn8DmcVTKn4I1RHzlmur5LbEZxSEr+zvf7Ifo="
          }
        }
      ]
    },

Subsequent tasks

Ensure the BLS key file is created atomically with priv_validator_key.json
BLS key non-validation logic for non-validator
Remove DelegatorAddress from the BlsPV structure and obtain it from GetValidatorByConsAddr() function of the Epoch(Staking) module.
~~Add the option to use the --bls-password flag when creating the BLS key~~ 1f14b31
Modify not to keep WrappedFilePV since LastSignState is not used

type WrappedFilePV struct {
	Key           WrappedFilePVKey
	LastSignState privval.FilePVLastSignState
}

…bls-key

… in erc2335 description

gitferry

Nice work! Some initial comments:

app/signer/private.go

client/flags/flags.go

cmd/babylond/cmd/create_bls_key.go

crypto/erc2335/erc2335.go

gitferry

Thank for the answers. One thing as I mentioned in the pr is the change the target branch. Other than that the code lgtm! Will approve once it's ready for review. Also, let's wait for review from @KonradStaniec.

How is CI going (I don't know why I can's see the ci results)?

wnjoon · 2025-01-14T12:49:20Z

Commit summary 1f14b31

Added the bls-password flag to be optionally provided when executing the create-bls-key CLI.
Removed code related to the DefaultConfig of BLS and updated the implementation accordingly.
Deleted unused flags.
Renamed the function for checking the existence of a file directory path (IsValidFilePath -> EnsureDirs).
Fixed linting errors.

Additionally, in the CI pipeline, there is an issue with a specific E2E test: test-e2e-cache-upgrade-v1. The problem appears to stem from the recent change requiring the BLS key to be created exclusively during the create-bls-key process. As a result, the node does not start if the key is missing.

While addressing the UX issue referred to in this context, it seems more efficient to resolve the related problem together.
I will proceed directly with the second PR.

Furthermore, aside from the test-e2e-cache-ibc-transfer test, which requires DockerHub login, all other E2E tests, and unit tests have been successfully executed locally.

KonradStaniec

few nits here and there, but in general LGTM! great progress 👍

KonradStaniec · 2025-01-14T13:32:36Z

crypto/erc2335/erc2335.go

+)
+
+type Erc2335KeyStore struct {
+	Crypto      map[string]interface{} `json:"crypto"`


Some doc for this field would be nice like what does this map hold (as type map[string]interface{} does not tell much to the reader)

KonradStaniec · 2025-01-14T13:34:58Z

crypto/erc2335/erc2335.go

+
+	keyJSONBytes, err := os.ReadFile(filePath)
+	if err != nil {
+		return Erc2335KeyStore{}, err


lets wrap errors here with some additional useful info given that this is public function which is pretty low level

KonradStaniec · 2025-01-14T13:36:54Z

crypto/erc2335/erc2335.go

+	encryptor := keystorev4.New()
+	cryptoFields, err := encryptor.Encrypt(privKey, password)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to encrypt private key")


usually in our codebases we are wrapping our errors with fmt.Errorf("error msg: %w", err) (which imo looks nicer though this is highly personal opinion), any reason for using errors ?

KonradStaniec · 2025-01-14T13:37:17Z

crypto/erc2335/erc2335.go

+	}
+
+	if err := json.Unmarshal(keyJSONBytes, &keystore); err != nil {
+		return Erc2335KeyStore{}, err


lets wrap the error here

KonradStaniec · 2025-01-14T13:39:05Z

crypto/erc2335/erc2335.go

+	password := make([]byte, 32)
+	_, err := rand.Read(password)
+	if err != nil {
+		panic(err)


this is a bit risky in such low level public operation which does not know context of the application.

Maybe:

if it is only for testing, lets make it private

it is supposed to be public, lets return error

KonradStaniec · 2025-01-14T13:40:51Z

privval/bls.go

+	// encrypt the bls12381 key to erc2335 type
+	erc2335BlsPvKey, err := erc2335.Encrypt(k.PrivKey, k.PubKey.Bytes(), password)
+	if err != nil {
+		panic(err)


seems weird to be panicking in low level library code. Lets wrap and return errors ? (and panic at callsites if necessary)

The logic for saving BLS keys to files is written to be consistent with FilePV in cometbft.

In addition, since Save is not executed during state transition, but only when setting keys before the chain is executed, the risk of panic should not be great.

Nevertheless, if necessary, I will rewrite it to return an error. Please give me your opinion.

I'm fine with either way

Lazar955 · 2025-01-14T13:47:01Z

crypto/erc2335/erc2335.go

+
+// wonjoon: encrypt key pair to erc2335 keystore
+// available to handle all keys in []byte format
+func Encrypt(privKey, pubKey []byte, password string) ([]byte, error) {


nit: proper go doc

wnjoon · 2025-01-15T06:35:43Z

Commit Summary 9dd45f3

Add comments based on go doc in erc2335.go and bls.go
Remove unused function in erc2335.go
- Replace not to generate random password during unit tests.
- Remove the save/load password function in erc2335 and modify related codes in bls.go
Wrapping errors to use fmt.Errorf in privval package

wnjoon · 2025-01-15T09:08:12Z

Here is the plan for the second PR:

This second PR is expected to be set R4R until next week.

Remove WrappedFilePV:

Since the LastSignState field has been removed, the WrappedFilePV structure is no longer necessary. Methods that previously relied on WrappedFilePV will be updated to directly reference FilePV and BlsPV from CometBFT.

Ensure Atomic Creation of BLS Key and priv_validator_key.json:

Further discussion is needed on “ensuring the atomic creation of the BLS key file along with priv_validator_key.json and removing the create-bls-key command”.

Since priv_validator_key.json can be automatically generated without running init due to Comet’s behavior, it may be necessary to modify Comet’s code to synchronize the timing of BLS key generation.
Alternatively, the BLS key could be generated on demand (e.g., during block generation) based on the node’s validator status. This would ensure that the Comet keys and the BLS key always coexist in the app.
The final implementation approach depends on whether key atomicity should be enforced at the Comet or app level.

Update Delegator Address Handling:

Modify the logic so that the delegator address is provided directly during the ExtendVote() function instead of being stored in BlsPV. This will follow the pseudo-code shared earlier.

Check Validator Status During InitPrivSigner:

Add a check in InitPrivSigner to determine if the node is a validator by verifying whether the Consensus public key(from FilePV) is included in the validator list. If the node is a validator, generate the BLS key; otherwise, skip key generation.

wnjoon added 6 commits January 10, 2025 20:06

feat: add erc2335 structure

dda7030

feat: separate bls structure

857ab38

fix: get bls password from prompt and create bls structure in create-…

255d25c

…bls-key

fix: move delegator address to bls structure and save to bls_key.json…

6138f62

… in erc2335 description

refactor: refactoring codes related to bls and test code

f6dd2c1

feat: update CHANGELOG

8037568

dongsam mentioned this pull request Jan 10, 2025

Separate BLS key from priv_validator_key.json #336

Open

gitferry reviewed Jan 13, 2025

View reviewed changes

gitferry requested a review from KonradStaniec January 13, 2025 13:14

gitferry reviewed Jan 14, 2025

View reviewed changes

wnjoon changed the base branch from main to feat/bls-keystore-improvement January 14, 2025 10:08

feat: add bls password flag option, refactor comments and fix lint

1f14b31

wnjoon marked this pull request as ready for review January 14, 2025 13:12

KonradStaniec approved these changes Jan 14, 2025

View reviewed changes

Lazar955 reviewed Jan 14, 2025

View reviewed changes

fix: add comments, wrapping errors and remove unused functions

9dd45f3

gitferry merged commit 339276c into babylonlabs-io:feat/bls-keystore-improvement Jan 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: BLS Key Separation and ERC2335 Implementation #396

feat: BLS Key Separation and ERC2335 Implementation #396

wnjoon commented Jan 10, 2025 •

edited

Loading

gitferry left a comment

gitferry left a comment

wnjoon commented Jan 14, 2025 •

edited

Loading

KonradStaniec left a comment

KonradStaniec Jan 14, 2025

wnjoon Jan 15, 2025

KonradStaniec Jan 14, 2025

wnjoon Jan 15, 2025

KonradStaniec Jan 14, 2025

wnjoon Jan 15, 2025

KonradStaniec Jan 14, 2025

wnjoon Jan 15, 2025

KonradStaniec Jan 14, 2025

wnjoon Jan 15, 2025

KonradStaniec Jan 14, 2025

wnjoon Jan 15, 2025

gitferry Jan 15, 2025

Lazar955 Jan 14, 2025

wnjoon Jan 15, 2025

wnjoon commented Jan 15, 2025

wnjoon commented Jan 15, 2025

feat: BLS Key Separation and ERC2335 Implementation #396

feat: BLS Key Separation and ERC2335 Implementation #396

Conversation

wnjoon commented Jan 10, 2025 • edited Loading

Description

ERC2335 structure

Change WrappedFilePV to include BLS structure and use cometbft's FilePV directly

BLS separation

Change DelegatorAddress to be stored in the description field of ERC2335 structure instead of storing it in priv_validator_key.json.

Test script

Subsequent tasks

gitferry left a comment

Choose a reason for hiding this comment

gitferry left a comment

Choose a reason for hiding this comment

wnjoon commented Jan 14, 2025 • edited Loading

KonradStaniec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wnjoon commented Jan 15, 2025

wnjoon commented Jan 15, 2025

Remove WrappedFilePV:

Ensure Atomic Creation of BLS Key and priv_validator_key.json:

Update Delegator Address Handling:

Check Validator Status During InitPrivSigner:

wnjoon commented Jan 10, 2025 •

edited

Loading

Change DelegatorAddress to be stored in the description field of ERC2335 structure instead of storing it in `priv_validator_key.json`.

wnjoon commented Jan 14, 2025 •

edited

Loading