Remove `ModifiedTxStatus` from the spec and the code by sergio-mena · Pull Request #8210 · tendermint/tendermint

sergio-mena · 2022-03-29T13:42:18Z

Removed field ModifiedTxStatus from ResponsePrepareProposal as it might be misleading to set it to UNMODIFIED when the total size of transactions passed to the App exceeds RequestPrepareProposal.MaxTxBytes. In this case, the Applications is required to modify the transactions by shedding as many as needed to conform to the size limit.
One of the commits contains the differences in protobuf-generated code on tip of master. As a result, running make abci-proto-gen on top of this PR, does not produce any uncommitted changes on auto-generated code

This reverts commit e576708.

williambanfield

A few fixups and language changes. Overall very happy with this change.

I think the loop flow of removing a value on the next iteration is a bit confusing and prone to bugs if maintainers in the future add more complex flow control to the loop, say breaking after a certain condition. I would re-work the loops that check if the Tx list is too large so that the loops only add the transaction to the list it is building if it allows the full list to stay under MaxBytes.

test/e2e/app/app.go

internal/state/errors.go

internal/state/execution_test.go

internal/consensus/state.go

internal/consensus/mempool_test.go

internal/state/execution_test.go

internal/test/factory/tx.go

internal/state/execution_test.go

williambanfield · 2022-03-29T15:56:24Z

spec/abci++/abci++_tmint_expected_behavior_002_draft.md

-    * If it does: remove transactions at the end of the list until the total byte size conforms to the limit,
-      then set `ResponsePrepareProposal.modified_tx_status` to `MODIFIED` and return.
-    * Else, set `ResponsePrepareProposal.modified_tx_status` to `UNMODIFIED` and return.
+* `PrepareProposal` should create a list of [TxRecord](./abci%2B%2B_methods_002_draft.md#txrecord) each containing a


I don't believe this link needs to be URL encoded. My browser appears to do it automatically, although, perhaps this isn't universal behavior?

Those are URL encoded all over. I've changed this one following your suggestion. If it works, I'll do another pass and change the rest

spec/abci++/abci++_tmint_expected_behavior_002_draft.md

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

abci/example/kvstore/kvstore.go

abci/types/application.go

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

…calls

sergio-mena · 2022-03-29T21:34:32Z

I think the loop flow of removing a value on the next iteration is a bit confusing and prone to bugs

Tell me about it! -_-
It should be better now

williambanfield

Looking good. Looks like the linter is complaining about the code not beeing gofmt-ed. Pretty common go practice is to setup your editor to automatically run gofmt on save. Happy to help set this with you if you'd like.

abci/example/kvstore/kvstore.go

internal/state/execution_test.go

spec/abci++/abci++_tmint_expected_behavior_002_draft.md

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

test/e2e/app/app.go

…ified-prepare-proposal

sergio-mena · 2022-03-31T21:43:55Z

markdown-link-check failing on anchor links (404), but those links are fine (they exist locally in the file)

thanethomson

Looks good to me 👍

williambanfield

Really happy how this turned out.

internal/state/execution_test.go

…8210) * Outstanding abci-gen changes to 'pb.go' files * Removed modified_tx_status from spec and protobufs * Fix sed for OSX * Regenerated abci protobufs with 'abci-proto-gen' * Code changes. UTs e2e tests passing * Recovered UT: TestPrepareProposalModifiedTxStatusFalse * Adapted UT * Fixed UT * Revert "Fix sed for OSX" This reverts commit e576708. * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed some comments * Added one test that tests error at the ABCI client + Fixed some mock calls * Addressed remaining comments * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed William's latest comments * Adressed Michael's comment * Fixed UT * Some md fixes * More md fixes * gofmt Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

* -----start------ * [cherrypicked] state: panic on ResponsePrepareProposal validation error (#8145) * state: panic on ResponsePrepareProposal validation error * lint++ Co-authored-by: Sam Kleinman <garen@tychoish.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * [cherrypicked] abci++: remove CheckTx call from PrepareProposal flow (#8176) * [cherrypicked] abci++: correct max-size check to only operate on added and unmodified (#8242) * [cherrypicked] Remove `ModifiedTxStatus` from the spec and the code (#8210) * Outstanding abci-gen changes to 'pb.go' files * Removed modified_tx_status from spec and protobufs * Fix sed for OSX * Regenerated abci protobufs with 'abci-proto-gen' * Code changes. UTs e2e tests passing * Recovered UT: TestPrepareProposalModifiedTxStatusFalse * Adapted UT * Fixed UT * Revert "Fix sed for OSX" This reverts commit e576708. * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Update internal/state/execution_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed some comments * Added one test that tests error at the ABCI client + Fixed some mock calls * Addressed remaining comments * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed William's latest comments * Adressed Michael's comment * Fixed UT * Some md fixes * More md fixes * gofmt Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * make proto-gen * Fixed testcase on PrepareProposal error * mockery Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: Sam Kleinman <garen@tychoish.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

sergio-mena added 9 commits March 29, 2022 15:27

Outstanding abci-gen changes to 'pb.go' files

7b8dd80

Removed modified_tx_status from spec and protobufs

9e2bfeb

Fix sed for OSX

e576708

Regenerated abci protobufs with 'abci-proto-gen'

29797f3

Code changes. UTs e2e tests passing

57217fe

Recovered UT: TestPrepareProposalModifiedTxStatusFalse

0f270a8

Adapted UT

a8ffc98

Fixed UT

e8f2066

Revert "Fix sed for OSX"

d84b3c6

This reverts commit e576708.

sergio-mena requested review from ancazamfir, cason, cmwaters, creachadair, ebuchman, jmalicevic, josef-widder, milosevic, thanethomson, tychoish and williambanfield as code owners March 29, 2022 13:42

williambanfield reviewed Mar 29, 2022

View reviewed changes

Update internal/state/execution_test.go

c43d9e2

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

creachadair reviewed Mar 29, 2022

View reviewed changes

abci/example/kvstore/kvstore.go Outdated Show resolved Hide resolved

abci/types/application.go Outdated Show resolved Hide resolved

sergio-mena and others added 6 commits March 29, 2022 20:11

Update abci/example/kvstore/kvstore.go

f905fb4

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

Update internal/state/execution_test.go

d0d1b32

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md

1e2c6a2

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

Addressed some comments

d9257e5

Added one test that tests error at the ABCI client + Fixed some mock …

820b445

…calls

Addressed remaining comments

497644f

sergio-mena mentioned this pull request Mar 30, 2022

abci++: Sync implementation and spec for vote extensions #8141

Merged

williambanfield reviewed Mar 30, 2022

View reviewed changes

williambanfield added this to the v0.36 milestone Mar 30, 2022

sergio-mena and others added 5 commits March 31, 2022 16:00

Update abci/example/kvstore/kvstore.go

a58a323

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

Update abci/example/kvstore/kvstore.go

b563ca0

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

Update abci/example/kvstore/kvstore.go

be1605b

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md

7369dbf

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>

Addressed William's latest comments

d533e2e

creachadair reviewed Mar 31, 2022

View reviewed changes

test/e2e/app/app.go Outdated Show resolved Hide resolved

sergio-mena added 6 commits March 31, 2022 21:49

Merge remote-tracking branch 'origin/master' into sergio/remove-unmod…

6921867

…ified-prepare-proposal

Adressed Michael's comment

82abc97

Fixed UT

91e0bec

Some md fixes

00d517a

More md fixes

5f73fbb

gofmt

4ae0b44

thanethomson approved these changes Apr 1, 2022

View reviewed changes

williambanfield approved these changes Apr 1, 2022

View reviewed changes

internal/state/execution_test.go Show resolved Hide resolved

williambanfield mentioned this pull request Apr 1, 2022

abci++: correct max-size check to only operate on added and unmodified #8242

Merged

Merge master into sergio/remove-unmodified-prepare-proposal

6a55c95

sergio-mena merged commit 8df38db into master Apr 4, 2022

sergio-mena deleted the sergio/remove-unmodified-prepare-proposal branch April 4, 2022 10:43

sergio-mena mentioned this pull request Jul 22, 2022

ABCI++ Prepare/Process Proposal #9053

Closed

35 tasks

sergio-mena mentioned this pull request Aug 3, 2022

Follow-up fixes to main PrepareProposal implementation #9162

Merged

3 tasks

Conversation

sergio-mena commented Mar 29, 2022

Uh oh!

williambanfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williambanfield Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

sergio-mena Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sergio-mena commented Mar 29, 2022

Uh oh!

williambanfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sergio-mena commented Mar 31, 2022

Uh oh!

thanethomson left a comment

Choose a reason for hiding this comment

Uh oh!

williambanfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants