Add minimal blocking ABCI crate

[thanethomson]

Closes #29

In order to reduce the amount of code for review, I've extracted portions of #750 and expanded on them, but purely for the blocking case (using the Rust standard library's networking functionality). I've now included the full range of ABCI requests/responses, but purely using their Protobuf representations (also to reduce the sheer quantity of code). Now that the investigative work in #750 has been done, adding async compatibility should be a relatively simple (but probably tedious) task - TBD in separate PRs.

There's a minimal kvstore-rs application that can be built into a binary, and the basic functionality works with a fresh Tendermint node. See the ABCI crate README for details:

📖 README rendered

• 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

[romac]

Great stuff, very clean and minimal, <3 the test suite :)

Left a few minor comments but otherwise it looks really good to me!

[romac]

Can we hold on to the original error here rather than its string representation?

[thanethomson]

I might be able to do that with eyre - I'll try it out 👍 Originally I had trouble with this because SendError is generic over T, which would imply that the error needs to be generic over T too. But if the error can be wrapped as a Box<dyn std::error::Error> this shouldn't be a problem, I think.

[romac]

Oh I see. Since it's just an example app it does not matter too much, just a nice-to-have (probably not worth it if we have to pull in eyre).

[thanethomson]

Here's the conundrum with eyre, as per https://docs.rs/eyre/0.6.5/eyre/struct.Report.html:

Report requires that the error is Send, Sync, and 'static.

This would imply that T must be Send, Sync and 'static too. In this case I'm trying to send a variant of the Command enum:

enum Command {
    /// Get the height of the last commit.
    GetInfo { result_tx: Sender<(i64, Vec<u8>)> },
    /// Get the key associated with `key`.
    Get {
        key: String,
        result_tx: Sender<(i64, Option<String>)>,
    },
    /// Set the value of `key` to to `value`.
    Set {
        key: String,
        value: String,
        result_tx: Sender<Option<String>>,
    },
    /// Commit the current state of the application, which involves recomputing
    /// the application's hash.
    Commit { result_tx: Sender<(i64, Vec<u8>)> },
}

std::sync::mpsc::Sender is !Sync, and I ideally need to be able to send it across the channel in order to get a dedicated response asynchronously.

Perhaps this is a hint that a better overall design is possible? I suppose falling back to Arc<Mutex<...>> instead of the handle/driver architecture would solve this. Or simply "flattening" std::sync::mpsc::SendError<T>, like I'm doing in the original code here. I can't think why the SendError would need to be generic over T in the first place?

[romac]

I think the current solution and architecture are fine.

Just wondering, have you tried just casting SendError to Box<dyn std::error::Error> and wrapping that one in ChannelSend (without using eyre at all)?

---

Regarding SendError being generic over T: that's because it's defined as

pub struct SendError<T>(pub T);

which allows you to get back the value that failed to be sent.

[thanethomson]

Just wondering, have you tried just casting SendError to Box and wrapping that one in ChannelSend (without using eyre at all)?

T would still need to implement Sync, afaik 😞

[xla]

Could you expand why the default pattern in here is preferred over not having a default implementation for these methods?

[thanethomson]

I was modelling it on what the BaseApplication does in the Go version: https://github.com/tendermint/tendermint/blob/master/abci/types/application.go#L34

[melekes]

👍

[melekes]

Could you explain why you've chosen this concurrency model (communication via channels) over something like a mutex & hashmap? As someone who's learning Rust it will be useful to me.

[thanethomson]

This may be a personal preference/readability thing 😁 I'd say there are pros and cons to each approach. I've always tended to prefer an actor-style model for managing state in concurrent systems (explicit-lock-free state encapsulation with message passing and pattern matching on messages), which I've tried to implement here in a lightweight way. It's easier for me to reason about such state machines in general (like this).

When it comes to contention, I'd imagine using mutexes would push bottlenecks down the stack towards the networking layer, outside of the app's direct control. With the actor-based model it'd be able to leverage memory (in the unbounded channel) to avoid pushing the bottleneck towards the networking layer. On more constrained devices, this channel-based approach wouldn't provide any benefit for the additional complexity cost, so mutexes would probably be the way to go.

Practically both models would probably work the same for this specific application, although I'm not sure which one would provide better performance (I'd have to benchmark to find out). I'd imagine that the mutex-based approach would probably provide better performance, speed-wise, as long as the app can keep up with incoming requests.

[brapse]

Good stuff & discussion 👍

[tomtau]

I don't have admin rights on https://github.com/tendermint/rust-abci but after the ABCI crate here gets published, perhaps it'll be good to archive the old repo and point to here?
@xla and @liamsi should have rights on https://crates.io/crates/abci

[tomtau]

Suggested change

	name = "tendermint-abci"
	name = "abci"

it can be called just this and published as a new breaking version of https://crates.io/crates/abci -- @xla @liamsi can help with that