Implement builder DSL for light client initialization, and other things #583

romac · 2020-09-24T19:58:28Z

Apologies for this huge, many-headed, multi-faced PR which would deserve to be split up but that has become quite difficult as I have been evolving it alongside the work on the ibc-rs side. Let me know if it's too much to handle, and I'll try to do it by the end of the week.

Here's what it does:

Expose a builder DSL for initializing light client instances and their supervisor (partially addresses light-node: Initialization Follow up #484)
Adapt the light node commands to make use of the new DSL
Refactor the Io component a bit to enable the aforementioned DSL
Improves error handling in the light node, just a bit (partially addresses light-node: Error Handling and Exiting #486)
Improves the definition of the supervisor Handle for use within ibc-rs

Disclaimer 0: Documentation for the builder DSL is really minimal at the moment.

Disclaimer 1: This PR does NOT address #497.

Disclaimer 2: This PR is lacking unit tests for the new builder DSL.

Disclaimer 3: I have not tested whether the light node CLI still works.

Disclaimer 4: The builder DSL could definitely still be improved, but I'd first like to use it within ibc-rs before making other changes.

Given all this, I think that the easiest way to review this PR is to:

Start with the first 6 commits (up to, and including, 07a2427).
Then review the builders themselves in light-client/src/builder.rs and light-client/src/builder/.
Then focus on the rest of the changes within the light-client directory.
And finally review the changes within the light-node directory.

I won't assign any reviewers given the poor state of the PR, so feel free to assign yourselves.

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md

codecov-commenter · 2020-09-25T09:49:11Z

Codecov Report

Merging #583 into master will decrease coverage by 0.9%.
The diff coverage is 6.4%.

@@           Coverage Diff            @@
##           master    #583     +/-   ##
========================================
- Coverage    43.0%   42.1%   -1.0%     
========================================
  Files         157     161      +4     
  Lines       10254   10472    +218     
  Branches     2502    2544     +42     
========================================
- Hits         4415    4413      -2     
- Misses       5482    5696    +214     
- Partials      357     363      +6

Impacted Files	Coverage Δ
light-client/examples/light_client.rs	`0.0% <0.0%> (ø)`
light-client/src/builder.rs	`0.0% <ø> (ø)`
light-client/src/builder/error.rs	`0.0% <0.0%> (ø)`
light-client/src/builder/light_client.rs	`0.0% <0.0%> (ø)`
light-client/src/builder/supervisor.rs	`0.0% <0.0%> (ø)`
light-client/src/components/io.rs	`0.0% <0.0%> (-2.8%)`	⬇️
light-client/tests/integration.rs	`0.0% <0.0%> (ø)`
light-node/src/commands/initialize.rs	`0.0% <0.0%> (ø)`
light-node/src/commands/start.rs	`0.0% <0.0%> (ø)`
light-node/src/rpc.rs	`52.1% <0.0%> (-5.0%)`	⬇️
... and 10 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 856084e...ee4bbaa. Read the comment docs.

melekes · 2020-09-25T13:24:59Z

light-client/src/builder/light_client.rs

+    ) -> Result<LightClientBuilder<HasTrustedState>, Error> {
+        self.validate(&trusted_state)?;
+
+        // TODO(liamsi, romac): it is unclear if this should be Trusted or only Verified


it may be a good idea to cross-verify the first header to check if all witnesses have the same view.

but in the end, you have to trust the state I think.

It may be a good idea to cross-verify the first header to check if all witnesses have the same view.

Sounds like a good idea, indeed!

@ebuchman What do you think? Should we address this in a follow-up PR?

Yup, if anyone disagrees should probably throw an error and refuse to start

yeah I agree. We currently just check that the chain ID's are the same but it would make more sense to cross-verify the first header. This way we can also check for responsiveness i.e. if a witness doesn't respond we drop them, and for correctness i.e. if one of the headers are different we halt and return the error

I have collected the outcome of the discussions regarding initialization into
tendermint/spec#159

We have to trust the given initial header. Cross-check doesn't do anything useful. It only opens the possibility that one fault peer can prevent the light node to start.

Might as well catch that error at startup and get the user to provide a different witness rather than catch it later and just drop the witness without replacement, no ?

Might as well catch that error at startup and get the user to provide a different witness rather than catch it later and just drop the witness without replacement, no ?

We can do that. Still, the user might need to do that several times, if the user is unlucky and always chooses another faulty peer. And these faulty peers don't even need to be clever but just return garbage with the right height. Still, if it makes sense in the practical setting we can do that. It is definitely safe.

light-client/src/builder/supervisor.rs

thanethomson · 2020-09-29T15:46:37Z

light-client/src/utils/block_on.rs

+/// Run a future to completion on a new thread, with the given timeout.
+///
+/// This function will block the caller until the given future has completed.
+pub fn block_on<F>(f: F, peer: PeerId, timeout: Option<Duration>) -> Result<F::Output, IoError>


I don't think we're going to solve the sync/async conundrum in the short term (and definitely not in this PR), but to get the two to play nicely together we should try to figure out a simple, low-effort way to spawn a single Tokio runtime in a separate thread to which we can submit async tasks from synchronous code.

thanethomson

I'd personally say that this looks good enough to merge if it works for ibc-rs 👍

Co-authored-by: Thane Thomson <[email protected]>

ebuchman · 2020-09-29T18:20:39Z

@romac what about the failing integration test?

romac · 2020-09-29T18:42:11Z

Weird! Will take a look asap

ebuchman

Awesome. A bunch of comments but don't think any of them need to block, just need to figure out / fix the failing test

ebuchman · 2020-09-29T20:56:58Z

light-client/src/builder/light_client.rs

+
+/// Builder for a light client [`Instance`]
+#[must_use]
+pub struct LightClientBuilder<State> {


Aren't generic parameters usually just one letter? How does one decide whether this should be S or State ?

I don't think there are clear guidelines on that in the Rust community, it's true that single letter parameter are more common overall, but I thought that spelling it out in this case would make it more obvious what the type param is about, especially since it this case it represents the state of the builder and not any other type, like eg. with Vec<T> where T just stands for any type. Happy to change it to a single letter regardless if you think that's more consistent.

ebuchman · 2020-09-29T20:57:18Z

light-client/src/builder/light_client.rs

+    light_store: Box<dyn LightStore>,
+
+    #[allow(dead_code)]
+    state: State,


Why do we need the field here?

We need a field here because in Rust type parameters of a struct have to appear in its fields.

We could use PhantomData<State>, but since the two type states are zero-sized anyway (same as PhantomData), this saves us an import here and some potential confusion wrt to PhantomData.

ebuchman · 2020-09-29T20:58:02Z

light-client/src/builder/light_client.rs

+    state: State,
+}
+
+impl<Current> LightClientBuilder<Current> {


So Current is another generic parameter, and this private method lets us go from any arbitrary state to any other arbitrary state.

It feels a bit like we're jumping through a few hoops to do this with the type parameters and seems like it might be cleaner to just have structs for each type that wrap the underlying LightClientBuilder, like we discussed, but maybe that's just me or I'm not used to seeing some of this.

This pattern can definitely be expressed both ways. I think it's just a matter of preference. I prefer the type param approach as it to me it expresses more clearly that the purpose of the struct stays the same, it's just in a different state. Perhaps it's just because I am more used to this pattern from Haskell or Scala, but in the end I don't feel that strongly either way. So if you feel like the wrapper structs would be more readable, I am happy to make the change.

ebuchman · 2020-09-29T20:59:22Z

light-client/src/builder/light_client.rs

+    /// Private method to move from one state to another
+    fn with_state<Next>(self, state: Next) -> LightClientBuilder<Next> {
+        LightClientBuilder {
+            peer_id: self.peer_id,


Can't we use .. for everything except state ?

Unfortunately not, using ..self yields the following error because self does not have the right type:

error[E0308]: mismatched types --> light-client/src/builder/light_client.rs:53:39 | 50 | impl<Current> LightClientBuilder<Current> { | ------- found type parameter 51 | /// Private method to move from one state to another 52 | fn with_state<Next>(self, state: Next) -> LightClientBuilder<Next> { | ---- expected type parameter 53 | LightClientBuilder { state, ..self } | ^^^^ expected type parameter `Next`, found type parameter `Current` | = note: expected struct `builder::light_client::LightClientBuilder<Next>` found struct `builder::light_client::LightClientBuilder<Current>` = note: a type parameter was expected, but a different one was found; you might be missing a type parameter or trait bound = note: for more information, visit https://doc.rust-lang.org/book/ch10-02-traits.html#traits-as-parameters

Lol ok. I think this and the need to have a dummy state field both motivate using the wrapper approach as I think that would solve both, but no biggie

ebuchman · 2020-09-29T21:05:10Z

light-client/src/builder/light_client.rs

+    ) -> Result<LightClientBuilder<HasTrustedState>, Error> {
+        self.validate(&trusted_state)?;
+
+        // TODO(liamsi, romac): it is unclear if this should be Trusted or only Verified


Yup, if anyone disagrees should probably throw an error and refuse to start

ebuchman · 2020-09-29T21:05:52Z

light-client/src/builder/light_client.rs

+    }
+
+    /// Fetch and set the latest block from the primary peer as the trusted state.
+    pub fn trust_primary_latest(self) -> Result<LightClientBuilder<HasTrustedState>, Error> {


This is a bit dangerous and should either come with a big warning or be removed all together

Let's remove it since we don't have any use for it at the moment.

ebuchman · 2020-09-29T21:06:57Z

light-client/src/builder/light_client.rs

+    }
+
+    /// Set the given light block as the initial trusted state.
+    pub fn trust_light_block(


Should this be public?

Good catch. It should not, as it is quite dangerous and is only currently used in the light client example. The latter could be rewritten to use trust_from_store instead so it's not needed at all. Will make it private.

ebuchman · 2020-09-29T21:09:23Z

light-client/src/builder/light_client.rs

+        // TODO(ismail, romac): actually verify more predicates of light block?
+
+        self.predicates
+            .validator_sets_match(light_block, &*self.hasher)


Important thing is actually the next_validators!

ebuchman · 2020-09-29T21:11:28Z

light-client/src/builder/light_client.rs

+    }
+
+    fn validate(&self, light_block: &LightBlock) -> Result<(), Error> {
+        // TODO(ismail, romac): actually verify more predicates of light block?


We should probably check:

is_within_trust_period

is_header_from_past

validator_sets_match (though this doesn't really matter since we will never use these vals)

next_validators_match

Everything else depends on the commit and/or previous blocks. We could verify the commit but it's not really necessary.

ebuchman · 2020-09-29T21:17:40Z

light-client/tests/integration.rs

+        let mut builder = PeerList::builder();
+        builder.primary(primary, primary_instance);
+        builder.witness(witness, witness_instance);
+        builder.build()


Can't these still be chained?

Not since the API change in 26d1efe.

I figured I would follow the current recommendations of the popular derive_builder crate. I agree that the chained version looks a lot better, so I would definitely not mind going back on that.

…it and it is quite dangerous

…ght client builder

romac · 2020-09-29T22:05:34Z

@thanethomson @ebuchman Thanks a lot for your inputs!

Just pushed a few commits which address some of the comments above. Will address the rest + CI failure tomorrow morning.

romac · 2020-09-30T15:45:15Z

@ebuchman @thanethomson This PR is ready for merge imho, now that we have figured out where the CI failure came from (#600).

ebuchman

Great!

romac added 20 commits September 24, 2020 19:10

Add Send + Sync bound on light client Handle

ad4f5e7

Remove default implementation on Handle trait

ef64d96

Whitespace

0801054

Fix light-node code after Handle update

08b73b5

Remove unused mut qualifier after Supervisor update

978190d

Derive Clone on SupervisorHandle

07a2427

Add DSL for building supervisor and light clients

363e883

Refactor Io component

fde8856

Refactor Supervisor builder

6f352fe

Re-add a TODO that got lost in the refactor

4de977f

Document builders

905a80b

Simplify return types of supervisor builder methods

f8a63ab

Update light client example to use builders

b6f9da9

Perform more validation when doing subjective initialization

9b142db

Add option to use trusted state alrady in store to light client builder

fa7c2f5

Use builders in light node start command

60dd63e

Improve API of PeerListBuilder

26d1efe

Finish adapting start command to use supervisor builder

d88b1f6

Fix WASM build

2a65f69

Expose a couple functions on the supervisor

ee4bbaa

romac mentioned this pull request Sep 25, 2020

Update to latest tendermint-rs and adapt relayer to new light client informalsystems/hermes#255

Merged

6 tasks

melekes reviewed Sep 25, 2020

View reviewed changes

romac added 4 commits September 29, 2020 13:23

Block on async tasks in a new thread to avoid nesting Tokio runtimes

610035a

Cleanup

49d687e

Nest std_ext module under utils

15b4e9c

Feature-guard block_on for WASM

36a0a1a

thanethomson reviewed Sep 29, 2020

View reviewed changes

light-client/src/builder/supervisor.rs Show resolved Hide resolved

thanethomson reviewed Sep 29, 2020

View reviewed changes

light-client/src/builder/supervisor.rs Outdated Show resolved Hide resolved

thanethomson reviewed Sep 29, 2020

View reviewed changes

thanethomson previously approved these changes Sep 29, 2020

View reviewed changes

Fix typo in comment

d2380be

Co-authored-by: Thane Thomson <[email protected]>

romac dismissed thanethomson’s stale review via d2380be September 29, 2020 16:18

romac added 2 commits September 29, 2020 18:36

Ensure that at least one witness is provided in the SupervisorBuilder

c95ebcd

Rename SupervisorBuilder::unwrap to inner to express is infallability

e0f28f2

ebuchman previously approved these changes Sep 29, 2020

View reviewed changes

romac added 3 commits September 29, 2020 23:41

Remove LightClientBuilder::trust_primary_latest since we do not need …

0e1709d

…it and it is quite dangerous

Make LightClientBuilder::trust_light_block method private

2705247

Perform more thorough validation of the trusted light block in the li…

3f496fd

…ght client builder

romac dismissed ebuchman’s stale review via 3f496fd September 29, 2020 22:04

Update light client integration test to use builders

ad4ba11

thanethomson mentioned this pull request Sep 30, 2020

light-client: Verification failure #600

Closed

ebuchman approved these changes Sep 30, 2020

View reviewed changes

ebuchman merged commit 34d7d02 into master Sep 30, 2020

ebuchman deleted the romac/light-client-ibc branch September 30, 2020 16:59

ancazamfir mentioned this pull request Oct 1, 2020

fix tendermint dependencies informalsystems/hermes#280

Merged

This was referenced Nov 21, 2020

light-node: Initialization Follow up #484

Closed

light-node: Error Handling and Exiting #486

Closed

Implement builder DSL for light client initialization, and other things #583

Implement builder DSL for light client initialization, and other things #583

Conversation

romac commented Sep 24, 2020 • edited Loading

codecov-commenter commented Sep 25, 2020

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thanethomson left a comment

Choose a reason for hiding this comment

ebuchman commented Sep 29, 2020

romac commented Sep 29, 2020

ebuchman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romac Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romac commented Sep 29, 2020

romac commented Sep 30, 2020

ebuchman left a comment

Choose a reason for hiding this comment

romac commented Sep 24, 2020 •

edited

Loading

romac Sep 29, 2020 •

edited

Loading