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

wnjoon · 2025-01-22T09:56:08Z

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
Please consider whether this proposal is necessary and give us your opinion.

gitferry

In general looks good to me! Let's wait approval from @KonradStaniec. Some comments:

gitferry · 2025-01-23T04:48:24Z

cmd/babylond/cmd/init.go

+		},
+	}
+	cmd.Flags().AddFlagSet(cosmosInitCmd.Flags())
+	cmd.Flags().String(flagBlsPassword, "", "The password for the BLS key. If a flag is set, the non-empty password should be provided. If a flag is not set, the password will be read from the prompt.")


Suggested change

cmd.Flags().String(flagBlsPassword, "", "The password for the BLS key. If a flag is set, the non-empty password should be provided. If a flag is not set, the password will be read from the prompt.")

cmd.Flags().String(flagBlsPassword, "", "The password for the BLS key. If the flag is not set, the password will be read from the prompt.")

fixed bbe2efd

gitferry · 2025-01-23T04:49:20Z

cmd/babylond/cmd/init.go

+	cmd := &cobra.Command{
+		Use:   cosmosInitCmd.Use,
+		Short: cosmosInitCmd.Short,
+		Long:  cosmosInitCmd.Long,


I think maybe we should extend comsosInitCmd.Long with BLS description

fixed bbe2efd

gitferry · 2025-01-23T05:01:44Z

cmd/babylond/cmd/migrate.go

+		Long: strings.TrimSpace(`migrate splits the contents of the priv_validator_key.json file, 
+		which contained both the bls and comet keys used in previous versions, into separate files.
+
+BLS keys are stored along with other validator keys in priv_validator_key.json in previous version,
+which should exist before running the command (via babylond init or babylond testnet).
+
+NOTE: Before proceeding with the migration, ensure you back up the priv_validator_key.json file to a secure location.
+This will help prevent potential loss of critical validator information in case of issues during the migration process.
+
+Example:
+$ babylond migrate-bls-key --home ./
+`,


Suggested change

Long: strings.TrimSpace(`migrate splits the contents of the priv_validator_key.json file,

which contained both the bls and comet keys used in previous versions, into separate files.

BLS keys are stored along with other validator keys in priv_validator_key.json in previous version,

which should exist before running the command (via babylond init or babylond testnet).

NOTE: Before proceeding with the migration, ensure you back up the priv_validator_key.json file to a secure location.

This will help prevent potential loss of critical validator information in case of issues during the migration process.

Example:

$ babylond migrate-bls-key --home ./

`,

Long: strings.TrimSpace(`Migration splits the contents of the priv_validator_key.json file,

which contained both the bls and comet keys used in previous versions, into separate files.

BLS keys are stored along with the Ed25519 validator key in priv_validator_key.json in the previous version,

which should exist before running the command (via babylond init or babylond testnet).

NOTE: Before proceeding with the migration, ensure you back up the priv_validator_key.json file to a secure location.

This will help prevent potential loss of critical validator information in case of issues during the migration process.

Example:

$ babylond migrate-bls-key --home ./

`,

fixed bbe2efd

gitferry · 2025-01-23T05:03:17Z

cmd/babylond/cmd/migrate.go

migrate.go -> migrate_bls.go

How about changing it to migrate_bls_key.go for consistency with create_bls_key.go?

fixed bbe2efd

gitferry · 2025-01-23T05:04:15Z

cmd/babylond/cmd/migrate.go

+	filepath := cmtcfg.PrivValidatorKeyFile()
+
+	if !cmtos.FileExists(filepath) {
+		return fmt.Errorf("priv_validator_key.json of previous version not found")


Suggested change

return fmt.Errorf("priv_validator_key.json of previous version not found")

return fmt.Errorf("priv_validator_key.json of previous version not found in %s", filepath)

fixed bbe2efd

gitferry · 2025-01-23T05:27:24Z

privval/file.go


 	"github.com/babylonlabs-io/babylon/crypto/bls12381"
 	checkpointingtypes "github.com/babylonlabs-io/babylon/x/checkpointing/types"
-	sdk "github.com/cosmos/cosmos-sdk/types"
+	cmtcrypto "github.com/cometbft/cometbft/crypto"
+	cmtprivval "github.com/cometbft/cometbft/privval"
 )



let's add var _ keeper.BlsSigner = &WrappedFilePV{}

fixed bbe2efd

gitferry · 2025-01-23T05:29:47Z

testutil/signer/private.go

@@ -34,8 +35,12 @@ func SetupTestPrivSigner() (*signer.PrivSigner, error) {
 	return privSigner, nil
 }

-func GenesisKeyFromPrivSigner(ps *signer.PrivSigner) (*checkpointingtypes.GenesisKey, error) {
-	valKeys, err := privval.NewValidatorKeys(ps.WrappedPV.GetValPrivKey(), ps.WrappedPV.GetBlsPrivKey())
+// func GenesisKeyFromPrivSigner(ps *signer.PrivSigner, delegatorAddress sdk.ValAddress) (*checkpointingtypes.GenesisKey, error) {


fixed bbe2efd

gitferry · 2025-01-23T05:30:10Z

x/checkpointing/keeper/bls_signer.go

@@ -9,10 +9,10 @@ import (
 )

 type BlsSigner interface {
-	GetAddress() sdk.ValAddress
+	// GetAddress() sdk.ValAddress


fixed bbe2efd

gitferry · 2025-01-23T05:33:54Z

x/checkpointing/vote_ext.go

-		_, _, err := curValSet.FindValidatorWithIndex(signer)
+
+		valConsAddr := k.GetConAddressFromPubkey()
+		val, err := k.GetValidatorByConsAddr(ctx, valConsAddr)


I wondered if we need this check at all, given that ExtendVote is only called if the validator is in the validator set. Wdyt? @KonradStaniec

I would leave it tbh it is not a big cost, and if something change on cometbft side (or they will be some bug) we will quickly catch it after upgrade

#434 (comment)

@KonradStaniec
According to what you said, I think it would be a good idea to discuss this in the next PR.

gitferry · 2025-01-23T05:35:51Z

x/checkpointing/vote_ext_test.go

+// WARN
+// This test code became untestable after deleting the DelegatorAddress field.
+//
+// // FuzzExtendVote_NotInValidatorSet tests the case where the
+// // private signer is not in the validator set
+// func FuzzExtendVote_NotInValidatorSet(f *testing.F) {
+// 	datagen.AddRandomSeedsToFuzzer(f, 10)
+
+// 	f.Fuzz(func(t *testing.T, seed int64) {
+// 		r := rand.New(rand.NewSource(seed))
+// 		// generate the validator set with 10 validators as genesis
+// 		genesisValSet, ps, err := datagen.GenesisValidatorSetWithPrivSigner(10)
+// 		require.NoError(t, err)
+
+// 		// the private signer is not included in the validator set
+// 		helper := testhelper.NewHelperWithValSetNoSigner(t, genesisValSet, ps)
+
+// 		ek := helper.App.EpochingKeeper
+
+// 		epoch := ek.GetEpoch(helper.Ctx)
+// 		require.Equal(t, uint64(1), epoch.EpochNumber)
+
+// 		// go to block 10, reaching epoch boundary
+// 		interval := ek.GetParams(helper.Ctx).EpochInterval
+// 		for i := uint64(0); i < interval-2; i++ {
+// 			_, err := helper.ApplyEmptyBlockWithSomeInvalidVoteExtensions(r)
+// 			require.NoError(t, err)
+// 		}
+
+// 		req := &abci.RequestExtendVote{
+// 			Hash:   datagen.GenRandomByteArray(r, types.HashSize),
+// 			Height: 10,
+// 		}
+
+// 		// error is expected because the BLS signer in not
+// 		// in the validator set
+// 		_, err = helper.App.ExtendVote(helper.Ctx, req)
+// 		require.Error(t, err)
+// 	})
+// }


Think we can remove it

fixed bbe2efd

gitferry · 2025-01-23T05:41:15Z

And we suggest a new discussion about lightweight BlsSigner interface with remove WrappedFilePV structure

Totally agree with the suggestion! For bls signing, the comet pv is not needed. WrappedFilePV is legacy code from where we embeded bls key into priv_validator_key.json.

KonradStaniec

apart from some leftover commented code, lgtm!

KonradStaniec · 2025-01-23T07:31:42Z

x/checkpointing/vote_ext.go

-		_, _, err := curValSet.FindValidatorWithIndex(signer)
+
+		valConsAddr := k.GetConAddressFromPubkey()
+		val, err := k.GetValidatorByConsAddr(ctx, valConsAddr)


I would leave it tbh it is not a big cost, and if something change on cometbft side (or they will be some bug) we will quickly catch it after upgrade

wnjoon · 2025-01-24T04:33:48Z

Commit Summary bbe2efd

Changed the logic in migrate-bls-key cmd to load and verify keys stored in separate files.
Removed unnecessary comments and added cmd description.
Error Extension and etc

wnjoon added 4 commits January 22, 2025 16:50

feat: add initcmd creating both comet and bls key files

e657998

feat: add migration cmd

d296e09

feat: fix e2e_upgrade_test by adding new build tag

fb3ce52

feat: update CHANGELOG

efc8fcb

wnjoon marked this pull request as ready for review January 22, 2025 10:00

gitferry reviewed Jan 23, 2025

View reviewed changes

KonradStaniec approved these changes Jan 23, 2025

View reviewed changes

feat: verify migration after saving files and etc

bbe2efd

gitferry merged commit 655d700 into babylonlabs-io:feat/bls-keystore-improvement Jan 24, 2025

dongsam mentioned this pull request Jan 31, 2025

feat: remove dependency cometpv remote signer b-harvest/babylon#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wnjoon commented Jan 22, 2025 •

edited

Loading

gitferry left a comment

gitferry Jan 23, 2025

wnjoon Jan 24, 2025

gitferry Jan 23, 2025

wnjoon Jan 24, 2025

gitferry Jan 23, 2025

wnjoon Jan 24, 2025

gitferry Jan 23, 2025

wnjoon Jan 23, 2025

wnjoon Jan 24, 2025

gitferry Jan 23, 2025

wnjoon Jan 24, 2025

gitferry Jan 23, 2025

wnjoon Jan 24, 2025

gitferry Jan 23, 2025

wnjoon Jan 24, 2025

gitferry Jan 23, 2025

wnjoon Jan 24, 2025

gitferry Jan 23, 2025

KonradStaniec Jan 23, 2025

wnjoon Jan 24, 2025 •

edited

Loading

gitferry Jan 23, 2025

wnjoon Jan 24, 2025

gitferry commented Jan 23, 2025 •

edited

Loading

KonradStaniec left a comment

KonradStaniec Jan 23, 2025

wnjoon commented Jan 24, 2025 •

edited

Loading

	cmd.Flags().String(flagBlsPassword, "", "The password for the BLS key. If a flag is set, the non-empty password should be provided. If a flag is not set, the password will be read from the prompt.")
	cmd.Flags().String(flagBlsPassword, "", "The password for the BLS key. If the flag is not set, the password will be read from the prompt.")

	return fmt.Errorf("priv_validator_key.json of previous version not found")
	return fmt.Errorf("priv_validator_key.json of previous version not found in %s", filepath)

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

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

Conversation

wnjoon commented Jan 22, 2025 • edited Loading

gitferry 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

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 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry commented Jan 23, 2025 • edited Loading

KonradStaniec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wnjoon commented Jan 24, 2025 • edited Loading

wnjoon commented Jan 22, 2025 •

edited

Loading

wnjoon Jan 24, 2025 •

edited

Loading

gitferry commented Jan 23, 2025 •

edited

Loading

wnjoon commented Jan 24, 2025 •

edited

Loading