Handle nil *Any in UnpackAny and add panic handler for tx decoding #7594

blushi · 2020-10-19T10:15:34Z

Description

closes: #7585

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-10-19T10:24:55Z

Codecov Report

Merging #7594 into master will decrease coverage by 0.00%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #7594      +/-   ##
==========================================
- Coverage   54.16%   54.15%   -0.01%     
==========================================
  Files         606      606              
  Lines       38193    38194       +1     
==========================================
- Hits        20686    20685       -1     
- Misses      15410    15411       +1     
- Partials     2097     2098       +1

amaury1093 · 2020-10-19T11:09:10Z

baseapp/helpers.go

+	if err != nil {
+		return sdk.GasInfo{}, nil, err
+	}
+	return app.runTx(runTxModeCheck, bz)


I would suggest to put all these functions inside this file into a baseapp/testutil package (in a follow-up PR).

baseapp/helpers.go

…e/7585-nil-any

amaury1093

lgtm. disclaimer: I pushed some commits.

amaury1093 · 2020-10-19T15:52:38Z

baseapp/baseapp_test.go

@@ -1274,7 +1283,7 @@ func TestTxGasLimits(t *testing.T) {
 				}
 			}()

-			count := tx.(*txTest).Counter
+			count := tx.(txTest).Counter


I couldn't explain why this change is needed, but it is.

We have a *txTest as input in Deliver(), it gets amino-encoded into bytes in Deliver(), and decoded in runTx. The decoded type is somehow txTest instead of *txTest

amaury1093 · 2020-10-19T15:52:45Z

baseapp/baseapp_test.go

@@ -1284,7 +1293,7 @@ func TestTxGasLimits(t *testing.T) {

 	routerOpt := func(bapp *BaseApp) {
 		r := sdk.NewRoute(routeMsgCounter, func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) {
-			count := msg.(msgCounter).Counter
+			count := msg.(*msgCounter).Counter


ditto (or not?)

baseapp/baseapp_test.go

alexanderbez · 2020-10-19T16:05:31Z

baseapp/baseapp_test.go

@@ -1284,7 +1293,7 @@ func TestTxGasLimits(t *testing.T) {

 	routerOpt := func(bapp *BaseApp) {
 		r := sdk.NewRoute(routeMsgCounter, func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) {
-			count := msg.(msgCounter).Counter
+			count := msg.(*msgCounter).Counter


aaronc · 2020-10-19T16:25:08Z

baseapp/abci.go

@@ -231,7 +226,7 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
 		panic(fmt.Sprintf("unknown RequestCheckTx type: %s", req.Type))


Do we still want to panic here and crash the state machine? Maybe for a separate PR but it would be good to make this method as bulletproof as possible.

aaronc · 2020-10-19T16:25:19Z

baseapp/abci.go

@@ -213,11 +213,6 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc
 func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
 	defer telemetry.MeasureSince(time.Now(), "abci", "check_tx")


Can telemetry panic?

aaronc · 2020-10-19T16:30:58Z

baseapp/test_helpers.go

+	return sdk.NewContext(app.deliverState.ms, header, false, app.logger)
+}
+
+func (app *BaseApp) NewUncachedContext(isCheckTx bool, header tmproto.Header) sdk.Context {


Can we add a doc string here?

codec/types/interface_registry.go

aaronc

utACK

Co-authored-by: Aaron Craelius <[email protected]>

…7594) * Handle nil any in UnpackAny * Add test * Add flag back * Update runTx signature * Update Simulate signature * Update calls to Simulate * Add txEncoder in baseapp * Fix TestTxWithoutPublicKey * Wrap errors * Use amino in baseapp tests * Add txEncoder arg to Check & Deliver * Fix gas in test * Fix remaining base app tests * Rename to amionTxEncoder * Update codec/types/interface_registry.go Co-authored-by: Aaron Craelius <[email protected]> * golangci-lint fix Co-authored-by: Amaury Martiny <[email protected]> Co-authored-by: Aaron Craelius <[email protected]> Co-authored-by: Cory Levinson <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

@fedekunze

* Update Encoding Doc for 0.40 (#7430) * Remove deprecated docs and add first guidelines to protobuf migration * Add conventions * Reorder and add more FAQ doc * Update guidelines for pb msg definitions * Use commit hash in github links Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * docs: Update "Basics" section (#7416) * Prettier * docs: Update "Basics" section * appcli -> appd * Better wording * Fix to appCodec * Add gRPC mention * Add grpc * Reference simapp code * Update docs/basics/accounts.md Co-authored-by: Marie Gauthier <[email protected]> * Add section about gRPC query services * Optional LegacyQuerierHandler * Clearer docs * Update docs/basics/app-anatomy.md Co-authored-by: Marie Gauthier <[email protected]> * Update docs/basics/app-anatomy.md Co-authored-by: Federico Kunze <[email protected]> * Address comments * Address comments * Update docs/basics/accounts.md Co-authored-by: Marie Gauthier <[email protected]> Co-authored-by: Federico Kunze <[email protected]> * Fix misbehaviour handling for solo machine (#7515) * add timestamp to SignatureAndData Add timestamp field to signature and data. Add ValidateBasic check for timestamp. Add ValidateBasic test. Update misbehaviour handler to use supplied timestamp. * fix typo * add timestamp check * fix lint Co-authored-by: Federico Kunze <[email protected]> * update solo machine specs (#7512) * update solo machine specs * update concepts * self review fixes * Apply suggestions from code review * add note on upgarding solo machines Co-authored-by: Federico Kunze <[email protected]> * Corrected 'unsafe-reset-all' help text (#7504) * Merge PR #7415: Return empty store tree on non-existing version * tendermint: update sdk to rc5 (#7527) * update sdk to rc5 * Update baseapp/params.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * Bump github.com/spf13/cobra from 1.0.0 to 1.1.0 (#7553) Bumps [github.com/spf13/cobra](https://github.com/spf13/cobra) from 1.0.0 to 1.1.0. - [Release notes](https://github.com/spf13/cobra/releases) - [Changelog](https://github.com/spf13/cobra/blob/master/CHANGELOG.md) - [Commits](spf13/cobra@v1.0.0...v1.1.0) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * remove test_utils.go in tm client (#7522) * remove test_utils from tm client * fix build * fix lint? * fix lint? * apply @fedekunze review suggestion * add tests as per @alessio suggestion * fix typo * ibc: cleanup channel types test (#7521) * Update Building Modules documentation (#7473) * Update module-manager.md * Update modules #messages doc * Fix typo * Update message implementation example link * Update messages-and-queries.md#queries doc * Update querier.md * Update handler.md * Update links in beginblock-endblock.md * Update keeper.md * Update invariants.md * Update genesis.md * Update module-interfaces.md#transaction-commands * Update module-interfaces.md#query-commands * Update module-interfaces.md#flags * Update module-interfaces.md#rest * Update structure.md * Update module-interfaces.md#grpc * Update errors.md * Update module-interfaces.md#grpc-gateway-rest * Add more info on swagger * Address comments * Fix go.sum * Fix app-anatomy.md * Update docs/building-modules/module-interfaces.md Co-authored-by: Federico Kunze <[email protected]> * Update docs/building-modules/querier.md Co-authored-by: Federico Kunze <[email protected]> * Update docs/building-modules/querier.md Co-authored-by: Federico Kunze <[email protected]> * Update docs/building-modules/module-interfaces.md Co-authored-by: Federico Kunze <[email protected]> * Update docs/building-modules/module-interfaces.md Co-authored-by: Federico Kunze <[email protected]> * Address part of review comments * Update old ref of RegisterQueryService * Add example code for Manager * Update queriers.md to query-services.md and refs * Add new query-services.md * Revert "Update old ref of RegisterQueryService" This reverts commit 1ea1ea8. * Update keeper.md * Update handler.md * Update handler.md * Update messages-and-queries.md * Update docs/basics/app-anatomy.md Co-authored-by: Amaury Martiny <[email protected]> * Update docs/building-modules/intro.md Co-authored-by: Amaury Martiny <[email protected]> * Fix typo Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: Amaury Martiny <[email protected]> * client: add GetAccount and GetAccountWithHeight to AccountRetriever (#7558) * client: add GetAccount and GetAccountWithHeight to AccountRetriever * update ADR * address comments from review * Add the option of emitting amino encoded json from the CLI (#7221) * Add the option of emitting amino encoded json * Update AMINO JSON serialization with ConvertTxToStdTx * Make the Amino flag more self documenting by serializing the BroadcastRequest type instead of StdTx * Handle amino encoding error * Update x/auth/client/cli/tx_multisign.go Co-authored-by: Alessio Treglia <[email protected]> * Update x/auth/client/cli/tx_sign.go Co-authored-by: Alessio Treglia <[email protected]> * Apply suggestions from code review Co-authored-by: Alessio Treglia <[email protected]> * Fix go format Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: Alessio Treglia <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * ibc: update solo machine client command (#7579) Co-authored-by: Federico Kunze <[email protected]> * docs: Revert SPEC-SPEC and update x/{auth,bank,evidence,slashing} (#7407) * Revert some changes from #7404 * Update x/slashing * Address review comments * Small tweak Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * remove id in localhost (#7577) Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: Christopher Goes <[email protected]> * update changelog * Update CHANGELOG.md Co-authored-by: Aleksandr Bezobchuk <[email protected]> * Fix solomachine cmds (#7581) * fix solo machine cli cmds * polish Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * simapp: use tmjson on InitChainer (#7514) Co-authored-by: Alessio Treglia <[email protected]> * [IAVL]: Bump to v0.15.0-rc4 (#7549) * iavl 0.15.0-rc4 version bump * update comments on error modes for store.LoadStore * update CONFIO_URL in makefile Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * Handle nil *Any in UnpackAny and add panic handler for tx decoding (#7594) * Handle nil any in UnpackAny * Add test * Add flag back * Update runTx signature * Update Simulate signature * Update calls to Simulate * Add txEncoder in baseapp * Fix TestTxWithoutPublicKey * Wrap errors * Use amino in baseapp tests * Add txEncoder arg to Check & Deliver * Fix gas in test * Fix remaining base app tests * Rename to amionTxEncoder * Update codec/types/interface_registry.go Co-authored-by: Aaron Craelius <[email protected]> * golangci-lint fix Co-authored-by: Amaury Martiny <[email protected]> Co-authored-by: Aaron Craelius <[email protected]> Co-authored-by: Cory Levinson <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Marie Gauthier <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Amaury Martiny <[email protected]> Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: colin axnér <[email protected]> Co-authored-by: Neeraj Murarka <[email protected]> Co-authored-by: Riccardo Montagnin <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Zaki Manian <[email protected]> Co-authored-by: Alessio Treglia <[email protected]> Co-authored-by: Christopher Goes <[email protected]> Co-authored-by: Aaron Craelius <[email protected]>

…osmos#7594) * Handle nil any in UnpackAny * Add test * Add flag back * Update runTx signature * Update Simulate signature * Update calls to Simulate * Add txEncoder in baseapp * Fix TestTxWithoutPublicKey * Wrap errors * Use amino in baseapp tests * Add txEncoder arg to Check & Deliver * Fix gas in test * Fix remaining base app tests * Rename to amionTxEncoder * Update codec/types/interface_registry.go Co-authored-by: Aaron Craelius <[email protected]> * golangci-lint fix Co-authored-by: Amaury Martiny <[email protected]> Co-authored-by: Aaron Craelius <[email protected]> Co-authored-by: Cory Levinson <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

blushi and others added 4 commits October 19, 2020 11:51

Handle nil any in UnpackAny

af3d5d8

Add test

978e2b9

Add flag back

9c876db

Merge branch 'master' into marie/7585-nil-any

2451f2b

blushi changed the title ~~Handle nil *Any in UnpackAny and add panic handlers to CheckTx and DeliverTx~~ Handle nil *Any in UnpackAny and add panic handler for tx decoding Oct 19, 2020

blushi and others added 4 commits October 19, 2020 12:33

Update runTx signature

55bb84e

Update Simulate signature

e77d85d

Update calls to Simulate

a8b8538

Add txEncoder in baseapp

9405505

amaury1093 reviewed Oct 19, 2020

View reviewed changes

baseapp/helpers.go Outdated Show resolved Hide resolved

amaury1093 added 8 commits October 19, 2020 13:25

Fix TestTxWithoutPublicKey

db112ba

Wrap errors

ece41c5

Use amino in baseapp tests

da621d5

Merge origin

d468fb8

Merge branch 'master' into marie/7585-nil-any

fa954f4

Add txEncoder arg to Check & Deliver

76df2e0

Fix gas in test

c22e713

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into mari…

42eaf62

…e/7585-nil-any

zmanian mentioned this pull request Oct 19, 2020

[backports] v0.40.0-rc1 #7590

Merged

31 tasks

clevinson added the backport/0.40.x (Stargate) label Oct 19, 2020

Fix remaining base app tests

28843ed

blushi marked this pull request as ready for review October 19, 2020 15:43

blushi requested review from aaronc, alessio and alexanderbez as code owners October 19, 2020 15:43

blushi requested review from robert-zaremba, anilcse and atheeshp October 19, 2020 15:45

blushi requested review from aleem1314 and clevinson October 19, 2020 15:45

amaury1093 approved these changes Oct 19, 2020

View reviewed changes

alexanderbez approved these changes Oct 19, 2020

View reviewed changes

Rename to amionTxEncoder

0a3eb4f

aaronc reviewed Oct 19, 2020

View reviewed changes

codec/types/interface_registry.go Show resolved Hide resolved

aaronc approved these changes Oct 19, 2020

View reviewed changes

Update codec/types/interface_registry.go

049dd5f

Co-authored-by: Aaron Craelius <[email protected]>

aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 19, 2020

clevinson and others added 2 commits October 19, 2020 10:38

golangci-lint fix

ddb88f3

Merge branch 'master' into marie/7585-nil-any

aa020ab

mergify bot merged commit eba4c8a into master Oct 19, 2020

mergify bot deleted the marie/7585-nil-any branch October 19, 2020 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle nil *Any in UnpackAny and add panic handler for tx decoding #7594

Handle nil *Any in UnpackAny and add panic handler for tx decoding #7594

blushi commented Oct 19, 2020

codecov bot commented Oct 19, 2020 •

edited

Loading

amaury1093 Oct 19, 2020

amaury1093 left a comment

amaury1093 Oct 19, 2020

amaury1093 Oct 19, 2020

alexanderbez Oct 19, 2020

alexanderbez Oct 19, 2020

aaronc Oct 19, 2020

aaronc Oct 19, 2020

aaronc Oct 19, 2020

aaronc left a comment

		@@ -231,7 +226,7 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
		panic(fmt.Sprintf("unknown RequestCheckTx type: %s", req.Type))

		@@ -213,11 +213,6 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc
		func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
		defer telemetry.MeasureSince(time.Now(), "abci", "check_tx")

Handle nil *Any in UnpackAny and add panic handler for tx decoding #7594

Handle nil *Any in UnpackAny and add panic handler for tx decoding #7594

Conversation

blushi commented Oct 19, 2020

Description

codecov bot commented Oct 19, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 19, 2020 •

edited

Loading