lite: Add synchronization in lite verify by HaoyangLiu · Pull Request #2396 · tendermint/tendermint

HaoyangLiu · 2018-09-14T04:00:11Z

Implement the idea in #2386

Add synchronization in lite verify
Change Certify to Verify

…all Certify to Verify

cwgoes

One question but code changes look OK to me otherwise. Can we add a few testcases to ensure that this works correctly?

cwgoes · 2018-09-17T04:17:27Z

lite/dynamic_verifier.go

+	//Get the exact trusted commit for h, and if it is
+	// equal to shdr, then don't even verify it,
+	// and just return nil.
+	trustedFCSameHeight, err := ic.trusted.LatestFullCommit(ic.chainID, shdr.Height, shdr.Height)


Is this stored in RAM or on disk?

Both of them will store the commits. Here we create disk db and memory db to cache the verified commits:

tendermint/lite/proxy/verifier.go

Line 16 in d419fff

memProvider := lite.NewDBProvider("trusted.mem", dbm.NewMemDB()).SetLimit(10)

Actually, I have changed the test to cover the new code. Next I will create a new test to simulate concurrent requests.

On-disk we can store all, in-memory should be an LRU cache with a configurable size.

jaekwon · 2018-09-18T06:54:50Z

Very cool. Has this been tested HaoyangLiu? What are the next steps?

HaoyangLiu · 2018-09-21T02:18:11Z

@jaekwon
Sorry for delayed replay. Recently I was so busy with IRISNET develop. I have add a small test the cover the new code. Next I will add more test and implement an LRU cache to improve performance.

HaoyangLiu · 2018-09-23T11:37:52Z

@cwgoes
Currently, two trust store will be created for tendermint lite: memory cache store and db store. The memory cache maximum size is 10. Mayge we should parameterize the size. The trust memory cache will only keep 10 of the highest trust commit, please refer to the cache deleting algorithm. Strictly speaking, though it is not the LRU cache algorithm, I think it is also reasonable and in many cases it is equivalent to LRU algorithm. For instances, in most user cases, user will query the latest blockchain state, such as account information. Then latest trust commits will be cached by memory cache store. The LRU cache algorithm will do the same operation.
In summary, I think the current memory cache store can meet our requirements. But there is an improvement point: parameterizing the memory cache size.
How do you think about this? @cwgoes

HaoyangLiu · 2018-09-23T14:13:21Z

@cwgoes @jaekwon
I have add a test named TestConcurrencyInquirerVerify to verify this changes.

codecov-io · 2018-09-23T14:20:01Z

Codecov Report

Merging #2396 into develop will decrease coverage by 0.49%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           develop    #2396     +/-   ##
==========================================
- Coverage    61.75%   61.25%   -0.5%     
==========================================
  Files          198      197      -1     
  Lines        16358    16337     -21     
==========================================
- Hits         10102    10008     -94     
- Misses        5434     5463     +29     
- Partials       822      866     +44

Impacted Files	Coverage Δ
cmd/tendermint/commands/lite.go	`15.38% <50%> (+2.22%)`	⬆️
lite/base_verifier.go	`76% <75%> (ø)`	⬆️
lite/dynamic_verifier.go	`62.18% <89.65%> (+5.93%)`	⬆️
libs/common/int.go	`17.85% <0%> (-82.15%)`	⬇️
libs/common/string.go	`61.53% <0%> (-15.13%)`	⬇️
state/store.go	`64.96% <0%> (-5.11%)`	⬇️
p2p/node_info.go	`45.61% <0%> (-5.04%)`	⬇️
libs/autofile/autofile.go	`69.69% <0%> (-4.96%)`	⬇️
state/services.go	`38.46% <0%> (-4.4%)`	⬇️
p2p/test_util.go	`56.47% <0%> (-4.14%)`	⬇️
... and 44 more

cwgoes

LGTM - needs a review from @jaekwon

cwgoes · 2018-09-26T11:53:49Z

Currently, two trust store will be created for tendermint lite: memory cache store and db store. The memory cache maximum size is 10. Mayge we should parameterize the size.

This sounds OK, parameterize cache size is definitely a good idea. Probably enough for now.

xla

👍 🍡

Some minor formatting changes, otherwise LGTM.

xla · 2018-09-26T11:59:11Z

lite/dynamic_verifier.go

 	lerr "github.com/tendermint/tendermint/lite/errors"
 	"github.com/tendermint/tendermint/types"
+	"sync"
+	"fmt"


Import order is incorrect it should be:

import ( // stdlib packages // third-party packages // repo package )

xla · 2018-09-26T11:59:54Z

lite/dynamic_verifier_test.go


 	dbm "github.com/tendermint/tendermint/libs/db"
 	log "github.com/tendermint/tendermint/libs/log"
+	"sync"


Same as above.

xla · 2018-09-26T12:01:47Z

lite/proxy/verifier.go

 )

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


This signature is getting quite large, would benefit from functional options, an example from another part of the codebase:

tendermint/privval/socket.go

Lines 41 to 54 in 0c9c329

// SocketPVOption sets an optional parameter on the SocketPV.

type SocketPVOption func(*SocketPV)

// SocketPVAcceptDeadline sets the deadline for the SocketPV listener.

// A zero time value disables the deadline.

func SocketPVAcceptDeadline(deadline time.Duration) SocketPVOption {

return func(sc *SocketPV) { sc.acceptDeadline = deadline }

}

// SocketPVConnDeadline sets the read and write deadline for connections

// from external signing processes.

func SocketPVConnDeadline(deadline time.Duration) SocketPVOption {

return func(sc *SocketPV) { sc.connDeadline = deadline }

}

@xla
I think function options may be not suitable for NewVerifier. Because NewVerifier use these parameters for processing network operation, not just variable assignment.

HaoyangLiu · 2018-09-26T12:15:21Z

@xla
Thanks for your feedback. I will modify the code as your suggestions.

ebuchman · 2018-09-28T23:20:41Z

lite/dynamic_verifier.go

 	source Provider
+
+	// pending map for synchronize concurrent verification requests
+	pendingVerifications map[int64]chan struct{}


Convention we use is to put the mtx directly above the things it protects.

Implement issues 2386: add synchronization in lite verify and change …

393203f

…all Certify to Verify

HaoyangLiu requested review from ebuchman, melekes and xla as code owners September 14, 2018 04:00

Replace make(chan struct{}, 0) with make(chan struct{})

5f1af39

cwgoes suggested changes Sep 17, 2018

View reviewed changes

cwgoes mentioned this pull request Sep 18, 2018

R4R: Update to Tendermint 0.24 (except NextValSet offsets) cosmos/cosmos-sdk#2219

Merged

5 tasks

jaekwon mentioned this pull request Sep 18, 2018

LCD refactors cosmos/cosmos-sdk#2352

Closed

xla added the C:light Component: Light label Sep 21, 2018

xla changed the title ~~WIP: Add synchronization in lite verify~~ [WIP] Add synchronization in lite verify Sep 21, 2018

Parameterize memroy cache size and add concurrent test

6bb3025

HaoyangLiu changed the title ~~[WIP] Add synchronization in lite verify~~ [R4R] Add synchronization in lite verify Sep 23, 2018

xla requested review from jaekwon and removed request for ebuchman, melekes and xla September 26, 2018 11:47

xla assigned cwgoes Sep 26, 2018

xla added this to the launch milestone Sep 26, 2018

xla changed the title ~~[R4R] Add synchronization in lite verify~~ lite: Add synchronization in lite verify Sep 26, 2018

cwgoes approved these changes Sep 26, 2018

View reviewed changes

xla approved these changes Sep 26, 2018

View reviewed changes

Refactor import order

4da2a04

ebuchman approved these changes Sep 28, 2018

View reviewed changes

ebuchman merged commit 8dda3c3 into tendermint:develop Sep 28, 2018

ebuchman mentioned this pull request Jan 20, 2019

Update verifier to prevent duplicate requests and cache verified headers #2386

Closed

	// SocketPVOption sets an optional parameter on the SocketPV.
	type SocketPVOption func(*SocketPV)

	// SocketPVAcceptDeadline sets the deadline for the SocketPV listener.
	// A zero time value disables the deadline.
	func SocketPVAcceptDeadline(deadline time.Duration) SocketPVOption {
	return func(sc *SocketPV) { sc.acceptDeadline = deadline }
	}

	// SocketPVConnDeadline sets the read and write deadline for connections
	// from external signing processes.
	func SocketPVConnDeadline(deadline time.Duration) SocketPVOption {
	return func(sc *SocketPV) { sc.connDeadline = deadline }
	}

Conversation

HaoyangLiu commented Sep 14, 2018

Uh oh!

cwgoes left a comment

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!

jaekwon commented Sep 18, 2018

Uh oh!

HaoyangLiu commented Sep 21, 2018

Uh oh!

HaoyangLiu commented Sep 23, 2018

Uh oh!

HaoyangLiu commented Sep 23, 2018

Uh oh!

codecov-io commented Sep 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cwgoes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwgoes commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xla left a comment

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!

HaoyangLiu commented Sep 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Sep 23, 2018 •

edited

Loading

cwgoes left a comment •

edited

Loading

cwgoes commented Sep 26, 2018 •

edited

Loading