Keymanager fixes for bad file writes #12284

james-prysm · 2023-04-14T22:58:31Z

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

addresses concerns in #12281 by serializing the process and making sure that local data is not updated until file write is done. if the file is disrupted and cannot finish then the process will roll back.
// 1) Copy the in memory keystore
// 2) Delete the keys from copied in memory keystore
// 3) Save the copy to disk
// 4) Reinitialize account store ~~from disk~~ reinitializing with copy instead
~~// 5) Verify keys are indeed deleted~~ removing this step as the in memory is updated with copy instead of reading from file.
// 6) Return API response

similar steps needed for Import
// 1) Copy the in memory keystore
// 2) Import the keys into copied in memory keystore
// 3) Save the copy to disk
// 4) Reinitialize account store ~~from disk~~ reinitializing with copy instead
~~// 5) Verify keys are indeed Imported~~ removing this step as the in memory is updated with copy instead of reading from file.
// 6) Return Statuses

Which issues(s) does this PR fix?

Fixes #12281

validator/keymanager/local/delete.go

validator/keymanager/local/delete_test.go

prestonvanloon

A few comments on the commentary, but haven't tested this locally yet

validator/keymanager/local/delete.go

validator/keymanager/local/import.go

validator/accounts/wallet_create.go

validator/keymanager/local/delete.go

validator/keymanager/local/import.go

prestonvanloon · 2023-04-16T22:40:08Z

validator/keymanager/local/import.go

 	}
+	//
+	// 6) Return Statuses
 	return statuses, nil
 }

 // ImportKeypairs directly into the keymanager.


I think this could be updated. This routine does more than import directly into km

do you suggest breaking this function up?

No, I think it's already split up well. I think the comment could mention how this first attempts to save to disk then updates the keymanager rather than "imports directly into keymanager"

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

… instead update the information based on memory on hand

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

nisdas · 2023-04-17T09:25:12Z

validator/keymanager/local/keymanager.go

-			km.accountsStore.PublicKeys = append(km.accountsStore.PublicKeys, pk)
-			km.accountsStore.PrivateKeys = append(km.accountsStore.PrivateKeys, sk)
-		}
+		updateAccountsStoreInMemory(km.accountsStore, privateKeys, publicKeys)


You don't set km.accountsStore after executing this function call. Its better to be explicit of what struct fields are being updated in this method.

updated the name to better represent this.

nisdas · 2023-04-17T09:28:37Z

validator/keymanager/local/keymanager.go

@@ -256,17 +264,70 @@ func (km *Keymanager) initializeAccountKeystore(ctx context.Context) error {
 }

 // CreateAccountsKeystore creates a new keystore holding the provided keys.
-func (km *Keymanager) CreateAccountsKeystore(
+func (km *Keymanager) CreateAccountsKeystore(ctx context.Context, privateKeys [][]byte, publicKeys [][]byte) (*AccountsKeystoreRepresentation, error) {
+	if err := km.CreateOrUpdateInMemoryAccountsStore(ctx, privateKeys, publicKeys); err != nil {


if this function succeeds but the one following it fails, will that cause issues ?
CreateOrUpdateInMemoryAccountsStore updates it in memory.

Will take a look again this is only used during the wallet creation process with 0 imports perhaps i can make this cleaner

This function is used in the create wallet function, if it works but the store doesn't it should fail the write. it's only used in tests otherwise besides that.

nisdas · 2023-04-17T09:29:37Z

validator/keymanager/local/import.go

-	if err := km.wallet.WriteFileAtPath(ctx, AccountsPath, AccountsKeystoreFileName, encodedAccounts); err != nil {
-		return nil, err
+	// 5) Verify keys are indeed Imported
+	if len(km.accountsStore.PublicKeys) < len(storeCopy.PublicKeys) {


any point of this check now ?

maybe not I can probably remove this... as the in memory already updated

prestonvanloon

Tested this locally and confirmed that the key is not deleted from the in memory keystore when the keystore is not successfully written to disk.

LGTM - but please address other comments

james-prysm and others added 7 commits April 14, 2023 11:57

WIP changes for keymanager

3c84cc8

WIP fix

c36cb62

WIP needs unit tests

071e533

fixing order

95c90f8

adding unit test

9956a19

Merge branch 'develop' into keymanager-fixes

360c021

fixing linter

5f489fe

james-prysm marked this pull request as ready for review April 14, 2023 23:14

james-prysm requested a review from a team as a code owner April 14, 2023 23:14

james-prysm requested review from kasey, rkapka and nisdas April 14, 2023 23:14

james-prysm changed the title ~~Keymanager delete fixes for bad file writes~~ Keymanager fixes for bad file writes Apr 14, 2023

james-prysm marked this pull request as draft April 14, 2023 23:24

james-prysm requested review from rauljordan and prestonvanloon April 14, 2023 23:24

updating unit tests and creating more reusable functions

49dff21

nisdas reviewed Apr 15, 2023

View reviewed changes

validator/keymanager/local/delete.go Outdated Show resolved Hide resolved

making accountStore copy method part of struct

62bc302

james-prysm marked this pull request as ready for review April 15, 2023 03:01

terencechain added the Priority: High High priority item label Apr 15, 2023

rauljordan reviewed Apr 16, 2023

View reviewed changes

validator/keymanager/local/delete.go Outdated Show resolved Hide resolved

validator/keymanager/local/delete_test.go Outdated Show resolved Hide resolved

rauljordan added 2 commits April 15, 2023 21:47

Update validator/keymanager/local/delete_test.go

50e6185

Update validator/keymanager/local/delete.go

eb1d5d2

rauljordan previously approved these changes Apr 16, 2023

View reviewed changes

prestonvanloon requested changes Apr 16, 2023

View reviewed changes

Update validator/keymanager/local/delete.go

f8fedf8

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

james-prysm dismissed rauljordan’s stale review via f8fedf8 April 17, 2023 01:31

james-prysm and others added 2 commits April 16, 2023 20:31

Update validator/keymanager/local/import.go

c901df6

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

Update validator/keymanager/local/delete.go

1fc1f20

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

james-prysm and others added 7 commits April 16, 2023 20:31

Update validator/keymanager/local/delete.go

fd31c6f

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

Update validator/keymanager/local/import.go

4807595

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

Update validator/keymanager/local/import.go

5e52c8c

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

Update validator/keymanager/local/delete.go

fde491c

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

addressing suggestion of not reinitializing from reading the file but…

1197737

… instead update the information based on memory on hand

Update validator/accounts/wallet_create.go

5bafd36

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

Merge branch 'develop' into keymanager-fixes

d3171a0

nisdas reviewed Apr 17, 2023

View reviewed changes

prestonvanloon previously approved these changes Apr 17, 2023

View reviewed changes

adding changes based on suggestions

4eab532

james-prysm dismissed prestonvanloon’s stale review via 4eab532 April 17, 2023 14:43

Merge branch 'develop' into keymanager-fixes

dbce512

prestonvanloon previously approved these changes Apr 17, 2023

View reviewed changes

making logs more consistent

701805f

james-prysm dismissed prestonvanloon’s stale review via 701805f April 17, 2023 15:36

james-prysm and others added 2 commits April 17, 2023 10:48

fixing linting

7cc67f9

Merge branch 'develop' into keymanager-fixes

2169387

rauljordan approved these changes Apr 17, 2023

View reviewed changes

prestonvanloon approved these changes Apr 17, 2023

View reviewed changes

prestonvanloon merged commit 10b438e into develop Apr 17, 2023

delete-merged-branch bot deleted the keymanager-fixes branch April 17, 2023 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keymanager fixes for bad file writes #12284

Keymanager fixes for bad file writes #12284

james-prysm commented Apr 14, 2023 •

edited

Loading

prestonvanloon left a comment

prestonvanloon Apr 16, 2023

james-prysm Apr 17, 2023

prestonvanloon Apr 17, 2023

nisdas Apr 17, 2023

james-prysm Apr 17, 2023

nisdas Apr 17, 2023

james-prysm Apr 17, 2023

james-prysm Apr 17, 2023

nisdas Apr 17, 2023

james-prysm Apr 17, 2023

prestonvanloon left a comment

Keymanager fixes for bad file writes #12284

Keymanager fixes for bad file writes #12284

Conversation

james-prysm commented Apr 14, 2023 • edited Loading

prestonvanloon 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

prestonvanloon left a comment

Choose a reason for hiding this comment

james-prysm commented Apr 14, 2023 •

edited

Loading