add secp256k1 + keccak256 to signature verification. by Jim8y · Pull Request #3205 · neo-project/neo

Jim8y · 2024-04-25T03:41:08Z

Description

This pr focus on adding support to secp256k1 signed transactions and hashed with Keccak256.

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

How Has This Been Tested?

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

…-secp256k1 # Conflicts: # src/Neo/Cryptography/Crypto.cs # src/Neo/SmartContract/ApplicationEngine.Crypto.cs

Hecate2 · 2024-04-25T05:07:06Z

src/Neo/SmartContract/ApplicationEngine.Crypto.cs

+        protected internal readonly static Dictionary<byte, ECCurve> ECCurveSelection = new(){
+            { 0x00, ECCurve.Secp256r1 },
+            { 0x01, ECCurve.Secp256k1 },
+        };
+


(This may not be the best place to put this dictionary)

roman-khimov

Why?

Jim8y · 2024-04-25T09:55:09Z

Why?

Hey @roman-khimov, this feature is required by some exchanges that only support secp256k1 to sign transactions, its hardware restrained and can not be updated. Thus, we need to add support to verify secp256k1 signed transactions. But we only verify it, we dont construct it.

roman-khimov · 2024-04-25T12:58:04Z

How come it wasn't a problem for years of Neo existence?

This raises so many compatibility questions that I don't even know where to begin. Addresses/wallets/script parsers/NeoFS accounts/NeoX accounts.

Likely the issue can be solved with crypto.verifyWithECDsa that has support for k1 (proxy contract that does actions based on incoming payloads signed with k1 key).

Jim8y · 2024-04-27T18:24:30Z

New syscall:

System.Crypto.CheckSigV2

System.Crypto.CheckMultisigV2

Data Type:

InvocationScript: Old format
VerificationScript: 1 byte curve| 1 byte hash | Old format

Curve:

0x00 r1,
0x01 k1,

Hash:

0x00 SHA256,
0x01 Keccak256

Exampe:

            using ScriptBuilder verificationScriptBuilder = new();
            verificationScriptBuilder.EmitRaw([0x01]); // Push curve secp256K1
            verificationScriptBuilder.EmitRaw([0x01]); // Push Keccak256 hasher
            verificationScriptBuilder.EmitPush(pubKey); // Push pubkey
            verificationScriptBuilder.EmitSysCall(ApplicationEngine.System_Crypto_CheckSigV2);

Price:

SignatureContractCostV2:
SignatureContractCost() + (isV2 ? ApplicationEngine.OpCodePriceTable[(byte)OpCode.PUSH1] * 2 : 0);

MultiSignatureContractCostV2:
MultiSignatureContractCost() + (isV2 ? ApplicationEngine.OpCodePriceTable[(byte)OpCode.PUSH1] * 2 : 0);

How to check if a verification script is V2:

       private static ReadOnlySpan<byte> ParseScriptV2(ReadOnlySpan<byte> script, out bool? isV2, out ECCurve? curve, out Hasher? hasher)
        {
            if (script.Length < 40) throw new FormatException("The verification script is too short.");
            isV2 = BinaryPrimitives.ReadUInt32LittleEndian(script[^4..]) == ApplicationEngine.System_Crypto_CheckSigV2 ||
                BinaryPrimitives.ReadUInt32LittleEndian(script[^4..]) == ApplicationEngine.System_Crypto_CheckMultisigV2;
            curve = ECCurve.Secp256r1;
            hasher = Hasher.SHA256;
            if (!isV2.Value)
            {
                return script;
            }

            curve = script[0] == 0x01 ? ECCurve.Secp256k1 : ECCurve.Secp256r1;
            hasher = script[1] == 0x01 ? Hasher.Keccak256 : Hasher.SHA256;
            return script[2..];
        }

Jim8y · 2024-04-27T18:49:15Z

Note: Still need to add it to the hard fork.

cschuchardt88 · 2024-04-27T18:50:26Z

src/Neo/Cryptography/Helper.cs

+
+        public static byte[] Keccak256(this ReadOnlySpan<byte> value)
+        {
+            return Keccak256(value.ToArray());


Suggested change

return Keccak256(value.ToArray());

return Keccak256([.. value]);

cschuchardt88 · 2024-04-27T18:50:41Z

src/Neo/Cryptography/Helper.cs

+
+        public static byte[] Keccak256(this Span<byte> value)
+        {
+            return Keccak256(value.ToArray());


Suggested change

return Keccak256(value.ToArray());

return Keccak256([.. value]);

cschuchardt88 · 2024-04-27T18:52:03Z

src/Neo/Network/P2P/Payloads/Transaction.cs

            for (int i = 0; i < hashes.Length; i++)
            {
-                if (IsSignatureContract(witnesses[i].VerificationScript.Span))
+                if (IsSignatureContract(witnesses[i].VerificationScript.Span, out bool? isV2, out ECCurve? curve, out Hasher? hasher))


Dont add ? nullable until project is convert to nullable project.

cschuchardt88 · 2024-04-27T18:55:16Z

src/Neo/Network/P2P/Payloads/Transaction.cs

                {
                    if (hashes[i] != witnesses[i].ScriptHash) return VerifyResult.Invalid;
-                    var pubkey = witnesses[i].VerificationScript.Span[2..35];
+                    var pubkey = isV2.Value ? witnesses[i].VerificationScript.Span[4..37] : witnesses[i].VerificationScript.Span[2..35];


Suggested change

var pubkey = isV2.Value ? witnesses[i].VerificationScript.Span[4..37] : witnesses[i].VerificationScript.Span[2..35];

var pubkey = isV2.Value ? witnesses[i].VerificationScript[4..37] : witnesses[i].VerificationScript[2..35];

cschuchardt88 · 2024-04-27T18:55:28Z

src/Neo/Network/P2P/Payloads/Transaction.cs

                    try
                    {
-                        if (!Crypto.VerifySignature(this.GetSignData(settings.Network), witnesses[i].InvocationScript.Span[2..], pubkey, ECCurve.Secp256r1))
+                        if (!Crypto.VerifySignature(this.GetSignData(settings.Network), witnesses[i].InvocationScript.Span[2..], pubkey, curve, hasher.Value))


Suggested change

if (!Crypto.VerifySignature(this.GetSignData(settings.Network), witnesses[i].InvocationScript.Span[2..], pubkey, curve, hasher.Value))

if (!Crypto.VerifySignature(this.GetSignData(settings.Network), witnesses[i].InvocationScript[2..], pubkey, curve, hasher.Value))

cschuchardt88 · 2024-04-27T18:55:41Z

src/Neo/Network/P2P/Payloads/Transaction.cs

                    }
                }
-                else if (IsMultiSigContract(witnesses[i].VerificationScript.Span, out var m, out ECPoint[] points))
+                else if (IsMultiSigContract(witnesses[i].VerificationScript.Span, out var m, out ECPoint[] points, out isV2, out curve, out hasher))


Suggested change

else if (IsMultiSigContract(witnesses[i].VerificationScript.Span, out var m, out ECPoint[] points, out isV2, out curve, out hasher))

else if (IsMultiSigContract(witnesses[i].VerificationScript, out var m, out ECPoint[] points, out isV2, out curve, out hasher))

AnnaShaleva · 2024-04-28T10:15:12Z

This raises so many compatibility questions that I don't even know where to begin. Addresses/wallets/script parsers/NeoFS accounts/NeoX accounts.

Just to be in sync with Discord: we've discussed it with @roman-khimov and prepared an alternative concept that do not require core protocol modifications and do not require custom verification contract deployment. The solution is rather simple: use custom witness verification script with a call to native CryptoLib's verifyWithECDsa. See the nspcc-dev/neo-go#3425 (it is an updated version) and don't hesitate to comment, I'll port it to the core in the nearest future.

cschuchardt88 · 2024-04-28T23:33:02Z

Can't they use NeoX to bridge to N3? Isnt that the point of NeoX?

shargon · 2024-04-29T08:24:04Z

It's possible to use his verifyWithECDsa as verification script without store the contract in the blockchain, isn't it?

A port of nspcc-dev/neo-go@1e2b438. This commit contains minor protocol extension needed for custom Koblitz-based verification scripts (an alternative to #3205). Replace native CryptoLib's verifyWithECDsa `curve` parameter by `curveHash` parameter which is a enum over supported pairs of named curves and hash functions. Even though this change is a compatible extension of the protocol, it changes the genesis state due to parameter renaming (CryptoLib's manifest is changed). But we're going to resync chain in 3.7 release anyway, so it's not a big deal. Also, we need to check mainnet and testnet compatibility in case if anyone has ever called verifyWithECDsa with 24 or 25 `curve` value. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

Koblitz-based and Keccak-based transaction witness verification for single signature and multisignature ported from nspcc-dev/neo-go#3425. An alternative to #3205. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

A port of nspcc-dev/neo-go@1e2b438. This commit contains minor protocol extension needed for custom Koblitz-based verification scripts (an alternative to #3205). Replace native CryptoLib's verifyWithECDsa `curve` parameter by `curveHash` parameter which is a enum over supported pairs of named curves and hash functions. NamedCurve enum mark as deprecated and replaced by NamedCurveHash with compatible behaviour. Even though this change is a compatible extension of the protocol, it changes the genesis state due to parameter renaming (CryptoLib's manifest is changed). But we're going to resync chain in 3.7 release anyway, so it's not a big deal. Also, we need to check mainnet and testnet compatibility in case if anyone has ever called verifyWithECDsa with 24 or 25 `curve` value. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

Koblitz-based and Keccak-based transaction witness verification for single signature and multisignature ported from nspcc-dev/neo-go#3425. An alternative to #3205. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

…#3209) * Native: extend CryptoLib's verifyWithECDsa with hasher parameter A port of nspcc-dev/neo-go@1e2b438. This commit contains minor protocol extension needed for custom Koblitz-based verification scripts (an alternative to #3205). Replace native CryptoLib's verifyWithECDsa `curve` parameter by `curveHash` parameter which is a enum over supported pairs of named curves and hash functions. NamedCurve enum mark as deprecated and replaced by NamedCurveHash with compatible behaviour. Even though this change is a compatible extension of the protocol, it changes the genesis state due to parameter renaming (CryptoLib's manifest is changed). But we're going to resync chain in 3.7 release anyway, so it's not a big deal. Also, we need to check mainnet and testnet compatibility in case if anyone has ever called verifyWithECDsa with 24 or 25 `curve` value. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * SmartContract: add extension to ScriptBuilder for System.Contract.Call Group the set of common operations required to emit System.Contract.Call appcall. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: add an example of custom Koblitz signature verification Koblitz-based and Keccak-based transaction witness verification for single signature and multisignature ported from nspcc-dev/neo-go#3425. An alternative to #3205. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * SmartContract: make multisig koblitz easier to parse 1. Make prologue be exactly the same as regular CheckMultisig. 2. But instead of "SYSCALL System.Crypto.CheckMultisig" do INITSLOT and K check. 3. This makes all of the code from INITSLOT below be independent of N/M, so one can parse the script beginning in the same way CheckMultisig is parsed and then just compare the rest of it with some known-good blob. 4. The script becomes a tiny bit larger now, but properties above are too good. Ported from nspcc-dev/neo-go@34ee294. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * SmartContract: use ABORT in Koblitz multisig Make the script a bit shorter. ABORTMSG would cost a bit more. Ported from nspcc-dev/neo-go@fb16891. Ref. nspcc-dev/neo-go#3425 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * SmartContract: reduce callflag scope for Koblitz verification scripts All flag is too wide. A port of nspcc-dev/neo-go@fe292f3. Ref. nspcc-dev/neo-go#3425 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: add tests for CryptoLib's verifyWithECDsa No functional changes, just add more unit-tests. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Native: update NamedCurveHash values for Keccak256 hasher Use 122 and 123 respectively for secp256k1Keccak256 and secp256r1Keccak256. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * SmartContract: move EmitAppCallNoArgs to the testing code We're not going to implement custom Koblitz witness generation at the core, and thus, the only user of this API is testing code. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Apply suggestions from code review clean ut lines * fix names * Cryptography: cache ECDomainParameters for Secp256r1 and Secp256k1 Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs * Update tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs --------- Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Jimmy <jinghui@wayne.edu>

NGDAdmin · 2024-05-10T02:48:15Z

Close because of #3209

add secp256k1 + keccak256 to signature verification.

361f7cb

Jim8y marked this pull request as draft April 25, 2024 03:41

Jimmy and others added 4 commits April 25, 2024 11:43

remvoe extra error

2f9bd00

use standalone bytes for hasher and curve

ac68c5c

use standalone bytes for hasher and curve

59f30de

Merge branch 'support-secp256k1' of github.com:Jim8y/neo into support…

0a6fb53

…-secp256k1 # Conflicts: # src/Neo/Cryptography/Crypto.cs # src/Neo/SmartContract/ApplicationEngine.Crypto.cs

Hecate2 reviewed Apr 25, 2024

View reviewed changes

roman-khimov reviewed Apr 25, 2024

View reviewed changes

Jimmy added 2 commits April 28, 2024 01:53

add UT and complete the transaction verification logic.

f4908c6

fix price and tests

5dc20cc

more formal check

d8f64a2

cschuchardt88 suggested changes Apr 27, 2024

View reviewed changes

AnnaShaleva mentioned this pull request Apr 28, 2024

native: add test for custom Koblitz witness verification script nspcc-dev/neo-go#3425

Merged

roman-khimov mentioned this pull request May 2, 2024

Neo v3.7.0 plan #2905

Closed

AnnaShaleva mentioned this pull request May 3, 2024

Support Koblitz-based and Keccak256-based custom witness verification #3209

Merged

19 tasks

NGDAdmin closed this May 10, 2024

	return Keccak256(value.ToArray());
	return Keccak256([.. value]);

	var pubkey = isV2.Value ? witnesses[i].VerificationScript.Span[4..37] : witnesses[i].VerificationScript.Span[2..35];
	var pubkey = isV2.Value ? witnesses[i].VerificationScript[4..37] : witnesses[i].VerificationScript[2..35];

	if (!Crypto.VerifySignature(this.GetSignData(settings.Network), witnesses[i].InvocationScript.Span[2..], pubkey, curve, hasher.Value))
	if (!Crypto.VerifySignature(this.GetSignData(settings.Network), witnesses[i].InvocationScript[2..], pubkey, curve, hasher.Value))

	else if (IsMultiSigContract(witnesses[i].VerificationScript.Span, out var m, out ECPoint[] points, out isV2, out curve, out hasher))
	else if (IsMultiSigContract(witnesses[i].VerificationScript, out var m, out ECPoint[] points, out isV2, out curve, out hasher))

Conversation

Jim8y commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Hecate2 Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

roman-khimov left a comment

Choose a reason for hiding this comment

Uh oh!

Jim8y commented Apr 25, 2024

Uh oh!

roman-khimov commented Apr 25, 2024

Uh oh!

Jim8y commented Apr 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New syscall:

Exampe:

Price:

How to check if a verification script is V2:

Uh oh!

Jim8y commented Apr 27, 2024

Uh oh!

cschuchardt88 Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva commented Apr 28, 2024

Uh oh!

cschuchardt88 commented Apr 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shargon commented Apr 29, 2024

Uh oh!

NGDAdmin commented May 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Jim8y commented Apr 25, 2024 •

edited

Loading

Jim8y commented Apr 27, 2024 •

edited

Loading

cschuchardt88 commented Apr 28, 2024 •

edited

Loading