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

Initial PublicKey Combine API #439

Merged
merged 6 commits into from
Nov 14, 2023
Merged

Initial PublicKey Combine API #439

merged 6 commits into from
Nov 14, 2023

Conversation

csjones
Copy link
Contributor

@csjones csjones commented Oct 8, 2023

Adding Swift APIs for based on secp256k1_ec_pubkey_combine #397

TODO:

  • Unit Tests (source from libsecp256k1/cashu)

Swift API Usage:

let privateKey = try! secp256k1.Signing.PrivateKey()
let publicKey = privateKey.publicKey

// The Combine API arguments are an array of PublicKey objects and an optional format 
publicKey.combine([publicKey], format: .uncompressed)

@csjones csjones added the 🔏 enhancement New feature or request label Oct 8, 2023
@what-the-diff
Copy link

what-the-diff bot commented Oct 8, 2023

PR Summary

  • New File Addition
    A new file named Combine.swift has been added under the secp256k1 folder. This is in line with our aim to enhance our code organization and make it more modular.

  • Extension to Public Key
    An extension to secp256k1.Signing.PublicKey has been created inside Combine.swift. This extension allows for a new PublicKey to be created by merging the current public key with other public keys.

  • Combine Function Introduction
    The new combine function has been designed to take an array of the current public keys (self) and other public keys (pubkeys). The function is designed to return a unified PublicKey object.

  • Internal Function Calls
    To streamline the process of combining and transforming public keys, the combine function calls two internal functions secp256k1_ec_pubkey_combine and secp256k1_ec_pubkey_serialize. This allows for efficient and accurate combination and serialization operations.

  • Return of Combined Key
    After all necessary validations are carried out, the combine function returns the combined PublicKey object ensuring enhanced collective key generation.

This pull request is designed to enhance our existing functionality by instituting improved key management and facilitating easier and more efficient public key combinations.

@zeugmaster
Copy link

zeugmaster commented Oct 29, 2023

First of all, thank you so much for putting this together! Super useful for what I am trying to build.
I have been testing the code for a couple of days and been getting some unexpected behaviour, but that might be more due to me being a beginner dev. Seemingly unrelated code changes lead to the function giving me underlyingCryptoError. Also using try! instead of a do - catch block gives me the same error and I can't really figure out why.

When I try to test combining two public keys, whenever it does work, it returns a different result than the python wrapper for secp256k1, although I was careful to make sure of the right order and the fact that the python implementation only combines the keys within the list/array and not the key the function was called on.

This is how I tested in Swift:
(hashToCurvesimply takes a string and hashes/rehashes it until the hash lies on the curve)

func test() {
    let publicKey = hashToCurve(message: "test1")
    let publicKey2 = hashToCurve(message: "test2")

    print("1: " + bytesToHexString(bytes: publicKey.dataRepresentation))
    print("2: " + bytesToHexString(bytes: publicKey2.dataRepresentation))
    
    let new:secp256k1.Signing.PublicKey
        
    do {
        new = try publicKey.combine([publicKey2], format: .compressed)
        print("RESULT " + bytesToHexString(bytes: new.dataRepresentation))
    } catch {
        print(error)
    }
}
1: 021b4f0e9851971998e732078544c96b36c3d01cedf7caa332359d6f1d83567014
2: 0260303ae22b998861bce3b28f33eec1be758a213c86c93c076dbe9f558c11c752
RESULT 034662b7e2fd30907601f3b16439ddb636792df7166ff0fe88275965eba37c5406

And this is the Python version:

point1 = hash_to_curve("test1".encode("utf-8"))
point2 = hash_to_curve("test2".encode("utf-8"))

print("1: " + point1.serialize().hex())
print("2: " + point2.serialize().hex())

new = PublicKey()
new.combine([point1.public_key, point2.public_key])

print("RESULT " + new.serialize().hex())
1: 021b4f0e9851971998e732078544c96b36c3d01cedf7caa332359d6f1d83567014
2: 0260303ae22b998861bce3b28f33eec1be758a213c86c93c076dbe9f558c11c752
RESULT 03d6a3a9d62c7650fcac18f9ee68c7a004ebad71b7581b683062213ad9f37ddb28

Notice how the public keys 1 & 2 are equal, but the result does not match.
So far I have tried replacing pubkeys.count with allPubKeys.count which lead to the result looking like this: RESULT 02000000016fdfe810000000016fdfeca800000001e461e270000000016fdfe780.
Unfortunately that is about as far as my troubleshooting abilities allow me to go without breaking everything 😅.
I am also just starting to learn Git and Github, so if there is anything I can do to help with this PR, please let me know.
Thanks again for you help and patience!

@csjones
Copy link
Contributor Author

csjones commented Oct 30, 2023

First of all, thank you so much for putting this together! Super useful for what I am trying to build. I have been testing the code for a couple of days and been getting some unexpected behaviour, but that might be more due to me being a beginner dev. Seemingly unrelated code changes lead to the function giving me underlyingCryptoError. Also using try! instead of a do - catch block gives me the same error and I can't really figure out why.

When I try to test combining two public keys, whenever it does work, it returns a different result than the python wrapper for secp256k1, although I was careful to make sure of the right order and the fact that the python implementation only combines the keys within the list/array and not the key the function was called on.

This is how I tested in Swift: (hashToCurvesimply takes a string and hashes/rehashes it until the hash lies on the curve)

func test() {
    let publicKey = hashToCurve(message: "test1")
    let publicKey2 = hashToCurve(message: "test2")

    print("1: " + bytesToHexString(bytes: publicKey.dataRepresentation))
    print("2: " + bytesToHexString(bytes: publicKey2.dataRepresentation))
    
    let new:secp256k1.Signing.PublicKey
        
    do {
        new = try publicKey.combine([publicKey2], format: .compressed)
        print("RESULT " + bytesToHexString(bytes: new.dataRepresentation))
    } catch {
        print(error)
    }
}
1: 021b4f0e9851971998e732078544c96b36c3d01cedf7caa332359d6f1d83567014
2: 0260303ae22b998861bce3b28f33eec1be758a213c86c93c076dbe9f558c11c752
RESULT 034662b7e2fd30907601f3b16439ddb636792df7166ff0fe88275965eba37c5406

And this is the Python version:

point1 = hash_to_curve("test1".encode("utf-8"))
point2 = hash_to_curve("test2".encode("utf-8"))

print("1: " + point1.serialize().hex())
print("2: " + point2.serialize().hex())

new = PublicKey()
new.combine([point1.public_key, point2.public_key])

print("RESULT " + new.serialize().hex())
1: 021b4f0e9851971998e732078544c96b36c3d01cedf7caa332359d6f1d83567014
2: 0260303ae22b998861bce3b28f33eec1be758a213c86c93c076dbe9f558c11c752
RESULT 03d6a3a9d62c7650fcac18f9ee68c7a004ebad71b7581b683062213ad9f37ddb28

Notice how the public keys 1 & 2 are equal, but the result does not match. So far I have tried replacing pubkeys.count with allPubKeys.count which lead to the result looking like this: RESULT 02000000016fdfe810000000016fdfeca800000001e461e270000000016fdfe780. Unfortunately that is about as far as my troubleshooting abilities allow me to go without breaking everything 😅. I am also just starting to learn Git and Github, so if there is anything I can do to help with this PR, please let me know. Thanks again for you help and patience!

This is great feedback, thank you! 😊

I'll push up some small changes that should help figure out what's going wrong with the current implementation.

@csjones
Copy link
Contributor Author

csjones commented Oct 30, 2023

@zeugmaster I was able to re-create the issue you described, my first thought is the .map is off. Alternatively, my understanding of how to use libsecp256k1's combine pubkey API could be incorrect. I need to spend more time researching. I've pushed a small test in the meantime that's basically the issue you detailed above (thanks again for for the detailed feedback!).

@zeugmaster
Copy link

zeugmaster commented Nov 5, 2023

@csjones I've tried a few things in the meantime to narrow down the potential causes and would love to hear what you think about them:

  1. Simplified the function to combine two static pubkeys to avoid using .map for the test. Seems to work somewhat, although the result is still not what I was expecting:
func simpleCombine(format: secp256k1.Format = .compressed) {
        let context = secp256k1.Context.rawRepresentation
        var publicKey = secp256k1_pubkey()
        var pubKeyLen = format.length
        var pubBytes = [UInt8](repeating: 0, count: pubKeyLen)

        let publicKey1 = try! secp256k1.Signing.PublicKey(dataRepresentation: "021b4f0e9851971998e732078544c96b36c3d01cedf7caa332359d6f1d83567014".bytes, format: .compressed)
        let publicKey2 = try! secp256k1.Signing.PublicKey(dataRepresentation: "0260303ae22b998861bce3b28f33eec1be758a213c86c93c076dbe9f558c11c752".bytes, format: .compressed)
        
        print(publicKey1)
        print(publicKey2)
        
        var newPubKey1 = secp256k1_pubkey()
        publicKey1.dataRepresentation.copyToUnsafeMutableBytes(of: &newPubKey1.data)
        let pointerKey1: UnsafePointer<secp256k1_pubkey>? = withUnsafePointer(to: &newPubKey1) { $0 }
        
        var newPubKey2 = secp256k1_pubkey()
        publicKey2.dataRepresentation.copyToUnsafeMutableBytes(of: &newPubKey2.data)
        let pointerKey2: UnsafePointer<secp256k1_pubkey>? = withUnsafePointer(to: &newPubKey2) { $0 }
        
        let keys = [pointerKey1, pointerKey2]
        print(keys)
        
        let ret1 = secp256k1_ec_pubkey_combine(context, &publicKey, keys, 2)
        print(ret1)         // prints 1, according to docs the return value for valid key combination
        print(publicKey)    // prints valid secp256k1_pubkey but does not match what the python implementation produces
        
        let ret2 = secp256k1_ec_pubkey_serialize(context, &pubBytes, &pubKeyLen, &publicKey, format.rawValue)
        print(ret2)         // prints 1
        
        print(pubBytes)
}
  1. Had ChatGPT whip up some C and slightly modified it to check how secp256k1 combination works in it's native language. This works exactly as expected and produces the same public key as the python wrapper:
#include <stdio.h>
#include <stdlib.h>
#include <assert.h>
#include "/opt/homebrew/Cellar/secp256k1/0.4.0/include/secp256k1.h"

int main() {
    // Initialize the secp256k1 context
    secp256k1_context *ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY);

    // Two static secp256k1 public keys in serialized form (compressed format, 33 bytes)
    unsigned char pubKey1[] = {
        0x02, 0x1b, 0x4f, 0x0e, 0x98, 0x51, 0x97, 0x19, 0x98, 0xe7, 0x32, 0x07, 0x85, 0x44, 0xc9, 0x6b,
        0x36, 0xc3, 0xd0, 0x1c, 0xed, 0xf7, 0xca, 0xa3, 0x32, 0x35, 0x9d, 0x6f, 0x1d, 0x83, 0x56, 0x70, 0x14
    };

    unsigned char pubKey2[] = {
        0x02, 0x60, 0x30, 0x3a, 0xe2, 0x2b, 0x99, 0x88, 0x61, 0xbc, 0xe3, 0xb2, 0x8f, 0x33, 0xee, 0xc1,
        0xbe, 0x75, 0x8a, 0x21, 0x3c, 0x86, 0xc9, 0x3c, 0x07, 0x6d, 0xbe, 0x9f, 0x55, 0x8c, 0x11, 0xc7, 0x52
    };

    // Parse the public keys
    secp256k1_pubkey pubkey1, pubkey2;
    int ret1 = secp256k1_ec_pubkey_parse(ctx, &pubkey1, pubKey1, sizeof(pubKey1));
    int ret2 = secp256k1_ec_pubkey_parse(ctx, &pubkey2, pubKey2, sizeof(pubKey2));
    
    assert(ret1 == 1);
    assert(ret2 == 1);

    // Combine the two public keys
    const secp256k1_pubkey *pubkeys[] = { &pubkey1, &pubkey2 };
    secp256k1_pubkey combined_pubkey;
    int ret_combine = secp256k1_ec_pubkey_combine(ctx, &combined_pubkey, pubkeys, 2);

    assert(ret_combine == 1);

    // Serialize the combined public key
    unsigned char combined_serialized_pubkey[33];
    size_t combined_pubkey_len = sizeof(combined_serialized_pubkey);
    int ret_serialize = secp256k1_ec_pubkey_serialize(ctx, combined_serialized_pubkey, &combined_pubkey_len, &combined_pubkey, SECP256K1_EC_COMPRESSED);

    assert(ret_serialize == 1);

    // Do something with the combined public key, like printing it out
    printf("Combined Public Key: ");
    for (size_t i = 0; i < combined_pubkey_len; i++) {
        printf("%02x", combined_serialized_pubkey[i]);
    }
    printf("\n");

    // Destroy the secp256k1 context
    secp256k1_context_destroy(ctx);

    return 0;
}

After many hours of tinkering and staring at results at byte-level 🤦‍♂️ I am once again at my wits' end. 😅
Could it maybe have something to do with the Context?

@csjones
Copy link
Contributor Author

csjones commented Nov 9, 2023

@csjones I've tried a few things in the meantime to narrow down the potential causes and would love to hear what you think about them:

  1. Simplified the function to combine two static pubkeys to avoid using .map for the test. Seems to work somewhat, although the result is still not what I was expecting:
func simpleCombine(format: secp256k1.Format = .compressed) {
        let context = secp256k1.Context.rawRepresentation
        var publicKey = secp256k1_pubkey()
        var pubKeyLen = format.length
        var pubBytes = [UInt8](repeating: 0, count: pubKeyLen)

        let publicKey1 = try! secp256k1.Signing.PublicKey(dataRepresentation: "021b4f0e9851971998e732078544c96b36c3d01cedf7caa332359d6f1d83567014".bytes, format: .compressed)
        let publicKey2 = try! secp256k1.Signing.PublicKey(dataRepresentation: "0260303ae22b998861bce3b28f33eec1be758a213c86c93c076dbe9f558c11c752".bytes, format: .compressed)
        
        print(publicKey1)
        print(publicKey2)
        
        var newPubKey1 = secp256k1_pubkey()
        publicKey1.dataRepresentation.copyToUnsafeMutableBytes(of: &newPubKey1.data)
        let pointerKey1: UnsafePointer<secp256k1_pubkey>? = withUnsafePointer(to: &newPubKey1) { $0 }
        
        var newPubKey2 = secp256k1_pubkey()
        publicKey2.dataRepresentation.copyToUnsafeMutableBytes(of: &newPubKey2.data)
        let pointerKey2: UnsafePointer<secp256k1_pubkey>? = withUnsafePointer(to: &newPubKey2) { $0 }
        
        let keys = [pointerKey1, pointerKey2]
        print(keys)
        
        let ret1 = secp256k1_ec_pubkey_combine(context, &publicKey, keys, 2)
        print(ret1)         // prints 1, according to docs the return value for valid key combination
        print(publicKey)    // prints valid secp256k1_pubkey but does not match what the python implementation produces
        
        let ret2 = secp256k1_ec_pubkey_serialize(context, &pubBytes, &pubKeyLen, &publicKey, format.rawValue)
        print(ret2)         // prints 1
        
        print(pubBytes)
}
  1. Had ChatGPT whip up some C and slightly modified it to check how secp256k1 combination works in it's native language. This works exactly as expected and produces the same public key as the python wrapper:
#include <stdio.h>
#include <stdlib.h>
#include <assert.h>
#include "/opt/homebrew/Cellar/secp256k1/0.4.0/include/secp256k1.h"

int main() {
    // Initialize the secp256k1 context
    secp256k1_context *ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY);

    // Two static secp256k1 public keys in serialized form (compressed format, 33 bytes)
    unsigned char pubKey1[] = {
        0x02, 0x1b, 0x4f, 0x0e, 0x98, 0x51, 0x97, 0x19, 0x98, 0xe7, 0x32, 0x07, 0x85, 0x44, 0xc9, 0x6b,
        0x36, 0xc3, 0xd0, 0x1c, 0xed, 0xf7, 0xca, 0xa3, 0x32, 0x35, 0x9d, 0x6f, 0x1d, 0x83, 0x56, 0x70, 0x14
    };

    unsigned char pubKey2[] = {
        0x02, 0x60, 0x30, 0x3a, 0xe2, 0x2b, 0x99, 0x88, 0x61, 0xbc, 0xe3, 0xb2, 0x8f, 0x33, 0xee, 0xc1,
        0xbe, 0x75, 0x8a, 0x21, 0x3c, 0x86, 0xc9, 0x3c, 0x07, 0x6d, 0xbe, 0x9f, 0x55, 0x8c, 0x11, 0xc7, 0x52
    };

    // Parse the public keys
    secp256k1_pubkey pubkey1, pubkey2;
    int ret1 = secp256k1_ec_pubkey_parse(ctx, &pubkey1, pubKey1, sizeof(pubKey1));
    int ret2 = secp256k1_ec_pubkey_parse(ctx, &pubkey2, pubKey2, sizeof(pubKey2));
    
    assert(ret1 == 1);
    assert(ret2 == 1);

    // Combine the two public keys
    const secp256k1_pubkey *pubkeys[] = { &pubkey1, &pubkey2 };
    secp256k1_pubkey combined_pubkey;
    int ret_combine = secp256k1_ec_pubkey_combine(ctx, &combined_pubkey, pubkeys, 2);

    assert(ret_combine == 1);

    // Serialize the combined public key
    unsigned char combined_serialized_pubkey[33];
    size_t combined_pubkey_len = sizeof(combined_serialized_pubkey);
    int ret_serialize = secp256k1_ec_pubkey_serialize(ctx, combined_serialized_pubkey, &combined_pubkey_len, &combined_pubkey, SECP256K1_EC_COMPRESSED);

    assert(ret_serialize == 1);

    // Do something with the combined public key, like printing it out
    printf("Combined Public Key: ");
    for (size_t i = 0; i < combined_pubkey_len; i++) {
        printf("%02x", combined_serialized_pubkey[i]);
    }
    printf("\n");

    // Destroy the secp256k1 context
    secp256k1_context_destroy(ctx);

    return 0;
}

After many hours of tinkering and staring at results at byte-level 🤦‍♂️ I am once again at my wits' end. 😅 Could it maybe have something to do with the Context?

Hey @zeugmaster, this information was super helpful, thank you! I've spent time recently playing with the code you shared and confirmed it's not the Context (though I was thinking it was sus at one point 😉 ).

Now, I am confident this code (copyToUnsafeMutableBytes in particular ) is what's causing the issue:

        var keys = allPubKeys.map {
            var newPubKey = secp256k1_pubkey()
            $0.dataRepresentation.copyToUnsafeMutableBytes(of: &newPubKey.data)
            let pointerKey: UnsafePointer<secp256k1_pubkey>? = withUnsafePointer(to: &newPubKey) { $0 }
            return pointerKey
        }

I pushed a new unit test just using the binding but replacing Context with .none raw values. Getting closer!

@csjones
Copy link
Contributor Author

csjones commented Nov 9, 2023

Hey @zeugmaster, pushed an update were the Combine API works! Let me know what your thoughts are 😁

@csjones csjones marked this pull request as ready for review November 9, 2023 10:26
@csjones csjones linked an issue Nov 9, 2023 that may be closed by this pull request
@zeugmaster
Copy link

@csjones Works perfecty now!
I can not thank you enough! This was a major roadblock and I couldn't have solved or implemented it myself. These are my first steps toward working in open source and becoming a dev and your kind support with this means a lot. If we ever cross paths in real life, remind me that I owe you a beer (or coffee)! :)

@csjones
Copy link
Contributor Author

csjones commented Nov 14, 2023

@csjones Works perfecty now!

I can not thank you enough! This was a major roadblock and I couldn't have solved or implemented it myself. These are my first steps toward working in open source and becoming a dev and your kind support with this means a lot. If we ever cross paths in real life, remind me that I owe you a beer (or coffee)! :)

You're welcome, I am thrilled to hear that the API is working perfectly for you now! 🎉 No need to worry about owing me a beer or coffee, but if we do cross paths, it would be great to connect and talk Bitcoin! 😄 I appreciate the great feedback you gave, keep up the great work! 👏🚀

@csjones csjones merged commit 0f5f60e into main Nov 14, 2023
2 checks passed
@csjones csjones deleted the COMBINE branch November 14, 2023 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔏 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to use secp256k1_ec_pubkey_combine
2 participants