-
Notifications
You must be signed in to change notification settings - Fork 226
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
Absorb rust-abci #29
Comments
that could be good for unifying some of the amino/protobuf types |
seems like a good idea - we should add something about this to the ADR-001: #28 |
It feels like pulling ABCI into this repo would just complicate things a bit. Unless this repo is slowly moving from just a client to a full blown tendermint core implementation...which would be really cool btw... |
Yeah, it can be a bit complicated at the moment, as the protobuf generated definitions versus the nice hand-written RPC types don't quite match up |
For what it's worth, I'd prefer generated RPC types for use in the KMS. I think the current handwritten ones are a bit overcomplicated, and using protos means they won't diverge. |
I think that could work -- one would need to modify https://github.com/tendermint/rust-abci/blob/develop/build.rs so that the generated ABCI types derive serde deserialisers etc. One thing to consider would be separating out the ABCI types into a dedicated |
Ideally we can use proto as the canonical definition for all the types, and re-use them both for ABCI and the RPC. This won't 100% work for everything yet as there are still some things that depend on amino encoding with prefixes in the RPC (namely pubkeys, but I think that's it). But we plan to transition even amino interfaces to proper proto3 wellknowntype Any soon, so this should be fully possible. Definitely something to work towards.
That's the plan! |
Would like to revisit this issue. Should a new ADR be written or an amendment be made to adr-001? |
@marbar3778 an adr or even a draft PR instead (or in parallel with that adr) would be awesome. |
Can we clarify the relationship between rust-abci and abci-rs? Has the former absorbed the latter in some way? There's also Nomic's abci2. My understanding is that it's not using async/await, but unlike the other impls, it does allow abci methods to return asynchronously, which would allow concurrent processing at the app layer, which is something we ultimately would want. This use case might also be taken care of by introducing a DeliverBlock instead, which would just ship all the txs at once, so the app could still process them concurrently (contemplated, and proposed in ABCIx, but not currently in tendermint) . If we don't have DeliverBlock, the design space seems to be mostly around whether we use async/await Rust, and whether the ABCI responses can be received asynchronously. If I understand correct, we basically have the following matrix now:
Ideally we'd be able to have one definitive abci crate we could all use, built up in layers to potentially satisfy all quadrants of this matrix as necessary. That said, I'm not sure there's a real use case for Synchronous ABCI, since Tendermint is already sending DeliverTx messages asynchronously in order (it doesnt wait to receive the result of one before sending the next). So it actually seems more like getting the right half of the matrix. Then its seems there's two options:
It's not clear to me if (2) would also require a non-async wrapper or if users should just be responsible for calling some block_on themselves if they want synchronous behaviour. cc @mappum here as well. I basically asked the same thing in tendermint/rust-abci#61 (comment) |
On a related note, if it may take some time to still figure this out, we could start by just merging the abci proto work in here, while we figure out how to unify abci-rs and abci2. Then at least we would maintain the abci protos here which are the primary thing that need updating and external crates could experiment with connection semantics without having to worry about the protos. Downside of this might be difficulty of testing without connection code, so we may have to depend on one of the connection implementations anyways |
rust-abci was using tokio 0.1. In an unreleased new version (it failed to release due to @zramsay's expired crates.io token #489 (comment) ) with dependency upgrades: tendermint/rust-abci@af2f34a -- it partially moved to tokio 0.2 and async/await internally.
I don't think that understanding is correct (or it depends on what is meant by "Synchronous ABCI") -- in both "rust-abci" and "abci-rs" requests and replies are still read/written asynchronously ("Synchronous ABCI" was "rust-abci" prior to version 0.5.3 -- which is when the discussion started); "abci2" uses synchronous I/O (within a single connection, it'll block -- it can probably be fixed by setting The high-level differences are:
I suggested "abci-rs" for absorption, because it's more feature-complete (e.g. handling of unix domain sockets or logical checks that can prevent runtime gotchas) and has more tests. Using traits can reduce some boilerplate, e.g. a default implementation for In the case of tendermint-rs, the ideal situation will be that the ABCI crate user will be able to seamlessly switch from "ABCI server" (with Tendermint implemented in Go talking to it) to the fully fledged Tendermint implementation in Rust (once ready; compiled to a single binary, just like Go applications can do with Tendermint now) -- with that goal in mind and tendermint-rs architecture plans, you can comment on #510 what makes most sense. |
Ok thanks. I'm off grid for the next few days but will follow up next week. |
@ebuchman I think this is the way to go. Can't we expose the external ABCI interface as before, then combine them in an |
Here "synchronous ABCI" means not being able to read multiple incoming ABCI messages before responding. while let Ok(request) = decode(&mut stream).await {
match request {
Some(request) => {
let response = inner.process(request).await;
if let Err(err) = encode(response, &mut stream).await {
error!(message = "Error while writing to stream", %err, ?peer_addr);
}
}
None => debug!(message = "Received empty request", ?peer_addr),
}
} The while loop cannot move to the next iteration and call The async IO doesn't really improve anything here, except that we could choose to use a single thread rather than 3 (4 with state-sync) and save a small amount of memory (at the cost of giving up parallelism for the encoding/decoding work). Actually, in this case async is hurting because each response gets a
I want to point out that concurrency for CheckTx is also important (e.g. maintaining N parallel mempools and doing as much tx validation work as early as possible across all CPU cores), and this doesn't address that. |
If I'm understanding correctly, these costs are both due to the use of |
ok, and the current pending abci-rs still has this: https://github.com/informalsystems/tendermint-rs/pull/489/files#diff-70126217abd1cb67e8ff8d0aef5825f4R146 One may be able to eliminate that, but as the responses need to be written in order, one will still want to have a communication queue (e.g. mpsc channel from the standard library) in there and separate out the response writing.
It'll be good to measure that and expand on that in decision consequences in: https://github.com/informalsystems/tendermint-rs/pull/510/files#diff-8560f0a125ba5a5011ace9b97f504ab5R50 There are a lot of moving pieces though, e.g. a particular runtime / its configuration and async-trait native support. IMHO the performance of the current abci server implementation is a bit of a secondary concern:
As I mentioned above, it depends on the tendermint-rs architecture plans -- if the "async-trait"-based applications fit well with that, I'm not sure -- #510 will ideally reflect / comment on that |
Maybe good to try some zero-cost async trait solution, like this and this |
So what do we think about using abci2 (non-async-await rust, asynchronous abci) as the core and layering an async-await interface and other application types (like from abci-rs) on top of it? Mentioned in #510 (comment) and I think this is basically what Tony is describing as option 1 in tendermint/rust-abci#61 (comment). I think this is also what tungstenite does (websockets lib) with the async-tungstenite wrapper |
It should be ok. coming back to
so it doesn't matter that much whether the abci server is based off std network stack like abci2 or tokio... what matters more is how well the end-user traits will fit with the rest of tendermint-rs |
Right, though it will be some time before we have a full node in Rust (probably minimum 12-18 months). I'm going to bump the milestone on this as we're currently focused on IBC and getting a release out that's compatible with tendermint v0.34. |
I've updated the code to concurrently execute |
Discuss absorbing the rust-abci repo as its own crate within this repo.
The text was updated successfully, but these errors were encountered: