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

Redact errors returned in reply #765

Merged
merged 8 commits into from
Mar 2, 2022
Merged

Conversation

ethanfrey
Copy link
Member

Part 1 of #759

The reply callback after executing a submessage no longer contains the entire error string on the failure case.
We only provide the error code and code space, which is guaranteed to be consistent (and part of the app hash)

@ethanfrey ethanfrey requested a review from alpe as a code owner February 23, 2022 19:48
@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #765 (daf0d00) into master (95d58b8) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #765      +/-   ##
==========================================
+ Coverage   58.51%   58.53%   +0.02%     
==========================================
  Files          49       49              
  Lines        5828     5832       +4     
==========================================
+ Hits         3410     3414       +4     
  Misses       2169     2169              
  Partials      249      249              
Impacted Files Coverage Δ
x/wasm/keeper/msg_dispatcher.go 87.75% <100.00%> (+0.52%) ⬆️

@ethanfrey
Copy link
Member Author

@webmaster128 you might be interested in this as well.
This is a minimal first step.
In existing contracts, the actual error messages returned In reply never make it to end users.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general

// sdk/11 is out of gas
// sdk/5 is insufficient funds (on bank send)
codespace, code, _ := sdkerrors.ABCIInfo(err, false)
return fmt.Sprintf("Error: %s/%d", codespace, code)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this "Error: " prefix because this string is embedded in the Err case of the result, so this would come from the containing structure?

I wonder if codespace: %s, code: %d would be more helpful to understand where the values comes from. Having the variable names available is how devs interact with those values at the Tendermint RPC and CosmJS level.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @webmaster128 that codespace: %s, code: %d is more readable for humans but codespace/code is easier to parse. In both cases, I would expect people to ask what the data means.

IMHO, let's start with codespace/code to optimize for parsing. We can help the devs with some doc page for common codes.
Hardcoding some constants would add complexity on the contract side as they may have to match for certain constants as well as the "codespace/code" types.

"Error: " as a prefix is a redundant information. A prefix may make sense though if we want to have different structures/ versions to be parseable, like
e1/sdk/11,e2/sdk/11/some-more-info, e3/sdk/11/some-other-info/123, e4/sdk/11/{"some":"json"}

Getting a bit crazy on this, we could also make the whole error message json so that it is easy to parse, better readable for humans and simpler to extend. {"codespace":"sdk", "code: "11"}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't parsability be a non-goal if we say there is no stability guarantee for the content of those error strings? The more we make them look like easy to parse, the mode users make assumptions on the format.

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is small but effective change that solves one part of the issue. 💐
I added some comments for improvements but the "error message text" one may got a bit beyond real requirements. Structured data can be added in a breaking error v2 design

@@ -131,8 +133,10 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk
},
}
} else {
// Issue #759 - we don't return error string for worries of non-determinism
logError(ctx, "Redacting submessage error", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the module logger to have some context added.

Suggested change
logError(ctx, "Redacting submessage error", err)
moduleLogger(ctx).Info("Redacting submessage error", "cause", err)

@@ -155,6 +159,22 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk
return rsp, nil
}

func logError(ctx sdk.Context, msg string, err error) {
logger := ctx.Logger()
if logger != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to check for nil. It is always set in prod code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but without this it panic'd in several tests

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the moduleLogger(ctx) as you requested, without the check, and got:

--- FAIL: TestDispatchSubmessages (0.00s)
    --- FAIL: TestDispatchSubmessages/multiple_msg_-_last_reply_returned (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x1bffec1]

goroutine 1336 [running]:
testing.tRunner.func1.2({0x1e4c100, 0x30cfc50})
	/usr/local/go/src/testing/testing.go:1209 +0x36c
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1212 +0x3b6
panic({0x1e4c100, 0x30cfc50})
	/usr/local/go/src/runtime/panic.go:1047 +0x266
github.com/CosmWasm/wasmd/x/wasm/keeper.moduleLogger({{0x0, 0x0}, {0x2514dc0, 0xc0011f67b0}, {{0x0, 0x0}, {0x0, 0x0}, 0x0, {0x0, ...}, ...}, ...})
	/go/src/github.com/cosmwasm/wasmd/x/wasm/keeper/keeper.go:958 +0x181
github.com/CosmWasm/wasmd/x/wasm/keeper.MessageDispatcher.DispatchSubmessages({{_, _}, {_, _}}, {{0x0, 0x0}, {0x2514dc0, 0xc0011f67b0}, {{0x0, 0x0}, ...}, ...}, ...)
	/go/src/github.com/cosmwasm/wasmd/x/wasm/keeper/msg_dispatcher.go:137 +0xb18
github.com/CosmWasm/wasmd/x/wasm/keeper.TestDispatchSubmessages.func29(0xc0018ac820)
	/go/src/github.com/cosmwasm/wasmd/x/wasm/keeper/msg_dispatcher_test.go:352 +0x495
testing.tRunner(0xc0018ac820, 0xc00056ce80)
	/usr/local/go/src/testing/testing.go:1259 +0x230
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1306 +0x727
FAIL	github.com/CosmWasm/wasmd/x/wasm/keeper	15.259s
FAIL

// sdk/11 is out of gas
// sdk/5 is insufficient funds (on bank send)
codespace, code, _ := sdkerrors.ABCIInfo(err, false)
return fmt.Sprintf("Error: %s/%d", codespace, code)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @webmaster128 that codespace: %s, code: %d is more readable for humans but codespace/code is easier to parse. In both cases, I would expect people to ask what the data means.

IMHO, let's start with codespace/code to optimize for parsing. We can help the devs with some doc page for common codes.
Hardcoding some constants would add complexity on the contract side as they may have to match for certain constants as well as the "codespace/code" types.

"Error: " as a prefix is a redundant information. A prefix may make sense though if we want to have different structures/ versions to be parseable, like
e1/sdk/11,e2/sdk/11/some-more-info, e3/sdk/11/some-other-info/123, e4/sdk/11/{"some":"json"}

Getting a bit crazy on this, we could also make the whole error message json so that it is easy to parse, better readable for humans and simpler to extend. {"codespace":"sdk", "code: "11"}

@@ -275,15 +275,15 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) {
subMsgError: true,
gasLimit: &subGasLimit,
// uses same gas as call without limit (note we do not charge the 40k on reply)
resultAssertions: []assertion{assertGasUsed(79000, 79040), assertErrorString("insufficient funds")},
resultAssertions: []assertion{assertGasUsed(77500, 77600), assertErrorString("sdk/5")},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New error stings are cheaper 👍

// sdk/11 is out of gas
// sdk/5 is insufficient funds (on bank send)
codespace, code, _ := sdkerrors.ABCIInfo(err, false)
return fmt.Sprintf("codespace: %s, code: %d", codespace, code)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a pretty nice error string. Can be parsed if really needed but does not screem PARSE ME.

@ethanfrey ethanfrey merged commit c3aeb1b into master Mar 2, 2022
@ethanfrey ethanfrey deleted the 759-redact-reply-errors branch March 2, 2022 20:06
@ethanfrey ethanfrey mentioned this pull request Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants