enable libsecp256k1_jni shared lib

[theuni]

This enables a libsecp256k1_jni shared lib to be built. It depends on libsecp256k1.

./configure --enable-jni should be all it takes assuming your java environment is setup properly. For now, there's no way to specify where the includes should be found if they're not located in $JAVA_HOME/include, as I'm not sure if that's necessary or not.

Notice that the lib has been renamed in the .java to reflect the lib's new name. This was done to match standard distro packaging names.

This is untested in a real java/jni build, verification that it actually works will be needed before merging.

[theuni]

ping @fatefree.
./autogen.sh && ./configure --enable-jni && make && sudo make install.

After that, you should be able to use the lib as before, with the exception that the name has changed. Would be great if you could test/verify.

[fatefree]

Thanks for your contribution! I gave it a try and ran into two issues..

1. I see a configure: error jni.h not found at C:/Progra~1/Java/jdk1.7.0_02/include/ which is actually the correct path. I believe just the check is wrong because if I remove this line from the configure file it proceeds to issue 2.

2. the jni.h header refers to a jni_md.h file which is not in the /include folder by default, it needs to be added as a second include path. This folder is different depending on enviroment. For windows, the include is found in java_home/include/win32 and linux is java_home/include/linux. (A bootleg check might be to simply add a new path to whatever folder is found inside /include.

I changed the make file just to see if it would work and put:
JNI_INCLUDES = -IC:/Progra1/Java/jdk1.7.0_02/include -IC:/Progra1/Java/jdk1.7.0_02/include/win32

After that, the make succeeds and I see a .lo file created, but not an .so. Is there another step to create the shared library out of the library object file?

[theuni]

Thanks for testing.

1. Could you please pastebin the config.log when running configure without skipping the first check? I'd like to see exactly why this fails. It works fine for me on Linux, there's probably something platform-specific at play there.
2. I see. In Linux (on my system, at least), those files are symlinked in the include dir. I've pushed a change so that they'll be found if not, and it should help find them for windows as well.

'make install' should install shared libs for you. In your case, it should be a dll.

[theuni]

Grr, I used "windows" rather than win32 for that path. I won't bother fixing it up yet since I'm not sure what to change it to... Do jdk's differentiate between win32 and win64? Or is the "include/win32" path used for both?

[fatefree]

I believe its win32 regardless, since that is the location of a 64bit jdk on my machine. I am using mingw for the record. Here is the pastebin:

http://pastebin.com/nmPi6HW6

[theuni]

Please paste the entire log so I can see the contents of the vars. I suspect the space in "Program Files" is the culprit.

[theuni]

@fatefree pushed a new version with improved search, not sure if it'll fix your issue or not. please test and paste the full log if it doesn't.

[sipa]

Code looks good, but I'd like some confirmation that this solves the issue.

[sipa]

Needs rebase and testing :)

[sipa]

@theuni is there a way to link in the library code statically, so the result doesn't need a second shared library?

[iangfc]

On Mac OSX: Configure is failing to find jni.h on OSX 10.9.5 / Java 1.7 and 1.8.
==> jni.h is located in $JAVA_HOME/include/
==> jni_md.h is located in $JAVA_HOME/include/darwin/

JAVA_HOME is self-set to (e.g.) /Library/Java/JavaVirtualMachines/jdk1.8.0_20.jdk/Contents/Home
These changes are as a result of Apple not shipping Java any more, it's a download from Oracle and so consequently OSX is normalised, looks a lot more like the others now.

I have an fixed version of m4/ax_jni_include_dir.m4 which does the right thing, which I'll do a pull request for when/if this present request is merged. (or mail me at iang at iang spot org.)

[iangfc]

@sipa: the contents of the fix works for me, altho merging requires care because it is a bit old.

[theuni]

Sorry, I have a few things to get to for libsecp256k1, but I've been busy with core. Will catch up here asap.

[theuni]

@sipa I did it that way at first, but decided that a separate lib made more sense going forward. Assuming the functions reachable from jni are stable (sign+verify), the jni lib would hardly ever need to change, and could be packaged with java apps easily. The real lib could then be updated separately with no other intervention from the app side.

That's probably naive though, and overlooking real-world complications. If you'd prefer to have them merged, I can put it back that way.

[sipa]

@theuni a stripped libsecp256k1.so now is 50 kB, so for avoiding storage or memory it's not actually useful. And in practice, yes, I think more (dependent) libraries is a nuisance.

[theuni]

OK. Will make the change.

[theuni]

rebased and updated to not use a separate lib. Other changes:

• on by default if headers are found since there's no separate lib
• should work on darwin where it didn't before (maybe?)

[sipa]

Your autodetection doesn't seem to work really - see Travis.

[theuni]

Grr, I remember fighting with this the first time around. Travis uses oracle's jdk, which has a different layout apparently. I'll dig up the fix needed.

[sipa]

Wait, why does libsecp256k1.la need to depend on libsecp256k1_jni.la?

[sipa]

@theuni ping.

[theuni]

Not sure what you're asking here. If jni is enabled, the objects need to be linked in.

If you're asking why it's a convenience lib and not just an appended source file, I did that because it requires extra include paths, and may require extra libs/lib paths (android for ex).

[sipa]

No, why does libsecp256k1.la depend on the jni library? I would expect the other way around?

[theuni]

Hmm, maybe we're not in sync here.

After your initial comments, I refactored this so that it doesn't produce a separate jni library anymore. It's all included in libsecp256k1.so. So the "jni library" is just a convenience lib containing org_bitcoin_NativeSecp256k1.o. The LIBADD here just adds the convenience lib into the final shared lib.

[sipa]

Ah, I was expecting a separate _jni library, just one that had the libsecp256k1 code statically linked in.

[sipa]

Sorry if I wasn't clear. I think it's a bit weird to have the library itself also function as front-end for itself...

[theuni]

I'm not sure I understand. If you're going to ship 2 libs, one without the jni symbol and 1 with.. why would one ever choose to use/deploy the one without? The size difference and added attack surface are practically zero, so I'm not understanding the benefit.

If _jni was shared and depended on the main lib, I'd agree. But I'm not following you in the static case.

[sipa]

Well, in terms of expected usage, I think you either want the library itself (which offers the API described in the .h file), or you want something to plug into Java (which offers the API described by the .java file). I wasn't expecting one library that could function as both, but in practice there's probably very little benefit in splitting it up.

One alternative could be to have a different top-level .c file which implements the Java API, which may be different (and for now, would definitely just be a subset), but that's probably overkill as well.

Concept ACK.

[sipa]

Rebase?

[theuni]

Rebased with the following changes:

• Added jni to the status message at the end of configure
• Fixed up the jni function to match the new _verify() api
• Updated java file to use the correct name

Btw, I'm happy to make whatever changes you'd like here. I'm not married to any particular configuration, I just wasn't following your logic in the last discussion.

[ghost]

Hi all, i added support for several more secp256k1 functions (sign, pubkey_create, sec/pub_verify) as well as associated testcases, which are all looking good - - this is a preliminary commit as i have yet to go in and clean it up with the appropriate comments, but i wanted to share what i had so that it has some eyes

https://github.com/faizkhan00/secp256k1/commit/7838a3ea4aabaa372cb9f4422531502aa6e43e60
comments, review, critique, nits all appreciated - - ! @theuni @sipa

[ghost]

Chatted with cfields/sipa about this on IRC, need to rebase with contexts handled once and only once in the JNI/Java wrapper, possibly using synch primitives

should have something mid-week or so with the update

[ghost]

Updated branch with pass-by-reference context instead of reinit-per-operation refactor

branch: https://github.com/faizkhan00/secp256k1/tree/jni-add-sign-verify-test

file-that-needs-review (pls!): https://github.com/faizkhan00/secp256k1/blob/jni-add-sign-verify-test/src/java/org_bitcoin_NativeSecp256k1.c

Thanks again

[theuni]

I think this needs some way of destroying the context. Maybe add a finalize() method that calls into c similar to the context init function? I know nothing of java though, so I'm clueless as to how that works on static members.

[ghost]

Yes, it seems I missed that method somehow, I've added a context_destroy() method that should now take care of that. About statics:

Static fields in Java aren't GC'ed until the class is unloaded or the classloader is unloaded, so the option for that is to move from static methods to a object-instantiation based approach, which would increase the complexity, for about 8 bytes of memory (java longs are 64 bit)- If its wanted I can look into doing this definitely, but I'm unsure if the added complexity would make the trade-off worthwhile.

[theuni]

@faizkhan00 The java long is just a pointer to malloc'd memory from the native side, which I believe is several hundred kb.

[ghost]

Worked on this a bit tonight, basically there is a edge case that I'm not 100% sure with how to handle:

We have builds on Travis for 32bit Linux , yet Travis doesn't A) support 32 bit JDKs (not sure how to fix) and B) the JNI lib is relatively untested with 32bit (fixable)

For the moment, I've disabled Travis for 32bit builds, and added documentation to the runner, but I'm not sure if that is the correct strategy for this instance. (https://travis-ci.org/9fe95262548e/secp256k1/jobs/61240914#L395)

Side note: I've also disabled the tests for make distcheck due to its unique build routine, you can check this also by looking at the output of ./run_jni_tests.sh in Travis. (https://travis-ci.org/9fe95262548e/secp256k1/jobs/61240912#L570)

Interesting

[theuni]

Ah, I was working on this at the same time. Here's my take on it: https://github.com/theuni/secp256k1/tree/travis-jni .
This adds a make check-java target, which anyone can run locally. Travis runs that as well.

The jar is cached for the future, which should help avoid any website downtime issues.
Build results here: https://travis-ci.org/theuni/secp256k1/builds/61244462

[ghost]

ah! nice work @theuni - i think i prefer your makefiles soln over hacky bash scripts ... so I think the only step is to include make check-java in .travis.yml, is that right?

[theuni]

@faizkhan00 it's already in there :)
See build 71.26 in the link above.

[ghost]

Haha I see , yea that works far far better than what I had in mind :) Cool! Nice work @theuni

[theuni]

@faizkhan00 just taking one last look at this.

I think it would make sense to re-work the native functions to pass the size parameters as separate int's rather than through the bytebuffers. As-is, I believe big-endian is broken.

So that would make this:

JNIEXPORT jint JNICALL Java_org_bitcoin_NativeSecp256k1_secp256k1_1ecdsa_1verify
  (JNIEnv* env, jclass classObject, jobject byteBufferObject, jlong ctx_l)
{
        secp256k1_context_t *ctx = (secp256k1_context_t*)ctx_l;

       unsigned char* data = (unsigned char*) (*env)->GetDirectBufferAddress(env, byteBufferObject);
       int sigLen = *((int*)(data + 32));
       int pubLen = *((int*)(data + 32 + 4));

look more like this:

JNIEXPORT jint JNICALL Java_org_bitcoin_NativeSecp256k1_secp256k1_1ecdsa_1verify
  (JNIEnv* env, jclass classObject, jobject byteBufferObject, jlong ctx_l, jint sigLen, jint pubLen)
{
       secp256k1_context_t *ctx = (secp256k1_context_t*)ctx_l;

       unsigned char* data = (unsigned char*) (*env)->GetDirectBufferAddress(env, byteBufferObject);

Obviously the function signature would have to be fixed up.

Thoughts?

[ghost]

@theuni Hey, I'm curious where you might be seeing platform-specific broken-ness... I would have thought this line in the Java code byteBuff.order(ByteOrder.nativeOrder()); would have appropriately detected endian-ness, am I missing something? Although, if you just prefer to have a more explicit function sig, I definitely think it looks better from a API perspective *and definitely can fix that up

I'm also pretty interested in checking endianness on a VM or via qemu, but I totally can understand if you think thats a tangent here :)

Interested to know more, I havent' dealt too heavily with platform-specific endianness issues in the past, and know how important they are to handle in this piece of code.

[theuni]

@faizkhan00 pushed a quick change to theuni@f69f471.

[theuni]

@faizkhan00 Yes, it looks like the nativeOrder() probably should've taken care of it. Still, I don't see why we should risk serializing/deserializing when we can pass by value more cleanly.

[ghost]

@theuni Ok, and yes, reviewing your code, the changes you made definitely make for a cleaner result, which is preferred, and also look good.

One question about this byteBuff.order(ByteOrder.LITTLE_ENDIAN); would it matter specifying what endianness it's in, coming out on the JNI side before GetDirectBufferAddress? I think I might prefer to leave that to the JVM's auto-detection, actually

[ghost]

@theuni I suppose I just was curious what we get from that change, irrespective of what the JVM is doing when using nativeOrder()

[theuni]

Whoops! Thanks for catching that!

I was playing around and forgot to revert that. Pushed a new change with that removed.

[ghost]

ok! the rest of it looks good to me :)

[ghost]

hey @theuni , looking over your branch a bit, i'm wondering - - is it worthwhile to add implementations for these functions? I think that with the code we have in place it should be pretty safe (and i feel fairly confident) in bringing these to JNI (perhaps with additional tests as well) - thoughts there?

    secp256k1_ec_pubkey_decompress
    secp256k1_ec_privkey_export
    secp256k1_ec_privkey_import
    secp256k1_ecdsa_sign_compact
    secp256k1_ecdsa_recover_compact

Perhaps also randomize() but that would entail synchronization safety, which could be a good thing to have anyway

[theuni]

@faizkhan00 Sure. Since you're doing the bulk of the work here now, how about adding the above (though you don't have to of course, it's fine as-is), then opening a new PR for fresh review?

I only have 2 complaints left, I believe. It'd be great if you could address them as you work on the other changes:

• tabs->spaces. Lots of tabs snuck in with your changes, please make sure that whitespace matches the rest of the codebase.
• On the native side of the JNI functions, there are several places where ret is ignored after library calls. Even if those should realistically never fail, there should still be a way to inform the Java side that they did.

If you're OK with it, at this point I'll close this PR and invite you re-open a fresh one when you're ready. You're welcome to squash down any of my stuff however it makes sense to you.

[ghost]

@theuni Ype yep, that sounds like a plan. I'll likely have time to work on this soon/tonight-ish, but I'll need to look into synch locking a bit more before committing anything- Sipa mentioned some really important failure modes that just got documented in the 'assumptions' thread, and I want to make sure we're good there.

• Tabbing: Yup I noticed this a bit while working and its annoying, good to hear spaces is preferred as that is how my editor is setup so should be no problem
• Ret: Yea this is an interesting one, I think handling this will make the Java side more of a proper native library (rather than a easy-to-use libsecp256k1) so I definitely see where that is going and will find some way to pass that through to the Java side.

Lets hold on closing the PR for the moment, I'll get those nits and funcs above addressed, and then when I open the new one, lets say we close this one *and continue the discussion there, ?

Squashing OK

[apoelstra]

Hi @faizkhan00,

For the record, I think pulling in Rust devtool dependencies to libsecp256k1 is not appropriate for the sake of JNI bindings; it's a lot of extra complexity and it feels weird to have this layer of indirection. When we talked on IRC I did not realize you were working on code to be merged into libsecp256k1 itself.

Andrew

[sipa]

Going to close this as outdated. Anyone feel free to take up the work of a testable JNI/whateverjava interface.