abci: Add relaxed local client synchronization models

[thanethomson]

Relates to #88.

In this PR, the proxy.ClientCreator interface is expanded such that the client creator has more control over the concurrency model imposed from Comet's perspective for each "connection" (i.e. group of ABCI methods).

This PR also effectively renames the "unsynchronized" local client creator to the "connection-synchronized" local client creator to be more clear on its purpose.

As an example of a more advanced local client concurrency model, it introduces a "consensus-synchronized" local client creator. This only imposes synchronization (i.e. a mutex) on the consensus "connection" to ensure that Comet serializes all of its consensus-related ABCI calls, while all other connections' concurrency models are left up to the application to decide.

It also introduces a fully "unsynchronized" local client creator, which imposes no restrictions on any of the ABCI calls. This is the most advanced kind of local ABCI client creator, which requires greater care on the application side when using it.

Should be able to be reviewed commit by commit.

---

PR checklist

• Tests written/updated
• Changelog entry added in .changelog (we use unclog to manage our changelog)
• Updated relevant documentation (docs/ or spec/) and code comments

[adizere]

From what I understand, the present PR has 3 local client creators:

1. localClientCreator, which has a global mutex shared by all 4 connections (consensus, mempool, query, snapshot), meaning full serialization across all 4 connection is enforced
2. connSyncLocalClientCreator, which has one mutex per each of the 4 connections, meaning that communication is serialized per-connection; across separate connections, communication can proceed in parallel
3. consensusSyncLocalClientCreator which has a mutex guarding the consensus connection, and no serialization mechanism guarding the other 3 connections

Should we also provide a fully unsynchronized local client creator, i.e., the unsyncLocalClientCreator from old PR #9660?

From what I understood with discussions with Marko, Dev, and Luke, unsyncLocalClientCreator is likely what they'd want to use in the future.

[thanethomson]

Should we also provide a fully unsynchronized local client creator, i.e., the unsyncLocalClientCreator from old PR #9660?

The question there is, do we want to leave the concurrency model up to the application when it comes to the consensus interaction? Or do we want to ensure serialization of all consensus-related requests to the application? My sense is that we'd want to actually enforce that, but just for consensus.

I can see how parallelizing vote extension verification, for instance, could be useful. But would we want to be able to parallelize Prepare/ProcessProposal and FinalizeBlock/Commit?

[alexanderbez]

I can see how parallelizing vote extension verification, for instance, could be useful. But would we want to be able to parallelize Prepare/ProcessProposal and FinalizeBlock/Commit?

This is going to tie into optimistic execution in the SDK I believe. It's hard to imagine what this might look like if CometBFT gives us complete control, but having full concurrent calls to ABCI consensus methods is a game changer for us IMO and something we should have support for.

[thanethomson]

It's hard to imagine what this might look like if CometBFT gives us complete control, but having full concurrent calls to ABCI consensus methods is a game changer for us IMO and something we should have support for.

Since it's Comet who ultimately calls the application for consensus-related requests, it's up to us to make sure that it's safe and correct to make those calls at the right time(s), in the right sequence, and that requests don't conflict with each other, which is why it seems to make sense to me for us to control the concurrency model just for the consensus-related requests. I have no concerns around offering fully concurrent calling for mempool, query and state sync-related requests (or at least allowing the application control over the concurrency there).

@alexanderbez do you have specific ideas in mind as to how you'd like/need the consensus-related calls to be parallelized?

[adizere]

I think we should add the unsyncLocalClient and then get this merged.

The motivation for adding the fully unsynchronized local client is that this is being used in production (ref1, ref2) already. The main disadvantages: (a) more code for Comet team to maintain, (b) more risk for application builders to misuse the unsynchronized API. We can mitigate risk (b) through documentation and code comments cc @andynog. We cannot mitigate risk (a), but arguably the fact that there exist forks with this custom modification is a bigger risk for the ecosystem.

[thanethomson]

Sure thing @adizere, I've now added the unsynchronized local client creator 👍

[ValarDragon]

Yay, glad this got in!

[adizere]

SDK just mentioned they plan to have this tested before end of the year, and the change can be rolled out before the next release of SDK (because of separately versioned modules).

[thanethomson]

Should we backport this to v0.38.x?

[adizere]

Not sure. @tac0turtle do you see the value in backporting this to 0.38?

[tac0turtle]

depends on when youre next release would be? if its before eoy or even january then it should be fine for us to wait as we will try to make serverv2 compatible with exisitng chains allowing chains to migrate to that sooner then an official release

[adizere]

depends on when youre next release would be? if its before eoy or even january then it should be fine for us to wait as we will try to make serverv2 compatible with exisitng chains allowing chains to migrate to that sooner then an official release

Great! Next release will be before EOY so then no need to backport. I might ask a couple of teams additionally if they need this backported. But by default we won't do it unless there's explicit need for backporting. Thanks!