Changed to allow offline splitting of validator keys with different passwords by gnongs · Pull Request #69 · ssvlabs/ssv-keys

gnongs · 2024-11-28T17:27:52Z

Fixes #<issue_number_goes_here>

It's a good idea to open an issue first for discussion.

Tests pass
Appropriate changes to README are included in PR

…e path

gnongs · 2024-11-29T04:36:11Z

@RaekwonIII @IlyaVi Could you please review this PR?

src/commands/actions/KeySharesAction.ts

IlyaVi · 2024-12-02T16:14:07Z

src/commands/actions/KeySharesAction.ts

+        }
+      }
+      else {
+        keystorePassword = await fsp.readFile(this.args.password, 'utf-8');


move this line outside of for loop in line 75

@IlyaVi If this line is moved outside the for loop at line 75, there will be an issue with setting the value of keystoreSet.

@gnongs let me rephrase my comment
what I meant is to have this in line 75
keystorePassword = this.args.password;
this will preserve the same approach as before your change and won't change anything for existing users in the way the use ssv-keys

@IlyaVi
Now I understand your intention.

What you want is for it to behave the same as before when using the same password.

Updated.

IlyaVi · 2024-12-02T16:14:49Z

src/commands/actions/KeySharesAction.ts

+
+    const validatedFiles = await this.validateKeystoreFiles(keystoreSet);
+
+    process.stdout.write(validatedFiles[0].keystoreFile);


debug print?

IlyaVi · 2024-12-02T16:15:45Z

README.md

-yarn cli shares --keystore=./keystore-files --password=test --operator-ids=1,2,3,4 --operator-keys=LS..,LS..,LS..,LS.. --output-folder=./ --owner-address=... --owner-nonce=..
+## folder structure
+keystore_folders
+├── 0xe020....


validator public key is redundant as folder name - change to something generic

IlyaVi · 2024-12-02T16:19:57Z

README.md


 # folder with multiple keystore files
-yarn cli shares --keystore=./keystore-files --password=test --operator-ids=1,2,3,4 --operator-keys=LS..,LS..,LS..,LS.. --output-folder=./ --owner-address=... --owner-nonce=..
+## folder structure


Separate this usecase with this sub header
Add comment to underline that --password parameter is actual password file name
"folder structure for keystore files with different password"

Add sub header for one password and multiple files. Command should be the same as before change

…fferent password files more clear

gnongs · 2024-12-04T17:59:16Z

@IlyaVi Could you check this again?

IlyaVi · 2024-12-08T14:51:43Z

src/commands/actions/KeySharesAction.ts

+          if (dirent.name.includes(this.args.password)) {
+            keystorePassword = await fsp.readFile(path.join(file, dirent.name), 'utf-8');
+          } else {
+            keystoreFile = path.join(file, dirent.name);


@gnongs if there are multiple keystore files in same sub directory you will add only last one to the array

@IlyaVi

Having multiple keystore files is not the folder structure I intend.
Would it be better to terminate the program in such cases?

IlyaVi · 2024-12-08T14:55:43Z

src/commands/actions/KeySharesAction.ts

+        }
+      }
+      else {
+        keystorePassword = await fsp.readFile(this.args.password, 'utf-8');


@gnongs let me rephrase my comment
what I meant is to have this in line 75
keystorePassword = this.args.password;
this will preserve the same approach as before your change and won't change anything for existing users in the way the use ssv-keys

IlyaVi · 2024-12-08T15:00:07Z

README.md

 ```bash
 # single file
-yarn cli shares --keystore=keystore.json --password=test --operator-ids=1,2,3,4 --operator-keys=LS..,LS..,LS..,LS.. --output-folder=./ --owner-address=... --owner-nonce=..
+yarn cli shares --keystore=./keystore.json --password=./password.txt --operator-ids=1,2,3,4 --operator-keys=LS..,LS..,LS..,LS.. --output-folder=./ --owner-address=... --owner-nonce=..


please revert this change, user will continue using plane password as parameter

IlyaVi · 2024-12-08T15:00:59Z

README.md

+└── keystore5.json
+password.txt
+
+yarn cli shares --keystore=./keystore_files --password=./password.txt --operator-ids=1,2,3,4 --operator-keys=LS..,LS..,LS..,LS.. --output-folder=./ --owner-address=... --owner-nonce=..


please change to plain password instead file name and remove "password.txt" from folder structure

IlyaVi · 2024-12-08T15:15:40Z

README.md

+    ├── keystore.json
+    └── password.txt
+
+yarn cli shares --keystore=./keystore_folders --password=password.txt --operator-ids=1,2,3,4 --operator-keys=LS..,LS..,LS..,LS.. --output-folder=./ --owner-address=... --owner-nonce=..


instead "password.txt" write here and in the folder structure - the name and extension are not important as long as file can be read as utf-8 encoded text

@IlyaVi

I tried to convey that it can be a simple file without an extension.

gnongs · 2025-01-05T08:34:10Z

Hey, @IlyaVi

The update is quite late… Sorry....

I have made all the updates to the parts you mentioned, and I have some thoughts on certain areas. Please share your feedback on those comments. :)

Need to get a opinion: Changed to allow offline splitting of validator keys with different passwords #69 (comment)

gnongs added 3 commits November 28, 2024 17:08

Changed to allow multiple validator keys with different passwords

6912627

Change example for command execution

b57a7cc

Changed the password for a single keystore file to also receive a fil…

e105570

…e path

gnongs changed the title ~~Enable password file~~ Changed to allow offline splitting of validator keys with different passwords Nov 28, 2024

gnongs added 2 commits November 29, 2024 22:57

Updated keystorePassword initial value

f7d0f26

add support for splitting keystore files with identical passwords

23d7341

IlyaVi requested changes Dec 2, 2024

View reviewed changes

gnongs added 4 commits December 4, 2024 17:22

Remove the logs used to debug the content of the loaded keystore file

0fb3adf

Remove dependency on the filename when locating the keystore file

11f39a4

Change folder/file names to more generic terms

805504d

Make the difference between using the same password file and using di…

a501324

…fferent password files more clear

gnongs requested a review from IlyaVi December 4, 2024 17:59

Remove the section containing incorrect information

96f9b0e

IlyaVi previously approved these changes Dec 8, 2024

View reviewed changes

gnongs added 3 commits January 3, 2025 14:51

Fix build error caused by missing closing curly brace

2a43fac

Change to direct input password value when using the same password

1771821

Update the README.md description related to the password

0ad5718

gnongs requested a review from IlyaVi January 5, 2025 08:34

gnongs dismissed IlyaVi’s stale review via 0ad5718 May 7, 2025 21:31


		const validatedFiles = await this.validateKeystoreFiles(keystoreSet);

		process.stdout.write(validatedFiles[0].keystoreFile);

Comments

Conversation

gnongs commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnongs commented Nov 29, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnongs commented Dec 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnongs commented Jan 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gnongs commented Nov 28, 2024 •

edited

Loading