[R4R] #1255 make keybase opened with readonly option for query-purpose cli commands #2489

ackratos · 2018-10-13T02:26:46Z

This is a PR for #1255

make keybase opened with readonly option for query-purpose to support better parallelization between gaiacli

This change enabled parallelization between read and write of keys db via gaiacli, so most of operations can be parallelized except multiple add/update keys to keys db at the same time (but sending transactions and serve rest-server can be parallelized!) in trust mode

There are more work to do to supprt non-trust mode, which have to made change to tendermint code. The remainning work mostly reside on make open and close dbprovider in verifier fine grained.

func NewVerifier(chainID, rootDir string, client lclient.SignStatusClient, logger log.Logger) (*lite.DynamicVerifier, error) {

	logger = logger.With("module", "lite/proxy")
	logger.Info("lite/proxy/NewVerifier()...", "chainID", chainID, "rootDir", rootDir, "client", client)

	memProvider := lite.NewDBProvider("trusted.mem", dbm.NewMemDB()).SetLimit(10)
	lvlProvider := lite.NewDBProvider("trusted.lvl", dbm.NewDB("trust-base", dbm.LevelDBBackend, rootDir))

just like golevel db, the cleveldb interface in tendermint should also expose readonly option to up-level caller:

func NewCLevelDB(name string, dir string) (*CLevelDB, error) {
	dbPath := filepath.Join(dir, name+".db")

	opts := levigo.NewOptions()
	opts.SetCache(levigo.NewLRUCache(1 << 30))
	opts.SetCreateIfMissing(true)
	db, err := levigo.Open(dbPath, opts)
	if err != nil {
		return nil, err
	}
	ro := levigo.NewReadOptions()
	wo := levigo.NewWriteOptions()
	woSync := levigo.NewWriteOptions()
	woSync.SetSync(true)
	database := &CLevelDB{
		db:     db,
		ro:     ro,
		wo:     wo,
		woSync: woSync,
	}
	return database, nil
}

This was manually tested:

start a single-node chain by gaiad start
start a rest-server by gaiacli rest-server --chain-id test-chain-2UzoT6 --insecure true --trust-node
query account balance while keep the rest-server running: gaiacli query account cosmos1uw7nvnfccr3fnumtjs82qvvn3z8uzw7fdznn3t --chain-id test-chain-2UzoT6

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests - investigating the failed test case
Updated relevant documentation (docs/) - no need update 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)

melekes · 2018-10-17T11:46:20Z

client/keys/utils.go

@@ -83,9 +84,9 @@ func ReadPassphraseFromStdin(name string) (string, error) {
 }

 // initialize a keybase based on the configuration
-func GetKeyBaseFromDir(rootDir string) (keys.Keybase, error) {
+func GetKeyBaseFromDir(rootDir string, readonly bool) (keys.Keybase, error) {


my humble proposal:

set readonly to true by default to increase security. and make a separate method GetKeyBaseFromDirWithWritePerm(rootDir string) and call it where needed.

I like this proposal. Do you mind modifying this @ackratos ?

will do that thanks @melekes , glad to see you here:)

fixed @melekes @jackzampolin

codecov · 2018-10-18T14:47:53Z

Codecov Report

Merging #2489 into develop will not change coverage.
The diff coverage is 50%.

@@           Coverage Diff            @@
##           develop    #2489   +/-   ##
========================================
  Coverage    60.28%   60.28%           
========================================
  Files          150      150           
  Lines         8589     8589           
========================================
  Hits          5178     5178           
  Misses        3067     3067           
  Partials       344      344

ackratos · 2018-10-18T15:01:48Z

I found my failed test Test_Bonding is flaky:

This PR #2474 failed it too:
https://circleci.com/gh/cosmos/cosmos-sdk/37073?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Is this a known one or is there anyone working on fixing it?

jackzampolin · 2018-10-18T17:22:23Z

@ackratos I think we fixed this in the latest develop. Please try rebasing.

ackratos · 2018-10-19T02:09:53Z

@ackratos I think we fixed this in the latest develop. Please try rebasing.

COOL, rebased.

jackzampolin · 2018-10-19T16:38:22Z

@ackratos Just a couple of linting errors now:

gometalinter --config=tools/gometalinter.json ./...
client/keys/utils.go:86:1:warning: exported function GetKeyBaseWithWritePerm should have comment or be unexported (golint)
client/keys/utils.go:91:1:warning: exported function GetKeyBaseFromDirWithWritePerm should have comment or be unexported (golint)
Makefile:193: recipe for target 'test_lint' failed
make: *** [test_lint] Error 1
Exited with code 2

ackratos · 2018-10-20T00:32:26Z

@ackratos Just a couple of linting errors now:

gometalinter --config=tools/gometalinter.json ./...
client/keys/utils.go:86:1:warning: exported function GetKeyBaseWithWritePerm should have comment or be unexported (golint)
client/keys/utils.go:91:1:warning: exported function GetKeyBaseFromDirWithWritePerm should have comment or be unexported (golint)
Makefile:193: recipe for target 'test_lint' failed
make: *** [test_lint] Error 1
Exited with code 2

fixed, sorry didn't check that:(

jackzampolin · 2018-10-20T00:46:16Z

Looks like dep is missing a dependancy. Can you run dep ensure from the project root commit any changes and push that to this branch?

…r parallelization between gaiacli

alessio · 2018-10-22T17:38:29Z

Hey, I'm the author of #2544. I'd be happy to drop my PR in favor of this 👍

alessio

I'd add a CloseDB() function to the KeyBase interface to allow the caller to explicitly release the lock on the db. This is a not blocking request from me though, I'm happy to merge this.

jackzampolin · 2018-10-22T17:42:11Z

@alessio Lets merge this and add the CloseDB() in a separate PR. Cool?

alessio · 2018-10-22T17:42:44Z

Oh and I'd like to see some tests too! :)

alessio · 2018-10-22T21:31:48Z

@ackratos I've made some changes to your PR: #2555

ackratos · 2018-10-23T02:36:25Z

@ackratos I've made some changes to your PR: #2555

Thanks, approved that. I think I can close my PR or you want get mine merged first? Is there anything I can do to get this one merged?

alessio · 2018-10-23T02:42:48Z

Yes, go ahead please. Thanks for contribution, and keep up the great work!

…

On Mon, Oct 22, 2018 at 7:36 PM, cong ***@***.***> wrote: @ackratos <https://github.com/ackratos> I've made some changes to your PR: #2555 <#2555> Thanks, approved that. I think I can close my PR right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2489 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN_7A0z2sJOH-o53bkP3czjXPoLC-zsks5unoCrgaJpZM4XaaDF> .

-- Alessio Treglia | [email protected] 0416 0004 A827 6E40 BB98 90FB E8A4 8AE5 311D 765A

ackratos · 2018-10-23T11:09:09Z

Yes, go ahead please.

I don't have permission to click the merge button, would you like do that?

alexanderbez · 2018-10-23T13:19:55Z

Thanks @ackratos for the contribution. How does this relate to #2555? Are these duplicates? Can one be closed?

ackratos · 2018-10-24T03:38:57Z

thanks for reviewing & merging this. Do you know when this change will be released? thx

cwgoes · 2018-10-24T15:30:26Z

thanks for reviewing & merging this. Do you know when this change will be released? thx

Very soon! Track progress here: #2588

jackzampolin · 2018-10-24T15:53:24Z

Thanks again for your contribution @ackratos!

ackratos requested review from cwgoes, ebuchman and rigelrozanski as code owners October 13, 2018 02:26

ackratos changed the title ~~#1255 make keybase opened with readonly option to support better para…~~ [WIP] #1255 make keybase opened with readonly option to support better para… Oct 13, 2018

ackratos changed the title ~~[WIP] #1255 make keybase opened with readonly option to support better para…~~ [WIP] #1255 make keybase opened with readonly option for query-purpose cli commands Oct 13, 2018

ackratos force-pushed the 1255-keybase-readonly branch from de3e740 to 9f606d9 Compare October 13, 2018 04:06

ackratos changed the title ~~[WIP] #1255 make keybase opened with readonly option for query-purpose cli commands~~ [R4R] #1255 make keybase opened with readonly option for query-purpose cli commands Oct 13, 2018

ackratos mentioned this pull request Oct 13, 2018

ERROR: resource temporarily unavailable for all keys operations using CLI after REST server is started #1255

Closed

ackratos changed the title ~~[R4R] #1255 make keybase opened with readonly option for query-purpose cli commands~~ [WIP] #1255 make keybase opened with readonly option for query-purpose cli commands Oct 14, 2018

ackratos force-pushed the 1255-keybase-readonly branch from 9f606d9 to b227b0e Compare October 16, 2018 03:18

ackratos changed the title ~~[WIP] #1255 make keybase opened with readonly option for query-purpose cli commands~~ [R4R] #1255 make keybase opened with readonly option for query-purpose cli commands Oct 16, 2018

melekes reviewed Oct 17, 2018

View reviewed changes

ackratos force-pushed the 1255-keybase-readonly branch from b227b0e to 0c070be Compare October 18, 2018 14:47

ackratos force-pushed the 1255-keybase-readonly branch from 0c070be to 9dd1d44 Compare October 19, 2018 02:09

ackratos force-pushed the 1255-keybase-readonly branch 2 times, most recently from aa9d96b to 57de66a Compare October 20, 2018 00:23

cosmos#1255 make keybase opened with readonly option to support bette…

6c623b2

…r parallelization between gaiacli

ackratos force-pushed the 1255-keybase-readonly branch from 57de66a to 6c623b2 Compare October 20, 2018 12:26

alessio mentioned this pull request Oct 22, 2018

R4R: Don't lock keys database on read only ops #2544

Closed

5 tasks

alessio self-requested a review October 22, 2018 17:38

alessio approved these changes Oct 22, 2018

View reviewed changes

cwgoes merged commit 6c623b2 into cosmos:develop Oct 23, 2018

ackratos mentioned this pull request Oct 24, 2018

Investigate ERROR: resource temporarily unavailable bnb-chain/node#206

Closed

ackratos deleted the 1255-keybase-readonly branch April 29, 2019 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R4R] #1255 make keybase opened with readonly option for query-purpose cli commands #2489

[R4R] #1255 make keybase opened with readonly option for query-purpose cli commands #2489

ackratos commented Oct 13, 2018 •

edited

Loading

melekes Oct 17, 2018

jackzampolin Oct 18, 2018

ackratos Oct 18, 2018

ackratos Oct 18, 2018

codecov bot commented Oct 18, 2018 •

edited

Loading

ackratos commented Oct 18, 2018

jackzampolin commented Oct 18, 2018

ackratos commented Oct 19, 2018

jackzampolin commented Oct 19, 2018

ackratos commented Oct 20, 2018 •

edited

Loading

jackzampolin commented Oct 20, 2018

alessio commented Oct 22, 2018

alessio left a comment

jackzampolin commented Oct 22, 2018

alessio commented Oct 22, 2018

alessio commented Oct 22, 2018

ackratos commented Oct 23, 2018 •

edited

Loading

alessio commented Oct 23, 2018 via email

ackratos commented Oct 23, 2018

alexanderbez commented Oct 23, 2018

ackratos commented Oct 24, 2018

cwgoes commented Oct 24, 2018

jackzampolin commented Oct 24, 2018

[R4R] #1255 make keybase opened with readonly option for query-purpose cli commands #2489

[R4R] #1255 make keybase opened with readonly option for query-purpose cli commands #2489

Conversation

ackratos commented Oct 13, 2018 • edited Loading

melekes Oct 17, 2018

Choose a reason for hiding this comment

jackzampolin Oct 18, 2018

Choose a reason for hiding this comment

ackratos Oct 18, 2018

Choose a reason for hiding this comment

ackratos Oct 18, 2018

Choose a reason for hiding this comment

codecov bot commented Oct 18, 2018 • edited Loading

Codecov Report

ackratos commented Oct 18, 2018

jackzampolin commented Oct 18, 2018

ackratos commented Oct 19, 2018

jackzampolin commented Oct 19, 2018

ackratos commented Oct 20, 2018 • edited Loading

jackzampolin commented Oct 20, 2018

alessio commented Oct 22, 2018

alessio left a comment

Choose a reason for hiding this comment

jackzampolin commented Oct 22, 2018

alessio commented Oct 22, 2018

alessio commented Oct 22, 2018

ackratos commented Oct 23, 2018 • edited Loading

alessio commented Oct 23, 2018 via email

ackratos commented Oct 23, 2018

alexanderbez commented Oct 23, 2018

ackratos commented Oct 24, 2018

cwgoes commented Oct 24, 2018

jackzampolin commented Oct 24, 2018

ackratos commented Oct 13, 2018 •

edited

Loading

codecov bot commented Oct 18, 2018 •

edited

Loading

ackratos commented Oct 20, 2018 •

edited

Loading

ackratos commented Oct 23, 2018 •

edited

Loading