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

fix hardfork issues #3234

Merged
merged 11 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Neo/ProtocolSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ public static ProtocolSettings Load(string path, bool optional = true)
/// <returns>The loaded <see cref="ProtocolSettings"/>.</returns>
public static ProtocolSettings Load(IConfigurationSection section)
{
return new ProtocolSettings
Custom = new ProtocolSettings
{
Network = section.GetValue("Network", Default.Network),
AddressVersion = section.GetValue("AddressVersion", Default.AddressVersion),
Expand All @@ -164,6 +164,7 @@ public static ProtocolSettings Load(IConfigurationSection section)
? EnsureOmmitedHardforks(section.GetSection("Hardforks").GetChildren().ToDictionary(p => Enum.Parse<Hardfork>(p.Key, true), p => uint.Parse(p.Value))).ToImmutableDictionary()
: Default.Hardforks
};
return Custom;
}

/// <summary>
Expand Down
7 changes: 7 additions & 0 deletions src/Neo/SmartContract/Native/ContractMethodAttribute.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,19 @@ internal class ContractMethodAttribute : Attribute
public long CpuFee { get; init; }
public long StorageFee { get; init; }
public Hardfork? ActiveIn { get; init; } = null;
public Hardfork? DeprecatedIn { get; init; } = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref. #3210 (comment) as TODO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to ActiveTil?


public ContractMethodAttribute() { }

public ContractMethodAttribute(Hardfork activeIn)
{
ActiveIn = activeIn;
}

public ContractMethodAttribute(bool isDeprecated, Hardfork deprecatedIn)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref. #3210 (comment) as TODO.

{
if (!isDeprecated) throw new ArgumentException("isDeprecated must be true", nameof(isDeprecated));
DeprecatedIn = deprecatedIn;
}
}
}
2 changes: 2 additions & 0 deletions src/Neo/SmartContract/Native/ContractMethodMetadata.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ internal class ContractMethodMetadata
public CallFlags RequiredCallFlags { get; }
public ContractMethodDescriptor Descriptor { get; }
public Hardfork? ActiveIn { get; init; } = null;
public Hardfork? DeprecatedIn { get; init; } = null;

public ContractMethodMetadata(MemberInfo member, ContractMethodAttribute attribute)
{
Expand All @@ -60,6 +61,7 @@ public ContractMethodMetadata(MemberInfo member, ContractMethodAttribute attribu
StorageFee = attribute.StorageFee;
RequiredCallFlags = attribute.RequiredCallFlags;
ActiveIn = attribute.ActiveIn;
DeprecatedIn = attribute.DeprecatedIn;
Descriptor = new ContractMethodDescriptor
{
Name = Name,
Expand Down
22 changes: 21 additions & 1 deletion src/Neo/SmartContract/Native/CryptoLib.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@
/// </summary>
public sealed partial class CryptoLib : NativeContract
{
private static readonly Dictionary<NamedCurve, ECCurve> curves = new()

Check warning on line 24 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 24 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 24 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 24 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 24 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 24 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'
{
[NamedCurve.secp256k1] = ECCurve.Secp256k1,

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve.secp256k1' is obsolete: 'secp256k1 value is obsolete and will be removed in future versions. Please, use NamedCurveHash.secp256k1SHA256 for compatible behaviour.'

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve.secp256k1' is obsolete: 'secp256k1 value is obsolete and will be removed in future versions. Please, use NamedCurveHash.secp256k1SHA256 for compatible behaviour.'

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

'NamedCurve.secp256k1' is obsolete: 'secp256k1 value is obsolete and will be removed in future versions. Please, use NamedCurveHash.secp256k1SHA256 for compatible behaviour.'

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

'NamedCurve.secp256k1' is obsolete: 'secp256k1 value is obsolete and will be removed in future versions. Please, use NamedCurveHash.secp256k1SHA256 for compatible behaviour.'

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

'NamedCurve.secp256k1' is obsolete: 'secp256k1 value is obsolete and will be removed in future versions. Please, use NamedCurveHash.secp256k1SHA256 for compatible behaviour.'

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

'NamedCurve.secp256k1' is obsolete: 'secp256k1 value is obsolete and will be removed in future versions. Please, use NamedCurveHash.secp256k1SHA256 for compatible behaviour.'
[NamedCurve.secp256r1] = ECCurve.Secp256r1,

Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve.secp256r1' is obsolete: 'secp256r1 value is obsolete and will be removed in future versions. Please, use NamedCurveHash.secp256r1SHA256 for compatible behaviour.'

Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve.secp256r1' is obsolete: 'secp256r1 value is obsolete and will be removed in future versions. Please, use NamedCurveHash.secp256r1SHA256 for compatible behaviour.'

Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

'NamedCurve.secp256r1' is obsolete: 'secp256r1 value is obsolete and will be removed in future versions. Please, use NamedCurveHash.secp256r1SHA256 for compatible behaviour.'

Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

'NamedCurve.secp256r1' is obsolete: 'secp256r1 value is obsolete and will be removed in future versions. Please, use NamedCurveHash.secp256r1SHA256 for compatible behaviour.'
};

private static readonly Dictionary<NamedCurveHash, (ECCurve Curve, Hasher Hasher)> s_curves = new()
{
[NamedCurveHash.secp256k1SHA256] = (ECCurve.Secp256k1, Hasher.SHA256),
Expand Down Expand Up @@ -85,7 +91,7 @@
/// <param name="signature">The signature to be verified.</param>
/// <param name="curveHash">A pair of the curve to be used by the ECDSA algorithm and the hasher function to be used to hash message.</param>
/// <returns><see langword="true"/> if the signature is valid; otherwise, <see langword="false"/>.</returns>
[ContractMethod(CpuFee = 1 << 15)]
[ContractMethod(Hardfork.HF_Cockatrice, CpuFee = 1 << 15)]
public static bool VerifyWithECDsa(byte[] message, byte[] pubkey, byte[] signature, NamedCurveHash curveHash)
{
try
Expand All @@ -98,5 +104,19 @@
return false;
}
}

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

Check warning on line 110 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 110 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 110 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 110 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (windows-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 110 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'

Check warning on line 110 in src/Neo/SmartContract/Native/CryptoLib.cs

View workflow job for this annotation

GitHub Actions / Test (macos-latest)

'NamedCurve' is obsolete: 'NamedCurve enum is obsolete and will be removed in future versions. Please, use an extended NamedCurveHash instead.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
try
{
return Crypto.VerifySignature(message, signature, pubkey, curves[curve]);
}
catch (ArgumentException)
{
return false;
}
}
}
}
12 changes: 11 additions & 1 deletion src/Neo/SmartContract/Native/NativeContract.cs
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,15 @@ private NativeContractsCache.CacheEntry GetAllowedMethods(IsHardforkEnabledDeleg
byte[] script;
using (ScriptBuilder sb = new())
{
foreach (ContractMethodMetadata method in methodDescriptors.Where(u => u.ActiveIn is null || hfChecker(u.ActiveIn.Value, index)))
foreach (ContractMethodMetadata method in methodDescriptors.Where(u
=>
// no hardfork is involved
u.ActiveIn is null && u.DeprecatedIn is null ||
// deprecated method hardfork is involved
u.DeprecatedIn is not null && hfChecker(u.DeprecatedIn.Value, index) == false ||
// active method hardfork is involved
u.ActiveIn is not null && hfChecker(u.ActiveIn.Value, index))
)
{
method.Descriptor.Offset = sb.Length;
sb.EmitPush(0); //version
Expand Down Expand Up @@ -366,6 +374,8 @@ internal async void Invoke(ApplicationEngine engine, byte version)
ContractMethodMetadata method = currentAllowedMethods.Methods[context.InstructionPointer];
if (method.ActiveIn is not null && !engine.IsHardforkEnabled(method.ActiveIn.Value))
throw new InvalidOperationException($"Cannot call this method before hardfork {method.ActiveIn}.");
if (method.DeprecatedIn is not null && engine.IsHardforkEnabled(method.DeprecatedIn.Value))
throw new InvalidOperationException($"Cannot call this method after hardfork {method.DeprecatedIn}.");
ExecutionContextState state = context.GetState<ExecutionContextState>();
if (!state.CallFlags.HasFlag(method.RequiredCallFlags))
throw new InvalidOperationException($"Cannot call this method with the flag {state.CallFlags}.");
Expand Down
22 changes: 14 additions & 8 deletions src/Neo/SmartContract/Native/NeoToken.cs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public sealed class NeoToken : FungibleToken<NeoToken.NeoAccountState>
"from", ContractParameterType.PublicKey,
"to", ContractParameterType.PublicKey,
"amount", ContractParameterType.Integer)]
[ContractEvent(3, name: "CommitteeChanged",
[ContractEvent(Hardfork.HF_Cockatrice, 3, name: "CommitteeChanged",
"old", ContractParameterType.Array,
"new", ContractParameterType.Array)]
internal NeoToken() : base()
Expand Down Expand Up @@ -203,14 +203,20 @@ internal override ContractTask OnPersistAsync(ApplicationEngine engine)
cachedCommittee.Clear();
cachedCommittee.AddRange(ComputeCommitteeMembers(engine.Snapshot, engine.ProtocolSettings));

var newCommittee = cachedCommittee.Select(u => u.PublicKey).ToArray();

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will case?

var index = engine.PersistingBlock?.Index ?? Ledger.CurrentIndex(engine.Snapshot);
if (engine.ProtocolSettings.IsHardforkEnabled(Hardfork.HF_Cockatrice, index))
{
engine.SendNotification(Hash, "CommitteeChanged", new VM.Types.Array(engine.ReferenceCounter) {
new VM.Types.Array(engine.ReferenceCounter, prevCommittee.Select(u => (ByteString)u.ToArray())) ,
new VM.Types.Array(engine.ReferenceCounter, newCommittee.Select(u => (ByteString)u.ToArray()))
});
var newCommittee = cachedCommittee.Select(u => u.PublicKey).ToArray();

if (!newCommittee.SequenceEqual(prevCommittee))
{
engine.SendNotification(Hash, "CommitteeChanged", new VM.Types.Array(engine.ReferenceCounter) {
new VM.Types.Array(engine.ReferenceCounter, prevCommittee.Select(u => (ByteString)u.ToArray())) ,
new VM.Types.Array(engine.ReferenceCounter, newCommittee.Select(u => (ByteString)u.ToArray()))
});
}
AnnaShaleva marked this conversation as resolved.
Show resolved Hide resolved
}
}
return ContractTask.CompletedTask;
Expand Down
4 changes: 2 additions & 2 deletions tests/Neo.UnitTests/SmartContract/Native/UT_RoleManagement.cs
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ public void TestSetAndGet()
publicKeys[0] = key1.PublicKey;
publicKeys[1] = key2.PublicKey;
publicKeys = publicKeys.OrderBy(p => p).ToArray();

List<Role> roles = new List<Role>() { Role.StateValidator, Role.Oracle, Role.NeoFSAlphabetNode, Role.P2PNotary };
// Notary effect after the fork.
List<Role> roles = new List<Role>() { Role.StateValidator, Role.Oracle, Role.NeoFSAlphabetNode };//, Role.P2PNotary };
AnnaShaleva marked this conversation as resolved.
Show resolved Hide resolved
foreach (var role in roles)
{
var snapshot1 = _snapshot.CreateSnapshot();
Expand Down