-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix hardfork issues #3234
Changes from all commits
a3319aa
a88ebe4
c1af443
dfa2520
c02d6f0
43236e6
d0433d6
18d4f46
5926b21
e5540ed
372eb6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
|
||||||
public ContractMethodAttribute() { } | ||||||
|
||||||
public ContractMethodAttribute(Hardfork activeIn) | ||||||
{ | ||||||
ActiveIn = activeIn; | ||||||
} | ||||||
|
||||||
public ContractMethodAttribute(bool isDeprecated, Hardfork deprecatedIn) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I'd suggest to use this option, and just put There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 GitHub Actions / Test (ubuntu-latest)
Check warning on line 24 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (ubuntu-latest)
Check warning on line 24 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (windows-latest)
Check warning on line 24 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (windows-latest)
Check warning on line 24 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (macos-latest)
|
||
{ | ||
[NamedCurve.secp256k1] = ECCurve.Secp256k1, | ||
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (ubuntu-latest)
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (ubuntu-latest)
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (ubuntu-latest)
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (ubuntu-latest)
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (windows-latest)
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (windows-latest)
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (windows-latest)
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (windows-latest)
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (macos-latest)
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (macos-latest)
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (macos-latest)
Check warning on line 26 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (macos-latest)
|
||
[NamedCurve.secp256r1] = ECCurve.Secp256r1, | ||
Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (ubuntu-latest)
Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (ubuntu-latest)
Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (ubuntu-latest)
Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (ubuntu-latest)
Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (windows-latest)
Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (windows-latest)
Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (macos-latest)
Check warning on line 27 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (macos-latest)
|
||
}; | ||
|
||
private static readonly Dictionary<NamedCurveHash, (ECCurve Curve, Hasher Hasher)> s_curves = new() | ||
{ | ||
[NamedCurveHash.secp256k1SHA256] = (ECCurve.Secp256k1, Hasher.SHA256), | ||
|
@@ -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 | ||
|
@@ -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 GitHub Actions / Test (ubuntu-latest)
Check warning on line 110 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (ubuntu-latest)
Check warning on line 110 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (windows-latest)
Check warning on line 110 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (windows-latest)
Check warning on line 110 in src/Neo/SmartContract/Native/CryptoLib.cs GitHub Actions / Test (macos-latest)
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may safely get rid of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to
ActiveTil
?