libsecp256k1 bindings: also use secp256k1 for point addition

[cculianu]

I saw this commit recently in Electrum: 65d896b

It was an easy port, but I have no idea if this helps or what. Is it worth doing? @markblundeberg help!

Note I ported it over by copy-pasting the code and adapting it without really understanding. I am weak at ECC crypto operations, sadly. I could use some help here.

All tox tests pass.. so that's a good sign.. I think. EC also runs and signs and verifies sigs ok both with and without schnorr.

No idea what I am doing though. :)

Should we merge this commit? Is it worth it? Help!

[cculianu]

Also Mark if you do end up helping here -- help me write tests for this? Or are the tests we already have sufficient?

(I couldn't easily port over the Electrum tests as they massively refactored bitcoin.py and so their tests wouldn't work for us unless somebody who understood crypto adapted them to our codebase a little bit...).

[markblundeberg]

Yeah this is a decent idea. Point addition isn't the slowest operation in EC math but it will definitely help if it's accelerated. I expect this would get used in the BIP32 key derivations.

The code looks exactly right too, no problems there. :) (If it runs, then it seems to be right!)

[cculianu]

Note that in the diff I modified the signature for secp256k1_ec_pubkey_combine on argument 3 to take instead of a POINTER(c_void_p) it now takes c_void_p.

This change affects your code here in schnorr.py:

Electron-Cash/lib/schnorr.py

Line 419 in 41874f8

res = seclib.secp256k1_ec_pubkey_combine(ctx, Rnew_buf, publist, 3)

The reason for my changing it to c_void_p is because the Electrum guys, in their commit, gave this function's arg their own signature POINTER(POINTER(c_char_p)), and they use it as such when calling it in add. c_void_p was the only type that basically allowed both codepaths to work (your schnorr code in schnorr.py and the stuff in ecc_fast.py) .

My reading of this situation is that it would work 100%. Can you sanity check me on that? :)

[markblundeberg]

Hmm the correct argument type should void **arg or perhaps char **arg, so that seems like too many pointers in their call signature (which I would read as char ***arg). Indeed just void *arg as you've done would be the most general approach and I can't see why ctypes wouldn't like it. 👍

[markblundeberg]

Hmm yeah actually those cast(pubkey1, POINTER(c_char_p)) in this commit should really be cast(pubkey1, c_char_p), and the line ptr_to_array_of_pubkey_ptrs = (POINTER(c_char_p) * 2)... should be array_of_pubkey_ptrs = (c_char_p * 2)... ! Man, casts let you get away with anything...

[markblundeberg]

For reference the C call signature:

SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_combine(
    const secp256k1_context* ctx,
    secp256k1_pubkey *out,
    const secp256k1_pubkey * const * ins,
    size_t n
) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);

where secp256k1_pubkey is a 64-byte struct.

[cculianu]

Hmm yeah actually those cast(pubkey1, POINTER(c_char_p)) in this commit should really be cast(pubkey1, c_char_p), and the line ptr_to_array_of_pubkey_ptrs = (POINTER(c_char_p) * 2)... should be array_of_pubkey_ptrs = (c_char_p * 2)... ! Man, casts let you get away with anything...

This makes my brain melt. I know C really well. I know Python well as well. But thinking about how ctypes tries to think about how C thinks about things makes my brain melt. You clearly are more of a ctypes master than me.

I'll leave it as-is for fear of touching it. After this is merged -- feel free to correct what they did.

I am pushing a commit now that adds some tests for this (I had to import a whole bunch of code from Electrum to do it -- but it's a good thing: they refactored that EC_KEY class in bitcoin.py into two more useful classes -- and having those classes going forward is a good thing.. I think).

Now bitcoin.py is full of so much redundant code.. ha!

[cculianu]

@markblundeberg Can you review this again? I pushed some changes that adds some stuff from Electrum (I actually wanted their tests for these operations -- but I ended up pulling in a bunch of stuff into bitcoin.py).

Can you review the changes in terms of relevancy? I am pretty sure the code is correct and the port is ok (although if your eyeballs catch something, that's good!).

All tests pass.

In particular I don't know what that signature65 business is and a bunch of utility functions added to bitcoin.py for this change also are enigmatic to me (there is some der_low_S stuff idk .. you know more than me!!).

Can you tell me if any of them need to be deleted? Maybe they are BTC-only things?

Also I realize bitcoin.py only grows more confusing now.. but my hope is we can slowly transition over to the Electrum refactor (they broke up the EC_KEY class into 2 more useful subclasses).

Thanks!

[markblundeberg]

I looked over the code and it seems generally reasonable, though for now it's just bloating the file I guess. :-)

Signature65 is the thing used in signed messages (65 byte signatures that allow unambiguous pubkey recovery).

[markblundeberg]

@SomberNight well that's one way to answer the question. Thanks :-)

[cculianu]

Ha ha — that works, @SomberNight.

Thanks Mark a ton for your review. I will update this PR soon to reflect discussion and ghost43’s changes.

I am thinking about the bloat issue — I may try and simplify the file by either re-using this code or some other approach.

[cculianu]

@markblundeberg Ok, I incorporated your changes but I want to let it sit some more. I am travelling but maybe I can find time to refactor what we have in bitcoin.py like I said to either use this code or .. something. Thanks again!