rpcclient: return FaultException in checkResOk if any

[fyfyrchik]

Problem

Consider this test.

func TestItem(t *testing.T) {
	bigSlice := stackitem.NewByteArray(make([]byte, stackitem.MaxSize/2))
	arr := stackitem.NewArray([]stackitem.Item{bigSlice, bigSlice})
	res := &result.Invoke{
		State:          "HALT",
		GasConsumed:    237626000,
		Script:         []byte{10},
		Stack:          []stackitem.Item{arr},
		FaultException: "",
		Notifications:  []state.NotificationEvent{},
	}
	data, err := json.Marshal(res)
	require.NoError(t, err)

	var received result.Invoke
	require.NoError(t, json.Unmarshal(data, &received))

	_, err = Item(&received, nil)
	require.True(t, len(received.FaultException) != 0)
	require.Contains(t, err.Error(), received.FaultException)
}

It fails with

--- FAIL: TestItem (0.00s)
    /repo/neo-go/pkg/rpcclient/unwrap/unwrap_test.go:204: 
        	Error Trace:	/repo/neo-go/pkg/rpcclient/unwrap/unwrap_test.go:204
        	Error:      	"result stack is empty" does not contain "json error: too big: size"
        	Test:       	TestItem

That is because in checkResOk checks only VM state which is HALT

neo-go/pkg/rpcclient/unwrap/unwrap.go

Line 401 in 9c83bef

if r.State != vmstate.Halt.String() {

Solution

Communicate the error precisely, otherwise it leads to hard-to-debug failures, requiring moderate knowledge of neo-go internals (it is not obvious that result stack is empty just cannot happen with good contracts, unless you know how VM works).

[fyfyrchik]

Not sure it is the best message, but error: context seems excessive, FaultException usually contains something.

[roman-khimov]

inconsistent result, HALTed with exception: %w?

[AnnaShaleva]

@fyfyrchik, let's fix this comment and merge the PR. We'll remove stack marshalling error from result.Invoke's FaultException in #3359.

[fyfyrchik]

fixed

[fyfyrchik]

On a side-note, I think it would be nice to have some flexibility in JSON marshaling of the stackitems: we care about the size in interops, but RPC is a different story.

[roman-khimov]

I don't think it's a correct fix. Unfortunately FaultException semantics is not documented clearly and there were changes in various ways, especially given that it's used both for application logs an invocation results. But semantically you do not have any exception during execution, transaction is successful, it's just that for whatever reason you can't represent the result. It's purely a stack problem, but it was made to be an exception in 930c439 because of neo-project/neo-modules#696 (which in fact was about application logs). C# node never does it for invocation results (it just mangles the stack) and I think it's more correct to do so.

[fyfyrchik]

I disagree here, but the compatibility is probably more important.

Another option we have is to attach FaultException context somewhere? (don't like this, too many places to change, eventually will forget in new unwrappers).
Or at least provide some mechanism to make user aware without the loss of usability of unwrap package?
Like this:

unwrap.Item(unwrap.FaultToError(res, err))

This way user opts-in to the semantics they want and it is backwards compatible.

[roman-khimov]

I think you want #3130 now, but it's a different story from this PR.

[fyfyrchik]

Hm, in some sense I want exactly the opposite:
In #3130 we want to get FaultException created by FAULT state (this is already an error, we just need to make it stand out).
I want to somehow now about the HALT ones, which can occur and are important.

[fyfyrchik]

For the sake of completeness: we could also make global switch, but I probably know the answer to this suggestion :)

[roman-khimov]

They can't. HALTed transactions have no exceptions. It's just a server-side issue (clients can refuse these replies as bogus though). We always tell people:

• check VM state first, it should be HALT
• if it's OK, you may have something on the stack to check
• if it's not, maybe there is something in the exception field

[fyfyrchik]

HALTed transactions have no exceptions

That's true. But then again, the solution with unwrap.FaultToError doesn't seem bad. And even in this PR we do not pretend it is an invocation error (as we do in vm.State branch two lines above). The problem is that unwrap.* functions are used in e.g. auto-generated code which doesn't follow the algorithm you have just described. Manual checks almost defeat the purpose of the unwrap package.

Basically, I can either use unwrap (and have an error which doesn't point me to the root cause, unless I know where to dig) or do manual checks.

Yet another solution is to handle this in the invoker? (not by default). invoker.NewStrict() doc comment can describe exactly the behaviour you opt-in to and it is easily noticeable.

I see that the solution is not straightforward as it seemed to be, but to me the problem is worth solving (be it server- or client- side).

Do we have any tracking issue for the semantics of FaultException?

[fyfyrchik]

Another point is unwrap.Nothing: if the VM has returned something but this something is too big, server+unwrap.Nothing will behave as if nothing has happened.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.69%. Comparing base (9c83bef) to head (1a3a494).
Report is 7 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3356   +/-   ##
=======================================
  Coverage   84.68%   84.69%           
=======================================
  Files         331      331           
  Lines       44932    44938    +6     
=======================================
+ Hits        38050    38058    +8     
+ Misses       5371     5369    -2     
  Partials     1511     1511           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AnnaShaleva]

Do we have any tracking issue for the semantics of FaultException?

Except the #3130 there are no related issues.

I see that the solution is not straightforward as it seemed to be, but to me the problem is worth solving (be it server- or client- side).

I agree, we need to do something about it, because it's just unexpected behaviour from the user PoW and hiding the original unmarshalling error doesn't seem to be correct.

• if it's OK, you may have something on the stack to check
• if it's not, maybe there is something in the exception field

I'm afraid we don't have it well-documented. And currently the state is that unwrapper doesn't pay any attention on that.

C# node never does it for invocation results (it just mangles the stack) and I think it's more correct to do so.

Some marshalling errors may inevitably occur during stack marshalling, and we have to report about them somehow. Do you suggest to remove stack marshalling errors from FaultException?

[roman-khimov]

unwrapper doesn't pay any attention on that.

It can just be a semantic correctness check then. Unmarshaling code shouldn't care about it, but checkResOK can check for it.

Do you suggest to remove stack marshalling errors from FaultException?

Yes. Treat them similarly to application logs.