[Neo VM: FIX] the GetString() methods of bytestring requires strictUTF8 #3110

Jim8y · 2024-01-29T14:01:26Z

Description

The GetString methods of StackItem requires strick UTF8.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

TestToStringInvalidUTF8

Test Configuration:
dotnet test

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

… set it differently.

roman-khimov · 2024-01-29T14:42:30Z

src/Neo.VM/EvaluationStack.cs

@@ -45,7 +45,7 @@ public override string ToString()
                            StackItemType.Pointer => $"({((Pointer)p).Position})",
                            StackItemType.Boolean => $"({p.GetBoolean()})",
                            StackItemType.Integer => $"({p.GetInteger()})",
-                            StackItemType.ByteString => $"(\"{p.GetString()}\")",
+                            StackItemType.ByteString => p.GetSpan().ToArray().TryGetString(out var str) ? $"(\"{str}\")" : $"(\"{Convert.ToBase64String(p.GetSpan())}\")",


Mixing base64 and strings (that can be valid base64 as well) is not a good idea to me, how can I differentiate 41414141 from 000000 then?

This also is an incompatible change.

Mixing base64 and strings (that can be valid base64 as well) is not a good idea to me, how can I differentiate 41414141 from 000000 then?

This also is an incompatible change.

@roman-khimov What? this is only for the ease of debug and unit test,,,, how come it can be incompatible to something? we dont print the executionstack anywhere but unit test (I added it in some pr that not yet merged). But maybe i should add somthing to tell if its base64 or string.

I don't know how exactly it's used in the current codebase, but it's public. If it's debug/test only, then maybe it's OK (can it be used by plugins/other tools?).

yes, cause i originally added this to debug the devpack, currently nowhere in the master branch is using it.

Doesn't matter it's in the main Neo.VM. If it's for tests then add it to the unit test's code. Don't add testing code to the main source. Add ONLY to test projects.

thought about it, but we will need it cross all unit tests, including devpack.

Shouldn't put test in main code. It can get mixed up to easily. Especially for a new person and if it's not documented. If it ends up being in main code. It needs to fail, unless running in a test. Just create new library for unit tests functionality that is the same for the test projects. What if for example someone end up getting alot of free gas cause they called a test function?

will update.

Refs. #3033 (not in any release yet, can be changed/removed). I agree with @cschuchardt88, this shouldn't be widely exposed if it's debug/test only.

…ded. As it is a test method and only unit test will need it.

Jim8y · 2024-01-31T13:16:29Z

@shargon @cschuchardt88 please check.

cschuchardt88 · 2024-02-01T08:03:07Z

This isn't a must but all filenames and classes should start with UT_ so we know it a Unit test. So when we are reading tests we know that it is production code or test code functions/classes.

tests/Neo.VM.Tests/UtEvaluationStack.cs

src/Neo.VM/Utility.cs

cschuchardt88

Like this @Jim8y https://github.com/neo-project/neo/pull/3103/files#diff-4faae5d98adc622a6e3016c1f98c157bd6cbabf32b55440248d30020edfd0b99

Look at filename and class.

Jim8y · 2024-02-01T10:06:37Z

Like this @Jim8y https://github.com/neo-project/neo/pull/3103/files#diff-4faae5d98adc622a6e3016c1f98c157bd6cbabf32b55440248d30020edfd0b99

Look at filename and class

This one alignes with existing file and tests, maybe update them all together in another pr. but before that, i think we should keep it this way. Take a look at: https://github.com/neo-project/neo/blob/master/tests/Neo.VM.Tests/Helpers/RandomHelper.cs

cschuchardt88 · 2024-02-01T10:08:44Z

Also for Test methods with attribute [TestMethod] or [Fact] you don't need to add UT_ just have the name be a description of what it's doing. for example Test_Blockchain_Invoke_On_ApllicationEngine

tests/Neo.VM.Tests/UtEvaluationStack.cs

Jim8y · 2024-02-01T10:11:27Z

Also i think we can deal with the name some time later, we should leave these to the editorstyle instead of manually.

cschuchardt88 · 2024-02-01T10:11:46Z

Like this @Jim8y https://github.com/neo-project/neo/pull/3103/files#diff-4faae5d98adc622a6e3016c1f98c157bd6cbabf32b55440248d30020edfd0b99
Look at filename and class

This one alignes with existing file and tests, maybe update them all together in another pr. but before that, i think we should keep it this way. Take a look at: https://github.com/neo-project/neo/blob/master/tests/Neo.VM.Tests/Helpers/RandomHelper.cs

I understand that. But this change will make life easier for the future or for someone coming in not knowing whether or not its core functioning code or test code they are debugging and reading so time isn't wasted.

Jim8y · 2024-02-01T10:13:00Z

I understand that name style is a thing, but there are many many rules everywhere, we should not take care of them manually, but use a tool to detect them.

cschuchardt88 · 2024-02-01T10:14:59Z

I understand that name style is a thing, but there are many many rules everywhere, we should not take care of them manually, but use a tool to detect them.

We can use another .editorconfig to do this. @shargon or @Jim8y maybe you can look into this. Im currently busy with cli product.

Jim8y · 2024-02-01T10:15:57Z

Surely will, @shargon you going to take this?

shargon

@Jim8y What about override ToString in all stack items?

Jim8y · 2024-02-06T14:08:10Z

@Jim8y What about override ToString in all stack items?

compound types is hard to print, will make the console impossible to trace, cause it will be printed on every opcode

shargon · 2024-02-06T16:18:17Z

@Jim8y What about override ToString in all stack items?

compound types is hard to print, will make the console impossible to trace, cause it will be printed on every opcode

It will print only the count, will be the same, but reusable

Jim8y · 2024-02-07T03:38:52Z

@Jim8y What about override ToString in all stack items?

compound types is hard to print, will make the console impossible to trace, cause it will be printed on every opcode

It will print only the count, will be the same, but reusable

@shargon just tried it out, even if you have to sting for each item, you still need a method for stack to print the stack. Just making it easier to implement, but still need somthing on both devpack side and vm side. Or just add it to the core.

Come on, @shargon and @cschuchardt88 are caring about different things, this will make our work hard, how about we focus on the functionality first? I have kept moving them here there, but they just work the same.

cschuchardt88 · 2024-02-07T03:46:13Z

Why don't you have a look at IApplicationEngineProvidor its used for tracing vm.

Jim8y · 2024-02-07T04:31:18Z

Why don't you have a look at IApplicationEngineProvidor its used for tracing vm.

We can access stack, but we can not access the things in the vm test. Have a new test project solely for a stack.print is not necessary. Cause we will either have to add it as reference in the devpack, or publish it, too much for a print.

cschuchardt88 · 2024-02-07T04:45:05Z

That kind of stack you trying to override?

cschuchardt88 · 2024-02-07T04:49:48Z

Jim8y · 2024-02-07T05:30:12Z

I� need to print the stack, not the stack items, but everything in the stack.

shargon · 2024-02-07T08:09:05Z

@shargon just tried it out, even if you have to sting for each item, you still need a method for stack to print the stack. Just making it easier to implement, but still need something on both devpack side and vm side. Or just add it to the core.

We can have both

shargon · 2024-02-07T08:12:39Z

tests/Neo.VM.Tests/Helpers/EvaluationStackHelper.cs

+{
+    public static string Print(this EvaluationStack stack)
+    {
+        return $"[{string.Join(", ", stack.Select(p => $"{p.Type}{p}"))}]";


In my opinion, we should move this to ToString no sense to duplicate code everywhere, and also is useful for debug

so you still need to have one in VM test and one in devpack test, what is the difference.

It doesn't hurt, and is good to override ToString to see something useful during debug, writes, etc

There should a Interface or some way to hook into the VM for override.

Please stop rushing code designs. It clear that no thought is put into the design when making code changes. Copy and Paste isn't the solution. I am not talking to anyone person, but us as a whole. I do it too. But we need to all stop and have plans. I think we should have in the 1st comment of the PR, what the plan is for design when doing design changes. Yes it takes more time, But we need the time to refactor and think it through.

There should a Interface or some way to hook into the VM for override.

This change is not required for core, is only for debugging and testing purpose

Jim8y · 2024-02-07T09:00:31Z

Good to go then

* master: Related to neo-project#3082 (comment) (neo-project#3119) [VM UT] update UT name (neo-project#3115)

* 'nullable' of github.com:Jim8y/neo: [Neo Fix] fix the store crash issue (neo-project#3124) [Neo VM: FIX] the GetString() methods of bytestring requires strictUTF8 (neo-project#3110)

Jim8y added 2 commits January 29, 2024 21:15

the GetString() methods of bytestring requires strictUTF8, but we can…

d8c5352

… set it differently.

update UTF8 rule and add UT

3c27880

roman-khimov reviewed Jan 29, 2024

View reviewed changes

Jim8y added 2 commits January 29, 2024 22:59

add comment and prefix to base64 string

b943758

fix UT

a4d9388

Jim8y marked this pull request as draft January 30, 2024 08:31

Rename ToString to Print and move it to the unit test where it is nee…

c6aca82

…ded. As it is a test method and only unit test will need it.

Jim8y marked this pull request as ready for review January 31, 2024 13:16

Jim8y commented Feb 1, 2024

View reviewed changes

tests/Neo.VM.Tests/UtEvaluationStack.cs Outdated Show resolved Hide resolved

Update tests/Neo.VM.Tests/UtEvaluationStack.cs

ab6f0d6

shargon mentioned this pull request Feb 1, 2024

print-stack neo-project/neo-devpack-dotnet#873

Merged

shargon reviewed Feb 1, 2024

View reviewed changes

src/Neo.VM/Utility.cs Outdated Show resolved Hide resolved

Update src/Neo.VM/Utility.cs

76a3cd4

shargon previously approved these changes Feb 1, 2024

View reviewed changes

cschuchardt88 requested changes Feb 1, 2024

View reviewed changes

Merge branch 'master' into fix-tostring

85dda24

Jim8y commented Feb 1, 2024

View reviewed changes

tests/Neo.VM.Tests/UtEvaluationStack.cs Outdated Show resolved Hide resolved

Update tests/Neo.VM.Tests/UtEvaluationStack.cs

84bc047

Jim8y dismissed shargon’s stale review via 84bc047 February 1, 2024 10:11

Merge branch 'master' into fix-tostring

a88e920

Jim8y added the Waiting for Review label Feb 4, 2024

shargon reviewed Feb 6, 2024

View reviewed changes

Merge branch 'master' into fix-tostring

4b580aa

Centralize print

3672a0e

shargon previously approved these changes Feb 7, 2024

View reviewed changes

shargon reviewed Feb 7, 2024

View reviewed changes

fix UT

5c7876c

Jim8y dismissed shargon’s stale review via 5c7876c February 7, 2024 09:00

Jim8y and others added 4 commits February 7, 2024 17:06

fix base64 format

1cbec67

Merge branch 'master' into fix-tostring

029aa9c

Override ToString

521ebe1

Clean using

a9fe368

shargon previously approved these changes Feb 7, 2024

View reviewed changes

Merge branch 'master' into fix-tostring

2a74538

* master: Related to neo-project#3082 (comment) (neo-project#3119) [VM UT] update UT name (neo-project#3115)

Jim8y dismissed shargon’s stale review via 2a74538 February 7, 2024 12:16

shargon approved these changes Feb 7, 2024

View reviewed changes

Merge branch 'master' into fix-tostring

35fd9e9

shargon merged commit 3e52756 into neo-project:master Feb 8, 2024
1 of 2 checks passed

Jim8y deleted the fix-tostring branch February 8, 2024 08:49

roman-khimov added this to the v3.7.0 milestone Feb 27, 2024

[Neo VM: FIX] the GetString() methods of bytestring requires strictUTF8 #3110

[Neo VM: FIX] the GetString() methods of bytestring requires strictUTF8 #3110

Conversation

Jim8y commented Jan 29, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Jim8y Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschuchardt88 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschuchardt88 Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y commented Jan 31, 2024

cschuchardt88 commented Feb 1, 2024 • edited Loading

cschuchardt88 left a comment • edited Loading

Choose a reason for hiding this comment

Jim8y commented Feb 1, 2024 • edited Loading

cschuchardt88 commented Feb 1, 2024 • edited Loading

Jim8y commented Feb 1, 2024

cschuchardt88 commented Feb 1, 2024

Jim8y commented Feb 1, 2024

cschuchardt88 commented Feb 1, 2024

Jim8y commented Feb 1, 2024

shargon left a comment

Choose a reason for hiding this comment

Jim8y commented Feb 6, 2024

shargon commented Feb 6, 2024

Jim8y commented Feb 7, 2024

cschuchardt88 commented Feb 7, 2024

Jim8y commented Feb 7, 2024 • edited Loading

cschuchardt88 commented Feb 7, 2024

cschuchardt88 commented Feb 7, 2024

Jim8y commented Feb 7, 2024

shargon commented Feb 7, 2024

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

Jim8y commented Feb 7, 2024

Jim8y Jan 29, 2024 •

edited

Loading

cschuchardt88 Jan 29, 2024 •

edited

Loading

cschuchardt88 Jan 30, 2024 •

edited

Loading

cschuchardt88 commented Feb 1, 2024 •

edited

Loading

cschuchardt88 left a comment •

edited

Loading

Jim8y commented Feb 1, 2024 •

edited

Loading

cschuchardt88 commented Feb 1, 2024 •

edited

Loading

Jim8y commented Feb 7, 2024 •

edited

Loading