Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate and phase out broadcast mode block. #12167

Closed
greg-szabo opened this issue Jun 6, 2022 · 7 comments · Fixed by #12659
Closed

Deprecate and phase out broadcast mode block. #12167

greg-szabo opened this issue Jun 6, 2022 · 7 comments · Fixed by #12659
Assignees

Comments

@greg-szabo
Copy link
Member

As mentioned in the below issue, broadcast mode block is deprecated. It should be removed, possibly in a granular manner. I'd start with removing it from the help section so people don't start using it.
Maybe a warning should be emitted in an newer release when a user explicitly sets it.
Eventually, it should be completely removed.

This is fair, but note, broadcast mode block is deprecated and should not really be used at all.

Originally posted by @alexanderbez in #12144 (comment)

@alexanderbez
Copy link
Contributor

I would just completely remove it outright and not backport it, such that the next release only has sync and async modes.

@alexanderbez
Copy link
Contributor

@julienrbrt or @facundomedica, might be a fun task to tackle 🙌

@tac0turtle
Copy link
Member

does it matter that sync and async are the same in the code of tendermint. @cmwaters mentioned it to me

@alexanderbez
Copy link
Contributor

What do you mean the same? async just sends the transaction w/o waiting for CheckTx

@facundomedica facundomedica self-assigned this Jun 7, 2022
@cmwaters
Copy link
Contributor

cmwaters commented Jun 8, 2022

What do you mean the same? async just sends the transaction w/o waiting for CheckTx

Yeah that's what I thought until I was looking at the code recently. Seems in v0.36 async and sync were going to be the same but we've filed an issue and will hopefully fix it soon.

@cmwaters
Copy link
Contributor

cmwaters commented Jun 8, 2022

I think in v0.37 we will look to simplify broadcasting txs to a single method. It's redundant to add the asynchronous method when txs can already be submitted in parallel.

@alexanderbez
Copy link
Contributor

Agreed, It should just be sync, i..e CheckTx is executed only. This covers 99% of use-cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants