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

feat: remove delegator address in WrappedFilePV, change init cmd and add migration cmd. #8

Draft
wants to merge 5 commits into
base: feat/bls-keystore-improvement
Choose a base branch
from

Conversation

wnjoon
Copy link
Collaborator

@wnjoon wnjoon commented Jan 16, 2025

Description

This PR changes the logic including the delegator address while removing the WrappedFilePV structure.

  • Cleaned up the unused field LastSignState in the WrappedFilePV structure and reduced the hierarchy by one level.
  • In babylond create-bls-key, only the bls key is generated based on the password entered as a prompt, and the delegator address is passed as an argument at the time of babylond gen-helper create-bls.
  • Changed to generate priv_validator_key.json and bls key files simultaneously during babylond init.
  • Removed DelegatorAddress from BlsPV struct and removed unnecessary logic to validate DelegatorAddress at ExtendVote().
  • Added an error message to suggest running the init or create-bls-key commands if the key file does not exist at babylond start time. This prevents the node from starting if the key file does not exist, and restricts key files to only be created via specific cli.
  • Add cmd for migration. (How to test for migration)
1. the separation feature would be inserted in a new rc release
2. validator stops the current node
3. validator download the new release
4. validator run the migration cmd
5. restart the node and voting should be resumed
  • Fix e2e test 'e2e-run-upgrade-v1'
    • Added a new build tag, “e2e_upgrade”, to the image used for the e2e test 'e2e-run-upgrade-v1'.
    • When this build tag is active, the previous version’s priv_validator_key.json file is automatically migrated to the updated structure, where the comet and BLS keys are separated. This ensures compatibility with the new key file format.
    • This migration is necessary because containers for pre-upgrade versions are pulled from Docker Hub, Without this adjustment, errors occur due to the outdated key file structure not being applied in the previous version.

How to test for migration

Before starting, two different versions of babylond are required. One is the previous version and the other is the changed version.

The previous version should generate priv_validator_key.json, which contains comet and bls keys. and the changed version generates separate files saving each key.

1. Start the node using the babylond version before changing.

$ ./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 gen-helpers create-bls $(./babylond keys show validator1 -a --keyring-backend test)
# copy content of gen-bls-x.json into checkpointing.genesis_keys field in genesis.json
$ ./babylond start

2. Stop the running node.

3. Execute migration from the changed version of babylond.

Before calling migration, check if the priv_validator_key.json file storing the two types of keys exists inside <home>/.babylond/config.

$ ./babylond migrate

4. Enter the password you will use to encrypt the bls key into the erc2335 structure.

5. Verify that the bls_key.json and bls_password.txt files are correctly created in <home>/.babylond/config.

6. Verify that priv_validator_key.json contains only keys for comet.

7. Restart the node using changed version of babylond.

$ ./babylond start

Error cases

  • If priv_validator_key.json does not exist when calling migration
  • If priv_validator_key.json does not satisfy the previous version of a structure containing comet and bls keys.

@wnjoon
Copy link
Collaborator Author

wnjoon commented Jan 17, 2025

Commit Summary

fd5e873

  • Recreate the WrappedFilePV structure with unnecessary fields removed.
  • Change BlsSigner interface implementation back to WrappedFilePV.

946eab5

  • Add and test InitCmd function that wraps cosmos' InitCmd.
  • Use the function used in create-bls-key to generate a bls key.

57ddd51

  • Remove DelegatorAddress validation when calling ExtendVote function

280de7e

  • Check if key file exists in InitPrivSigner when starting node and output action message

@@ -115,3 +115,7 @@ func (k Keeper) CheckMsgCreateValidator(ctx context.Context, msg *stakingtypes.M
func (k Keeper) GetPubKeyByConsAddr(ctx context.Context, consAddr sdk.ConsAddress) (cmtprotocrypto.PublicKey, error) {
return k.stk.GetPubKeyByConsAddr(ctx, consAddr)
}

func (k Keeper) GetValidatorByConsAddr(ctx context.Context, consAddr sdk.ConsAddress) (stakingtypes.Validator, error) {
Copy link
Member

@dongsam dongsam Jan 20, 2025

Choose a reason for hiding this comment

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

Consider Removing GetValidatorPubkey and FilePVKey

  • If the BLS ValidatorPubkey exists, we can retrieve the associated ValidatorAddr through its pairing (via blsKeysToAddr).
  • This would eliminate the need for GetValidatorPubkey.
  • Consequently, the FilePVKey from Comet in WrappedFilePV would also become unnecessary and could be removed.
  • Removing FilePVKey would reduce overall complexity.

Observations

  • Reading FilePVKey at the app level seems unnatural.
  • In cases where CometBFT uses a PrivValidator that does not rely on FilePV (e.g., Horcrux, tmkms or other SignerClient), FilePV will not exist.
  • Without FilePV, it may not be possible to perform vote extensions.

Benefits of Removing FilePV

  • Simplifies the architecture by removing dependency on FilePVKey.
  • Aligns better with modern PrivValidator implementations that utilize external signing solutions like Horcrux.

@dongsam
Copy link
Member

dongsam commented Jan 20, 2025

@wnjoon The base branch of this PR currently overlaps with the branch for #396. To prevent branches from different PRs from mixing, please fetch https://github.com/babylonlabs-io/babylon/tree/feat/bls-keystore-improvement into the b-harvest repository and update the base branch of this PR

@dongsam
Copy link
Member

dongsam commented Jan 20, 2025

@wnjoon Could you add plan on description for migration cmd?

1. the separation feature would be inserted in a new rc release
2. validator stops the current node
3. validator download the new release
4. validator run the migration cmd
5. restart the node and voting should be resumed

@wnjoon wnjoon changed the base branch from b-harvest/bls-key-separation to feat/bls-keystore-improvement January 20, 2025 09:03
@wnjoon wnjoon force-pushed the feat/wrapped-filepv-removal branch from dbfe05a to d914469 Compare January 20, 2025 09:31
@wnjoon
Copy link
Collaborator Author

wnjoon commented Jan 20, 2025

Commit Summary

cd6ffcf

  • Add migration cmd

cmd/babylond/cmd/migrate.go Outdated Show resolved Hide resolved
cmd/babylond/cmd/migrate.go Outdated Show resolved Hide resolved
cmd/babylond/cmd/migrate_test.go Outdated Show resolved Hide resolved
@wnjoon
Copy link
Collaborator Author

wnjoon commented Jan 21, 2025

Commit Summary

4e41051

  • Change cmd to migrate0bls-key
  • Add verification to check previous keys are equal to keys in new files
  • Add a NOTE to back up previously used key files

@wnjoon
Copy link
Collaborator Author

wnjoon commented Jan 21, 2025

Commit Summary

d055b84

  • Modify migration logic (save files after verification)
  • Add password prompt (missed)
  • Add test case to verify new file after migration

@wnjoon wnjoon force-pushed the feat/wrapped-filepv-removal branch from d055b84 to d296e09 Compare January 22, 2025 07:51
@wnjoon wnjoon changed the title feat: remove wrappedFilePV and delegator, fix create-bls cmd and etc feat: remove delegator address in WrappedFilePV, change init cmd and add migration cmd. Jan 22, 2025
gitferry pushed a commit to babylonlabs-io/babylon that referenced this pull request Jan 24, 2025
…add migration cmd

This PR includes the following changes:

**1. Remove delegator address in WrappedFilePV:**

- Removed Delegator Address field from WrappedFilePV. Delegator Address
can be obtained from kvStore of x/checkpointing module via bls public
key.
- Simplify the structure of WrappedFilePV.

**2. Addition of InitCmd**:

- InitCmd has been updated to generate both Comet and BLS keys through
babylond init.
- The --bls-password flag allows you to specify a BLS password. If the
flag is not provided, the password will be requested interactively via a
prompt.

**3. BLS Key File Migration**:

- The babylond migrate-bls-key command allows the
priv_validator_key.json file from previous versions to be converted into
the separated key file structure used in the current version.
- If the priv_validator_key.json file does not exist or already contains
only Comet key information (indicating it is in the latest format), no
migration will occur.

**4. Fix for `e2e-run-upgrade-v1` Error**:

- Resolved an issue in e2e-run-upgrade-v1 during e2e testing caused by
differences in the key file structures between the current version and
the images pulled from Docker Hub for pre-upgrade containers.
- Introduced a new build tag, e2e_upgrade, applied when building the e2e
Docker image. This ensures that `babylond start` checks the
priv_validator_key.json file, and if it is in an older format, it is
automatically migrated.

And we suggest a new discussion about lightweight BlsSigner interface
with remove FilePVKey from comet in WrappedFilePV.

- When BLS public key is known, the validator address required for Vote
extensions can be obtained using the kvStore of the `x/checkpointing`
module.
- Therefore, the BlsSigner interface can be fully implemented in
BlsPVKey, and the FilePV from comet is not needed.
- In cases where CometBFT uses a PrivValidator that does not rely on
FilePV (e.g., Horcrux or other SignerClient), the FilePV will not be
present. Note that remote signers are currently not supported.
- [comment for
reference](b-harvest#8 (comment))
- Please consider whether this proposal is necessary and give us your
opinion.
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.

2 participants