abci-cli: Add process_proposal command to abci-cli
#8901
Conversation
hvanz
requested review from
ebuchman,
cmwaters,
tychoish,
williambanfield,
creachadair,
sergio-mena,
jmalicevic,
thanethomson and
ancazamfir
as code owners
abci/cmd/abci-cli/abci-cli.go
Outdated
| @@ -682,6 +700,38 @@ func cmdPrepareProposal(cmd *cobra.Command, args []string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func cmdProcessProposal(cmd *cobra.Command, args []string) error { | |||
| if len(args) == 0 { | |||
There was a problem hiding this comment.
If the app accepts empty blocks, would we want to do this?
There was a problem hiding this comment.
Both PrepareProposal and ProcessProposal in kvstore accept empty blocks (that is, requests with the field Txs having an empty array). PrepareProposal will just return an empty array in the field TxRecords, and ProcessProposal will return "accept". But empty blocks will never reach the application through abci-cli, because abci-cli does not accept an empty list of arguments for these two types of requests. So to keep it consistent with the application, probably the if len(args) == 0 ... block should be removed, for both commands.
There was a problem hiding this comment.
My initial question was motivated by this thought:
"Would it be good for abci-cli to accept ProcessProposal without arguments to denote an empty block?"
There was a problem hiding this comment.
If it is correct behaviour for TM to accept empty blocks then Hernan is right, we should remove this check from both commands unless we think that for the kvstore app it does not make sense. I don't think it matters much, but if ProcessProposal accepts empty blocks, then PrepareProposal should too. We should keep the two consistent.
There was a problem hiding this comment.
So what is the correct behaviour? Does Tendermint accept empty blocks? And the applications?
There was a problem hiding this comment.
I think so, although not sure about Query. Please don't modify Query for the time being
If you run into problems with regressions, etc, let me know
There was a problem hiding this comment.
I pushed a commit to allow empty blocks in PrepareProposal, ProcessProposal, and FinalizeBlock. CheckTx is not about blocks, it's only for one transaction, so I didn't change it.
There was a problem hiding this comment.
The changes look good but can you add a test in the files (the ones Sergio added) that have no transactions to make sure nothing will explode (maybe we have places where we assume that the transaction array is not empty) like in the actual function implementations within kvstore.
There was a problem hiding this comment.
Please check the tests that I added. In particular, prepare_proposal for an empty block does not display in the console code: OK. This is expected because code: OK is printed for each transaction, and in this case there are none.
There was a problem hiding this comment.
Thanks Hernán. I just pushed a small commit to keep the tutorial in sync with ex1.abci.out
|
As discussed offline, I pushed a commit to include the new command into the "tutorial" abci-cli tests. Please take a look |
|
Seems all good if we want the transactions to be rejected after "Prepare" is removed then all is fine. |
Note that I left the bug Hernán found with MBT (I think he has a patch to fix it): we should remove the |
|
Yes, I think that's better done as a separate PR. |
jmalicevic
changed the title
process_proposal command to abci-cliprocess_proposal command to abci-cli
|
I think this change deserves an entry into the changelog. |
That's a very good point |
…ci-cli (#8901) * Add `process_proposal` command to abci-cli * Added process proposal to the 'tutorial' examples * Added entry in CHANGELOG_PENDING.md * Allow empty blocks in PrepareProposal, ProcessProposal, and FinalizeBlock * Fix minimum arguments * Add tests for empty block * Updated abci-cli doc Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
* [cherrypicked] abci-cli: added `PrepareProposal` command to cli (#8656) * Prepare prosal cli * [cherrypicked + fixes] abci-cli: Add `process_proposal` command to abci-cli (#8901) * Add `process_proposal` command to abci-cli * Added process proposal to the 'tutorial' examples * Added entry in CHANGELOG_PENDING.md * Allow empty blocks in PrepareProposal, ProcessProposal, and FinalizeBlock * Fix minimum arguments * Add tests for empty block * Updated abci-cli doc Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> * Addressed @williambanfield's comment Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> Co-authored-by: Hernán Vanzetto <hernan.vanzetto@gmail.com>
Closes #8644