fix hardfork issues by Jim8y · Pull Request #3234 · neo-project/neo

Jim8y · 2024-05-15T00:24:46Z

Description

This pr fixes the hardfork issue in the 3.7.

Fixes # (issue)

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Test Configuration:

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

vang1ong7ang · 2024-05-15T01:06:28Z

is the manifests of native contracts fixed?

vang1ong7ang · 2024-05-15T06:42:11Z

@Jim8y I think the commit c1af443 introduces trouble

src/Neo/SmartContract/Native/NativeContract.cs

roman-khimov · 2024-05-15T07:01:44Z

src/Neo/SmartContract/Native/RoleManagement.cs

            if (Ledger.CurrentIndex(snapshot) + 1 < index)
                throw new ArgumentOutOfRangeException(nameof(index));
+            // Fix hardfork for https://github.com/neo-project/neo/pull/3172
+            if (role == Role.P2PNotary && !ProtocolSettings.Custom.IsHardforkEnabled(Hardfork.HF_Cockatrice, Ledger.CurrentIndex(snapshot)))


Breaks NeoFS chains immediately. I've tried to explain why we're doing it this way numerous times, but looks like no one cares about NeoFS and compatibility.

breaks neofs or break the mainnet, this is a problem. We will have it but can not make it take effect untill we have the fork.

Please, revert #3172 then. It causes more problems than it solves with this HF logic.

BTW, it can only be exploited by committee. Nasty committee.

BTW, it can only be exploited by committee. Nasty committee.

Hardly ever committee will designate this role before Notary subsystem implementation is finished. Thus, I'd vote for keeping P2PNotary role without binding to hardfork.

BTW, it can only be exploited by committee

Sorry, not exactly. getDesignatedByRole will return empty array instead of failing.

Please, revert #3172 then. It causes more problems than it solves with this HF logic.

I think reverting #3172 is better than fixing. maybe #3172 can be introduced back after there's good way for node upgrading

This reverts commit c1af443.

AnnaShaleva · 2024-05-15T07:21:39Z

src/Neo/SmartContract/Native/ContractMethodAttribute.cs

        public long CpuFee { get; init; }
        public long StorageFee { get; init; }
        public Hardfork? ActiveIn { get; init; } = null;
+        public Hardfork? DeprecatedIn { get; init; } = null;


BTW, this is a part of #3210. Add a reference to the PR description, please.

Also, we need the same functionality to be implemented for events. It's not hard to extend it, read the original issue.

This pr is a hotpatch for the hf issues, i prefer to keep it as simple as possible.

Ref. #3210 (comment) as TODO.

Rename to ActiveTil?

AnnaShaleva · 2024-05-15T07:28:48Z

src/Neo/SmartContract/Native/ContractMethodAttribute.cs

            ActiveIn = activeIn;
        }
+
+        public ContractMethodAttribute(bool isDeprecated, Hardfork deprecatedIn)


Suggested change

public ContractMethodAttribute(bool isDeprecated, Hardfork deprecatedIn)

public ContractMethodAttribute(Hardfork? activeIn, Hardfork deprecatedIn)

I'd suggest to use this option, and just put null as the first argument for those methods that were always active but need to be deprecated.

Existing solution already works, i may update it after we have confirmed that this pr works fine.

Ref. #3210 (comment) as TODO.

AnnaShaleva · 2024-05-15T07:31:51Z

src/Neo/SmartContract/Native/CryptoLib.cs

+
+        // This is for solving the hardfork issue in https://github.com/neo-project/neo/pull/3209
+        [ContractMethod(true, Hardfork.HF_Cockatrice, CpuFee = 1 << 15)]
+        public static bool VerifyWithECDsa(byte[] message, byte[] pubkey, byte[] signature, NamedCurve curve)


You may safely get rid of NamedCurve type here and use NamedCurveHash, just check that curve falls within the expected range (22 or 23) and throw if not. You can do this because NamedCurveHash is compatible. If you implement this, then no 'NamedCurve' is obsolete linter warnings will be raised.

This is a hotpatch, i prefer to keep it simple and use original logic as much as possible.

Every TODO is an issue, otherwise it won't be done. See #3236.

src/Neo/SmartContract/Native/NeoToken.cs

vang1ong7ang · 2024-05-15T12:11:40Z

manifest of NeoToken contract still causes fork

event CommitteeChanged should be fixed

superboyiii

Tested OK. This fixed.

tests/Neo.UnitTests/SmartContract/Native/UT_RoleManagement.cs

AnnaShaleva · 2024-05-16T07:36:12Z

src/Neo/SmartContract/Native/CryptoLib.cs

+
+        // This is for solving the hardfork issue in https://github.com/neo-project/neo/pull/3209
+        [ContractMethod(true, Hardfork.HF_Cockatrice, CpuFee = 1 << 15)]
+        public static bool VerifyWithECDsa(byte[] message, byte[] pubkey, byte[] signature, NamedCurve curve)


Every TODO is an issue, otherwise it won't be done. See #3236.

shargon

Seems good to me, only the rename to ActiveTil, but also it's ok like this

vncoelho · 2024-05-16T09:39:48Z

src/Neo/SmartContract/Native/NeoToken.cs

-
-                if (!newCommittee.SequenceEqual(prevCommittee))
+                // Hardfork check for https://github.com/neo-project/neo/pull/3158
+                // New notification will case 3.7.0 and 3.6.0 have different behavior


vncoelho

Good job @Jim8y

The deprecatedIn function was really needed for many cases and pending to be done!

DeprecatedIn hardfork of every native method (if not null) should be included into the set of used hardforks, otherwise no contract update will be performed on this hardfork activation: https://github.com/neo-project/neo/blob/08f2dfc56762bb43be33e873bc3659bad368d4d6/src/Neo/SmartContract/Native/NativeContract.cs#L284 This commit should be a part of #3234 and a part of 3.7.4. Luckily, this new DeprecatedIn functionality is used only for the old native CryptoLib's verifyWithECDsa method with Cockatrice hardfork. And luckily, there are other CryptoLib's methods with ActiveIn set to Cockatrice hardfork. Due to these two facts this bug does not affect mainnet/testnet, and thus we don't need 3.7.5 with this fixincluded, so this fix may safely be postponed to 3.8.0. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

DeprecatedIn hardfork of every native method (if not null) should be included into the set of used hardforks, otherwise no contract update will be performed on this hardfork activation: https://github.com/neo-project/neo/blob/08f2dfc56762bb43be33e873bc3659bad368d4d6/src/Neo/SmartContract/Native/NativeContract.cs#L284 This commit should be a part of #3234 and a part of 3.7.4. Luckily, this new DeprecatedIn functionality is used only for the old native CryptoLib's verifyWithECDsa method with Cockatrice hardfork. And luckily, there are other CryptoLib's methods with ActiveIn set to Cockatrice hardfork. Due to these two facts this bug does not affect mainnet/testnet, and thus we don't need 3.7.5 with this fix included. So this fix may safely be postponed to 3.8.0. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

DeprecatedIn hardfork of every native method (if not null) should be included into the set of used hardforks, otherwise no contract update will be performed on this hardfork activation: https://github.com/neo-project/neo/blob/08f2dfc56762bb43be33e873bc3659bad368d4d6/src/Neo/SmartContract/Native/NativeContract.cs#L279-L284 https://github.com/neo-project/neo/blob/08f2dfc56762bb43be33e873bc3659bad368d4d6/src/Neo/SmartContract/Native/ContractManagement.cs#L69-L73 This commit should be a part of #3234 and a part of 3.7.4. Luckily, this new DeprecatedIn functionality is used only for the old native CryptoLib's verifyWithECDsa method with Cockatrice hardfork. And luckily, there are other CryptoLib's methods with ActiveIn set to Cockatrice hardfork. Due to these two facts this bug does not affect mainnet/testnet, and thus we don't need 3.7.5 with this fix included. So this fix may safely be postponed to 3.8.0. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

DeprecatedIn hardfork of every native method (if not null) should be included into the set of used hardforks, otherwise no contract update will be performed on this hardfork activation: https://github.com/neo-project/neo/blob/08f2dfc56762bb43be33e873bc3659bad368d4d6/src/Neo/SmartContract/Native/NativeContract.cs#L279-L284 https://github.com/neo-project/neo/blob/08f2dfc56762bb43be33e873bc3659bad368d4d6/src/Neo/SmartContract/Native/ContractManagement.cs#L69-L73 This commit should be a part of #3234 and a part of 3.7.4. Luckily, this new DeprecatedIn functionality is used only for the old native CryptoLib's verifyWithECDsa method with Cockatrice hardfork. And luckily, there are other CryptoLib's methods with ActiveIn set to Cockatrice hardfork. Due to these two facts this bug does not affect mainnet/testnet, and thus we don't need 3.7.5 with this fix included. So this fix may safely be postponed to 3.8.0. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Shargon <shargon@gmail.com>

…gins * 'latest-plugins' of github.com:Jim8y/neo: (21 commits) fix: custom plugins won't shown by command `plugins` (neo-project#3269) COVERALL: Improve maintenance and readbility of some variables (neo-project#3248) Update nuget (neo-project#3262) [**Part-2**] Neo module/master fixes (neo-project#3244) Fix `dotnet pack` error (neo-project#3266) Fix and Update devcontainer.json to use Dockerfile (neo-project#3259) Add optimization to template (neo-project#3247) Optimize plugin's models (neo-project#3246) fix CancelTransaction !signers.Any() (neo-project#3263) COVERALL: fix broken by changing report from lcov to cobertura (neo-project#3252) fix TraverseIterator count (neo-project#3261) Native: include DeprecatedIn hardfork into usedHardforks (neo-project#3245) [**Part-1**] `neo-module/master` (neo-project#3232) Make `ApplicationEngine.LoadContext` protection level `public` (neo-project#3243) improve parse method in neo-cli (neo-project#3204) Fix neo-project#3239 (neo-project#3242) Neo.CLI: enable hardforks for NeoFS mainnet (neo-project#3240) v3.7.4 (neo-project#3237) fix hardfork issues (neo-project#3234) Update src/Neo.CLI/CLI/MainService.Plugins.cs ... # Conflicts: # src/Neo.CLI/CLI/MainService.Plugins.cs

Port neo-project/neo#3234, should be a part of #3351. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

fix hardfork issues

a3319aa

Jim8y marked this pull request as draft May 15, 2024 00:24

Jim8y requested a review from NGDAdmin May 15, 2024 00:25

fix hardfork for 3172

a88ebe4

Jim8y marked this pull request as ready for review May 15, 2024 00:44

Jim8y added the Waiting for Review label May 15, 2024

Jim8y added 3 commits May 15, 2024 11:25

fix 3195

c1af443

set custom setting if load from config.

dfa2520

fix hf index

c02d6f0

roman-khimov reviewed May 15, 2024

View reviewed changes

Jim8y added 2 commits May 15, 2024 15:04

fix UT

43236e6

Revert "fix 3195"

d0433d6

This reverts commit c1af443.

AnnaShaleva reviewed May 15, 2024

View reviewed changes

revert role fix

18d4f46

Jim8y added 2 commits May 15, 2024 20:40

fix manifest

5926b21

format

e5540ed

superboyiii approved these changes May 16, 2024

View reviewed changes

cschuchardt88 previously approved these changes May 16, 2024

View reviewed changes

AnnaShaleva mentioned this pull request May 16, 2024

Get rid of deprecated NamedCurve enum #3236

Closed

AnnaShaleva reviewed May 16, 2024

View reviewed changes

AnnaShaleva mentioned this pull request May 16, 2024

Improve native contract update mechanism #3210

Closed

revert role UT

372eb6e

Jim8y dismissed cschuchardt88’s stale review via 372eb6e May 16, 2024 08:00

shargon approved these changes May 16, 2024

View reviewed changes

NGDAdmin approved these changes May 16, 2024

View reviewed changes

NGDAdmin merged commit d56d4eb into neo-project:master May 16, 2024

AnnaShaleva mentioned this pull request May 16, 2024

Bind 3.7 protocol changes to Cockatrice nspcc-dev/neo-go#3440

Closed

vncoelho reviewed May 16, 2024

View reviewed changes

vang1ong7ang mentioned this pull request May 17, 2024

checklist of 3.7.x PRs that will cause FORK #3233

Closed

4 tasks

AnnaShaleva mentioned this pull request May 20, 2024

Native: include DeprecatedIn hardfork into usedHardforks #3245

Merged

9 tasks

AnnaShaleva mentioned this pull request Jun 28, 2024

DeprecatedIn for events #3362

Merged

15 tasks

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request May 13, 2025

native: move CommitteeChanged event emission under Cockatrice

9314607

Port neo-project/neo#3234, should be a part of #3351. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request May 13, 2025

native: move CommitteeChanged event emission under Cockatrice

8a28306

Port neo-project/neo#3234, should be a part of #3351. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva mentioned this pull request May 13, 2025

native: move CommitteeChanged event emission under Cockatrice nspcc-dev/neo-go#3892

Merged

	public ContractMethodAttribute(bool isDeprecated, Hardfork deprecatedIn)
	public ContractMethodAttribute(Hardfork? activeIn, Hardfork deprecatedIn)

Conversation

Jim8y commented May 15, 2024

Description

Type of change

Checklist:

Uh oh!

vang1ong7ang commented May 15, 2024

Uh oh!

vang1ong7ang commented May 15, 2024

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!

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vang1ong7ang May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

vang1ong7ang commented May 15, 2024

Uh oh!

superboyiii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shargon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vncoelho left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

AnnaShaleva May 15, 2024 •

edited

Loading

vang1ong7ang May 15, 2024 •

edited

Loading