Verify all headers in order instead of bisect by liamsi · Pull Request #3360 · tendermint/tendermint

liamsi · 2019-02-28T10:11:25Z

Still need to address the TODOs below but this should be ready for review.

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

- add method that fetches, verifies, and stores all headers in order from the latest trusted up to the requested height

codecov-io · 2019-02-28T10:26:52Z

Codecov Report

Merging #3360 into develop will decrease coverage by 0.26%.
The diff coverage is 46.93%.

@@             Coverage Diff             @@
##           develop    #3360      +/-   ##
===========================================
- Coverage    64.42%   64.15%   -0.27%     
===========================================
  Files          214      214              
  Lines        17514    17552      +38     
===========================================
- Hits         11283    11261      -22     
- Misses        5296     5350      +54     
- Partials       935      941       +6

Impacted Files	Coverage Δ
lite/commit.go	`48.83% <30.76%> (+0.11%)`	⬆️
lite/dynamic_verifier.go	`49.35% <52.77%> (-18.99%)`	⬇️
privval/signer_service_endpoint.go	`83.63% <0%> (-5.46%)`	⬇️
libs/events/events.go	`93.2% <0%> (-4.86%)`	⬇️
consensus/reactor.go	`70.36% <0%> (-2.13%)`	⬇️
consensus/state.go	`78.82% <0%> (-0.24%)`	⬇️
blockchain/reactor.go	`72.46% <0%> (+0.96%)`	⬆️
blockchain/pool.go	`81.69% <0%> (+2.03%)`	⬆️
consensus/ticker.go	`95.83% <0%> (+4.16%)`	⬆️

liamsi · 2019-02-28T10:29:58Z

lite/dynamic_verifier.go

+		// Update height for height.
+		// TODO: do we want the lite client to store the intermediate commits,
+		//  or only the latest/last one?
+		if err := dv.verifyAndSave(trustedFC, nextFC); err != nil {


I'm not sure about this but there are actually no reasons to save the intermediate full-commits here, right?
@ebuchman @melekes @cwgoes

How do we benefit from storing them? maybe only that if something fails, we'll start from the last saved commit, no?

I think the only benefit would be that if the application constantly queries state of the past, we have more trusted (verified) full commits / signed headers stored locally which can be used as a starting point for this queries instead having to query them from dv.source. My intuition would be that storing the last header / full commit should be sufficient but this intuition is not based on any numbers/data.

If we want to query past state, we need past Merkle roots (app hashes). I don't think we need the full commits though.

Thanks Chris. Probably just storing the latest full commit should be enough. The full commit contains the AppHashes via the Header:

tendermint/lite/commit.go

Lines 16 to 20 in 15f6211

type FullCommit struct {

SignedHeader types.SignedHeader `json:"signed_header"`

Validators *types.ValidatorSet `json:"validator_set"`

NextValidators *types.ValidatorSet `json:"next_validator_set"`

}

tendermint/types/block.go

Lines 689 to 690 in 15f6211

type SignedHeader struct {

*Header `json:"header"`

tendermint/types/block.go

Line 376 in 15f6211

AppHash cmn.HexBytes `json:"app_hash"` // state after txs from the previous block

We could probably get rid of some (most?) of the fields here for the lite client.

For now, storing only the latest FullCommit sounds like the most reasonable thing to do.

liamsi · 2019-02-28T14:15:33Z

Although this is still WIP but I appreciate a first quick early review as I'm new to the lite package.
cc @cwgoes @ebuchman @melekes

lite/dynamic_verifier_test.go

-	// Souce doesn't have fcz[9] so cert.LastTrustedHeight wont' change.
+	// We do not jump forward using trusted full commits.
+	// Instead, we need to store the "gap" (height 7) in source s.t.
+	// Verfiy can retrieve it from somewhere.


- only verify that valset for height SignedHeader.Height+1 is non-empty and matches the hash in the signed header (NextValidatorsHash) - all other checks were already done by the BaseVerifier - at least log mysteriously ignored error (for now)

melekes · 2019-03-06T08:33:19Z

lite/dynamic_verifier.go

 // for height h, using repeated applications of bisection if necessary.
 //
 // Returns ErrCommitNotFound if source provider doesn't have the commit for h.
+// TODO: bisection is disabled for now.


can you link the issue here too

lite/dynamic_verifier.go

+		return FullCommit{}, err
+	}
+	// We already were at the requested height.
+	if trustedFC.Height() == h {


melekes · 2019-03-06T08:37:51Z

lite/dynamic_verifier.go

+		// Update height for height.
+		// TODO: do we want the lite client to store the intermediate commits,
+		//  or only the latest/last one?
+		if err := dv.verifyAndSave(trustedFC, nextFC); err != nil {


How do we benefit from storing them? maybe only that if something fails, we'll start from the last saved commit, no?

- add sanity check for returned height and panic if violated - add issue to comment about bisecting

jaekwon · 2019-03-24T02:18:13Z

#3259 (comment)

Instead of merging this in, I'll incorporate some fixes into a new branch I'm working on for weak subjectivity.

…rovided by a seed node (backport tendermint#3360) (tendermint#3396) Closes tendermint#486. Commits: 1. Make `TestConnectionSpeedForPeerReceivedFromSeed` to fail, as it will make the node to connect to more than the configured `MaxNumOutboundPeers` 2. Solve the issue by introducing a channel to wake up `ensurePeersRoutine()`. In this way, the node will not to wait until the next `defaultEnsurePeersPeriod` (30s) before attempting again to connect to new peers (`ensurePeers()` method) whose addresses were provide by the seed node. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request tendermint#3360 done by [Mergify](https://mergify.com). --------- Co-authored-by: Daniel <daniel.cason@informal.systems>

…rovided by a seed node (backport tendermint#3360) (tendermint#3397) Closes tendermint#486. Commits: 1. Make `TestConnectionSpeedForPeerReceivedFromSeed` to fail, as it will make the node to connect to more than the configured `MaxNumOutboundPeers` 2. Solve the issue by introducing a channel to wake up `ensurePeersRoutine()`. In this way, the node will not to wait until the next `defaultEnsurePeersPeriod` (30s) before attempting again to connect to new peers (`ensurePeers()` method) whose addresses were provide by the seed node. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request tendermint#3360 done by [Mergify](https://mergify.com). --------- Co-authored-by: Daniel <daniel.cason@informal.systems>

Verify and store all headers in order

91c682c

- add method that fetches, verifies, and stores all headers in order from the latest trusted up to the requested height

liamsi requested review from ebuchman and melekes as code owners February 28, 2019 10:11

do not lint updateToHeight for unused code

b3b1567

liamsi commented Feb 28, 2019

View reviewed changes

liamsi requested a review from cwgoes February 28, 2019 13:27

rename method & add comment

b9e3904

ebuchman reviewed Feb 28, 2019

View reviewed changes

liamsi added 3 commits March 1, 2019 12:45

fix typo

bac714e

move nextValset check into FullCommit as it used there anyways

7554163

melekes reviewed Mar 6, 2019

View reviewed changes

liamsi added 5 commits March 11, 2019 21:50

Do not store intermediate full commits when verifying each header / FC.

83c2b33

review comments: sanity check and comment

d43b306

- add sanity check for returned height and panic if violated - add issue to comment about bisecting

fix nolint:unused statement

c77c4f9

Merge in latest develop and fix test

13e1428

Fix another test (assumed we store all verified headers in trusted)

2f80413

liamsi changed the title ~~WIP: Verify all headers in order instead of bisect~~ Verify all headers in order instead of bisect Mar 20, 2019

jaekwon closed this Mar 24, 2019

xla deleted the ismail/lite_dont_bisect branch March 28, 2019 08:15

	type FullCommit struct {
	SignedHeader types.SignedHeader `json:"signed_header"`
	Validators *types.ValidatorSet `json:"validator_set"`
	NextValidators *types.ValidatorSet `json:"next_validator_set"`
	}

Conversation

liamsi commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

liamsi Feb 28, 2019 • 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!

melekes Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

liamsi Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

cwgoes Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

liamsi Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

liamsi commented Feb 28, 2019

Uh oh!

This comment was marked as resolved.

Uh oh!

melekes Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

melekes Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

jaekwon commented Mar 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

liamsi commented Feb 28, 2019 •

edited

Loading

codecov-io commented Feb 28, 2019 •

edited

Loading

liamsi Feb 28, 2019 •

edited

Loading