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.

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

Add ABCI crate #750

Closed
wants to merge 33 commits into from
Closed

Add ABCI crate #750

wants to merge 33 commits into from

Conversation

thanethomson
Copy link
Contributor

@thanethomson thanethomson commented Dec 16, 2020

Closes #29

📖 Rendered crate README

Trivial examples (so far) of running the same ABCI app (a simple echo app) on different runtimes:


  • Support for a simple multi-threaded ABCI app (non-async)
  • Add support for all ABCI requests/responses
  • Provide a simple testing framework for testing ABCI apps
  • Provide an async ABCI client
  • Support for async-std
  • Support for a synchronous ABCI client (non-async)

The goals above are listed in decreasing order of importance.


  • 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

@tomtau
Copy link
Contributor

tomtau commented Dec 21, 2020

fyi @devashishdxt

@devashishdxt
Copy link

Hi @thanethomson, did you take a look at #489?

@thanethomson
Copy link
Contributor Author

Hi @thanethomson, did you take a look at #489?

Yes I did, thanks! I wanted to compare your solution to my own take on the problem. I have a few significant architectural differences in mind, but needed to build them out first to be able to articulate them clearly.

As for which solution we'll end up merging, that's yet to be decided.

@codecov-io
Copy link

codecov-io commented Jan 13, 2021

Codecov Report

Merging #750 (1713d47) into master (f337b26) will increase coverage by 0.4%.
The diff coverage is 63.4%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #750     +/-   ##
========================================
+ Coverage    53.6%   54.0%   +0.4%     
========================================
  Files         197     215     +18     
  Lines       13682   14451    +769     
  Branches     3483    3671    +188     
========================================
+ Hits         7341    7812    +471     
- Misses       5942    6209    +267     
- Partials      399     430     +31     
Impacted Files Coverage Δ
abci/src/result.rs 0.0% <0.0%> (ø)
proto/src/lib.rs 68.4% <ø> (ø)
tendermint/src/abci/request/info.rs 0.0% <0.0%> (ø)
tendermint/src/abci/response/info.rs 0.0% <0.0%> (ø)
tendermint/src/error.rs 23.0% <0.0%> (-10.3%) ⬇️
abci/src/application/echo.rs 38.0% <38.0%> (ø)
tendermint/src/abci/request/echo.rs 38.8% <38.8%> (ø)
tendermint/src/abci/response/echo.rs 38.8% <38.8%> (ø)
abci/src/application.rs 44.4% <44.4%> (ø)
abci/src/runtime.rs 62.1% <51.8%> (ø)
... and 36 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 f337b26...1713d47. Read the comment docs.

@tac0turtle
Copy link
Contributor

Is there a timeline for merging this? There is a team asking when they can use tm 0.34 with a rust ABCI.

@thanethomson
Copy link
Contributor Author

Is there a timeline for merging this? There is a team asking when they can use tm 0.34 with a rust ABCI.

I'm still working on it at the moment. It's a little uncertain right now due to some Rust-related technicalities.

What're their requirements? e.g. do they want a sync/async interface? If async, which runtime(s) do they want support for? That way I can prioritize the features accordingly and possibly get something workable out sooner.

@tac0turtle
Copy link
Contributor

They do not have a specific need for one runtime over another. They are building out a minimal app for their needs, but it is unclear which interface they need (sync/async interface)

};

/// Implemented by each runtime we support.
pub trait Runtime: 'static {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, why we don't have a crate for this yet🤔️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically this Runtime trait's somewhat crate-specific right now. But when GATs land in Rust stable I should be able to make it more generic and maybe extract a crate from it. Later today I'll hopefully be working on the non-async interface for the Runtime trait, which will effectively make this crate usable with just std as well.

Do you perhaps know of any other libraries that offer this kind of functionality? I've tried looking, but can't find any, but maybe I'm just not searching for the right terms.

Copy link
Contributor

@yihuang yihuang Jan 24, 2021

Choose a reason for hiding this comment

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

There's async_executors which has similar purpose but it don't cover networking apis.
For networking stuff, one path might worth explore is, make a generic Async, and build networking abstractions(TcpStream) on top of it. But that means much more work than what you already have here.
I ported Async to tokio before, only proof-of-concept stuff though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's hard to find something that'll have the same effect as what I'm looking for right now with this Runtime trait.

I think it's actually due to shortcomings in the Rust language in being able to express certain types of generic structures, but again, I'm hoping those shortcomings will be addressed once GATs are available on stable.

I built a little proof-of-concept using GATs for creating MPSC channel abstractions with Rust nightly and it seems to work. This kind of language feature should easily allow for the creation of these kinds of common abstractions over different runtimes.

@devashishdxt
Copy link

FYI, I've pushed changes with support for both, sync and async, APIs for building ABCI server in abci-rs. Here's a link to its documentation.

@thanethomson thanethomson mentioned this pull request Jan 28, 2021
5 tasks
@thanethomson
Copy link
Contributor Author

FYI, I've pushed changes with support for both, sync and async, APIs for building ABCI server in abci-rs. Here's a link to its documentation.

Thanks for letting us know @devashishdxt! I wanted to find out from you, what's the rationale behind having separate traits for each of the connections in abci-rs?

@devashishdxt
Copy link

I wanted to find out from you, what's the rationale behind having separate traits for each of the connections in abci-rs?

As mentioned in the ABCI documentation, tendermint creates 4 separate connections and each connection sends different types of requests. The advantage of having 4 different traits is that we can execute each tendermint connection in their respective async task (similar to sync threads). So, for example, once tasks for each connection have started, the code will not allow non-consensus requests on consensus connection and will return an error in case it receives a non-consensus request.

@thanethomson
Copy link
Contributor Author

The advantage of having 4 different traits is that we can execute each tendermint connection in their respective async task (similar to sync threads).

Interesting idea. I'm still not sure of the benefits of the approach given the additional complexity.

My personal preference is a handle/driver architecture, such as in this kvstore example app, which allows me to more easily reason about the application as a deterministic state machine (where all application state is encapsulated in a single thread, but accessible from multiple networking threads).

@devashishdxt
Copy link

Interesting idea. I'm still not sure of the benefits of the approach given the additional complexity.

My personal preference is a handle/driver architecture, such as in this kvstore example app, which allows me to more easily reason about the application as a deterministic state machine.

I agree that having one trait to implement all the ABCI functionality is much more simpler. But, having 4 different traits makes the ABCI server implementation more accurate as compared to ABCI specification. In addition to this, it is also more robust against buggy tendermint implementation as it'll be able to find out deviations from specifications.

where all application state is encapsulated in a single thread, but accessible from multiple networking threads

I'm not sure what you mean by this.

@tony-iqlusion
Copy link
Collaborator

It seems like if you you should be able to mix and match these strategies, e.g. having 4 traits but one type that impls them if you so desire

@devashishdxt
Copy link

It seems like if you you should be able to mix and match these strategies, e.g. having 4 traits but one type that impls them if you so desire

Agreed. We can also create a super-trait, which, when implemented, auto implements all 4 traits.

@thanethomson
Copy link
Contributor Author

Closing in favour of #794

I'll keep this branch for now to keep track of the Runtime trait-based approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Absorb rust-abci
7 participants