Allow for async broadcast mode
Closed this issue · 4 comments
We should support all three tx broadcast modes - Block, Sync and Async.
Currently we only support Block mode in SyncBroadcastMsg and Sync mode in QueueBroadcastMsg.
For consistency, I propose we rename these to
BlockBroadcastMsgSyncBroadcastMsgAsyncBroadcastMsg
FYI enabling async mode is easy enough - just one flag to enable here
ee48e12#diff-14d1fe1d4854b5e6a2d53344bc5b613f414c352c1f002899da69ff912bbe1a54R349 (this is in a non-merged separate branch). Obviously should be split/refactored to a proper separate implementation however, as this current one just hijacks the QueueBroadcastMsg to be all async
The methods are named according to user perspective, instead of making a leaking abstraction over cosmos names.
What we want from SyncBroadcastMsg is to block until Tx is accepted or rejected with return value.
What we want from QueueBroadcastMsg is to enqueue the Tx in some form of a queue, where messages are effectively batched into transactions. We don't care about return values here.
These functions cannot be misused, for example if you call them 10 times in parallel, there won't be a nonce issue. They are thread safe.
Exposing low-level interface just opens possibilities for confusion and errors. Every single developer will use AsyncBroadcastMsg and will run into nonce issues. Because Cosmos SDK is hard to understand and code with, we make own SDK to provide user-friendly methods, not just aliases to Cosmos low-level types (actually, also colliding with Tendermint's)
So instead of discussing what should be done (let's rename everything) or how it should be done (let's change a flag), we should discuss "why" rationale there.
So my question is, why you would need AsyncBroadcastMsg and which is a use case? After this is settled, I can decide how to extend current API to include the option for that flow too.
It seems that Block mode is discouraged from use as well as it waits for tx on node's side and can exhaust the resources and hang the stuff up. So instead the chain-api client process should be subscribed to events websocket to get Tx confirmations from there. This adds additional layer of problems here, since the client itself is stateless, so events of confirmed Txns should be either polled, or be injected into event queue from outside. I think polling is fine.
This patch 97aa0a4 addresses the issues described above + bug fixes for the Queue method, tested with the new stresser.
I've also added AsyncBroadcastMsg, but keep in mind that it is not related to Tendermint's broadcast mode and the purpose of this method is to allow an external scheduler to wait for Tx, instead of internal wait loop as in the new SyncBroadcastMsg.
