Fix stack exception name #855

superboyiii · 2023-11-30T02:51:56Z

Close #854

src/ApplicationLogs/LogReader.cs

Jim8y · 2023-11-30T07:24:28Z

a new field?

superboyiii · 2023-11-30T07:38:04Z

a new field?

Two kind of exception(one from vm, one from applicationlog), but they're using the same trigger. applicationlog exception could rewrite vm exception...

superboyiii · 2023-11-30T08:30:42Z

Now it's the same result as invocation. @roman-khimov

roman-khimov · 2023-11-30T10:02:06Z

The behavior we have in NeoGo now is that we return an array of elements in the stack. They can be proper stack items or they can be strings. Suppose you have a script leaving 1 and some bad item on the stack, with NeoGo you'd get

"stack": [
    {"type":"Integer","value":"1"},
    "error of some kind"
]

With this PR that would be

"stack": "error of some kind"

And this obviously loses more data that can be valuable for debugging, the number of elements left on the stack and all the proper elements can be helpful in many cases. I'd prefer having this kind of output in the end both for application log and test invocations.

superboyiii · 2023-11-30T10:12:45Z

"stack": "error of some kind"

So if 1000 stacks return 1000 error messages? And they are the same hard code message...

roman-khimov · 2023-11-30T10:21:27Z

One stack item --- one error message, you serialize them one by one anyway, either you get a proper JSON or you get an error.

superboyiii · 2023-12-01T03:05:35Z

@shargon What's your opinion?

shargon · 2023-12-01T03:10:48Z

If we add the stack, i prefer to add the stack item type, and the value, otherwise I prefer to use the exception field

superboyiii · 2023-12-01T09:39:36Z

Tested. @shargon @roman-khimov

shargon · 2023-12-01T09:53:24Z

Why we don't use the exception field?

superboyiii · 2023-12-01T10:01:51Z

Why we don't use the exception field?

These two exceptions are generated from different place, but applicationlog exception could re-write vm exception which doesn't make sense and can make some Dapp backend confusing a lot for handling these exceptions. For example: Ghostmarket.

roman-khimov

How about test invocations (GetInvokeResult)?

OK otherwise.

src/ApplicationLogs/LogReader.cs

shargon · 2023-12-01T14:40:10Z

Why we don't use the exception field?

These two exceptions are generated from different place, but applicationlog exception could re-write vm exception which doesn't make sense and can make some Dapp backend confusing a lot for handling these exceptions. For example: Ghostmarket.

What about change the exception field to be an array, also with type?

roman-khimov · 2023-12-01T17:14:18Z

If it'd be separated, you wouldn't know which elements are bad. In some cases it might be useful. Imagine 3 items on the stack, the second one is bad, but the other two are OK and can be serialized, if you get two stack items and one exception, what was the original index of the element that has caused this exception?

cschuchardt88 · 2023-12-03T07:43:31Z

#807 And #842 implements AppclaitionEngine.Log for the meantime in application log for debug mode. Until core can be update to do it properly.

superboyiii · 2023-12-04T02:48:48Z

Done. @roman-khimov @shargon

src/ApplicationLogs/LogReader.cs

superboyiii · 2023-12-05T06:43:51Z

@AnnaShaleva Could you review again? Any more improvement needed?

superboyiii · 2023-12-05T08:04:58Z

@Liaojinghui @shargon Is it OK for you?

cschuchardt88 · 2023-12-05T08:40:17Z

src/ApplicationLogs/LogReader.cs

+ }
+ catch (Exception ex)
+ {
+ stack.Add("error: " + ex);


I dont think will output will help much. Seeing how the only reason it would fail is because its an interop and cant be serialized. Better to put an empty or null stackitem instead.

cschuchardt88 · 2023-12-05T08:41:17Z

src/ApplicationLogs/LogReader.cs

+ }
+ catch (Exception ex)
+ {
+ stack.Add("error: " + ex);


I dont think will output will help much. Seeing how the only reason it would fail is because its an interop and cant be serialized. Better to put an empty or null stackitem instead.

cschuchardt88 · 2023-12-05T08:41:44Z

src/RpcServer/RpcServer.SmartContract.cs

+ }
+ catch (Exception ex)
+ {
+ stack.Add("error: " + ex);


src/ApplicationLogs/LogReader.cs

src/RpcServer/RpcServer.SmartContract.cs

superboyiii · 2023-12-05T08:57:59Z

Done @shargon

cschuchardt88 · 2023-12-05T09:02:34Z

src/ApplicationLogs/LogReader.cs

@@ -147,7 +147,7 @@ public static JObject BlockLogToJson(Block block, IReadOnlyList<Blockchain.Appli
 }
 catch (Exception ex)
 {
- stack.Add("error: " + ex);
+ stack.Add("error: " + ex.Message);


Suggested change

stack.Add("error: " + ex.Message);

stack.Add(item.GetInterface<object>().GetType().Name);

The error means that there was a stackitem, but it can't be serialized and therefore can't be presented in JSON output. Replacing it with NULL or any other element (empty/string with a name) would be wrong because that's not what's on the stack. Proper stack item (like suggested) would be misinterpreted as a real output from the execution, it has to be some error string in case of serialization failure.

But the ones that can't be output to json are iterators (storage); at least what i have ran into (on testnet).

cschuchardt88 · 2023-12-05T09:03:02Z

src/RpcServer/RpcServer.SmartContract.cs

@@ -102,7 +102,7 @@ private JObject GetInvokeResult(byte[] script, Signer[] signers = null, Witness[
 }
 catch (Exception ex)
 {
- stack.Add("error: " + ex);
+ stack.Add("error: " + ex.Message);


Suggested change

stack.Add("error: " + ex.Message);

stack.Add(item.GetInterface<object>().GetType().Name);

In this way, it will throw exception directly but can't get result stack!

Compare to current code:

cschuchardt88 · 2023-12-05T09:03:25Z

src/ApplicationLogs/LogReader.cs

@@ -99,7 +99,7 @@ public static JObject TxLogToJson(Blockchain.ApplicationExecuted appExec)
 }
 catch (Exception ex)
 {
- stack.Add("error: " + ex);
+ stack.Add("error: " + ex.Message);


Suggested change

stack.Add("error: " + ex.Message);

stack.Add(item.GetInterface<object>().GetType().Name);

This can break applicationlog getting the stack:

But you can see it's on-chain:

shargon · 2023-12-05T09:11:21Z

Maybe add the error description will not be deterministic between neo-go and c#?

cschuchardt88 · 2023-12-05T09:12:49Z

I think we should be doing this the proper way, indicating that there is an valid stackitem. We shouldnt be hiding it or saying there is something wrong with it; when there is not.

Edit:
#807 and neo-express handle the stackitems this way.

roman-khimov · 2023-12-05T10:13:01Z

error description will not be deterministic between neo-go and c#?

Yes, it will differ.

shargon · 2023-12-05T10:51:27Z

error description will not be deterministic between neo-go and c#?

Yes, it will differ.

Then is better to change the message

roman-khimov · 2023-12-05T11:45:48Z

I think we can settle on the fact that if you're receiving a string instead of a stack item, it's an error string and the original stack item couldn't be represented. And this string can be implementation-specific, just like data in JSON-RPC errors.

superboyiii · 2023-12-06T03:41:37Z

@shargon @roman-khimov need approve

* 'master' of github.com:neo-project/neo-modules: Fix workflow (neo-project#857) Fix stack exception name (neo-project#855) update workflow (neo-project#856) Refac build configs (neo-project#846) Hotfix for neo-project#850 (neo-project#853) Fix RpcNativeContract (neo-project#851)

Fix stack exception name

527b885

roman-khimov reviewed Nov 30, 2023

View reviewed changes

src/ApplicationLogs/LogReader.cs Outdated Show resolved Hide resolved

fix stack

e7e3e8f

Jim8y previously approved these changes Nov 30, 2023

View reviewed changes

show all type and value of stack item

2ced172

superboyiii dismissed Jim8y’s stale review via 2ced172 December 1, 2023 09:38

roman-khimov reviewed Dec 1, 2023

View reviewed changes

src/ApplicationLogs/LogReader.cs Outdated Show resolved Hide resolved

fix for GetInvokeResult

78d62b3

AnnaShaleva reviewed Dec 4, 2023

View reviewed changes

src/ApplicationLogs/LogReader.cs Show resolved Hide resolved

src/ApplicationLogs/LogReader.cs Outdated Show resolved Hide resolved

src/ApplicationLogs/LogReader.cs Outdated Show resolved Hide resolved

catch all exceptions

a6bf3a1

AnnaShaleva approved these changes Dec 5, 2023

View reviewed changes

cschuchardt88 reviewed Dec 5, 2023

View reviewed changes

shargon reviewed Dec 5, 2023

View reviewed changes

src/ApplicationLogs/LogReader.cs Outdated Show resolved Hide resolved

src/ApplicationLogs/LogReader.cs Outdated Show resolved Hide resolved

src/RpcServer/RpcServer.SmartContract.cs Outdated Show resolved Hide resolved

fix-message

b205496

cschuchardt88 requested changes Dec 5, 2023

View reviewed changes

shargon previously approved these changes Dec 5, 2023

View reviewed changes

Merge branch 'master' into fix-applicationlog-exception

7e761f6

superboyiii dismissed shargon’s stale review via 7e761f6 December 6, 2023 03:33

Merge branch 'master' into fix-applicationlog-exception

533a1e8

shargon approved these changes Dec 6, 2023

View reviewed changes

shargon merged commit f4a5705 into master Dec 6, 2023
2 checks passed

shargon deleted the fix-applicationlog-exception branch December 6, 2023 06:42

	stack.Add("error: " + ex.Message);
	stack.Add(item.GetInterface<object>().GetType().Name);

Fix stack exception name #855

Fix stack exception name #855

Conversation

superboyiii commented Nov 30, 2023

Jim8y commented Nov 30, 2023

superboyiii commented Nov 30, 2023 • edited Loading

superboyiii commented Nov 30, 2023

roman-khimov commented Nov 30, 2023

superboyiii commented Nov 30, 2023 • edited Loading

roman-khimov commented Nov 30, 2023

superboyiii commented Dec 1, 2023

shargon commented Dec 1, 2023

superboyiii commented Dec 1, 2023

shargon commented Dec 1, 2023

superboyiii commented Dec 1, 2023

roman-khimov left a comment

Choose a reason for hiding this comment

shargon commented Dec 1, 2023

roman-khimov commented Dec 1, 2023

cschuchardt88 commented Dec 3, 2023

superboyiii commented Dec 4, 2023

superboyiii commented Dec 5, 2023

superboyiii commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superboyiii commented Dec 5, 2023

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

shargon commented Dec 5, 2023

cschuchardt88 commented Dec 5, 2023 • edited Loading

roman-khimov commented Dec 5, 2023

shargon commented Dec 5, 2023

roman-khimov commented Dec 5, 2023

superboyiii commented Dec 6, 2023

superboyiii commented Nov 30, 2023 •

edited

Loading

superboyiii commented Nov 30, 2023 •

edited

Loading

cschuchardt88 commented Dec 5, 2023 •

edited

Loading