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

Use strictMode during deploy #2849

Merged
merged 5 commits into from
Apr 10, 2023
Merged

Use strictMode during deploy #2849

merged 5 commits into from
Apr 10, 2023

Conversation

shargon
Copy link
Member

@shargon shargon commented Mar 12, 2023

Close #2848

@shargon shargon requested a review from devhawk March 12, 2023 09:46
@Jim8y
Copy link
Contributor

Jim8y commented Mar 12, 2023

Does not this change need a fork? How about setting it efficent after xxx height?

@shargon
Copy link
Member Author

shargon commented Mar 13, 2023

@superboyiii could you test it on mainnet?

@superboyiii
Copy link
Member

@superboyiii could you test it on mainnet?

I will

@shargon shargon changed the title Check invalid jumps Use strictMode during deploy Mar 13, 2023
@steven1227
Copy link
Member

Can we include this one in 3.6.0 release ?

@shargon
Copy link
Member Author

shargon commented Mar 17, 2023

@superboyiii could you test it with #2810 ? I would like to merge these two ones.

@roman-khimov
Copy link
Contributor

@shargon, I doubt we're ready for #2810 in mainnet.

@superboyiii
Copy link
Member

Can we include this one in 3.6.0 release ?

Sure, done

@superboyiii
Copy link
Member

@superboyiii could you test it with #2810 ? I would like to merge these two ones.

We need fork height in #2810, otherwise, it will break everything!

@superboyiii
Copy link
Member

Tested on mainnet, seems the storge differs a lot by this PR.
For example:
1679451612560
We can follow the same fork height of #2810 once it's added to ensure it will not break anything.

@roman-khimov
Copy link
Contributor

Something is wrong. Either with a patch, or with a test. Script checking is "strict" in NeoGo and we're stateroot-compatible with 3.5.0 C# node for mainnet. This PR is very much different from #2810.

@superboyiii
Copy link
Member

Something is wrong. Either with a patch, or with a test. Script checking is "strict" in NeoGo and we're stateroot-compatible with 3.5.0 C# node for mainnet. This PR is very much different from #2810.

I will double check again. But how could it be stateroot-compatible with 3.5.0 C# node? It's already changed the code of native contract - ContractManagement, the state root should be changed from genesis block. I mean the storge of genesis block will be differed.

@roman-khimov
Copy link
Contributor

You can either rebase this PR to 3.5.0 and try it this way (it'll work), or collect state change data for the current master and current master + this PR. NeoGo 0.101.1 is 3.5.0-compatible, it doesn't have current 3.5.0..master changes from C# node, but it has a logic similar to "strict" mode and it's stateroot-compatible (because "strictness" doesn't require genesis block changes).

@superboyiii
Copy link
Member

superboyiii commented Mar 31, 2023

You can either rebase this PR to 3.5.0 and try it this way (it'll work), or collect state change data for the current master and current master + this PR. NeoGo 0.101.1 is 3.5.0-compatible, it doesn't have current 3.5.0..master changes from C# node, but it has a logic similar to "strict" mode and it's stateroot-compatible (because "strictness" doesn't require genesis block changes).

OK, find the problem. My local based master was pull including #2841, however I pull this branch several days ago when #2841 has not been merged yet. So the difference should be caused by #2841

@superboyiii
Copy link
Member

Yes, tested, it's compatible.
1680508575326

@shargon shargon merged commit a20eb4b into master Apr 10, 2023
@shargon shargon deleted the check-script branch April 10, 2023 09:14
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Jul 4, 2023
This check is good and was present here since #1729, but it was
accidently removed from the reference implementation (see the
discussion in neo-project/neo#2848). The
removal of this check from the C# node leaded to the T5 testnet state
diff since 1670095 heigh which causes inability to process new blocks
since 2272533 height (see #3049). This check was added back to the
C# node in neo-project/neo#2849, but it is
planned to be the part of the upcoming 3.6.0 C# node release.

We need to keep our testnet healthy, thus, strict contract script
check will be temporary removed from the node code and is planned
to be added back to be a part of the next 3.6.0-compatible release.

Close #3049.
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Jul 4, 2023
This check is good and was present here since #1729, but it was
accidently removed from the reference implementation (see the
discussion in neo-project/neo#2848). The
removal of this check from the C# node leaded to the T5 testnet state
diff since 1670095 heigh which causes inability to process new blocks
since 2272533 height (see #3049). This check was added back to the
C# node in neo-project/neo#2849, but it is
planned to be the part of the upcoming 3.6.0 C# node release.

We need to keep our testnet healthy, thus, strict contract script
check will be temporary removed from the node code and is planned
to be added back to be a part of the next 3.6.0-compatible release.

Close #3049.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Jul 4, 2023
This check is good and was present here since #1729, but it was
accidently removed from the reference implementation (see the
discussion in neo-project/neo#2848). The
removal of this check from the C# node leaded to the T5 testnet state
diff since 1670095 heigh which causes inability to process new blocks
since 2272533 height (see #3049). This check was added back to the
C# node in neo-project/neo#2849, but it is
planned to be the part of the upcoming 3.6.0 C# node release.

We need to keep our testnet healthy, thus, strict contract script
check will be temporary removed from the node code and is planned
to be added back to be a part of the next 3.6.0-compatible release.

Close #3049.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Jul 8, 2023
This check is good and was present here since #1729, but it was
accidently removed from the reference implementation (see the
discussion in neo-project/neo#2848). The
removal of this check from the C# node leaded to the T5 testnet state
diff since 1670095 heigh which causes inability to process new blocks
since 2272533 height (see #3049). This check was added back to the
C# node in neo-project/neo#2849, but it is
planned to be the part of the upcoming 3.6.0 C# node release.

We need to keep our testnet healthy, thus, strict contract script
check will be temporary removed from the node code and is planned
to be added back to be a part of the next 3.6.0-compatible release.

Close #3049.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@dusmart
Copy link

dusmart commented Sep 16, 2023

Does not this change need a fork? How about setting it efficent after xxx height?

Great question. I thought that everyone won't care too much about the hardfork.

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 this pull request may close these issues.

Native Management contract allows to deploy contract with invalid method offset
8 participants