[CLI][Localnet][Bug] - [localnet_client_debug] Error 137 - Issue #607 #608

deblasis · 2023-03-23T11:57:11Z

Description

This PR improves the keybase initialization by:

doing the work only if the private_keys.yaml has been changed
creating a backup of the keybase
loading it (which is way cheaper than parsing the yaml and rehydrating the keybase from scratch)

Before

At 1:46 I run make localnet_client_debug and you can see it takes a long time plus it's a very expensive operation

localnet_cpu_main.mp4

After

At 1:12 I run make localnet_client_debug and it's basically instantaneous with no further spikes in CPU/RAM usage

localnet_cpu_memoized.mp4

Where is the magic?
We do the expensive operation only when needed and that means when the private_keys.yaml has changed.

This is handled by a small binary that calculates the hash of the yaml file, compares it with the one we have already (if any) and if needed, it recreates a backup of the keybase that can be rehydrated from the debug client.

Sloppy recording but it shows the whole process:

localnet_cpu_memoization.mp4

Issue

Fixes #607

Type of change

Please mark the relevant option(s):

List of changes

Updated debug keybase initialization
Created small binary to help with the integrity check

Testing

make develop_test
LocalNet w/ all of the steps outlined in the README

Required Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
I have tested my changes using the available tooling
I have updated the corresponding CHANGELOG

If Applicable Checklist

I have updated the corresponding README(s); local and/or global
I have added tests that prove my fix is effective or that my feature works
I have added, or updated, mermaid.js diagrams in the corresponding README(s)
I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

github-actions

The changelog validation failed with the following output:

"Missing changelog in module: app/

Missing changelog in module: build/

Changelog verification failed. See error messages for more detail.
"

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.

github-actions

The changelog validation failed with the following output:

"Missing changelog in module: app/

Missing changelog in module: build/

Changelog verification failed. See error messages for more detail.
"

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.

deblasis · 2023-03-23T12:35:44Z

@0xBigBoss and @h5law , I am not tagging you guys as reviewers because I don't want to assume that you want to spend time on this at all but if you are curious, I have made some changes to improve the keybase initialization process and I would love your feedback.

h5law · 2023-03-23T12:41:48Z

app/client/keybase/debug/keystore.go

-	// Add the keys if the keybase contains less than 999
-	curAddr, _, err := kb.GetAll()
-	if err != nil {
+	if err := restoreBadgerDB(build.DebugKeybaseBackup, db); err != nil {


This logic is much cleaner 10/10 🚀

build/debug_keybase/main.go

h5law · 2023-03-23T12:46:58Z

build/debug_keybase/main.go

+	hashString := fmt.Sprintf("%x.md5", sourceYamlHash)
+	hashFilePath := filepath.Join(targetFolderPath, hashString)
+	targetFilePath := filepath.Join(targetFolderPath, "debug_keybase.bak")
+	_, err = os.Stat(hashFilePath)


Not completely necissary but we have the function FileExists in shared/utils/file_utils.go

// Check file at the given path exists func FileExists(path string) (bool, error) { _, err := os.Stat(path) if err == nil { return true, nil } if errors.Is(err, fs.ErrNotExist) { return false, nil } return false, err }

If you don't decide to change to use this see the comment below

This is great! Thanks for pointing that out:

Much better

🙏

build/debug_keybase/main.go

h5law

@deblasis This is great. I was concerned about the time it took to load the keys even with the use of WriteBatch this is such a great improvement. I left two minor comments but overall everything looks great as usual with your work!

PS. Love the emojis in the print lines 👀

Co-authored-by: harry <[email protected]>

github-actions

The changelog validation failed with the following output:

"Missing changelog in module: app/

Missing changelog in module: build/

Changelog verification failed. See error messages for more detail.
"

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.

github-actions

The changelog validation failed with the following output:

"Missing changelog in module: app/

Missing changelog in module: build/

Changelog verification failed. See error messages for more detail.
"

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.

deblasis · 2023-03-23T14:30:51Z

@h5law

@deblasis This is great. I was concerned about the time it took to load the keys even with the use of WriteBatch this is such a great improvement. I left two minor comments but overall everything looks great as usual with your work!

PS. Love the emojis in the print lines 👀

You are too kind, sir, thank you very much. I have applied your suggestions 🙏

okdas · 2023-03-23T17:31:15Z

Oh, that long re-hydration is annoying, I agree. In previous pr @0xBigBoss suggested to reuse the volume with already imported keys which would require to import keys only once. Still, your change makes it even better!

How do you feel about committing a binary file into the repo? I usually try to avoid this but in that case I can't think of a better way to handle this and avoid storing a binary in git.

The check succeeded, dismissing the review comment.

deblasis · 2023-03-23T22:28:05Z

@okdas

Oh, that long re-hydration is annoying, I agree. In #591 (comment) @0xBigBoss suggested to reuse the volume with already imported keys which would require to import keys only once. Still, your change makes it even better!

Mounting a volume also works I guess. Code can always be changed/improved. I needed a solution fast and this was the fastest thing I could come up with.

How do you feel about committing a binary file into the repo? I usually try to avoid this but in that case I can't think of a better way to handle this and avoid storing a binary in git.

Fair point, I thought about it, but personally, I think it's a trade-off worth making considering the improved DevX.

Of course, I might be wrong.

0xBigBoss · 2023-03-24T01:44:44Z

@deblasis looks great! I agree with @okdas that adding a binary to the repo "feels" like the wrong approach, but for a speed up this fast for only half a mb, i'm in.

Olshansk

🙌

app/client/keybase/debug/keystore.go

build/debug_keybase/main.go

github-actions

The changelog validation failed with the following output:

"Latest date in app/docs/CHANGELOG.md is incorrect.
Latest: 2023-03-23, Current: 2023-03-24

Latest date in build/docs/CHANGELOG.md is incorrect.
Latest: 2023-03-23, Current: 2023-03-24

Latest date in libp2p/docs/CHANGELOG.md is incorrect.
Latest: 2023-03-23, Current: 2023-03-24

Changelog verification failed. See error messages for more detail.
"

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.

github-actions

The changelog validation failed with the following output:

"Latest date in libp2p/docs/CHANGELOG.md is incorrect.
Latest: 2023-03-23, Current: 2023-03-24

Changelog verification failed. See error messages for more detail.
"

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.

…eybase_init

The check succeeded, dismissing the review comment.

deblasis · 2023-03-24T11:25:01Z

@Olshansk I didn't merge it because of this #608 (comment)

Please let me know your thoughts about that.

Olshansk · 2023-03-24T19:11:59Z

@deblasis

…p-modules * pokt/main: [Utility][Persistence][Savepoints/Rollbacks] Refactor UtilityContext into UtilityUnitOfWork (Issue #563) (#577) [Consensus] Add Quorum Certificate to the block (#593) docs(wiki): ValueError: Duplicate wiki path: devlog/template (#620) Remove unused import [Infra][P2P] Bug: "address not found in peerstore" error in localnet_debug_client when scaling up Localnet - Issue #604 (#611) [CLI][Localnet][Bug] - [localnet_client_debug] Error 137 - Issue #607 (#608) [Documentation] Fix links to RPC module docs in README (#603)

deblasis added 5 commits March 23, 2023 10:42

feat(build): debug_keybase snapshotter

eb9cb58

fix(build): updates in private_keys.yaml handling

0cf1c47

refactor(cli): updated to use the DebugKeybaseBackup

868984b

feat(Makefile): added refresh_debug_keybase make target

ce156d7

feat(build): keybase backup artifacts

15d607b

deblasis added bug Something isn't working - expected behaviour is incorrect infra Core infrastructure - not protocol related client work needed to interface with the node (rpc, cli, etc..) labels Mar 23, 2023

deblasis added this to the M1: Pocket PoS (Proof of Stake) milestone Mar 23, 2023

deblasis self-assigned this Mar 23, 2023

github-actions bot previously requested changes Mar 23, 2023

View reviewed changes

chore(cli): nits

a7ea14b

github-actions bot previously requested changes Mar 23, 2023

View reviewed changes

deblasis marked this pull request as ready for review March 23, 2023 12:33

deblasis requested a review from Olshansk March 23, 2023 12:33

h5law reviewed Mar 23, 2023

View reviewed changes

build/debug_keybase/main.go Outdated Show resolved Hide resolved

h5law reviewed Mar 23, 2023

View reviewed changes

build/debug_keybase/main.go Outdated Show resolved Hide resolved

h5law approved these changes Mar 23, 2023

View reviewed changes

Update build/debug_keybase/main.go

d6baa4d

Co-authored-by: harry <[email protected]>

github-actions bot previously requested changes Mar 23, 2023

View reviewed changes

refactor(build): using utils.FileExists

1c696ae

github-actions bot previously requested changes Mar 23, 2023

View reviewed changes

deblasis added 3 commits March 23, 2023 22:12

chore(build): nits

c741167

docs(app): CHANGELOG

19aeb26

docs(build): CHANGELOG

fdc299e

chore(build): lint

1dbc665

Olshansk approved these changes Mar 24, 2023

View reviewed changes

app/client/keybase/debug/keystore.go Outdated Show resolved Hide resolved

build/debug_keybase/main.go Show resolved Hide resolved

build/debug_keybase/main.go Show resolved Hide resolved

build/debug_keybase/main.go Show resolved Hide resolved

deblasis added 2 commits March 24, 2023 09:38

chore(app): nits

32c957b

chore(build): nits

15daa63

github-actions bot previously requested changes Mar 24, 2023

View reviewed changes

deblasis added 2 commits March 24, 2023 11:00

docs(app): CHANGELOG

aee322b

docs(build): CHANGELOG

917d7ae

github-actions bot previously requested changes Mar 24, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into issue/memoize_debug_k…

0d9f5c2

…eybase_init

fix(build): typo

d2e951e

deblasis merged commit b12811d into main Mar 24, 2023

Olshansk deleted the issue/memoize_debug_keybase_init branch March 24, 2023 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLI][Localnet][Bug] - [localnet_client_debug] Error 137 - Issue #607 #608

[CLI][Localnet][Bug] - [localnet_client_debug] Error 137 - Issue #607 #608

deblasis commented Mar 23, 2023 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

deblasis commented Mar 23, 2023 •

edited

Loading

h5law Mar 23, 2023

h5law Mar 23, 2023

deblasis Mar 23, 2023

h5law left a comment

github-actions bot left a comment

github-actions bot left a comment

deblasis commented Mar 23, 2023

okdas commented Mar 23, 2023

deblasis commented Mar 23, 2023 •

edited

Loading

0xBigBoss commented Mar 24, 2023

Olshansk left a comment

github-actions bot left a comment

github-actions bot left a comment

deblasis commented Mar 24, 2023

Olshansk commented Mar 24, 2023

[CLI][Localnet][Bug] - [localnet_client_debug] Error 137 - Issue #607 #608

[CLI][Localnet][Bug] - [localnet_client_debug] Error 137 - Issue #607 #608

Conversation

deblasis commented Mar 23, 2023 • edited Loading

Description

Before

After

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

deblasis commented Mar 23, 2023 • edited Loading

h5law Mar 23, 2023

Choose a reason for hiding this comment

h5law Mar 23, 2023

Choose a reason for hiding this comment

deblasis Mar 23, 2023

Choose a reason for hiding this comment

h5law left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

deblasis commented Mar 23, 2023

okdas commented Mar 23, 2023

deblasis commented Mar 23, 2023 • edited Loading

0xBigBoss commented Mar 24, 2023

Olshansk left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

deblasis commented Mar 24, 2023

Olshansk commented Mar 24, 2023

deblasis commented Mar 23, 2023 •

edited

Loading

deblasis commented Mar 23, 2023 •

edited

Loading

deblasis commented Mar 23, 2023 •

edited

Loading