Native: revert Update/Deploy callflag change for pre-Aspidochelone#3909
Merged
Native: revert Update/Deploy callflag change for pre-Aspidochelone#3909
Conversation
#2653 changed required callflags of native ContractManagement's Update and Deploy methods from States|AllowNotify to All. This change didn't affect N3 mainnet/T5 (ref. #2673), but the problem is that this change affected NeoFS mainnet network (see nspcc-dev/neo-go#2848 and commits description). This commit fixes state difference between Go and C# nodes at height 451626 of NeoFS mainnet. Note that this commit does not affect existing N3 mainnet/testnet states, so no resynchronisation is required on update. The difference itself: ``` go run scripts/compare-dumps/compare-dumps.go ./godump-echidna-neofs-mainnet/ ../../neo-project/neo/neo-cli-notary-mainnet/Storage_0572dfa5/ Processing directory BlockStorage_0 Processing directory BlockStorage_100000 Processing directory BlockStorage_200000 Processing directory BlockStorage_300000 Processing directory BlockStorage_400000 Processing directory BlockStorage_500000 file BlockStorage_500000/dump-block-452000.json: block 451626, changes length mismatch: 25 vs 11 compare-dumps dumpDirA dumpDirB exit status 1 ``` Go node application log for the problem transaction: ``` anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "getapplicationlog", "params": ["0x5028585a5c27b7f357771fa8b512c2d1b0ba40dcb3ea30e67d3db2d75d2da60d"] }' localhost:40332 | json_pp % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 592 100 450 100 142 461k 145k --:--:-- --:--:-- --:--:-- 578k { "id" : 1, "jsonrpc" : "2.0", "result" : { "executions" : [ { "exception" : null, "gasconsumed" : "900316660", "invocations" : null, "notifications" : [ { "contract" : "0xfffdc93764dbaddd97c48f252a53ea4643faa3fd", "eventname" : "Update", "state" : { "type" : "Array", "value" : [ { "type" : "ByteString", "value" : "r6jbcP2s5N7DIvRYS2ZiFdP7YXA=" } ] } } ], "stack" : [ { "type" : "Any" } ], "trigger" : "Application", "vmstate" : "HALT" } ], "txid" : "0x5028585a5c27b7f357771fa8b512c2d1b0ba40dcb3ea30e67d3db2d75d2da60d" } } ``` C# application log for the same transaction: ``` anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "getapplicationlog", "params": ["0x5028585a5c27b7f357771fa8b512c2d1b0ba40dcb3ea30e67d3db2d75d2da60d"] }' localhost:50332 | json_pp % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 439 0 297 100 142 3502 1674 --:--:-- --:--:-- --:--:-- 5226 { "id" : 1, "jsonrpc" : "2.0", "result" : { "executions" : [ { "exception" : "Cannot call this method with the flag States, AllowNotify.", "gasconsumed" : "5684010", "notifications" : [], "stack" : [], "trigger" : "Application", "vmstate" : "FAULT" } ], "txid" : "0x5028585a5c27b7f357771fa8b512c2d1b0ba40dcb3ea30e67d3db2d75d2da60d" } } ``` Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Member
|
I will test mainnet as well based on C# node |
vncoelho
reviewed
Apr 22, 2025
Member
vncoelho
left a comment
There was a problem hiding this comment.
I think it is good to go if confirmed that no changes on mainnet.
shargon
reviewed
Apr 22, 2025
Member
shargon
left a comment
There was a problem hiding this comment.
Dirty fix for all invocations...
shargon
reviewed
Apr 22, 2025
Member
shargon
left a comment
There was a problem hiding this comment.
@AnnaShaleva we should try to fix it with manifests
Make these checks implementation-specific in order not to affect the general native invocation code. Ref. #3909 (review). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
AnnaShaleva
commented
Apr 22, 2025
Member
Author
There was a problem hiding this comment.
@AnnaShaleva we should try to fix it with manifests
Yes, this way it should also work.
also checking the Id
I don't think that ID check is needed with the updated implementation because Deploy/Update handlers are bound to native Management, we already sure that ID is -1.
@roman-khimov, please also take a look. This fix differs from NeoGo version, but essentially the behaviour will be almost the same (except for edge cases where transaction may fail after Invoke-level flags check but before Management-level handler execution itself). But even for these cases transaction is FAULTed, so there shouldn't be any difference.
roman-khimov
approved these changes
Apr 22, 2025
Contributor
roman-khimov
left a comment
There was a problem hiding this comment.
Works for me as well.
Member
Author
|
It is confirmed that Go/C# states are compatible for both NeoFS Mainnet and Testnet (current master + this PR + #3178): Up to of 9957963 height of NeoFS Mainnet.Up to of 10769566 height of NeoFS Testnet.N3 chains synchronisation is in progress. |
Member
Author
@superboyiii, just for the record: I've just finished N3 mainnet synchronisation and discovered hardfork activation bug during states comparison. So to properly test this PR on mainnet you either need to use #3910 or need to explicitly set |
Member
Author
|
Go/C# N3 Mainnet/Testnet dumps also checked up to Aspidochelone+ height, everything is OK (with #3910 patch applied over this PR). So synchronisation checks are passed, this PR is ready to be merged from our side. |
Wi1l-B0t
approved these changes
Apr 24, 2025
Member
|
Tested OK from C# node |
superboyiii
approved these changes
Apr 24, 2025
cschuchardt88
pushed a commit
to cschuchardt88/neo
that referenced
this pull request
Jun 8, 2025
…eo-project#3909) * Native: revert Update/Deploy callflag change for pre-Aspidochelone neo-project#2653 changed required callflags of native ContractManagement's Update and Deploy methods from States|AllowNotify to All. This change didn't affect N3 mainnet/T5 (ref. neo-project#2673), but the problem is that this change affected NeoFS mainnet network (see nspcc-dev/neo-go#2848 and commits description). This commit fixes state difference between Go and C# nodes at height 451626 of NeoFS mainnet. Note that this commit does not affect existing N3 mainnet/testnet states, so no resynchronisation is required on update. The difference itself: ``` go run scripts/compare-dumps/compare-dumps.go ./godump-echidna-neofs-mainnet/ ../../neo-project/neo/neo-cli-notary-mainnet/Storage_0572dfa5/ Processing directory BlockStorage_0 Processing directory BlockStorage_100000 Processing directory BlockStorage_200000 Processing directory BlockStorage_300000 Processing directory BlockStorage_400000 Processing directory BlockStorage_500000 file BlockStorage_500000/dump-block-452000.json: block 451626, changes length mismatch: 25 vs 11 compare-dumps dumpDirA dumpDirB exit status 1 ``` Go node application log for the problem transaction: ``` anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "getapplicationlog", "params": ["0x5028585a5c27b7f357771fa8b512c2d1b0ba40dcb3ea30e67d3db2d75d2da60d"] }' localhost:40332 | json_pp % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 592 100 450 100 142 461k 145k --:--:-- --:--:-- --:--:-- 578k { "id" : 1, "jsonrpc" : "2.0", "result" : { "executions" : [ { "exception" : null, "gasconsumed" : "900316660", "invocations" : null, "notifications" : [ { "contract" : "0xfffdc93764dbaddd97c48f252a53ea4643faa3fd", "eventname" : "Update", "state" : { "type" : "Array", "value" : [ { "type" : "ByteString", "value" : "r6jbcP2s5N7DIvRYS2ZiFdP7YXA=" } ] } } ], "stack" : [ { "type" : "Any" } ], "trigger" : "Application", "vmstate" : "HALT" } ], "txid" : "0x5028585a5c27b7f357771fa8b512c2d1b0ba40dcb3ea30e67d3db2d75d2da60d" } } ``` C# application log for the same transaction: ``` anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "getapplicationlog", "params": ["0x5028585a5c27b7f357771fa8b512c2d1b0ba40dcb3ea30e67d3db2d75d2da60d"] }' localhost:50332 | json_pp % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 439 0 297 100 142 3502 1674 --:--:-- --:--:-- --:--:-- 5226 { "id" : 1, "jsonrpc" : "2.0", "result" : { "executions" : [ { "exception" : "Cannot call this method with the flag States, AllowNotify.", "gasconsumed" : "5684010", "notifications" : [], "stack" : [], "trigger" : "Application", "vmstate" : "FAULT" } ], "txid" : "0x5028585a5c27b7f357771fa8b512c2d1b0ba40dcb3ea30e67d3db2d75d2da60d" } } ``` Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: move Update/Deploy callflags check to Management implementation Make these checks implementation-specific in order not to affect the general native invocation code. Ref. neo-project#3909 (review). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> --------- Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
#2653 changed required callflags of native ContractManagement's Update and Deploy methods from
States|AllowNotifytoAll. This change didn't affect N3 mainnet/T5 (ref. #2673), but the problem is that this change affected NeoFS mainnet network (see nspcc-dev/neo-go#2848 and nspcc-dev/neo-go@5d478b5 in particular).This commit fixes state difference between Go and C# nodes at height 451626 of NeoFS mainnet. Note that this commit does not affect existing N3 mainnet/testnet states, so no resynchronisation is required on the node update.
The difference itself:
Go node application log for the problem transaction in block 451626
C# application log for the same transaction
Type of change
How Has This Been Tested?
We used this fix in NeoGo and it is confirmed that this fix does not affect existing N3 chains state. However, I'm now syncing chains to ensure that the same fix works for C# node, just in case. @superboyiii, you may also take a look.
Checklist: