light-client: expose latest_trusted on Supervisor Handle #394

xla · 2020-06-29T19:20:50Z

Part of #219 and preparation for #363. Wanted to split out the new additions to the light-client Supervisor and its Handle to make it easier to review and move forward. The declared goal of this change-set is to introduce new API to full-fill the planned endpoints of #219.

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

liamsi · 2020-06-29T21:06:09Z

light-client/src/supervisor.rs

        Self { sender }
    }

+    /// Get latest trusted block from the [`Supervisor`].
+    pub fn latest_trusted(&mut self) -> Result<Option<LightBlock>, Error> {


In which case does this return Ok(None)?

When the underlying primary returns None for its latest_trusted. This is really a direct pass-through, and judging by the method on the Instance the latest trusted being none should be expected and is non-errornous behaviour.

Makes sense. Need to check this out locally. I'm still wondering, when a primary doesn't yet have any trusted light block in store:
https://sourcegraph.com/github.com/informalsystems/tendermint-rs@b3a5b1e70cd2d7fb06361950d10951d51b3dce06/-/blob/light-client/src/supervisor.rs#L53-55

If the primary always returns a trusted block, we should encode that in our types. While primary/witness might only be a operational concern as @milosevic pointed out during the call yesterday, there might a possibility to otherwise encode this expectation. For example with an enum distinguishing between node types/states. What would be the right way to find answers to these questions?

This might be orthogonal to this PR but I think, we'd need to enumerate in which cases the primary does not yet have a trusted light block and then figure out if we want to treat these cases as errors for some use-cases instead (maybe further up, e.g. in the rpc).

Follow-up in #395

Left a comment in #395. Otherwise, I fully agree that we should try encode such invariants as possible in the types themselves.

liamsi · 2020-06-29T21:17:09Z

light-client/src/supervisor.rs

-    /// The verification has succeded
-    VerificationSuccess(Box<LightBlock>),
-    /// The verification has failed
-    VerificationFailure(Error),


Interesting. I wasn't aware that these are needed for internal communication only. Looks like a good simplification. Especially removing the pub from Events (now HandleInput) makes sense then 👍 I would definitely like to hear what @romac / @brapse think about this, too.

Yes they are internal only so a simplification here might makes sense.

To give a bit more context, up until now this enum was a mix of events. Some of them used to send callbacked events from the Handle up to its Supervisor, while others where only used as intermediates inside of Handle methods. This led to the use of wildcard match arms in places where only a subset of events where relevant, with undefined or panic behaviour. Additionally it would mask the case where a new variant was added but not accounted for in vital places, e.g. the Supervisor run loop. By isolating the enum to the single purpose of Handle -> Supervisor comms, all these downsides are removed and we get some help from the compiler when extending the surface between the two of them.

Good catch! I wasn't so happy either with having both input and outputs/internal events in the same enum, but didn't see/look for the opportunity to simplify all this. 👍

More thoughts on the matter in #185 (review)

romac

Great stuff @xla!

light-client/src/supervisor.rs

romac · 2020-06-30T09:23:11Z

light-client/src/supervisor.rs

-    /// The verification has succeded
-    VerificationSuccess(Box<LightBlock>),
-    /// The verification has failed
-    VerificationFailure(Error),


Good catch! I wasn't so happy either with having both input and outputs/internal events in the same enum, but didn't see/look for the opportunity to simplify all this. 👍

romac · 2020-06-30T09:30:05Z

light-client/src/supervisor.rs

        Self { sender }
    }

+    /// Get latest trusted block from the [`Supervisor`].
+    pub fn latest_trusted(&mut self) -> Result<Option<LightBlock>, Error> {


Left a comment in #395. Otherwise, I fully agree that we should try encode such invariants as possible in the types themselves.

liamsi · 2020-06-30T12:04:09Z

What would it take to get this out of draft into ready for review? I think the changes look good and make sense 👍

xla · 2020-06-30T12:12:12Z

What would it take to get this out of draft into ready for review? I think the changes look good and make sense +1

I still would like to extend the API to full-fill all needs of the /status endpoint as described in #219. Also happy to split that up if we want smaller chunks for review. Latter might even be better, What ya all think?

liamsi · 2020-06-30T13:00:55Z

Also happy to split that up if we want smaller chunks for review. Latter might even be better, What ya all think?

I'd slightly prefer to merge this and get the remaining changes in a separate PR but either way is good to me.

romac · 2020-06-30T14:11:35Z

light-client/src/supervisor.rs

+        Handle::new(self.sender.clone())
+    }
+
+    #[pre(self.peers.primary().is_some())]


I think we should remove this precondition (and other similar ones on verify_to_highest/target) since we are actually raising a proper error if there is no primary, but this can be done in a follow-up (eg. by me).

Tracking this in #400

liamsi

LGTM 👍

liamsi · 2020-06-30T14:18:07Z

CHANGES.md

+
+Light Client:
+
+- Expose latest_trusted from Supervisor Handle ([#394])


Did we decide to have the light client changelog here or in the separate crate?
@brapse

Probably the changes related to the light client need a more general description and intro in the changelog but this is good for now 👌

Just looked for precedence and there were entries for Light Client further down.

Improve semantics of supervisor events

8eba90f

xla added light-client Issues/features which involve the light client feature labels Jun 29, 2020

xla added this to the Light Node milestone Jun 29, 2020

xla requested review from brapse, romac, milosevic and liamsi June 29, 2020 19:20

xla self-assigned this Jun 29, 2020

Implement latest_trusted on Supervisor

b3a5b1e

xla mentioned this pull request Jun 29, 2020

light-node: RPC server #363

Merged

liamsi reviewed Jun 29, 2020

View reviewed changes

xla mentioned this pull request Jun 30, 2020

light-client: in what scenarios can a primary have no trusted LightBlock #395

Closed

romac reviewed Jun 30, 2020

View reviewed changes

xla changed the title ~~light-client: extend API of Supervisor Handle~~ light-client: expose latest_trusted on Supervisor Handle Jun 30, 2020

xla added 2 commits June 30, 2020 15:12

Remove result type alias

bca7fe8

Update changes

a40dd11

xla marked this pull request as ready for review June 30, 2020 13:24

romac approved these changes Jun 30, 2020

View reviewed changes

romac mentioned this pull request Jun 30, 2020

light-client: remove preconditions in user-facing functions in favor of proper error handling #400

Closed

liamsi approved these changes Jun 30, 2020

View reviewed changes

brapse merged commit ebda75b into master Jun 30, 2020

brapse deleted the xla/219-extend-supervisor-handle branch June 30, 2020 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

light-client: expose latest_trusted on Supervisor Handle #394

light-client: expose latest_trusted on Supervisor Handle #394

xla commented Jun 29, 2020 •

edited

Loading

liamsi Jun 29, 2020 •

edited

Loading

xla Jun 29, 2020

liamsi Jun 29, 2020

xla Jun 30, 2020

liamsi Jun 30, 2020

xla Jun 30, 2020

romac Jun 30, 2020

liamsi Jun 29, 2020 •

edited

Loading

brapse Jun 30, 2020

xla Jun 30, 2020

romac Jun 30, 2020

xla Jun 30, 2020

romac left a comment

romac Jun 30, 2020

romac Jun 30, 2020

liamsi commented Jun 30, 2020

xla commented Jun 30, 2020

liamsi commented Jun 30, 2020

romac Jun 30, 2020

romac Jun 30, 2020

liamsi left a comment

liamsi Jun 30, 2020

xla Jun 30, 2020


		Light Client:

		- Expose latest_trusted from Supervisor Handle ([#394])

light-client: expose latest_trusted on Supervisor Handle #394

light-client: expose latest_trusted on Supervisor Handle #394

Conversation

xla commented Jun 29, 2020 • edited Loading

liamsi Jun 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

liamsi Jun 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

romac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi commented Jun 30, 2020

xla commented Jun 30, 2020

liamsi commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xla commented Jun 29, 2020 •

edited

Loading

liamsi Jun 29, 2020 •

edited

Loading

liamsi Jun 29, 2020 •

edited

Loading