adr-002 lite client #54

ebuchman · 2019-10-30T22:21:01Z

Lite client ADR. A bit backwards because we've already got WIPs for the implementation, but trying to get this written up before we make much more progress there.

Link for nicer reading: https://github.com/interchainio/tendermint-rs/blob/bucky/adr-lite/docs/architecture/adr-002-lite-client.md

docs/architecture/adr-002-lite-client.md

ValarDragon · 2019-11-01T18:54:47Z

docs/architecture/adr-002-lite-client.md

+#### Commit
+
+A commit is a collection of votes that we will want to iterate over so we can
+verify each vote.


Can we make the API s.t. we can upgrade to aggregate signatures w/o an API change in the lite client?

Essentially make one function that goes from validator index to boolean representing "has signed", and one aggregate signature struct. Verify checks that 2/3rds of stake has signed, and that aggregate signature.verify() verifies.

I thot about this a bit. I feel it might be a bit too early for that. This thing is already pretty abstract and I'm not sure it will pay yet to make it even more so.

Also, come to think of it, will aggregate sigs work with the skipping method? If I have an aggregate signature, can I check if +1/3 of some other validator set (not necessarily the validator set that produced the sig) signed it?

You have a better impression of this than I do, but it feels to me like aggregate signatures shouldn't be too far out? Related projects are already using them (LibraBFT, Nervos (https://github.com/cryptape/overlord/) a TM fork)

To check an aggregate signature, you must have all the signing public keys. Is this 1/3rd check needed for checkSupport? If so, I believe that it is fine, since you are provided the signing public keys in the validator set hash order to verify.

Ok so a number of things have come to my attention that suggest this abstraction isn't good enough:

Doesn't support aggregate sigs

Doesn't support batch verification (whoops!)

Testing is actually not much easier than using the actual Tendermint data types.

So all this leads me to believe that the commit trait should be abstracted further to not expose individual signatures but only information about whether say X% signed. In the interest of making some progress in the short term though I think we'll move forward as is and open an issue to deal with this at a later date.

As for the +1/3 check, we have the following situation:

I trust validator set V

I get a commit C from validator set V', where V != V'

I want to check if +1/3 of V is in V' and signed C

the +1/3 from V may comprise an arbitrary percentage of V' (ie. they could be 1% or 99%, it doesn't matter).

Can this be done for an aggregate sig?

Yes this can be done.
The aggregate signature will contain a list of who signed it, so you have the same situation as the non aggregate case. (The verifier has a set of signers, and has to check if 1/3 of V is in this set)

docs/architecture/adr-002-lite-client.md

liamsi

Wohooo 🎉 Left some comments.

liamsi · 2019-11-04T07:11:57Z

docs/architecture/adr-002-lite-client.md

+
+### Manager
+
+For lack of a better name. The Manager co-ordiantes the syncing and is the


Looking at the diagram, maybe the Manager should simply be named "lite client" or "lite node" as it is the point where everything else is used and put together (to form an actual lite client).
Looking at the subsections, this component seems more about syncing only (?). Then, it could be named Syncer (or Tracker).

Noted here #56

docs/architecture/adr-002-lite-client.md

Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com>

liamsi

👍

docs/architecture/adr-002-lite-client.md

liamsi · 2019-12-15T15:22:55Z

docs/architecture/adr-002-lite-client.md

+  will:
+    - check that vals[H] and vals[H+1] are correctly reflected in the header
+    - check that the commit is for the header
+    - check that +2/3 of the validators correctly signed the header hash


Everywhere else you specify which header we are talking about (by explicitly listing a height H via header[H]). I think it would be good be consistent in this listing, too. (same for validators)

Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com>

ebuchman · 2019-12-27T22:44:35Z

Ok I addressed all the feedback. Also updated it to match the latest version of the code and included some notes about that. Added a proper image too.

ebuchman · 2019-12-27T23:09:11Z

I don't understand why this is failing. The latest master has been merged in and it passes locally ...

ebuchman · 2019-12-28T00:19:42Z

Ok we should update this per #105 ... removed the Validator trait!

ebuchman · 2019-12-28T04:32:09Z

Ok thanks to @jibrown we actually have a really nice diagram of the data flow for the whole thing now! Might need a few updates to the text to align with the new image though.

ebuchman · 2020-01-22T22:44:27Z

Pushed an ADR refactor. ADR002 is now an index into light client ADRs, and the core verification is ADR 003. Still going to do some cleanup/alignment on the new ADR 003 and just leave the others as WIPs to be done in future PRs.

liamsi · 2020-01-22T23:53:30Z

The clippy messages look somewhat familiar. I think they should be fixed in the latest master.

ebuchman · 2020-01-23T17:47:20Z

Traits are good. Just want to go through the "implementing traits" part and then this is good to merge and I will work on the other two (CLI and fork detection)

codecov-io · 2020-02-10T04:24:27Z

Codecov Report

❗ No coverage uploaded for pull request base (master@3018cf0). Click here to learn what that means.
The diff coverage is 54.73%.

@@            Coverage Diff            @@
##             master      #54   +/-   ##
=========================================
  Coverage          ?   36.08%           
=========================================
  Files             ?       90           
  Lines             ?     3140           
  Branches          ?      490           
=========================================
  Hits              ?     1133           
  Misses            ?     1750           
  Partials          ?      257

Impacted Files	Coverage Δ
tendermint/src/amino_types/block_id.rs	`13.79% <ø> (ø)`
tendermint/src/rpc/client.rs	`0% <ø> (ø)`
tendermint/src/amino_types/signature.rs	`35.71% <ø> (ø)`
tendermint/src/amino_types/remote_error.rs	`0% <ø> (ø)`
tendermint/src/amino_types/time.rs	`35.29% <ø> (ø)`
tendermint/src/block.rs	`0% <ø> (ø)`
tendermint/src/amino_types/version.rs	`62.5% <ø> (ø)`
tendermint/src/amino_types/message.rs	`100% <ø> (ø)`
tendermint/src/error.rs	`12.3% <0%> (ø)`
tendermint/src/amino_types.rs	`0% <0%> (ø)`
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3018cf0...ce03b67. Read the comment docs.

ebuchman · 2020-02-10T04:26:47Z

Ok, I think this is finally done. It's been realigned with the latest code and describes only what's already been implemented in the lite and lite_impl. Let's merge.

* adr-002 lite client (#54) * adr-002 lite client * finish adr * address Dev's comments * lite -> light * Apply suggestions from code review Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com> * updates from review * Apply suggestions from code review Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com> * update for better abstraction and latest changes * note about detection * update image. manager -> syncer * update image * update image * More detailed diagram of lite client data flow (#106) * refactor * refactor into more adrs * minor fixes from review * sync adr-003 with latest and call it * rust code blocks Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com> Co-authored-by: jibrown <jackie.ilana.brown@gmail.com> * working on adr-004 Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com> Co-authored-by: jibrown <jackie.ilana.brown@gmail.com>

* Boilerplate: Add lite-node crate - ran `abscissa new lite-node` - added deps (Cargo.toml) and minimal changes to README.md - add to root workspace * Added config options & copied code into new app crate - copied from tendermint-lite/src/main.rs to lite-node/src/command/start.rs * Delete tendermint-lite: replaced by lite-node * lite -> light * minor improvements to comments / docs minor improvements to comments / docs * Fix a few merge hicks (catch up with latest changes from master) rename some vars and more logical bisection in cmd * fix rebasing hicks * Bucky/abscissify adr (#148) * adr-002 lite client (#54) * adr-002 lite client * finish adr * address Dev's comments * lite -> light * Apply suggestions from code review Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com> * updates from review * Apply suggestions from code review Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com> * update for better abstraction and latest changes * note about detection * update image. manager -> syncer * update image * update image * More detailed diagram of lite client data flow (#106) * refactor * refactor into more adrs * minor fixes from review * sync adr-003 with latest and call it * rust code blocks Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com> Co-authored-by: jibrown <jackie.ilana.brown@gmail.com> * working on adr-004 Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com> Co-authored-by: jibrown <jackie.ilana.brown@gmail.com> * Dealing with the merging in master aftermath * merged in master * Fix merge master fallout (related to #169) * Use `abscissa_tokio` and resolve merge conflicts * New stable rust -> new clippy errs -> fixed Co-authored-by: Ethan Buchman <ethan@coinculture.info> Co-authored-by: jibrown <jackie.ilana.brown@gmail.com>

ebuchman added 2 commits October 30, 2019 18:27

adr-002 lite client

3a2b8a9

Merge branch 'master' into bucky/adr-lite

62e3765

ebuchman changed the title ~~WIP: adr-002 lite client~~ adr-002 lite client Nov 1, 2019

finish adr

777d01e

ValarDragon reviewed Nov 1, 2019

View reviewed changes

docs/architecture/adr-002-lite-client.md Outdated Show resolved Hide resolved

ValarDragon reviewed Nov 1, 2019

View reviewed changes

docs/architecture/adr-002-lite-client.md Outdated Show resolved Hide resolved

ValarDragon reviewed Nov 1, 2019

View reviewed changes

docs/architecture/adr-002-lite-client.md Outdated Show resolved Hide resolved

ValarDragon reviewed Nov 1, 2019

View reviewed changes

docs/architecture/adr-002-lite-client.md Outdated Show resolved Hide resolved

ValarDragon reviewed Nov 1, 2019

View reviewed changes

docs/architecture/adr-002-lite-client.md Outdated Show resolved Hide resolved

address Dev's comments

615618f

liamsi reviewed Nov 4, 2019

View reviewed changes

ValarDragon reviewed Nov 5, 2019

View reviewed changes

docs/architecture/adr-002-lite-client.md Outdated Show resolved Hide resolved

lite -> light

9f84f6a

ebuchman mentioned this pull request Nov 6, 2019

Light Client ADR Follow Up #56

Closed

ebuchman and others added 2 commits November 6, 2019 16:12

updates from review

fc71f24

Apply suggestions from code review

79ec990

Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com>

ValarDragon mentioned this pull request Nov 12, 2019

Lite clients checking between 1/3 + 1 and 2/3 +1 sigs tendermint/spec#59

Closed

liamsi previously approved these changes Nov 12, 2019

View reviewed changes

liamsi reviewed Dec 4, 2019

View reviewed changes

docs/architecture/adr-002-lite-client.md Outdated Show resolved Hide resolved

liamsi reviewed Dec 4, 2019

View reviewed changes

docs/architecture/adr-002-lite-client.md Outdated Show resolved Hide resolved

liamsi reviewed Dec 15, 2019

View reviewed changes

Apply suggestions from code review

4d21adf

Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com>

ebuchman dismissed liamsi’s stale review via 4d21adf December 27, 2019 20:35

ebuchman added 4 commits December 27, 2019 17:22

update for better abstraction and latest changes

8ccd617

note about detection

72d2db9

update image. manager -> syncer

77d2651

Merge branch 'master' into bucky/adr-lite

70970bc

update image

56b6080

ebuchman force-pushed the bucky/adr-lite branch from a4d02bc to 4dc0391 Compare December 27, 2019 22:52

update image

4dc0391

More detailed diagram of lite client data flow (#106)

e2cb9ac

ebuchman added 2 commits January 22, 2020 13:48

refactor

ca8690a

refactor into more adrs

e657803

minor fixes from review

d6b68cb

ebuchman added 3 commits January 23, 2020 09:47

Merge branch 'master' into bucky/adr-lite

3018cf0

Merge branch 'master' into bucky/adr-lite

8fbbc88

sync adr-003 with latest and call it

9932b16

rust code blocks

ce03b67

ebuchman merged commit f9287e0 into master Feb 10, 2020

ebuchman deleted the bucky/adr-lite branch February 10, 2020 04:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adr-002 lite client #54

adr-002 lite client #54

ebuchman commented Oct 30, 2019 •

edited

Loading

ValarDragon Nov 1, 2019

ebuchman Nov 1, 2019

ValarDragon Nov 4, 2019 •

edited

Loading

ebuchman Nov 6, 2019

ValarDragon Nov 6, 2019

liamsi left a comment

liamsi Nov 4, 2019

ebuchman Nov 6, 2019

liamsi left a comment

liamsi Dec 15, 2019

ebuchman commented Dec 27, 2019

ebuchman commented Dec 27, 2019

ebuchman commented Dec 28, 2019

ebuchman commented Dec 28, 2019

ebuchman commented Jan 22, 2020

liamsi commented Jan 22, 2020

ebuchman commented Jan 23, 2020

codecov-io commented Feb 10, 2020 •

edited

Loading

ebuchman commented Feb 10, 2020


		### Manager

		For lack of a better name. The Manager co-ordiantes the syncing and is the

adr-002 lite client #54

adr-002 lite client #54

Conversation

ebuchman commented Oct 30, 2019 • edited Loading

ValarDragon Nov 1, 2019

Choose a reason for hiding this comment

ebuchman Nov 1, 2019

Choose a reason for hiding this comment

ValarDragon Nov 4, 2019 • edited Loading

Choose a reason for hiding this comment

ebuchman Nov 6, 2019

Choose a reason for hiding this comment

ValarDragon Nov 6, 2019

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

liamsi Nov 4, 2019

Choose a reason for hiding this comment

ebuchman Nov 6, 2019

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

liamsi Dec 15, 2019

Choose a reason for hiding this comment

ebuchman commented Dec 27, 2019

ebuchman commented Dec 27, 2019

ebuchman commented Dec 28, 2019

ebuchman commented Dec 28, 2019

ebuchman commented Jan 22, 2020

liamsi commented Jan 22, 2020

ebuchman commented Jan 23, 2020

codecov-io commented Feb 10, 2020 • edited Loading

Codecov Report

ebuchman commented Feb 10, 2020

ebuchman commented Oct 30, 2019 •

edited

Loading

ValarDragon Nov 4, 2019 •

edited

Loading

codecov-io commented Feb 10, 2020 •

edited

Loading