Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

light-client: expose latest_trusted on Supervisor Handle #394

Merged
merged 4 commits into from
Jun 30, 2020
Merged

light-client: expose latest_trusted on Supervisor Handle #394

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8eba90f
Improve semantics of supervisor events
xla Jun 29, 2020
b3a5b1e
Implement latest_trusted on Supervisor
xla Jun 29, 2020
bca7fe8
Remove result type alias
xla Jun 30, 2020
a40dd11
Update changes
xla Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions light-client/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ pub enum ErrorKind {
#[error("store error")]
Store,

#[error("no primary")]
NoPrimary,

#[error("no witnesses")]
NoWitnesses,

Expand Down
121 changes: 62 additions & 59 deletions light-client/src/supervisor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
use contracts::pre;
use crossbeam_channel as channel;

use tendermint::evidence::{ConflictingHeadersEvidence, Evidence};

use crate::{
bail,
callback::Callback,
Expand All @@ -10,32 +15,21 @@ use crate::{
types::{Height, LightBlock, PeerId, Status},
};

use tendermint::evidence::{ConflictingHeadersEvidence, Evidence};

use contracts::pre;
use crossbeam_channel as channel;

/// Type alias for readability
pub type VerificationResult = Result<LightBlock, Error>;
xla marked this conversation as resolved.
Show resolved Hide resolved

/// Events which are exchanged between the `Supervisor` and its `Handle`s.
/// Input events sent by the [`Handle`]s to the [`Supervisor`]. They carry a [`Callback`] which is
/// used to communicate back the responses of the requests.
#[derive(Debug)]
pub enum Event {
// Inputs
enum HandleInput {
/// Terminate the supervisor process
Terminate(Callback<()>),
/// Verify to the highest height, call the provided callback with result
VerifyToHighest(Callback<VerificationResult>),
xla marked this conversation as resolved.
Show resolved Hide resolved
/// Verify to the given height, call the provided callback with result
VerifyToTarget(Height, Callback<VerificationResult>),
xla marked this conversation as resolved.
Show resolved Hide resolved

// Outputs
/// The supervisor has terminated
Terminated,
/// The verification has succeded
VerificationSuccess(Box<LightBlock>),
/// The verification has failed
VerificationFailure(Error),
Copy link
Member

@liamsi liamsi Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More thoughts on the matter in #185 (review)

/// Get the latest trusted block.
LatestTrusted(Callback<Result<Option<LightBlock>, Error>>),
}

/// An light client `Instance` packages a `LightClient` together with its `State`.
Expand Down Expand Up @@ -108,9 +102,9 @@ pub struct Supervisor {
/// Reporter of fork evidence
evidence_reporter: Box<dyn EvidenceReporter>,
/// Channel through which to reply to `Handle`s
sender: channel::Sender<Event>,
sender: channel::Sender<HandleInput>,
/// Channel through which to receive events from the `Handle`s
receiver: channel::Receiver<Event>,
receiver: channel::Receiver<HandleInput>,
}

impl std::fmt::Debug for Supervisor {
Expand All @@ -131,7 +125,7 @@ impl Supervisor {
fork_detector: impl ForkDetector + 'static,
evidence_reporter: impl EvidenceReporter + 'static,
) -> Self {
let (sender, receiver) = channel::unbounded::<Event>();
let (sender, receiver) = channel::unbounded::<HandleInput>();

Self {
peers,
Expand All @@ -142,6 +136,17 @@ impl Supervisor {
}
}

/// Create a new handle to this supervisor.
pub fn handle(&mut self) -> Handle {
Handle::new(self.sender.clone())
}

#[pre(self.peers.primary().is_some())]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking this in #400

fn latest_trusted(&self) -> Result<Option<LightBlock>, Error> {
let primary = self.peers.primary().ok_or_else(|| ErrorKind::NoPrimary)?;
Ok(primary.latest_trusted())
}

/// Verify to the highest block.
#[pre(self.peers.primary().is_some())]
pub fn verify_to_highest(&mut self) -> Result<LightBlock, Error> {
Expand Down Expand Up @@ -277,11 +282,6 @@ impl Supervisor {
.detect_forks(light_block, &trusted_state, self.peers.witnesses())
}

/// Create a new handle to this supervisor.
pub fn handle(&mut self) -> Handle {
Handle::new(self.sender.clone())
}

/// Run the supervisor event loop in the same thread.
///
/// This method should typically be called within a new thread with `std::thread::spawn`.
Expand All @@ -290,21 +290,22 @@ impl Supervisor {
let event = self.receiver.recv().unwrap();

match event {
Event::Terminate(callback) => {
HandleInput::LatestTrusted(callback) => {
let outcome = self.latest_trusted();
callback.call(outcome);
}
HandleInput::Terminate(callback) => {
callback.call(());
return;
}
Event::VerifyToTarget(height, callback) => {
HandleInput::VerifyToTarget(height, callback) => {
let outcome = self.verify_to_target(height);
callback.call(outcome);
}
Event::VerifyToHighest(callback) => {
HandleInput::VerifyToHighest(callback) => {
let outcome = self.verify_to_highest();
callback.call(outcome);
}
_ => {
// TODO: Log/record unexpected event
}
}
}
}
Expand All @@ -313,51 +314,58 @@ impl Supervisor {
/// A handle to a `Supervisor` which allows to communicate with
/// the supervisor across thread boundaries via message passing.
pub struct Handle {
sender: channel::Sender<Event>,
sender: channel::Sender<HandleInput>,
}

impl Handle {
/// Crate a new handle that sends events to the supervisor via
/// the given channel. For internal use only.
pub fn new(sender: channel::Sender<Event>) -> Self {
fn new(sender: channel::Sender<HandleInput>) -> Self {
Self { sender }
}

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

@liamsi liamsi Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case does this return Ok(None)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up in #395

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let (sender, receiver) = channel::bounded::<Result<Option<LightBlock>, Error>>(1);

let callback = Callback::new(move |result| {
sender.send(result).unwrap();
});

// TODO(xla): Transform crossbeam errors into proper domain errors.
self.sender
.send(HandleInput::LatestTrusted(callback))
.unwrap();

// TODO(xla): Transform crossbeam errors into proper domain errors.
receiver.recv().unwrap()
}

/// Verify to the highest block.
pub fn verify_to_highest(&mut self) -> VerificationResult {
self.verify(Event::VerifyToHighest)
self.verify(HandleInput::VerifyToHighest)
}

/// Verify to the block at the given height.
pub fn verify_to_target(&mut self, height: Height) -> VerificationResult {
self.verify(|callback| Event::VerifyToTarget(height, callback))
self.verify(|callback| HandleInput::VerifyToTarget(height, callback))
}

/// Verify either to the latest block (if `height == None`) or to a given block (if `height == Some(height)`).
fn verify(
&mut self,
make_event: impl FnOnce(Callback<VerificationResult>) -> Event,
make_event: impl FnOnce(Callback<VerificationResult>) -> HandleInput,
) -> VerificationResult {
let (sender, receiver) = channel::bounded::<Event>(1);
let (sender, receiver) = channel::bounded::<VerificationResult>(1);

let callback = Callback::new(move |result| {
// We need to create an event here
let event = match result {
Ok(header) => Event::VerificationSuccess(Box::new(header)),
Err(err) => Event::VerificationFailure(err),
};

sender.send(event).unwrap();
sender.send(result).unwrap();
});

let event = make_event(callback);
self.sender.send(event).unwrap();

match receiver.recv().unwrap() {
Event::VerificationSuccess(header) => Ok(*header),
Event::VerificationFailure(err) => Err(err),
_ => todo!(),
}
receiver.recv().unwrap()
}

/// Async version of `verify_to_highest`.
Expand All @@ -368,7 +376,7 @@ impl Handle {
&mut self,
callback: impl FnOnce(VerificationResult) -> () + Send + 'static,
) {
let event = Event::VerifyToHighest(Callback::new(callback));
let event = HandleInput::VerifyToHighest(Callback::new(callback));
self.sender.send(event).unwrap();
}

Expand All @@ -381,25 +389,20 @@ impl Handle {
height: Height,
callback: impl FnOnce(VerificationResult) -> () + Send + 'static,
) {
let event = Event::VerifyToTarget(height, Callback::new(callback));
let event = HandleInput::VerifyToTarget(height, Callback::new(callback));
self.sender.send(event).unwrap();
}

/// Terminate the underlying supervisor.
pub fn terminate(&mut self) {
let (sender, receiver) = channel::bounded::<Event>(1);
let (sender, receiver) = channel::bounded::<()>(1);

let callback = Callback::new(move |_| {
sender.send(Event::Terminated).unwrap();
sender.send(()).unwrap();
});

self.sender.send(Event::Terminate(callback)).unwrap();
self.sender.send(HandleInput::Terminate(callback)).unwrap();

while let Ok(event) = receiver.recv() {
match event {
Event::Terminated => return,
_ => continue,
}
}
receiver.recv().unwrap()
}
}