abci++: correct max-size check to only operate on added and unmodified#8242
Merged
williambanfield merged 1 commit intomasterfrom Apr 1, 2022
Merged
abci++: correct max-size check to only operate on added and unmodified#8242williambanfield merged 1 commit intomasterfrom
williambanfield merged 1 commit intomasterfrom
Conversation
creachadair
approved these changes
Apr 1, 2022
sergio-mena
reviewed
Apr 4, 2022
| for _, cur := range append(unmodifiedCopy, addedCopy...) { | ||
| size += int64(len(cur)) | ||
| if size > maxSizeBytes { | ||
| return fmt.Errorf("transaction data size %d exceeds maximum %d", size, maxSizeBytes) |
Contributor
There was a problem hiding this comment.
I think size here might be misleading in the error message, as it we cut the sum short as soon as it exceeds maxSizeBytes. I would just leave maxSizeBytes in the message
35 tasks
3 tasks
sergio-mena
added a commit
that referenced
this pull request
Aug 3, 2022
* -----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>
samricotta
pushed a commit
that referenced
this pull request
Aug 16, 2022
* -----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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
while reviewing #8210, I realized that we may not be properly accounting for the
MaxBytesper the spec. We only wantMaxBytesto be checked against the added / unmodified transactions. We were checking against all tx in theResponsePrepareProposal. This pull request fixes the validation logic.