Redact errors returned in reply #765

ethanfrey · 2022-02-23T19:48:35Z

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)

codecov · 2022-02-23T20:51:58Z

Codecov Report

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

@@            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 · 2022-02-23T21:09:53Z

@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.

webmaster128

LGTM in general

webmaster128 · 2022-02-24T08:16:17Z

x/wasm/keeper/msg_dispatcher.go

+	// 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)


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.

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"}

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.

alpe

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

alpe · 2022-02-25T13:46:44Z

x/wasm/keeper/msg_dispatcher.go

@@ -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)


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)

alpe · 2022-02-25T13:48:49Z

x/wasm/keeper/msg_dispatcher.go

@@ -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 {


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

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

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

alpe · 2022-02-25T14:07:41Z

x/wasm/keeper/msg_dispatcher.go

+	// 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)


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"}

alpe · 2022-02-25T14:08:24Z

x/wasm/keeper/submsg_test.go

@@ -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")},


New error stings are cheaper 👍

webmaster128 · 2022-03-02T09:16:59Z

x/wasm/keeper/msg_dispatcher.go

+	// 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)


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

This reverts commit 66ac1f5.

Fix test setup and assertions

Redact errors returned in reply

446c664

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

ethanfrey added 2 commits February 23, 2022 21:13

Fix lint error, failing test

8f75ad1

Fix submessage tests

30bb290

webmaster128 mentioned this pull request Feb 23, 2022

Create dedicated SubMsgResult CosmWasm/cosmwasm#1232

Merged

webmaster128 reviewed Feb 24, 2022

View reviewed changes

alpe approved these changes Feb 25, 2022

View reviewed changes

Adjust error string

3953bda

webmaster128 reviewed Mar 2, 2022

View reviewed changes

ethanfrey and others added 3 commits March 2, 2022 10:17

Handle nil logger again

66ac1f5

Revert "Handle nil logger again"

654fcec

This reverts commit 66ac1f5.

Fix test setup and assertions

b2217a3

ethanfrey force-pushed the 759-redact-reply-errors branch from 10dbfe8 to 66ac1f5 Compare March 2, 2022 18:53

Merge pull request #770 from CosmWasm/759-redact-reply-errors-tests

daf0d00

Fix test setup and assertions

ethanfrey merged commit c3aeb1b into master Mar 2, 2022

ethanfrey deleted the 759-redact-reply-errors branch March 2, 2022 20:06

ethanfrey mentioned this pull request Mar 7, 2022

Redact query errors #775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redact errors returned in reply #765

Redact errors returned in reply #765

ethanfrey commented Feb 23, 2022

codecov bot commented Feb 23, 2022 •

edited

Loading

ethanfrey commented Feb 23, 2022

webmaster128 left a comment

webmaster128 Feb 24, 2022

alpe Feb 25, 2022

webmaster128 Mar 1, 2022

alpe left a comment •

edited

Loading

alpe Feb 25, 2022

alpe Feb 25, 2022

ethanfrey Mar 2, 2022

ethanfrey Mar 2, 2022

alpe Feb 25, 2022

alpe Feb 25, 2022

webmaster128 Mar 2, 2022

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

Redact errors returned in reply #765

Redact errors returned in reply #765

Conversation

ethanfrey commented Feb 23, 2022

codecov bot commented Feb 23, 2022 • edited Loading

Codecov Report

ethanfrey commented Feb 23, 2022

webmaster128 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

alpe left a comment • edited Loading

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

codecov bot commented Feb 23, 2022 •

edited

Loading

alpe left a comment •

edited

Loading