Check ABI for notifications by erikzhang · Pull Request #2810 · neo-project/neo

erikzhang · 2022-09-02T02:15:14Z

superboyiii · 2022-09-02T02:39:10Z

A little worried that this will break old contract and make data incompatible.

roman-khimov · 2022-09-02T06:19:31Z

We need to check mainnet of course, if something breaks this should be a hardfork, but it's a valuable improvement in general.

Now if we could trust NEP-11/NEP-17 manifest specifications (#1883)...

vncoelho · 2022-09-02T09:52:07Z

That can break and may need a hardfork as you mentioned.

However, in a long-term perspective this is a good compliance to be followed by contracts as @roman-khimov emphasized.

vncoelho

The PR is precise @erikzhang ,as always, code looks simple, we will test asap.

superboyiii · 2022-09-08T10:18:19Z

I can't approve this, we need fork height, otherwise will be break at block 693180 in mainnet. @erikzhang @shargon

roman-khimov · 2022-09-08T11:41:18Z

otherwise will be break at block 693180 in mainnet

Do you have the hash of the contract causing the problem? We may want to search through the whole chain for these inconsistencies and notify contract developers because they need to upgrade their (broken) contracts before the hardfork height.

superboyiii · 2022-09-09T04:16:38Z

Do you have the hash of the contract causing the problem? We may want to search through the whole chain for these inconsistencies and notify contract developers because they need to upgrade their (broken) contracts before the hardfork height.

0x08e7a002104fc8f3130e8ac74f550e8a3022e55f should be from HumswapBowls

roman-khimov · 2022-09-21T20:59:39Z

My current list (up to 2215218 height) is:

INFO    BAD EVENT       {"contract": "08e7a002104fc8f3130e8ac74f550e8a3022e55f", "event": "Transfer", "error": "parameter 3 type mismatch: ByteArray vs Integer"}
INFO    BAD EVENT       {"contract": "23dcef205da5ed0f48241c8f148bd76dafbc014d", "event": "Transfer", "error": "parameter 3 type mismatch: ByteArray vs Integer"}
INFO    BAD EVENT       {"contract": "42ff28c97a4e77308c2083a7ae4fc1e7fd837364", "event": "OnPausedChanged", "error": "parameter 0 type mismatch: Boolean vs Integer"}
INFO    BAD EVENT       {"contract": "4768c475e4c8465f2edf97f265c85950dfebc787", "event": "ApplySwapToPool", "error": "parameter 2 type mismatch: Boolean vs Integer"}
INFO    BAD EVENT       {"contract": "4768c475e4c8465f2edf97f265c85950dfebc787", "event": "ComputeSwap", "error": "parameter 2 type mismatch: Boolean vs Integer"}
INFO    BAD EVENT       {"contract": "4768c475e4c8465f2edf97f265c85950dfebc787", "event": "OnNEP17Payment", "error": "parameter 3 type mismatch: Boolean vs Integer"}
INFO    BAD EVENT       {"contract": "4768c475e4c8465f2edf97f265c85950dfebc787", "event": "Swap", "error": "parameter 3 type mismatch: Boolean vs Integer"}
INFO    BAD EVENT       {"contract": "4768c475e4c8465f2edf97f265c85950dfebc787", "event": "SwapFailure", "error": "parameter 3 type mismatch: Boolean vs Integer"}
INFO    BAD EVENT       {"contract": "577a51f7d39162c9de1db12a6b319c848e4c54e5", "event": "Debug", "error": "event Debug does not exist"}
INFO    BAD EVENT       {"contract": "65a27d7a6dcdf960bf2992f88b7b70f1057471e3", "event": "Authorized", "error": "parameter 2 type mismatch: Boolean vs Integer"}
INFO    BAD EVENT       {"contract": "6e644dda08a62f3fc9d14e824d1a3bd816a0c2d5", "event": "Fault", "error": "mismatch between the number of parameters and items"}
INFO    BAD EVENT       {"contract": "82be94eb34072de6e38b1ec212302c490a998c08", "event": "Authorized", "error": "parameter 2 type mismatch: Boolean vs Integer"}
INFO    BAD EVENT       {"contract": "9fa5ffe173dcd8c55dd51ed9720e0d1c9553442c", "event": "Authorized", "error": "parameter 2 type mismatch: Boolean vs Integer"}
INFO    BAD EVENT       {"contract": "cc638d55d99fc81295daccbaf722b84f179fb9c4", "event": "OrderCancelled", "error": "parameter 0 type mismatch: ByteArray vs Integer"}
INFO    BAD EVENT       {"contract": "cc638d55d99fc81295daccbaf722b84f179fb9c4", "event": "OrderCreated", "error": "parameter 1 type mismatch: ByteArray vs Integer"}
INFO    BAD EVENT       {"contract": "cc638d55d99fc81295daccbaf722b84f179fb9c4", "event": "OrderFilled", "error": "parameter 1 type mismatch: ByteArray vs Integer"}
INFO    BAD EVENT       {"contract": "de3fe8d30d98e049272db4b7bd641943b1e86872", "event": "Transfer", "error": "parameter 3 type mismatch: ByteArray vs Integer"}
INFO    BAD EVENT       {"contract": "edf179715646c9f07bd10b52c1eec44407297b8c", "event": "OnPausedChanged", "error": "parameter 0 type mismatch: Boolean vs Integer"}

And it's not a small one (please recheck though). As much as I like this change we can't just enable it at some height and hope it works, there will be some breakage and I don't think it's acceptable for mainnet. Suggestions are:

for 3.5.0 make this a logged warning instead of a hard failure
contact developers of all involved contracts and notify them that this warning will lead to failure in 3.6.0 (with some rough date estimate)
allow for some event "normalization", we have a lot of Boolean vs Integer for example, but booleans are routinely created by programs with PUSH0 and PUSH1 that create integers and it's not a contract developer's fault, so just like we substitute Buffer with ByteString (for a different reason, but still) we can take an Integer and cast it to Boolean in the Notify itself
a bit more controversial, but this can also be done to Integer emitted in place of ByteString
monitor the chain, check contract updates
release 3.6.0 with a hardfork that will change warning to failure

ixje · 2022-09-23T07:28:57Z

My current list (up to 2215218 height) is:

Very useful. I created an issue internally to address the Bool - Integer conversion and then for the contracts created with boa it should be a matter of recompiling with the latest version.

roman-khimov · 2022-09-23T08:11:46Z

Normalizing in compiler is an option too, even a better one, it's just that it requires contract updates and I'm not sure it can be done in timely manner for mainnet.

ixje · 2022-09-23T08:24:35Z

Normalizing in compiler is an option too, even a better one, it's just that it requires contract updates and I'm not sure it can be done in timely manner for mainnet.

I totally agree with that. I was mostly posting with the idea of indicating we're working on having a migration plan ready for the affected contracts created with boa, so the contract authors should have to spend as little time as possible on it.

roman-khimov · 2022-10-03T12:29:49Z

src/Neo/SmartContract/ApplicationEngine.Runtime.cs

+                case ContractParameterType.Integer:
+                    return aType == StackItemType.Integer;
+                case ContractParameterType.ByteArray:
+                case ContractParameterType.String:


Don't we want to have some additional checks for the String type? The way it is now it's absolutely the same as ByteArray, maybe some differentiation can be useful like if String could only contain valid UTF-8 byte sequence.

@shargon, how about this additional check?

@AnnaShaleva Done

superboyiii · 2022-10-17T08:06:19Z

@erikzhang Let's move on. It's in 3.5.0 release checklist.

superboyiii · 2022-10-19T07:03:47Z

for 3.5.0 make this a logged warning instead of a hard failure

Any example on how to log warning? I'm a little worried if we log it in cli or rpc, still can't be noticed by most people.

roman-khimov · 2022-10-19T08:03:27Z

Well, it's a usual log message (like the one from dBFT or RPC for example), but I know neo-cli does it in a bit different manner than neo-go. We have it in 0.99.4 release already and it's just another log message. Yes, people may not notice it, but they at least will have some chance and it also is tightly related to proper scheduling/announcement of this change. If developers will be aware of the change they will try to take a look at how their contracts behave, if they care about them. If they don't, well, then the contract is obsolete already in some ways.

superboyiii · 2022-10-26T07:51:35Z

Well, it's a usual log message (like the one from dBFT or RPC for example), but I know neo-cli does it in a bit different manner than neo-go. We have it in 0.99.4 release already and it's just another log message. Yes, people may not notice it, but they at least will have some chance and it also is tightly related to proper scheduling/announcement of this change. If developers will be aware of the change they will try to take a look at how their contracts behave, if they care about them. If they don't, well, then the contract is obsolete already in some ways.

Maybe we can send mail to these contract deployers since their emails are in manifest.

superboyiii · 2022-10-27T09:02:21Z

I've sent a mail to these admins to ask them pay attention for this change, I think we could move on.

mfbz · 2022-10-27T21:44:19Z

Hi all,
I saw from the list that our OnPausedChanged method is triggering a "Boolean vs Integer" error.
What are we doing wrong?

This is our event:
public static event Action<bool> OnPausedChanged;

This is how it's described in the manifest:
{"name":"OnPausedChanged","parameters":[{"name":"obj","type":"Boolean"}]}

This is how we are using it:

/// <summary>
/// Update contract paused state without access control.
/// </summary>
/// <remarks>
/// Internal function without access restriction.
/// </remarks>
/// <param name="paused">The new paused value to be set.</param>
protected static void UpdatePaused(bool paused)
{
	PausedStore.Put(paused);
	OnPausedChanged(paused);
}

superboyiii · 2022-10-28T05:38:37Z

Hi all, I saw from the list that our OnPausedChanged method is triggering a "Boolean vs Integer" error. What are we doing wrong?

This is our event: public static event Action<bool> OnPausedChanged;

This is how it's described in the manifest: {"name":"OnPausedChanged","parameters":[{"name":"obj","type":"Boolean"}]}

This is how we are using it:
/// <summary>
/// Update contract paused state without access control.
/// </summary>
/// <remarks>
/// Internal function without access restriction.
/// </remarks>
/// <param name="paused">The new paused value to be set.</param>
protected static void UpdatePaused(bool paused)
{
	PausedStore.Put(paused);
	OnPausedChanged(paused);
}

Nothing wrong, currently Neo push integer instead of bool in VM which will cause break in ABI check. It will be fixed in neo-project/neo-vm#497

mfbz · 2022-10-28T07:14:18Z

Ok, so we don't have to change anything?

superboyiii · 2022-10-28T11:28:04Z

Ok, so we don't have to change anything?

Yes!

Follow the neo-project/neo#2884. A part of the neo-project/neo#2810. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva · 2023-08-16T11:05:03Z

src/Neo/SmartContract/ApplicationEngine.Runtime.cs

                case ContractParameterType.ByteArray:
                case ContractParameterType.String:
-                    return aType is StackItemType.Any or StackItemType.ByteString or StackItemType.Buffer &&
-                        Utility.StrictUTF8.GetString(item.GetSpan()) is not null; // Prevent any non-UTF8 string
+                    {
+                        if (aType is StackItemType.Any or StackItemType.ByteString or StackItemType.Buffer)
+                        {


Now this check is being performed for both cases when type is either ContractParameterType.ByteArray or ContractParameterType.String. But the initial idea was to perform this check only for ContractParameterType.String and let the ContractParameterType.ByteArray be as is.

AnnaShaleva · 2023-08-16T13:29:00Z

src/Neo/SmartContract/ApplicationEngine.Runtime.cs

+                case ContractParameterType.String:
+                    {
+                        if (aType is StackItemType.Any or StackItemType.ByteString or StackItemType.Buffer)
+                        {
+                            try
+                            {
+                                _ = Utility.StrictUTF8.GetString(item.GetSpan()); // Prevent any non-UTF8 string
+                                return true;
+                            }
+                            catch { }
+                        }
+                        return false;
+                    }


And the last question from my side: if we have Null stackitem passed as notification parameter and in manifest it is declared ContractParameterType.String, what should happen? The previous version allowed this, but seems that the current code will throw the InvalidCastException exception on item.GetSpan() call for Null stackitem. But I think it's valid to pass Null stackitem as ContractParameterType.String.

@shargon, @roman-khimov, what do you think?

@AnnaShaleva, you're programming in Go, string can't be null there!

Jokes aside, I don't think I have any strong preference here. I'm slightly more in favor of making strings always have some contents (even if it's "", basically the way it is now), but if there are useful cases where Null is applicable, we can accept it too. We can also try having more strict policy for now and then relax it if needed (it's always easier than the other way around).

Let's then adjust a bit the current check so that it'll be more explicit:

if (aType is StackItemType.ByteString or StackItemType.Buffer)

StackItemType.Any normal indicates NULL. But normally depends on where it is.
you can always check for StackItem.IsNull or StackItem.Null
Maybe treat strings the same as string.IsNullOrEmpty() does.

I think that should be empty, not null

@shargon, good, then see the #2810 (comment), please.

superboyiii · 2023-08-17T08:06:57Z

I've compared all block data, at least it's compatible before HFBasilisk on both mainnet and testnet. Will make some tests about expected behaviour after HFBasilisk.

src/Neo/SmartContract/ApplicationEngine.Runtime.cs

Co-authored-by: Anna Shaleva <anna@nspcc.ru>

Follow the neo-project/neo#2884. A part of the neo-project/neo#2810. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

Follow the neo-project/neo#2810 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

Follow the neo-project/neo#2884. A part of the neo-project/neo#2810. Fix failing tests along the way (a lot of them have invalid notifications format which differs from the one declared in manifest). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

Follow the neo-project/neo#2810 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

cschuchardt88 · 2023-09-07T09:41:12Z

// is
Runtime.Notify("Hello", new[] { "World" });
// the as
[DisplayName("Hello")]
public static event Action<string> event_name;
event_name("world");

Just wondering if the above calls the same function in the VM. I don't make smart contracts so i don't know.

Check ABI for notifications

e88062f

erikzhang mentioned this pull request Sep 2, 2022

fix nep11 tracker neo-project/neo-modules#755

Closed

vncoelho reviewed Sep 2, 2022

View reviewed changes

superboyiii mentioned this pull request Sep 5, 2022

Neo v3.5.0 Checklist #2811

Closed

shargon previously approved these changes Sep 6, 2022

View reviewed changes

Merge branch 'master' into check-abi-for-notifications

f0b3071

roman-khimov mentioned this pull request Sep 20, 2022

ABI checks for notifications nspcc-dev/neo-go#2703

Closed

roman-khimov mentioned this pull request Sep 23, 2022

Notification normalization in the compler nspcc-dev/neo-go#2713

Closed

Merge branch 'master' into check-abi-for-notifications

c7e9ced

roman-khimov reviewed Oct 3, 2022

View reviewed changes

superboyiii mentioned this pull request Oct 28, 2022

Add PUSHT and PUSHF neo-project/neo-vm#497

Merged

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Aug 10, 2023

core: move contract notifications check under HFBasilisk

4bb7591

Follow the neo-project/neo#2884. A part of the neo-project/neo#2810. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

Jimmy and others added 2 commits August 15, 2023 19:46

Merge branch 'master' into check-abi-for-notifications

b812394

Check UTF8 string

93b4126

shargon previously approved these changes Aug 16, 2023

View reviewed changes

Cleaner way

be2ef40

shargon dismissed their stale review via be2ef40 August 16, 2023 10:26

AnnaShaleva reviewed Aug 16, 2023

View reviewed changes

Fix Buffer

780103a

AnnaShaleva reviewed Aug 16, 2023

View reviewed changes

AnnaShaleva reviewed Aug 17, 2023

View reviewed changes

src/Neo/SmartContract/ApplicationEngine.Runtime.cs Outdated Show resolved Hide resolved

steven1227 approved these changes Aug 18, 2023

View reviewed changes

Jim8y previously approved these changes Aug 18, 2023

View reviewed changes

Update src/Neo/SmartContract/ApplicationEngine.Runtime.cs

3f10c96

Co-authored-by: Anna Shaleva <anna@nspcc.ru>

Jim8y dismissed their stale review via 3f10c96 August 18, 2023 08:11

AnnaShaleva approved these changes Aug 18, 2023

View reviewed changes

Jim8y approved these changes Aug 18, 2023

View reviewed changes

shargon approved these changes Aug 18, 2023

View reviewed changes

shargon merged commit 773b96a into master Aug 18, 2023

shargon deleted the check-abi-for-notifications branch August 18, 2023 09:24

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Aug 18, 2023

core: move contract notifications check under HFBasilisk

fe1f812

Follow the neo-project/neo#2884. A part of the neo-project/neo#2810. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Aug 18, 2023

smartcontract: disallow Null and non-utf8 String

892d51f

Follow the neo-project/neo#2810 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Aug 18, 2023

smartcontract: disallow Null and non-utf8 String

bb2a99d

Follow the neo-project/neo#2810 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

roman-khimov mentioned this pull request Sep 7, 2023

reconsider PR #2841 #2900

Closed

vang1ong7ang mentioned this pull request Sep 16, 2023

checklist of 3.6.0 PRs that will cause FORK #2910

Closed

5 tasks

roman-khimov mentioned this pull request Sep 21, 2023

Contract call arguments are not type-checked against contract's manifest #1891

Open

cschuchardt88 mentioned this pull request Dec 25, 2023

Fix: ban notify from contract developer neo-project/neo-devpack-dotnet#845

Merged

roman-khimov mentioned this pull request Jul 25, 2024

[Neo Core Fix] Check params at Calling Contract #3438

Closed

16 tasks

Conversation

erikzhang commented Sep 2, 2022

Uh oh!

superboyiii commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roman-khimov commented Sep 2, 2022

Uh oh!

vncoelho commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vncoelho left a comment

Choose a reason for hiding this comment

Uh oh!

superboyiii commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roman-khimov commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

superboyiii commented Sep 9, 2022

Uh oh!

roman-khimov commented Sep 21, 2022

Uh oh!

ixje commented Sep 23, 2022

Uh oh!

roman-khimov commented Sep 23, 2022

Uh oh!

ixje commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superboyiii commented Oct 17, 2022

Uh oh!

superboyiii commented Oct 19, 2022

Uh oh!

roman-khimov commented Oct 19, 2022

Uh oh!

superboyiii commented Oct 26, 2022

Uh oh!

superboyiii commented Oct 27, 2022

Uh oh!

mfbz commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

superboyiii commented Oct 28, 2022

Uh oh!

mfbz commented Oct 28, 2022

Uh oh!

superboyiii commented Oct 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superboyiii commented Aug 17, 2023

Uh oh!

Uh oh!

cschuchardt88 commented Sep 7, 2023

Uh oh!

Reviewers

Assignees

Labels

superboyiii commented Sep 2, 2022 •

edited

Loading

vncoelho commented Sep 2, 2022 •

edited

Loading

superboyiii commented Sep 8, 2022 •

edited

Loading

roman-khimov commented Sep 8, 2022 •

edited

Loading

ixje commented Sep 23, 2022 •

edited

Loading

mfbz commented Oct 27, 2022 •

edited

Loading