neo Check ABI for notifications

Close #1879

Sep 02 '22 02:09 erikzhang

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

Sep 02 '22 02:09 superboyiii

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

Sep 02 '22 06:09 roman-khimov

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.

Sep 02 '22 09:09 vncoelho

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

Sep 08 '22 10:09 superboyiii

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.

Sep 08 '22 11:09 roman-khimov

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

Sep 09 '22 04:09 superboyiii

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

Sep 21 '22 20:09 roman-khimov

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.

Sep 23 '22 07:09 ixje

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.

Sep 23 '22 08:09 roman-khimov

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.

Sep 23 '22 08:09 ixje

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

Oct 17 '22 08:10 superboyiii

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.

Oct 19 '22 07:10 superboyiii

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.

Oct 19 '22 08:10 roman-khimov

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.

Oct 26 '22 07:10 superboyiii

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

Oct 27 '22 09:10 superboyiii

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);
}

Oct 27 '22 21:10 mfbz

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 https://github.com/neo-project/neo-vm/pull/497

Oct 28 '22 05:10 superboyiii

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

Oct 28 '22 07:10 mfbz

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

Yes!

Oct 28 '22 11:10 superboyiii

Wait this to be merged in 3.6.0

Nov 25 '22 03:11 superboyiii

Can we make it a logged warning for 3.6.0? If we're to merge it as is it'll break mainnet for sure, but a logged warning may bring a bit more attention to it.

Apr 04 '23 15:04 roman-khimov

Ready to test @superboyiii

Aug 09 '23 08:08 shargon

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.

Aug 17 '23 08:08 superboyiii

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

Sep 07 '23 09:09 cschuchardt88