Stub: Create MockTendermint

[zbuc]

Writing full integration tests is difficult, and our existing tests making use of MockClient (see swap_and_swap_claim.rs) are Not Great (they replicate pieces of application code within the test and do not actually execute the application behavior as it is executed in a real deployment).

It would instead be better if we had a MockTendermint that kept track of some internal state and had the ability to execute a series of transactions via faked ABCI messages sent across some transport (calling against the App::* methods directly? A sidecar process calling via the local network?)

More details/design to follow

[conorsch]

More details/design to follow

Suggestion for Testnet 57: hold a design meeting and capture the requirements here. That's necessary for us to implement this meaningfully. We want better tests! And this ask is a core part of that push. There are external contributors we should engage to discuss spec here.

[conorsch]

Perhaps something like https://github.com/informalsystems/CometMock would be helpful. Let's take a spike and see if that tool works with our existing smoke test.

[conorsch]

Took a look at adding websocket support to TendermintProxy, because cometmock only supports websocket calls on its tm api service. With an assist from @plaidfinch, that refactor seems to have landed. However, it appears cometmock isn't able to communicate with our abci service; logs from tower-abci say:

thread 'tokio-runtime-worker' panicked at 'called Result::unwrap()on anErr value: Custom { kind: Other, error: "bytes remaining on stream" }', /home/conor/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tower-abci-0.8.0/src/v034/server.rs:145:70

So next I'll dig in over there to add more logging knobs and see if I can't sort out what about the abci is broken.

[erwanor]

This is a shot in the dark, but the tendermint socket protocol has changed between v034 and v037, so I think that means you must use perform_v034 with the websocket client, rather than perform.

[erwanor]

Took a look at adding websocket support to TendermintProxy, because cometmock only supports websocket calls on its tm api service. With an assist from @plaidfinch, that refactor seems to have landed. However, it appears cometmock isn't able to communicate with our abci service; logs from tower-abci say:

thread 'tokio-runtime-worker' panicked at 'called Result::unwrap()on anErr value: Custom { kind: Other, error: "bytes remaining on stream" }', /home/conor/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tower-abci-0.8.0/src/v034/server.rs:145:70

So next I'll dig in over there to add more logging knobs and see if I can't sort out what about the abci is broken.

I have had time to peek at the code, and I have noticed two things: first, cometmock does in fact support JSONRPC over http (see: here). It also has a websocket endpoint, but that's also the case for cometbft, there's nothing special about this. The second, more important point, is that ABCI connections are performed using gRPC (see here) wherease tower-abci uses the recommended length prefixed socket protocol (spec).

Instead of making tower-abci support gRPC ABCI connections, we should work with the maintainer of CometMock to support the TSP. Specifying the correct compatibility mode when forwarding proxied requests to tendermint will also be necessary, but that's not the current problem. The issue here is that pd (via tower-abci) and the mocked cometbft server are speaking different wire protocols across the ABCI boundary.

[hdevalence]

What's the downside to adding gRPC support to tower-abci?

We could have a second Server implementation that translates Tonic-generated types into domain types and invokes the inner tower::Services, and wouldn't need to jump through the hoops the TSP server does to translate into requests/responses.

We wouldn't have to wait on any upstream, and it would give us optionality to migrate to grpc communication with no significant changes to pd.

[erwanor]

What's the downside to adding gRPC support to tower-abci?

We could have a second Server implementation that translates Tonic-generated types into domain types and invokes the inner tower::Services, and wouldn't need to jump through the hoops the TSP server does to translate into requests/responses.

We wouldn't have to wait on any upstream, and it would give us optionality to migrate to grpc communication with no significant changes to pd.

That's a good idea, i suggested supporting TSP in cometmock because it looked the easiest since it would just require adding an option that calls the cometbft supplied TSP methods. Supporting gRPC in tower-abci sounds good though, but I think we would also need to switch over to gRPC from TSP in pd, so that it can communicate with Cometmock

[hdevalence]

Ideally we should not switch over pd, but just instantiate the services in a test binary that uses the pd crate as a library.

[erwanor]

@hdevalence Note that since tendermint-proto does not include the ABCI gRPC service definitions, we will have to define them within tower-abci so that we can use it with tonic::transport. That's not great, but the ABCI service is relatively small and self-contained. Alternatively, we could have tendermint-proto include the definitions under a "grpc" feature, which is better but requires upstream changes.

[erwanor]

@conorsch FYI, I was able to confirm the hypothesis laid out in my previous comment. I quickly hacked a binary of mock cometbft that uses the go gRPC binding for the Echo request and tower-abci made progress.

[erwanor]

Submitted informalsystems/tendermint-rs#1338 for inclusion in tendermint-rs

[conorsch]

Beautiful. Thanks for driving this forward, @erwanor. Now that we're waiting for upstream, is there any immediate progress we can make on this issue? I'm inclined to check with upstream weekly to encourage review, and in the meantime, switch to other tasks. Seems like a good time to make progress on #2263 so we're ready for 037 in the near future.

[erwanor]

@conorsch we need the server definitions to be merged into upstream before we can cut a release of tower-abci that includes gRPC support, but we can concurrently start work on the implementation, which is what i'm going to do. Also, the maintainer of cometmock kindly added support for the socket protocol so we should test that it works and maybe use that in the meantime.

In terms of progress that could be made here, sketching out what the integration testing with cometmock looks like is a big task. Henry mentioned having a dedicated binary that plugs into pd as a library. Maybe this is something @zbuc and you want to flesh out potentially.

[conorsch]

the maintainer of cometmock kindly added support for the socket protocol so we should test that it works and maybe use that in the meantime.

Fantastic! I'll try this and report back.

. Henry mentioned having a dedicated binary that plugs into pd as a library. Maybe this is something @zbuc and you want to flesh out potentially.

Sounds good, I've got some time with @zbuc next week and we'll discuss plans here together.

[conorsch]

Haven't made much headway on getting CometMock to work with our ABCI service, even with the addition of socket support. On the first connection over ABCI, receiving InitChain, our tower-abci service fails with invalid varint, an error which appears to be coming from prost, and denotes having received zero-byte message from the ABCI client. Notably this is different from the error we saw when trying to use grpc, which was ""bytes remaining on stream".

The CometMock project is trying to maintain compatibility with both v0.34.x and v0.37.x versions of Tendermint. Unfortunately, the 0.34.x branch no longer builds. We already plan to upgrade to 0.37 soon, but aren't quite ready to do so yet. Next, I plan to reach out to the maintainer of CometMock, and have a synchronous chat to talk through some of the wire-format trouble I'm having.

[conorsch]

Met with the maintainer of CometMock today, who's going to take a look at our WIP branch and advise on comparable TSP support. We're optimistic we can unblock ABCI communication over sockets, which is the first step in our testing spike. Thereafter, we're eager to explore both the time-travel and forced-misbehavior APIs, which would unblock testing like #1209. However, I also learned that CM by default places every tx in its own block, disregarding the mempool, and ignoring CheckTx handlers. I'm not sure what the implications of that are for our testing needs, but we have a willing collaborator, and we'll tackle that challenge if and when it blocks us.

[conorsch]

The reason for the invalid varint empty bytes stream was a 500ms timeout on the cometmock side, which wasn't nearly enough time to get a response back from abci. Bumping that timeout to 20s [sic] resolves, and I'm able to run out smoke test suite using cometmock as a stand-in for tendermint. That's rather tremendous. Next, I'll try to stub out a small driver crate, so we can talk to cometmock api endpoints from within the tests.

[conorsch]

Providing some context here, so that we can close out this issue in favor of a newly specified ask.

We spent some time with CometMock, which has a lot of nice features:

• drop-in replacement for cometbft, so we can plug it into our integration tests
• custom API for meta events, like "fast forward n minutes" or "commit double signing violation"

However, it also falls short of the original ask here:

• using a mock client in the integration tests reduces the utility of the integration tests, because they're no longer exercising production code
• even with the "fast-forward" knobs, we're still paying a lot of wall time overhead sending network packets back and forth

Ideally, we want a solution that can be integration into cargo test and have state machine checks run in memory, rather than across a network boundary. I'll work with @cratelyn on a new issue spec that she can use to kickstart this effort again.