R4R: don't lock keybase on lcd startup #3514

alessio · 2019-02-06T01:48:12Z

Replace all GetKeyBase* functions family in favor of
NewKeyBaseFromDir and NewKeyBaseFromHomeFlag.
Provide a lazy loading implementation of Keybase that locks the underlying
storage only for the time needed to perform the required operation.
Added Keybase reference to TxBuilder struct.
Remove Get prefix from all TxBuilder's getters

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2019-02-06T02:28:10Z

Codecov Report

Merging #3514 into develop will increase coverage by 4.14%.
The diff coverage is 20%.

@@             Coverage Diff             @@
##           develop    #3514      +/-   ##
===========================================
+ Coverage    55.51%   59.66%   +4.14%     
===========================================
  Files          141      131      -10     
  Lines        10445     9681     -764     
===========================================
- Hits          5799     5776      -23     
+ Misses        4306     3565     -741     
  Partials       340      340

jackzampolin · 2019-02-06T04:58:22Z

client/context/context.go

@@ -162,6 +164,12 @@ func GetAccountDecoder(cdc *codec.Codec) auth.AccountDecoder {
 	}
 }

+// WithKeybase returns a copy of the context with an updated keybase.
+func (ctx CLIContext) WithKeybase(kb cryptokeys.Keybase) CLIContext {


jackzampolin · 2019-02-06T05:16:26Z

crypto/keys/lazy_keybase.go

+	dir  string
+}
+
+func NewLazyKeybase(name, dir string) Keybase {


jackzampolin · 2019-02-06T05:23:40Z

x/auth/client/txbuilder/txbuilder.go

-	if err != nil {
-		return
+// MakeSignature builds a StdSignature given keybase, key name, passphrase, and a StdSignMsg.
+func MakeSignature(keybase crkeys.Keybase, name, passphrase string,


++ I like this!! 😎

jackzampolin

Big fan of this!

- GetKeyBase() -> NewKeyBaseFromHomeFlag() - GetKeyBaseFromDir(rootDir string) > NewKeyBaseFromDir(rootDir string)

//signedTx, err := func() (auth.StdTx, error) { // oldClientHome := viper.Get(cli.HomeFlag) // defer func() { viper.Set(cli.HomeFlag, oldClientHome) }() // viper.Set(cli.HomeFlag, clientDir) // return txBldr.SignStdTx(nodeDirName, app.DefaultKeyPass, tx, false) //}()

It's far more idiomatic to not have such prefix.

jleni · 2019-02-06T16:42:57Z

btw, I have already rebased my changes on top of this PR! 👍

jleni · 2019-02-06T16:46:21Z

client/lcd/test_helpers.go

+	// TODO: this should be set in NewRestServer
+	// and pass down the CLIContext to achieve
+	// parallelism.
+	viper.Set(cli.HomeFlag, dir)


Maybe this could return a cleanup function to defer.
Something similar is being done here:

cosmos-sdk/server/test_helpers.go

Lines 43 to 53 in e1f0767

func SetupViper(t *testing.T) func() {

rootDir, err := ioutil.TempDir("", "mock-sdk-cmd")

require.Nil(t, err)

viper.Set(cli.HomeFlag, rootDir)

viper.Set(client.FlagName, "moniker")

return func() {

err := os.RemoveAll(rootDir)

if err != nil {

// TODO: Handle with #870

panic(err)

}

I am adding a temporary directory helper in a future PR.

I don't see this as an urgent change to block the PR

client/context/context.go

alexanderbez

LGTM -- left a few minor remarks

alexanderbez · 2019-02-06T18:26:08Z

client/keys/utils.go

@@ -19,15 +17,12 @@ import (
 // KeyDBName is the directory under root where we store the keys
 const KeyDBName = "keys"

-// keybase is used to make GetKeyBase a singleton
-var keybase keys.Keybase


alexanderbez · 2019-02-06T18:34:45Z

crypto/keys/lazy_keybase.go

+	return lazyKeybase{name: name, dir: dir}
+}
+
+func (lkb lazyKeybase) List() ([]Info, error) {


I know this will be a huge PIA, but can we add godocs for all of these please?

alessio requested review from cwgoes, ebuchman and rigelrozanski as code owners February 6, 2019 01:48

alessio changed the title ~~WIP: don't lock keybase on lcd startup~~ R4R: don't lock keybase on lcd startup Feb 6, 2019

jackzampolin reviewed Feb 6, 2019

View reviewed changes

crypto/keys/lazy_keybase.go

dir string

}

func NewLazyKeybase(name, dir string) Keybase {

Copy link

Member

jackzampolin Feb 6, 2019

Choose a reason for hiding this comment

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

++

jackzampolin reviewed Feb 6, 2019

View reviewed changes

jackzampolin approved these changes Feb 6, 2019

View reviewed changes

Alessio Treglia added 9 commits February 6, 2019 00:26

do not lock keybase unconditionally on rest server bootstrap

3a21126

lazy keybase implementation

a0239ec

Update Gopkg.lock

c81f944

Code cleanup

63a37d7

Renaming

445d736

- GetKeyBase() -> NewKeyBaseFromHomeFlag() - GetKeyBaseFromDir(rootDir string) > NewKeyBaseFromDir(rootDir string)

Fix localnet

2f8ef45

//signedTx, err := func() (auth.StdTx, error) { // oldClientHome := viper.Get(cli.HomeFlag) // defer func() { viper.Set(cli.HomeFlag, oldClientHome) }() // viper.Set(cli.HomeFlag, clientDir) // return txBldr.SignStdTx(nodeDirName, app.DefaultKeyPass, tx, false) //}()

Fix missing doc string

28d631d

Remove Get_ prefix from all TxBuider getters

25a853d

It's far more idiomatic to not have such prefix.

Update PENDING.md

5da49fb

alessio force-pushed the alessio/dont-lock-keybase-on-lcd-startup branch from eedeffe to 5da49fb Compare February 6, 2019 08:41

Dead code cleanup

6bbc8cd

alessio requested review from alexanderbez, fedekunze, jaekwon, jleni and mossid February 6, 2019 14:46

jleni mentioned this pull request Feb 6, 2019

R4R: Increasing test coverage - First Pass #3517

Merged

4 tasks

jleni approved these changes Feb 6, 2019

View reviewed changes

alexanderbez reviewed Feb 6, 2019

View reviewed changes

client/context/context.go Outdated Show resolved Hide resolved

Remove keybase from Context, it inadvertendly sneaked in

3d68604

acquire Keybase from TxBuilder

cc5299b

alexanderbez approved these changes Feb 6, 2019

View reviewed changes

jackzampolin merged commit 9a57ce0 into develop Feb 6, 2019

jackzampolin deleted the alessio/dont-lock-keybase-on-lcd-startup branch February 6, 2019 19:23

This was referenced Feb 6, 2019

WIP: Fix keybase lock issues #2655

Closed

Changing password on LCD fails #2649

Closed

sabau mentioned this pull request Feb 7, 2019

Web deploy prerequisites luniehq/lunie#1851

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: don't lock keybase on lcd startup #3514

R4R: don't lock keybase on lcd startup #3514

alessio commented Feb 6, 2019 •

edited

Loading

codecov bot commented Feb 6, 2019 •

edited

Loading

jackzampolin Feb 6, 2019

jackzampolin Feb 6, 2019

jackzampolin Feb 6, 2019

jackzampolin left a comment

jleni commented Feb 6, 2019

jleni Feb 6, 2019

jleni Feb 6, 2019

alexanderbez left a comment

alexanderbez Feb 6, 2019

alexanderbez Feb 6, 2019

	func SetupViper(t *testing.T) func() {
	rootDir, err := ioutil.TempDir("", "mock-sdk-cmd")
	require.Nil(t, err)
	viper.Set(cli.HomeFlag, rootDir)
	viper.Set(client.FlagName, "moniker")
	return func() {
	err := os.RemoveAll(rootDir)
	if err != nil {
	// TODO: Handle with #870
	panic(err)
	}

R4R: don't lock keybase on lcd startup #3514

R4R: don't lock keybase on lcd startup #3514

Conversation

alessio commented Feb 6, 2019 • edited Loading

codecov bot commented Feb 6, 2019 • edited Loading

Codecov Report

jackzampolin Feb 6, 2019

Choose a reason for hiding this comment

jackzampolin Feb 6, 2019

Choose a reason for hiding this comment

jackzampolin Feb 6, 2019

Choose a reason for hiding this comment

jackzampolin left a comment

Choose a reason for hiding this comment

jleni commented Feb 6, 2019

jleni Feb 6, 2019

Choose a reason for hiding this comment

jleni Feb 6, 2019

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Feb 6, 2019

Choose a reason for hiding this comment

alexanderbez Feb 6, 2019

Choose a reason for hiding this comment

alessio commented Feb 6, 2019 •

edited

Loading

codecov bot commented Feb 6, 2019 •

edited

Loading