tendermint / rust-abci

A rust implementation of the ABCI protocol for tendermint core
Apache License 2.0
116 stars 34 forks source link

Rearchitecting Rust ABCI #61

Closed tomtau closed 4 years ago

tomtau commented 5 years ago

as discussed in https://github.com/tendermint/rust-abci/pull/50 https://github.com/tendermint/rust-abci/issues/31

there are 3 connections:

messages exchanges in info and mempool connections probably do not require to borrow ABCI applications as mutable, so it may be possible to rearchitect ABCI without having the mutex on the entire application (so that, for example, CheckTX/Info/Query can be processed while consensus operations are going)

ebuchman commented 4 years ago

I'm aware of this (see aforementioned DKG which uses this architecture), however speaking as someone who both ran qmail in the '90s and is running Rust-based ABCI apps using this architecture today, I'm really curious to know why. Complex multiprocess architectures are a pain in the ass to debug. I personally think it's much simpler use no more processes than are strictly necessary: simplicity to me means fewer moving pieces.

Heh. The "why" was more from a deterministic state machine perspective, where the ABCI app is hidden from the world and shouldn't engage in operations which could be non-deterministic, like network requests. But it's increasingly looking like the multi-process complexity isn't worth it and we just need ABCI applications and frameworks to make sure the deterministic state machine is properly isolated from non-determinism (eg. see the recent move of the Cosmos REST api back into the gaiad binary ...).

ebuchman commented 4 years ago

You could still provide the exact same top-level async interface in devashishdxt/abci-rs using nomic-io/abci2 under-the-hood.

@mappum is the same true in the other direction? If one direction is more natural / simpler, we should probably try to do that so we can consolidate to a single implementation that provides the interfaces folks need, even if that's both async/await and non async/await.

devashishdxt commented 4 years ago

You could still provide the exact same top-level async interface in devashishdxt/abci-rs using nomic-io/abci2 under-the-hood.

We can provide the same interface using abci2 but, under-the-hood, it'll still be using synchronous IO which defeats the whole purpose of using async/await. I'm not aware of any way to expose synchronous and asynchronous interfaces using the same API (if there is a way to do this, I'd really like to know) in Rust (yet).

ethanfrey commented 4 years ago

I read this thread a while ago, and was on the side of no async in abci apps. But it seemed like it got settled that way cuz zcash or parity does it... something like that. I generally agree with https://github.com/tendermint/rust-abci/issues/61#issuecomment-547298226 In the end, the code went async, without clear consensus .

Can someone explain why. The state-machine itself, which reads/writes to a merkle store and does math should be synchronous, right? And we just need to run three even threads, which need to keep a strict ordering of the messages they process. Even if we want to do some sort of pipelining,

If this is really needed for some sort of external compatibility and not for inherent architecture reasons, I think we should have two implementations of this level.

tarcieri commented 4 years ago

If one direction is more natural / simpler, we should probably try to do that so we can consolidate to a single implementation that provides the interfaces folks need, even if that's both async/await and non async/await.

I think there are effectively two directions you can go here:

  1. Threads w\ async wrappers: use 3(?) threads for ABCI components (i.e. a natively multithreaded model), then optionally/additively use channel-based wrappers for async interop. You could then have multiple optional dependencies/runtimes gated as Cargo features which provide an async executor and channel-based async abstractions, e.g. optional tokio or async-std features which use e.g. tokio::sync::mpsc::channel or async_std::sync::channel, or perhaps even higher level abstractions like tower::Service. This would let you spawn an ABCI app as e.g. a tokio::task which runs on an asynchronous executor/runtime, but still processes incoming events in a synchronously-behaving request/response model, where one channel is used to send requests, and the other relay the responses, and behind the scenes there's a dedicated thread which "thunks" ABCI events over these channels.
  2. Natively async which runs an executor in the background to provide synchronous behavior: I've seen some libraries do this (e.g. reqwest) and I'm not really a big fan... it's all of the baggage of an async runtime without the benefits. But it is an alternative to the above, and definitely optimizes for async users.

Of these two, I'm weakly a fan of the former: it means you can write higher-level abstractions specialized to particular executors, or use existing ones like tower. It also means any additional dependencies (tokio, tower, async-std) are purely additive on top of a base implementation that hopefully needs little more than std in terms of a runtime (which, as it were, is exactly how Cargo features are supposed to work). With this approach, you could even natively support multiple async runtimes, so as to avoid problems like "this is written with async-std, is it going to work with tokio"?

There is a finer grained middle ground: you can build the core protocol processing in separate, fine-grained request processing functions and response processing functions, then implement both a threaded blocking runtime and an async runtime in terms of the same core functions, with the former using std::io and the latter using e.g. tokio.

However, though some parts can be shared, this still involves writing a lot of the functionality twice ("WET", c'est la Rust), and can be somewhat annoying. I can't think of any popular crates that have done this successfully either offhand, or I'd give an example.

tomtau commented 4 years ago

These are good points. I think it'll be good to list some of these consequences in https://github.com/informalsystems/tendermint-rs/pull/510/files#diff-8560f0a125ba5a5011ace9b97f504ab5R41

I'll close this issue, as the "tendermint/rust-abci" will likely be archived and the future development (reflecting these changes) will continue under https://github.com/informalsystems/tendermint-rs/ (when this is finalised and merged https://github.com/informalsystems/tendermint-rs/pull/489 )