Abscissify light node #125

liamsi · 2020-01-11T06:38:45Z

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-01-11T08:12:45Z

lite-node/tests/acceptance.rs

+
+/// Use `LiteNodeConfig::default()` value if no config or args
+#[test]
+#[ignore]


Note: To do proper tests here (throughout the file), we'd need to mock the requester.

liamsi · 2020-01-11T08:15:16Z

lite-node/src/config.rs

+    /// The duration until we consider a trusted state as expired.
+    pub trusting_period: Duration,
+    /// Subjective initialization.
+    pub subjective_init: SubjectiveInit,


We need to figure out a clever way to represent TrustThreshold as sth that can be written to a file. It's currently just defined via it's behaviour.

liamsi · 2020-01-11T08:15:46Z

lite-node/src/commands/start.rs

+            let now = &SystemTime::now();
+            lite::verify_and_update_bisection(
+                latest_peer_height,
+                THRESHOLD, // TODO


https://github.com/interchainio/tendermint-rs/pull/125/files#r365508008

liamsi · 2020-01-11T08:15:56Z

lite-node/src/commands/start.rs

+
+// TODO: this should also somehow be configurable ...
+// we can't simply add this as a field in the config because this either would
+// be a trait (`TrustThreshold`) or immediately and impl thereof (`TrustThresholdOneThird`).


https://github.com/interchainio/tendermint-rs/pull/125/files#r365508008

liamsi · 2020-01-11T08:19:15Z

lite-node/src/lib.rs

+pub mod commands;
+pub mod config;
+pub mod error;
+pub mod prelude;


Everything below prelude should likely live somewhere else (in a different module)?

xla

I know this is WIP, but I'd like to see some rationale why a framework is a good choice to bootstrap two commands. At a glance it looks like a lot of overhead to setup, could you outline what dimensions should be considered and how abscissa is going to make any other (e.g. structopt, pico-args into function invocation) option non-viable. Obviously I come with the bias of fearing complexities in places where they aren't necessary, likely tho I'm missing a lot of important points. It would be good to also provide such rationale to other readers, was there a discussion/decision that informed this change and its necessity?

tarcieri · 2020-01-14T02:12:26Z

@xla I can give a short rationale:

Abscissa provides solutions for: command-line option parsing, components (i.e. dependency injection), configuration, error handling, logging, secrets management, and terminal interactions.

To the extent it has "a lot of overhead to setup", I'd argue that it has less than wiring all of these things up subsystem-by-subsystem, and that you probably need all of these things.

could you outline what dimensions should be considered and how abscissa is going to make any other (e.g. structopt, pico-args into function invocation) option non-viable

Abscissa provides the full functionality of structopt + clap, with arguably a better DSL (provided by gumdrop, which is also being used by rage). However, where structopt + clap provides one of the high-level features of Abscissa (command-line option parsing), it requires 26 dependencies, roughly half of the ~50 total Abscissa requires to provide 7 different subsystems.

I'll also note that Abscissa is being used by Zebra, a clean-room Rust implementation of a Zcash node, as well as Tendermint KMS. As such, we're able to work on common solutions to problems like how to integrate Abscissa and Tokio.

If you'd like to read more about Abscissa, here's a blog post: https://iqlusion.blog/introducing-abscissa-rust-application-framework

ebuchman

Sweet! We should try to capture the rationale for using Abscissa here in a short ADR.

light-node/src/commands/start.rs

liamsi · 2020-01-29T18:18:20Z

tendermint-lite/src/main.rs

- * trusted state and store it in the store ...
- * TODO: this should take traits ... but how to deal with the State ?
- * TODO: better name ?
- */


@ebuchman @Shivani912 @milosevic I'll move this into the lite crate in a separate PR. Note that we need to rework the spec to match the initialization via a pair of subjective (height, vals_hash). Currently, it only talks about init. through a header.

codecov-io · 2020-02-08T13:03:06Z

Codecov Report

Merging #125 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
+ Coverage   38.64%   38.71%   +0.06%     
==========================================
  Files          88       88              
  Lines        2919     2919              
  Branches      440      440              
==========================================
+ Hits         1128     1130       +2     
+ Misses       1506     1504       -2     
  Partials      285      285

Impacted Files	Coverage Δ
tendermint/src/lite/types.rs	`48.57% <100%> (ø)`	⬆️
tendermint/src/block/height.rs	`30.55% <0%> (+5.55%)`	⬆️

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 c2864b6...398be33. Read the comment docs.

- ran `abscissa new lite-node` - added deps (Cargo.toml) and minimal changes to README.md - add to root workspace

- copied from tendermint-lite/src/main.rs to lite-node/src/command/start.rs

minor improvements to comments / docs

rename some vars and more logical bisection in cmd

* 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>

ebuchman · 2020-02-12T16:21:30Z

What's needed to move this into Ready for Review? Would be awesome to get merged so future work (eg fork detection) can be based on it

liamsi · 2020-02-12T16:26:24Z

I haven't tested it against a running node. I think everything that worked in the previous main.rs is still working as before though (only we have config flags / can have config files now). I'll move this ready to review now.

liamsi · 2020-02-12T16:28:06Z

Have a bunch of merge conflicts now. Will resolve them after I resolved those in #142 .

…issify_lite_node # Conflicts: # tendermint-lite/src/main.rs

…issify_lite_node

…issify_lite_node # Conflicts: # tendermint-lite/src/main.rs

…issify_lite_node

gterzian · 2020-03-05T05:44:00Z

light-node/src/commands/start.rs

+    Ok(())
+}
+
+fn block_on<F: Future>(future: F) -> F::Output {


Have you considered not building a new runtime on each call?

You could probably do something like:

lazy_static! { pub static ref RUNTIME: Runtime = { Builder::new() .basic_scheduler() .enable_all() .build() .unwrap() }; } fn block_on<F: Future>(future: F) -> F::Output { RUNTIME.block_on(future) }

You might want to check out abscissa_tokio which allows you to launch the Tokio runtime as an Abscissa component (so you can spawn things onto it during application startup if you so desire) and boot the runtime using:

https://docs.rs/abscissa_tokio/0.5.1/abscissa_tokio/#add-tokiocomponent-to-your-abscissa-application

use crate::application::APPLICATION; impl Runnable for StartCmd { fn run(&self) { abscissa_tokio::run(&APPLICATION, async { println!("now running inside the Tokio runtime"); }); } }

It seems the current approach is to block on the future in the current thread, I assume the solution linked to above spawns the future onto Tokio asynchronously instead? It might be useful and it would seem like a change from the current approach.

I think you are right @gterzian.

Note that currently there is only one call to this particular block_on method and runtime. But yes, we need to do this properly throughout the repo. We currently copy&pasted this throughout the repo when async/await was introduced here.

Currently the rpc integration tests, above start command and the requester use this approach. Maybe worth to have a closer look into this in a separate issue, too?

@gterzian the semantics are the same, but yes it would use the Tokio executor.

You can make it synchronous by replacing block_on with await to wait for the future to complete, otherwise the semantics are the same (except with async/await, you can add async behavior in the future)

Yes I think it's worth discussing the overall approach in a separate issue(it shouldn't matter much to create a new runtime on each call for now).

Yes if we call await from inside an async context we'll get the equivalent of the current block_on, so if we run the entire application on Tokio, we could indeed use await instead of block_on, and I think the tricky part is block_on is currently not called from an async context?

Probably best to keep the current block_on, and then figure out how we want to run the light client.

…issify_lite_node

brapse · 2020-03-06T18:34:30Z

docs/architecture/adr-005-light-client-fork-detection.md

@@ -45,3 +45,47 @@ one correct full node in order to detect conflicts in a timely fashion. We keep
 this mechanism simple for now, but in the future a more advanced peer discovery
 mechanism may be utilized.

+
+#### Sequential Sync


Should this be split into it's own PR?

Sure! It was a separate PR (see #148) but can definitely be broken out again if it makes sense.

brapse · 2020-03-09T10:09:23Z

Reading through the last comments it seems like we are more or less OK with the imperfect state of this PR (most objections broken out into follow up issues). It also seems that keeping it un-merged is a liability. Unless there are concrete objections in the next day or so I'm gunna go ahead and merge this.

liamsi · 2020-03-10T13:54:53Z

Agree with @brapse here. Review and merge as soon as possible makes sense. There is probably a lot to followup on. E.g. writing and enabling some acceptance / integration tests that run this against an actual tendermint instance is a major piece that is missing, even for an MVP. But we can followup on this leveraging the outcome of #120.

…issify_lite_node # Conflicts: # tendermint-lite/src/main.rs

tarcieri · 2020-03-10T20:26:37Z

FYI, if you do end up merging #171 you'll almost certainly want to use abscissa_tokio

liamsi · 2020-03-11T09:17:07Z

Just merged #171. I'll look into abscissa_tokio and resolve merge conflicts here (I plan to do so before end of the week). I hope we can merge this PR (with its imperfections as Sean said) afterwards.

…issify_lite_node # Conflicts: # tendermint-lite/Cargo.toml # tendermint-lite/src/main.rs

liamsi · 2020-03-12T20:54:49Z

Done! @tarcieri @brapse @gterzian

gterzian · 2020-03-13T05:56:43Z

light-node/src/commands/start.rs

+impl Runnable for StartCmd {
+    /// Start the application.
+    fn run(&self) {
+        if let Err(err) = abscissa_tokio::run(&APPLICATION, async {


Perhaps add a comment here about what this does? I assume spawning a task to run the async block? What kind of runtime does it start, threaded or current thread? If threaded, how many? I'd say we just need a basic scheduler for now as in https://docs.rs/tokio/0.1.20/tokio/runtime/current_thread/index.html

If we do want to parallelize some operations, I'm not sure a threaded tokio runtime is the way to do it, we might rather want to use our own thread-pool, something like rayon, or something entirely different.

The runtime is started as an Abscissa component. You can use whatever runtime you'd like... see the From<Runtime> impl on TokioComponent:

https://docs.rs/abscissa_tokio/0.5.1/abscissa_tokio/struct.TokioComponent.html

Right now it's using the default, which is equivalent to tokio::runtime::Runtime::new():

https://github.com/interchainio/tendermint-rs/pull/125/files#diff-e4205fba6a4d881725a2a98ea6858b7aR87

By using an Abscissa component to manage the runtime, other components can spawn additional tasks on the reactor or leverage other integrations (e.g. tracing, which Zebra is using extensively)

brapse

Thanks for this. Will look forward the the next iterations of this mentioned in the comments and follow up issues 🙌

liamsi force-pushed the ismail/abscissify_lite_node branch 4 times, most recently from 7dbeca3 to 1226b4b Compare January 11, 2020 08:11

liamsi commented Jan 11, 2020

View reviewed changes

xla reviewed Jan 13, 2020

View reviewed changes

ebuchman reviewed Jan 19, 2020

View reviewed changes

light-node/src/commands/start.rs Outdated Show resolved Hide resolved

liamsi changed the title ~~Abscissify lite node~~ Abscissify light node Jan 19, 2020

liamsi commented Jan 29, 2020

View reviewed changes

liamsi added 7 commits February 8, 2020 13:06

Boilerplate: Add lite-node crate

5b4562e

- 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

73004d8

- copied from tendermint-lite/src/main.rs to lite-node/src/command/start.rs

Delete tendermint-lite: replaced by lite-node

85f2bb3

lite -> light

4ee6dfd

minor improvements to comments / docs

7cc055f

minor improvements to comments / docs

Fix a few merge hicks (catch up with latest changes from master)

02cb776

rename some vars and more logical bisection in cmd

fix rebasing hicks

5cd3724

liamsi force-pushed the ismail/abscissify_lite_node branch from 8c5a779 to 5cd3724 Compare February 8, 2020 13:13

liamsi mentioned this pull request Feb 12, 2020

Bucky/abscissify adr #148

Merged

liamsi marked this pull request as ready for review February 12, 2020 16:26

Merge remote-tracking branch 'remotes/origin/master' into ismail/absc…

8576487

…issify_lite_node # Conflicts: # tendermint-lite/src/main.rs

liamsi added 6 commits February 12, 2020 17:02

Dealing with the merging in master aftermath

d41487e

🔀🔀all these merge conflicts 🔀🔀

c2864b6

Merge remote-tracking branch 'remotes/origin/master' into ismail/absc…

398be33

…issify_lite_node

Merge remote-tracking branch 'remotes/origin/master' into ismail/absc…

2b17e28

…issify_lite_node # Conflicts: # tendermint-lite/src/main.rs

merged in master

42ef409

Merge remote-tracking branch 'remotes/origin/master' into ismail/absc…

1da4922

…issify_lite_node

liamsi mentioned this pull request Mar 4, 2020

Build broken after recent hyper upgrade #165

Closed

gterzian reviewed Mar 5, 2020

View reviewed changes

Merge remote-tracking branch 'remotes/origin/master' into ismail/absc…

9c4fd2f

…issify_lite_node

liamsi mentioned this pull request Mar 6, 2020

Run a tendermint node in CI to test the RPC & light client #120

Closed

brapse reviewed Mar 6, 2020

View reviewed changes

gterzian mentioned this pull request Mar 9, 2020

Light-client concurrency model #167

Closed

brapse mentioned this pull request Mar 9, 2020

Explore clippy rules and apply quality of life recommendations #122

Closed

5 tasks

liamsi added 2 commits March 10, 2020 16:37

Merge remote-tracking branch 'remotes/origin/master' into ismail/absc…

ba0afe4

…issify_lite_node # Conflicts: # tendermint-lite/src/main.rs

Fix merge master fallout (related to #169)

43c74a5

liamsi mentioned this pull request Mar 11, 2020

Turn lite::types::Requester into an async trait #171

Merged

5 tasks

liamsi added 2 commits March 12, 2020 20:58

Merge remote-tracking branch 'remotes/origin/master' into ismail/absc…

1d2d04f

…issify_lite_node # Conflicts: # tendermint-lite/Cargo.toml # tendermint-lite/src/main.rs

Use abscissa_tokio and resolve merge conflicts

6150a72

New stable rust -> new clippy errs -> fixed

a005ae9

gterzian reviewed Mar 13, 2020

View reviewed changes

brapse approved these changes Mar 14, 2020

View reviewed changes

brapse merged commit 471ac8f into master Mar 14, 2020

brapse deleted the ismail/abscissify_lite_node branch March 14, 2020 08:33

liamsi mentioned this pull request Apr 9, 2020

Add client queries to the relayer informalsystems/hermes#38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abscissify light node #125

Abscissify light node #125

liamsi commented Jan 11, 2020 •

edited

Loading

liamsi Jan 11, 2020 •

edited

Loading

liamsi Jan 11, 2020

liamsi Jan 11, 2020

liamsi Jan 11, 2020

liamsi Jan 11, 2020

xla left a comment

tarcieri commented Jan 14, 2020 •

edited

Loading

ebuchman left a comment •

edited

Loading

liamsi Jan 29, 2020 •

edited

Loading

codecov-io commented Feb 8, 2020 •

edited

Loading

ebuchman commented Feb 12, 2020

liamsi commented Feb 12, 2020

liamsi commented Feb 12, 2020 •

edited

Loading

gterzian Mar 5, 2020 •

edited

Loading

tarcieri Mar 5, 2020 •

edited

Loading

gterzian Mar 6, 2020

liamsi Mar 6, 2020

tarcieri Mar 6, 2020

gterzian Mar 9, 2020 •

edited

Loading

gterzian Mar 9, 2020

brapse Mar 6, 2020

liamsi Mar 6, 2020

brapse commented Mar 9, 2020 •

edited

Loading

liamsi commented Mar 10, 2020 •

edited

Loading

tarcieri commented Mar 10, 2020

liamsi commented Mar 11, 2020 •

edited

Loading

liamsi commented Mar 12, 2020 •

edited

Loading

gterzian Mar 13, 2020 •

edited

Loading

tarcieri Mar 13, 2020

brapse left a comment

Abscissify light node #125

Abscissify light node #125

Conversation

liamsi commented Jan 11, 2020 • edited Loading

liamsi Jan 11, 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

xla left a comment

Choose a reason for hiding this comment

tarcieri commented Jan 14, 2020 • edited Loading

ebuchman left a comment • edited Loading

Choose a reason for hiding this comment

liamsi Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Feb 8, 2020 • edited Loading

Codecov Report

ebuchman commented Feb 12, 2020

liamsi commented Feb 12, 2020

liamsi commented Feb 12, 2020 • edited Loading

gterzian Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

tarcieri Mar 5, 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

gterzian Mar 9, 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

brapse commented Mar 9, 2020 • edited Loading

liamsi commented Mar 10, 2020 • edited Loading

tarcieri commented Mar 10, 2020

liamsi commented Mar 11, 2020 • edited Loading

liamsi commented Mar 12, 2020 • edited Loading

gterzian Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brapse left a comment

Choose a reason for hiding this comment

liamsi commented Jan 11, 2020 •

edited

Loading

liamsi Jan 11, 2020 •

edited

Loading

tarcieri commented Jan 14, 2020 •

edited

Loading

ebuchman left a comment •

edited

Loading

liamsi Jan 29, 2020 •

edited

Loading

codecov-io commented Feb 8, 2020 •

edited

Loading

liamsi commented Feb 12, 2020 •

edited

Loading

gterzian Mar 5, 2020 •

edited

Loading

tarcieri Mar 5, 2020 •

edited

Loading

gterzian Mar 9, 2020 •

edited

Loading

brapse commented Mar 9, 2020 •

edited

Loading

liamsi commented Mar 10, 2020 •

edited

Loading

liamsi commented Mar 11, 2020 •

edited

Loading

liamsi commented Mar 12, 2020 •

edited

Loading

gterzian Mar 13, 2020 •

edited

Loading