diff --git a/CHANGES.md b/CHANGES.md index 6c769efc9..c542ed91e 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,12 +1,15 @@ -## pending +# pending Light Client: - Expose latest_trusted from Supervisor Handle ([#394]) - Turn `Handle` into a trait for ease of integration and testability ([#401]) +- Improve `Supervisor` ergonomics according to [ADR-007] ([#403]) [#394]: https://github.com/informalsystems/tendermint-rs/pull/394 [#401]: https://github.com/informalsystems/tendermint-rs/pull/401 +[#403]: https://github.com/informalsystems/tendermint-rs/pull/403 +[ADR-007]: https://github.com/informalsystems/tendermint-rs/blob/master/docs/architecture/adr-007-light-client-supervisor-ergonomics.md ## [0.14.1] (2020-06-23) @@ -274,7 +277,7 @@ This release is compatible with [tendermint v0.28] [#205]: https://github.com/tendermint/kms/pull/219 [#181]: https://github.com/tendermint/kms/pull/181 [tendermint v0.29]: https://github.com/tendermint/tendermint/blob/master/CHANGELOG.md#v0290 -[tendermint v0.28]: https://github.com/tendermint/tendermint/blob/master/CHANGELOG.md#v0280 +[tendermint v0.28]: https://github.com/tendermint/tendermint/blob/master/CHANGELOG.md#v0280 [#30]: https://github.com/interchainio/tendermint-rs/pull/30 [#16]: https://github.com/interchainio/tendermint-rs/pull/16 [#15]: https://github.com/interchainio/tendermint-rs/pull/15 diff --git a/docs/architecture/adr-007-light-client-supervisor-ergonomics.md b/docs/architecture/adr-007-light-client-supervisor-ergonomics.md index 5128a5b3c..b2bedb806 100644 --- a/docs/architecture/adr-007-light-client-supervisor-ergonomics.md +++ b/docs/architecture/adr-007-light-client-supervisor-ergonomics.md @@ -3,6 +3,7 @@ ## Changelog * 2020-06-30: Initial draft +* 2020-07-02: approved with corrections ## Context @@ -32,7 +33,7 @@ server and ibc relayer. ## Status -Proposed +Approved ## Consequences diff --git a/light-client/examples/light_client.rs b/light-client/examples/light_client.rs index 097fb16ef..fdc9a5023 100644 --- a/light-client/examples/light_client.rs +++ b/light-client/examples/light_client.rs @@ -160,19 +160,19 @@ fn sync_cmd(opts: SyncOpts) { ProdEvidenceReporter::new(peer_addr), ); - let mut handle = supervisor.handle(); + let handle = supervisor.handle(); std::thread::spawn(|| supervisor.run()); loop { - handle.verify_to_highest_async(|result| match result { + match handle.verify_to_highest() { Ok(light_block) => { - println!("[ info ] synced to block {}", light_block.height()); + println!("[info] synced to block {}", light_block.height()); } - Err(e) => { - println!("[ error ] sync failed: {}", e); + Err(err) => { + println!("[error] sync failed: {}", err); } - }); + } std::thread::sleep(Duration::from_millis(800)); } diff --git a/light-client/src/callback.rs b/light-client/src/callback.rs deleted file mode 100644 index 9d4c59ae7..000000000 --- a/light-client/src/callback.rs +++ /dev/null @@ -1,26 +0,0 @@ -use std::fmt; - -/// A boxed `FnOnce(A) -> () + Send`. -pub struct Callback { - inner: Box () + Send>, -} - -impl fmt::Debug for Callback { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("Callback").finish() - } -} - -impl Callback { - /// Box the given closure in a `Callback`. - pub fn new(inner: impl FnOnce(A) -> () + Send + 'static) -> Self { - Self { - inner: Box::new(inner), - } - } - - /// Call the underlying closure on `result`. - pub fn call(self, result: A) { - (self.inner)(result); - } -} diff --git a/light-client/src/errors.rs b/light-client/src/errors.rs index 10d72103b..97a35254d 100644 --- a/light-client/src/errors.rs +++ b/light-client/src/errors.rs @@ -1,6 +1,9 @@ //! Toplevel errors raised by the light client. +use std::fmt::Debug; + use anomaly::{BoxError, Context}; +use crossbeam_channel as crossbeam; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -57,6 +60,9 @@ pub enum ErrorKind { #[error("invalid light block: {0}")] InvalidLightBlock(#[source] VerificationError), + + #[error("internal channel disconnected")] + ChannelDisconnected, } impl ErrorKind { @@ -107,3 +113,15 @@ impl ErrorExt for ErrorKind { } } } + +impl From> for ErrorKind { + fn from(_err: crossbeam::SendError) -> Self { + Self::ChannelDisconnected + } +} + +impl From for ErrorKind { + fn from(_err: crossbeam::RecvError) -> Self { + Self::ChannelDisconnected + } +} diff --git a/light-client/src/lib.rs b/light-client/src/lib.rs index 74580ae1b..60f63e3a0 100644 --- a/light-client/src/lib.rs +++ b/light-client/src/lib.rs @@ -12,7 +12,6 @@ //! See the `light_client` module for the main documentation. -pub mod callback; pub mod components; pub mod contracts; pub mod errors; diff --git a/light-client/src/supervisor.rs b/light-client/src/supervisor.rs index d9bedbc72..b9a75812b 100644 --- a/light-client/src/supervisor.rs +++ b/light-client/src/supervisor.rs @@ -2,49 +2,35 @@ use crossbeam_channel as channel; use tendermint::evidence::{ConflictingHeadersEvidence, Evidence}; -use crate::{ - bail, - callback::Callback, - errors::{Error, ErrorKind}, - evidence::EvidenceReporter, - fork_detector::{Fork, ForkDetection, ForkDetector}, - light_client::LightClient, - peer_list::PeerList, - state::State, - types::{Height, LightBlock, PeerId, Status}, -}; +use crate::bail; +use crate::errors::{Error, ErrorKind}; +use crate::evidence::EvidenceReporter; +use crate::fork_detector::{Fork, ForkDetection, ForkDetector}; +use crate::light_client::LightClient; +use crate::peer_list::PeerList; +use crate::state::State; +use crate::types::{Height, LightBlock, PeerId, Status}; pub trait Handle { /// Get latest trusted block from the [`Supervisor`]. - fn latest_trusted(&mut self) -> Result, Error>; + fn latest_trusted(&self) -> Result, Error> { + todo!() + } /// Verify to the highest block. - fn verify_to_highest(&mut self) -> Result; + fn verify_to_highest(&self) -> Result { + todo!() + } /// Verify to the block at the given height. - fn verify_to_target(&mut self, height: Height) -> Result; - - /// Async version of `verify_to_highest`. - /// - /// The given `callback` will be called asynchronously with the - /// verification result. - fn verify_to_highest_async( - &mut self, - callback: impl FnOnce(Result) -> () + Send + 'static, - ); - - /// Async version of `verify_to_target`. - /// - /// The given `callback` will be called asynchronously with the - /// verification result. - fn verify_to_target_async( - &mut self, - height: Height, - callback: impl FnOnce(Result) -> () + Send + 'static, - ); + fn verify_to_target(&self, _height: Height) -> Result { + todo!() + } /// Terminate the underlying [`Supervisor`]. - fn terminate(&mut self); + fn terminate(&self) -> Result<(), Error> { + todo!() + } } /// Input events sent by the [`Handle`]s to the [`Supervisor`]. They carry a [`Callback`] which is @@ -52,13 +38,13 @@ pub trait Handle { #[derive(Debug)] enum HandleInput { /// Terminate the supervisor process - Terminate(Callback<()>), + Terminate(channel::Sender<()>), /// Verify to the highest height, call the provided callback with result - VerifyToHighest(Callback>), + VerifyToHighest(channel::Sender>), /// Verify to the given height, call the provided callback with result - VerifyToTarget(Height, Callback>), + VerifyToTarget(Height, channel::Sender>), /// Get the latest trusted block. - LatestTrusted(Callback, Error>>), + LatestTrusted(channel::Sender>), } /// A light client `Instance` packages a `LightClient` together with its `State`. @@ -98,7 +84,7 @@ impl Instance { /// removed. /// /// The supervisor is intended to be ran in its own thread, and queried -/// via a `Handle`, sync- or asynchronously. +/// via a `Handle`. /// /// ## Example /// @@ -111,12 +97,13 @@ impl Instance { /// /// loop { /// // Asynchronously query the supervisor via a handle -/// handle.verify_to_highest_async(|result| match result { +/// let maybe_block = handle.verify_to_highest(); +/// match maybe_block { /// Ok(light_block) => { -/// println!("[ info ] synced to block {}", light_block.height()); +/// println!("[info] synced to block {}", light_block.height()); /// } /// Err(e) => { -/// println!("[ error ] sync failed: {}", e); +/// println!("[error] sync failed: {}", e); /// } /// }); /// @@ -314,26 +301,26 @@ impl Supervisor { /// Run the supervisor event loop in the same thread. /// /// This method should typically be called within a new thread with `std::thread::spawn`. - pub fn run(mut self) { + pub fn run(mut self) -> Result<(), Error> { loop { - let event = self.receiver.recv().unwrap(); + let event = self.receiver.recv().map_err(ErrorKind::from)?; match event { - HandleInput::LatestTrusted(callback) => { + HandleInput::LatestTrusted(sender) => { let outcome = self.latest_trusted(); - callback.call(Ok(outcome)); + sender.send(outcome).map_err(ErrorKind::from)?; } - HandleInput::Terminate(callback) => { - callback.call(()); - return; + HandleInput::Terminate(sender) => { + sender.send(()).map_err(ErrorKind::from)?; + return Ok(()); } - HandleInput::VerifyToTarget(height, callback) => { + HandleInput::VerifyToTarget(height, sender) => { let outcome = self.verify_to_target(height); - callback.call(outcome); + sender.send(outcome).map_err(ErrorKind::from)?; } - HandleInput::VerifyToHighest(callback) => { + HandleInput::VerifyToHighest(sender) => { let outcome = self.verify_to_highest(); - callback.call(outcome); + sender.send(outcome).map_err(ErrorKind::from)?; } } } @@ -354,73 +341,43 @@ impl SupervisorHandle { } fn verify( - &mut self, - make_event: impl FnOnce(Callback>) -> HandleInput, + &self, + make_event: impl FnOnce(channel::Sender>) -> HandleInput, ) -> Result { let (sender, receiver) = channel::bounded::>(1); - let callback = Callback::new(move |result| { - sender.send(result).unwrap(); - }); + let event = make_event(sender); + self.sender.send(event).map_err(ErrorKind::from)?; - let event = make_event(callback); - self.sender.send(event).unwrap(); - - receiver.recv().unwrap() + receiver.recv().map_err(ErrorKind::from)? } } - impl Handle for SupervisorHandle { - fn latest_trusted(&mut self) -> Result, Error> { - let (sender, receiver) = channel::bounded::, Error>>(1); - - let callback = Callback::new(move |result| { - sender.send(result).unwrap(); - }); + fn latest_trusted(&self) -> Result, Error> { + let (sender, receiver) = channel::bounded::>(1); - // TODO(xla): Transform crossbeam errors into proper domain errors. self.sender - .send(HandleInput::LatestTrusted(callback)) - .unwrap(); + .send(HandleInput::LatestTrusted(sender)) + .map_err(ErrorKind::from)?; - // TODO(xla): Transform crossbeam errors into proper domain errors. - receiver.recv().unwrap() + Ok(receiver.recv().map_err(ErrorKind::from)?) } - fn verify_to_highest(&mut self) -> Result { + fn verify_to_highest(&self) -> Result { self.verify(HandleInput::VerifyToHighest) } - fn verify_to_target(&mut self, height: Height) -> Result { - self.verify(|callback| HandleInput::VerifyToTarget(height, callback)) - } - - fn verify_to_highest_async( - &mut self, - callback: impl FnOnce(Result) -> () + Send + 'static, - ) { - let event = HandleInput::VerifyToHighest(Callback::new(callback)); - self.sender.send(event).unwrap(); + fn verify_to_target(&self, height: Height) -> Result { + self.verify(|sender| HandleInput::VerifyToTarget(height, sender)) } - fn verify_to_target_async( - &mut self, - height: Height, - callback: impl FnOnce(Result) -> () + Send + 'static, - ) { - let event = HandleInput::VerifyToTarget(height, Callback::new(callback)); - self.sender.send(event).unwrap(); - } - - fn terminate(&mut self) { + fn terminate(&self) -> Result<(), Error> { let (sender, receiver) = channel::bounded::<()>(1); - let callback = Callback::new(move |_| { - sender.send(()).unwrap(); - }); - - self.sender.send(HandleInput::Terminate(callback)).unwrap(); + self.sender + .send(HandleInput::Terminate(sender)) + .map_err(ErrorKind::from)?; - receiver.recv().unwrap() + Ok(receiver.recv().map_err(ErrorKind::from)?) } }