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

EcRecover #3633

Closed
wants to merge 59 commits into from
Closed

EcRecover #3633

wants to merge 59 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Dec 18, 2024

Description

Close #3628

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • 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?

  • Crypto tests

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

Jim8y and others added 26 commits August 8, 2024 00:24
* Add entries to Designation event

* Change to HF_Echidna

* Add UT

* Add count
* add base64url

* active in

* update placehold hf height

* fix hf issue and move methods to proper place.

* fix test

* use identifymodel instead.
* Add entries to Designation event

* Change to HF_Echidna

* Add UT

* Add count
* add base64url

* active in

* update placehold hf height

* fix hf issue and move methods to proper place.

* fix test

* use identifymodel instead.
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
* Add entries to Designation event

* Change to HF_Echidna

* Add UT

* Add count
* add base64url

* active in

* update placehold hf height

* fix hf issue and move methods to proper place.

* fix test

* use identifymodel instead.
# Conflicts:
#	src/Neo/Neo.csproj
#	src/Neo/SmartContract/Native/RoleManagement.cs
@shargon shargon requested a review from Jim8y December 18, 2024 12:06
@shargon shargon changed the base branch from master to HF_Echidna December 18, 2024 12:07
@shargon shargon marked this pull request as ready for review December 18, 2024 12:09
// Test from https://eips.ethereum.org/EIPS/eip-2098
var privateKey = "1234567890123456789012345678901234567890123456789012345678901234".HexToBytes();

var expectedPubKey1 = (Neo.Cryptography.ECC.ECCurve.Secp256k1.G * privateKey).ToArray();

Choose a reason for hiding this comment

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

Expected values
pubKeyX - 0xe90c7d3640a1568839c31b70a893ab6714ef8415b9de90cedfc1c8f353a6983e
pubKeyY - 0x625529392df7fa514bdd65a2003f6619567d79bee89830e63e932dbd42362d34

eth address for reference - 0x2e988A386a799F506693793c6A5AF6B54dfAaBfB


Console.WriteLine($"Expected PubKey: {expectedPubKey1.ToHexString()}");
var message1 = Encoding.UTF8.GetBytes("Hello World");
var messageHash1 = message1.Keccak256();

Choose a reason for hiding this comment

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

This is supposed to be an Ethereum signed message which is not explicitly stated in ERC-2098

function toEthSignedMessageHash(bytes memory message) internal pure returns (bytes32) {
        return
            keccak256(bytes.concat("\x19Ethereum Signed Message:\n", bytes(Strings.toString(message.length)), message));
}

Expected value: 0xa1de988600a42c4b4ab089b619297c17d53cffae5d5120d82d8a92d0bb3b78f2

Hecate2 and others added 2 commits December 31, 2024 10:58
* Fix UT

* Update src/Neo/ProtocolSettings.cs

Co-authored-by: nan01ab <[email protected]>

* Update src/Neo/ProtocolSettings.cs

Co-authored-by: nan01ab <[email protected]>

* Update src/Neo/ProtocolSettings.cs

Co-authored-by: Christopher Schuchardt <[email protected]>

---------

Co-authored-by: Jimmy <[email protected]>
Co-authored-by: nan01ab <[email protected]>
Co-authored-by: Christopher Schuchardt <[email protected]>
@Hecate2
Copy link
Contributor

Hecate2 commented Dec 31, 2024

Fixed EIP-2098 tests, but failed in tests creating wallet in neo-cli. Cannot recognize the reason.

@Jim8y
Copy link
Contributor

Jim8y commented Dec 31, 2024

final version:

==Specification==

===Native Contract Interface===

Three methods will be added to CryptoLib in HF_Echidna:

====Method 1: Recovery from Message and Signature====

{
    "name": "secp256k1Recover",
    "safe": true,
    "parameters": [
        {
            "name": "message",
            "type": "ByteArray"
        },
        {
            "name": "hasher",
            "type": "Integer"
        },
        {
            "name": "signature",
            "type": "ByteArray"
        }
    ],
    "returntype": "ByteArray"
}

====Method 2: Recovery from Message Components====

{
    "name": "secp256k1Recover",
    "safe": true,
    "parameters": [
        {
            "name": "message",
            "type": "ByteArray"
        },
        {
            "name": "hasher",
            "type": "Integer"
        },
        {
            "name": "r",
            "type": "ByteArray"
        },
        {
            "name": "s",
            "type": "ByteArray"
        },
        {
            "name": "v",
            "type": "Integer"
        }
    ],
    "returntype": "ByteArray"
}

====Method 3: Recovery from Pre-computed Hash====

{
    "name": "secp256k1Recover",
    "safe": true,
    "parameters": [
        {
            "name": "hash",
            "type": "ByteArray"
        },
        {
            "name": "signature",
            "type": "ByteArray"
        }
    ],
    "returntype": "ByteArray"
}

===Method Specification===

The methods MUST follow these rules:

  1. Input Requirements for Method 1 (secp256k1Recover with message):

    • message: Original message bytes
    • hasher: Hash function ID
      • 1: SHA256
      • 2: Keccak256
      • Others: Reserved for future use
    • signature: 65 bytes (32 bytes r + 32 bytes s + 1 byte v) or 64 bytes for EIP-2098 format
  2. Input Requirements for Method 2 (secp256k1Recover with components):

    • message: Original message bytes
    • hasher: Hash function ID (same as above)
    • r: 32 bytes
    • s: 32 bytes
    • v: Recovery ID (27 or 28)
  3. Input Requirements for Method 3 (secp256k1Recover with hash):

    • hash: 32-byte hash of the original message
    • signature: 65 bytes (32 bytes r + 32 bytes s + 1 byte v) or 64 bytes for EIP-2098 format
  4. Return Value (all methods):

    • Success: 33-byte compressed public key in SEC format
    • Failure: Returns null if:
      • Invalid signature/component length
      • Invalid recovery value (v)
      • Invalid signature format
      • Recovery failure
      • Null input parameters

@Hecate2
Copy link
Contributor

Hecate2 commented Dec 31, 2024

I think the name hasher can be confusing, between a function and a person who executes the hash. hashFunc can be better, but I do not mind leaving it as is.

@Jim8y
Copy link
Contributor

Jim8y commented Dec 31, 2024

I think the name hasher can be confusing, between a function and a person who executes the hash. hashFunc can be better, but I do not minding leaving it as is.

I remember the hasher name is from the builtin Hasher enum.

public enum Hasher : byte

@cschuchardt88
Copy link
Member

cschuchardt88 commented Dec 31, 2024

I think the name hasher can be confusing, between a function and a person who executes the hash. hashFunc can be better, but I do not minding leaving it as is.

I remember the hasher name is from the builtin Hasher enum.

public enum Hasher : byte

Well I told you all so, once again!!! 😃 When it comes to standards and naming conventions no one listens to me. You dont care -- I dont care. Maybe bad english is the problem.

@Jim8y
Copy link
Contributor

Jim8y commented Dec 31, 2024

I think the name hasher can be confusing, between a function and a person who executes the hash. hashFunc can be better, but I do not minding leaving it as is.

I remember the hasher name is from the builtin Hasher enum.

public enum Hasher : byte

Well I told you all so, once again!!! 😃 When it comes to standards and naming conventions no one listens to me. You dont care -- I dont care. Maybe bad english is the problem.

Bro, i would care for sure, but as you said, maybe english is the problem, but once you point it out and fix it in a pr, i promise you i will support you.

@Jim8y Jim8y mentioned this pull request Jan 1, 2025
14 tasks
@shargon
Copy link
Member Author

shargon commented Jan 2, 2025

We can change the name

I think the name hasher can be confusing, between a function and a person who executes the hash. hashFunc can be better, but I do not minding leaving it as is.

I remember the hasher name is from the builtin Hasher enum.

public enum Hasher : byte

Well I told you all so, once again!!! 😃 When it comes to standards and naming conventions no one listens to me. You dont care -- I dont care. Maybe bad english is the problem.

@Jim8y
Copy link
Contributor

Jim8y commented Jan 11, 2025

@OT-kraftchain coredev team prefers to keep only one method in the native contract, can you please pick one that works for you the best.

Base automatically changed from HF_Echidna to master January 23, 2025 09:52
@OT-kraftchain
Copy link

@OT-kraftchain coredev team prefers to keep only one method in the native contract, can you please pick one that works for you the best.

Hey @Jim8y, I think
public static byte[] Secp256K1Recover(byte[] hash, byte[] signature)
makes the most sense since it gives the user the most freedom to use whatever kind of hash and signature they want

@shargon
Copy link
Member Author

shargon commented Jan 23, 2025

@OT-kraftchain coredev team prefers to keep only one method in the native contract, can you please pick one that works for you the best.

Hey @Jim8y, I think public static byte[] Secp256K1Recover(byte[] hash, byte[] signature) makes the most sense since it gives the user the most freedom to use whatever kind of hash and signature they want

signature format (32,32,1) ?

@OT-kraftchain
Copy link

signature format (32,32,1) ?

Yeah, sounds about right

@shargon shargon mentioned this pull request Jan 24, 2025
15 tasks
@shargon
Copy link
Member Author

shargon commented Jan 24, 2025

Moved to #3696

@shargon shargon closed this Jan 24, 2025
@shargon shargon deleted the ec-recover branch January 24, 2025 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recover Public Key from Signature for Secp256k1
8 participants