Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StrictMode during contract deploy/update will break the T5 after 3.6 release #2873

Closed
AnnaShaleva opened this issue Jul 4, 2023 · 12 comments · Fixed by #2881
Closed

StrictMode during contract deploy/update will break the T5 after 3.6 release #2873

AnnaShaleva opened this issue Jul 4, 2023 · 12 comments · Fixed by #2881

Comments

@AnnaShaleva
Copy link
Member

Describe the bug
#2849 brings back the strict contract script checking during contract deploy and update. This patch was checked for the mainnet against hardforks creation (see #2849 (comment)), and it's confirmed that #2849 doesn't cause any states differences in mainnet.

However, in NeoGo we've had this strict script check before the #2849 and it's currently enabled in NeoGo 0.101.2 running in T5. A couple of days ago we've faced with the issue nspcc-dev/neo-go#3049: our T5 node stopped accepting blocks since 2272533 height.

We've checked the roots of this problem, it's caused by the fact that strict contract script check is enabled in our node. Since #2849 is included into 3.6 release, it will break the current T5. We either need a hardfork for T5 that enables contract script checking code starting from some height or we need to deploy a new testnet after 3.6 release. Otherwise current T5 will be broken by 3.6 release and won't be able to accept new blocks after 2272533.

To Reproduce
Please, see the checking results in nspcc-dev/neo-go#3049 (comment).

Expected behavior
Testnet should function normally after 3.6 release.

Platform:

  • Version: 3.5.0
@roman-khimov
Copy link
Contributor

We need T6. Time to refresh the testnet.

@shargon
Copy link
Member

shargon commented Jul 4, 2023

Only testnet is affected?

@AnnaShaleva
Copy link
Member Author

AnnaShaleva commented Jul 4, 2023

Yes. Current mainnet states perfectly match between C# and Go nodes, and mainnet's NeoGo node curently also has this strict script check enabled.

@superboyiii
Copy link
Member

superboyiii commented Jul 5, 2023

#2810 need a fork height, so it can also be applied on #2849. If we deploy an T6 testnet, we should make sure if all projects on Neo will not be affected.

@AnnaShaleva
Copy link
Member Author

@superboyiii, as Roman mentioned in #2810 (comment), it would be better to add just a logger warning for #2810 in 3.6. Otherwise it will break a lot of contracts in mainnet which is quite unexpected for their maintainers. With logger warning in 3.6 the users will have a chance to gracefully update their contracts. And logger warning doesn’t require a hardfork height, thus, don’t we want to think about new T6 testnet setup for #2849?

@AnnaShaleva
Copy link
Member Author

If T5 hardfork is preferable, then we probably need to add some enabling/disabling code to #2849 within the scope of this issue.

@shargon
Copy link
Member

shargon commented Jul 5, 2023

Also #2827 should be included

@superboyiii
Copy link
Member

If T5 hardfork is preferable, then we probably need to add some enabling/disabling code to #2849 within the scope of this issue.

@shargon What's your opinion?

@shargon
Copy link
Member

shargon commented Jul 6, 2023

If T5 hardfork is preferable, then we probably need to add some enabling/disabling code to #2849 within the scope of this issue.

@shargon What's your opinion?

We need versioning from the beginning
#2987

@superboyiii
Copy link
Member

If T5 hardfork is preferable, then we probably need to add some enabling/disabling code to #2849 within the scope of this issue.

@shargon What's your opinion?

We need versioning from the beginning neo-project/neo#2987

We need release v3.6.0 this week so maybe not much time for #2987

@superboyiii
Copy link
Member

@roman-khimov @shargon We need move on. I suggest making a fork height temporarily for testnet. Then we could further discuss on #2987, make the solution into v3.7.0.

@roman-khimov
Copy link
Contributor

Notice that if 3.6 was released in ~April this wouldn't be an issue.

Now I don't see any other choice but a fork height kludge for 3.6. Maybe a special one (not using a proper hardfork name) since it can be removed in some future version when T5 will be gone (likely after the hackathon).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants