Cosmos-sdk: Add `--async` option to all send commands

[cwgoes]

Inspired by https://github.com/cosmos/cosmos-sdk/commit/c87dc16e568645a734717f29af0a0790206c3609, cc @ValarDragon.

Without --async, send commands should behave as they do now (waiting for transaction inclusion). With --async, they should instead immediately return the transaction hash (which can be queried through other means).

Additionally, gaiacli should cache the updated account sequence, even if the transaction hasn't yet been committed, for future sends.

[davekaj]

I could take a stab at this. from looking at c87dc16 , a lot of the functionality is already implemented.

on first look it seems like all that needs to be done is adding --async flag to gaiacli, having that tied into the BroadcastTxAsync() from dev/spam_tx, grabbing the txHash from BroadcastTxAsync() through res.Hash and return the txhash to gaiacli . And then also adding gaiacli to cache the updated sequence accounts.

if you haven't started on it, i'd be glad to do it! @ValarDragon

[ValarDragon]

The issue with my commit was it wasn't working as intended. The testnet went down so I couldn't continue working on it, but if you want to take a look at it go for it! (I don't think I'll get time for the next two days)

Here's the intended functionality: adding an --async flag will cause a command (such as send) to not wait for a block to commit. I want to be able to have multiple sends from the same account in the same block. Here is what I was using to test this:
https://gist.github.com/ValarDragon/4299f474ecd9a0f3fb2c909efd208ca1
This worked with send with normal sends, where each send waited for the block to commit. (So it would add 1 tx per block) swapping out for spam on my branch, nothing was getting sent. By the time I got time to debug, the testnet had a consensus failure lol.

[davekaj]

okay cool, yeah I'll start on it tomorrow. i might need to get my hands on some of those test tokens like "devtoken" to be able to send through a lot of tx's. judging from the genesis file there are a lot of people with test tokens so im sure i will be able to get some

[ValarDragon]

Cool! Whats your address, I'll send you a bunch of devToken

[davekaj]

cosmosaccaddr19v5tphwgule2sy59yltzatu7cpdyc7rx2hv576

[ValarDragon]

Sent!

[davekaj]

made a bit of progress, got a script to continually call spam and it worked, sent 50 tokens back and forth between two accounts, asynchonously with multiple tx's on a single block

tomorrow I will add the --async flag to gaiacli send, get the tx hash to return, and also have gaiacli store the sequence instead of having the script update it .

then you can take a look at it and we'll go from there!

[ValarDragon]

Thats awesome!

[davekaj]

A couple of questions I've came across while trying to implement:

1) The async flag is working with tx hash returned. Would it be helpful to pass it an int to represent how many txs need to be sent? (i.e. --async=5, would send off 5 async txs). I think this would make it easy for the cli to cache the incrementing sequence numbers.

2) If you do not agree with 1), do you have any other suggested ways to cache the sequence in the cli? I was thinking of maybe querying the checktx state , but then it isn't asynchronous anymore so I don't think this helps.

3) I will add the async flag to the send cli function gaia, basecoin, and democoin. Any other places it should be added?

@ValarDragon

[cwgoes]

@davekaj Phenomenal! Answering on behalf of @ValarDragon and myself:

1. Could be useful, but I'm not quite sure what you mean - send the same transaction five times, with sequence numbers incremented each time? We're planning to implement multi-Msg transactions prelaunch - https://github.com/cosmos/cosmos-sdk/pull/1266 - so I'm not quite sure what the use case for that would be (other than transaction throughput testing); did you have one in mind?
2. The CLI will need to cache sequence numbers on the local filesystem, probably in a database in ~/.gaiacli. Check out https://github.com/tendermint/tmlibs for a simple DB implementation.
3. We're working on better abstracting the CLI functionality so that flags such as --async would work for every command (and only need to be written once), but those sound good for now.

[ValarDragon]

I don't think we need to include a number of transactions to send off in the golang code, since that will only be used in testing. I think that makes more sense to do for the testnets via a python script.

[davekaj]

1) the only usecase I had was for sending off multiple txs, but @ValarDragon is right it should just be done with a python script.
2) okay I'll start working on trying to implement that today
3) sounds good

[davekaj]

Hey @cwgoes @ValarDragon

I implemented a sequence database that is stored locally. Here's how it works

• upon creation of a new key with gaiacli keys add, it will instantiate the DB and add in the new name of the account, with sequence 0
• it will always update whenever a new tx or async tx is sent
• if the sequence is off from the actual sequence stored in tendermint, it will query tendermint and get it, and get them in sync

Check it out here : https://github.com/cosmos/cosmos-sdk/compare/develop...davekaj:davekaj/span-tx2

It still needs some testing, and some polishing up, but before I do that I was thinking, would it be at all useful to add the sequence info to just be stored in the keys database?

right now keys are stored at .gaiacli/keys/keys.db' and in my updated version, sequences are stored at.gaiacli/sequence/sequence.db`

it if were to be done, i think it would involve editing https://github.com/tendermint/go-crypto/blob/master/keys/types.go and https://github.com/tendermint/go-crypto/blob/master/keys/keybase.go

Please let me know if the current way i've implemented it is good, and I will continue to polish and test and make a PR. Or let me know if sequences should be stored locally in the keys.db so that we have 1 db instead of 2

[cwgoes]

It still needs some testing, and some polishing up, but before I do that I was thinking, would it be at all useful to add the sequence info to just be stored in the keys database?

Yes. We are merging the keybase into the SDK anyways - https://github.com/tendermint/go-crypto/issues/143 - at which point it will be easy to do. Hope to have that merge completed later today.

The rest of your proposal sounds good to me.

[davekaj]

awesome i will wait for the keybase to be merged before I make my PR!

[ValarDragon]

can you go ahead and make the PR, just so we can start reviewing it? Don't worry about squashing the commit, we can do that later

[davekaj]

sure no problem, ill make the PR today. it might be a bit weird because I will be editing go-crypto keybase in vendor deps, and I can't push that in the PR. maybe I will make a branch off go-crypto and provide a link just so you can see what it will look like when the keybase is fully inside SDK

also I was testing it agaisnt the testnet, but at this point tendermint and the sdk have breaking changes with the testnet if I use the up to date develop branches. but I know it works on gaia-6002, so I work with the most up to date develop branches, and just write a test to test async

[ValarDragon]

Oh sorry, didn't realize you had to edit go-cryptp code. Don't worry about making a PR then if its a bunch of extra work. Thanks for doing this :)

[davekaj]

okay I will wait until the keybase is merged into the sdk, it will make the whole process a lot simpler

[ebuchman]

add the sequence info to just be stored in the keys database

Let's not do this - I don't think it's the right place. That's stateful information that pertains specifically to the latest state of the application, it doesn't really belong in the keybase, which is just a place to store keys.

Maybe you can convince me otherwise but seems like that would be unecessarily confounding concerns.

Ideally this change can be made without messing about in the crypto pkg

[cwgoes]

Let's not do this - I don't think it's the right place. That's stateful information that pertains specifically to the latest state of the application, it doesn't really belong in the keybase, which is just a place to store keys.

Do you think we should store it locally somewhere? In a separate database?

We were planning to merge the keybase API into the SDK anyways, since that requires Ledger and we didn't want to require CGO in Tendermint. Keys & sequence information could still be stored separately though.

[ebuchman]

We were planning to merge the keybase API into the SDK anyways

Doesn't mean we should sacrifice good modularity.

As for storing nonces, I think we should consider persisting "wallet data" which can include things like transaction history and nonces, but I wouldn't lump that in with keybase. In any case I don't think it's priority for now since we can fetch it over the network.

[davekaj]

Sounds like sequence storage will be put on hold until after mainnet launch

I can still create a PR for adding the async flag to the cli (the work is already done). this would still allow a user to write a script with --sequence to send many async tx's in a row, and multiple in a block

Let me know if I should do that, or if I should just put this all on hold @cwgoes

[ValarDragon]

Please make that PR! The async option is something that will be awesome to have, and I want to update my script to use it on the next testnet!

[cwgoes]

Let me know if I should do that, or if I should just put this all on hold @cwgoes

Please do; that would still be much appreciated.

[davekaj]

@ValarDragon @cwgoes PR created , once reviewed I will add in the same flag to democoin and basecoin and docs and the it can be merged