enable libsecp256k1_jni shared lib

theuni commented at 7:27 pm on September 21, 2014: contributor

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 commented at 7:28 pm on September 21, 2014: contributor

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.

theuni cross-referenced this on Sep 21, 2014 from issue Is there an option to create a shared library for Java applications? by fatefree

fatefree commented at 9:14 pm on September 21, 2014: none

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

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.
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:/Progra~~1/Java/jdk1.7.0_02/include -IC:/Progra~~1/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 commented at 10:41 pm on September 21, 2014: contributor

Thanks for testing.

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.
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 commented at 10:51 pm on September 21, 2014: contributor

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 commented at 9:11 pm on September 22, 2014: none

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 commented at 10:18 pm on September 22, 2014: contributor

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 commented at 10:56 pm on September 22, 2014: contributor

@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 commented at 3:58 am on September 28, 2014: contributor

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

sipa commented at 7:54 am on November 5, 2014: contributor

Needs rebase and testing :)

sipa commented at 2:31 pm on November 5, 2014: contributor

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

iangfc commented at 11:41 pm on November 5, 2014: contributor

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 commented at 7:28 pm on November 8, 2014: contributor

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

theuni commented at 7:43 pm on November 8, 2014: contributor

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

theuni commented at 7:45 pm on November 12, 2014: contributor

@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 commented at 7:49 pm on November 12, 2014: contributor

@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 commented at 7:53 pm on November 12, 2014: contributor

OK. Will make the change.

theuni force-pushed on Nov 13, 2014

theuni commented at 4:53 am on November 13, 2014: contributor

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 commented at 10:48 am on November 13, 2014: contributor

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

theuni commented at 7:49 pm on November 13, 2014: contributor

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.

theuni force-pushed on Nov 13, 2014

in Makefile.am: in 88ec15216a outdated

57@@ -53,8 +58,11 @@ endif
58 
59 libsecp256k1_la_SOURCES = src/secp256k1.c
60 libsecp256k1_la_CPPFLAGS = -I$(top_srcdir)/include $(SECP_INCLUDES)
61-libsecp256k1_la_LIBADD = $(COMMON_LIB) $(SECP_LIBS)
62+libsecp256k1_la_LIBADD = $(COMMON_LIB) $(JNI_LIB) $(SECP_LIBS)

sipa commented at 11:26 am on November 14, 2014:

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

sipa commented at 11:52 pm on December 10, 2014:

@theuni ping.

theuni commented at 11:57 pm on December 10, 2014:

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 commented at 11:58 pm on December 10, 2014:

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

theuni commented at 0:05 am on December 11, 2014:

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 commented at 0:08 am on December 11, 2014:

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

sipa commented at 0:09 am on December 11, 2014:

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 commented at 0:16 am on December 11, 2014:

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 commented at 10:21 pm on December 13, 2014:

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 commented at 11:02 pm on December 16, 2014: contributor

Rebase?

theuni force-pushed on Dec 17, 2014

jni: update jni for api change

Also silence an unnecessary warning.

a4bdfa846d

theuni commented at 3:27 am on December 17, 2014: contributor

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.

build: add --enable-jni configure option

jni headers will be used if possible, otherwise jni support is disabled

6594d85a2f

build: cleanup jni m4

- Don't error if not found
- Don't use AC_CHECK_FILE which is disabled on cross
- Check for darwin path

2d9055f22c

sipa commented at 11:49 am on December 17, 2014: contributor

It would be nice if someone could test this. @TheBlueMatt? @iangfc?

iangfc commented at 7:31 pm on December 22, 2014: contributor

Yep, it works, once the following error is cleaned up: “configure: error: jni support explicitly requested but headers were not found” Fixed by adjusting build-aux/m4/ax_jni_include_dir.m4 to default to _JINC=…/include and only check for …/Headers if former is not found and is darwin. (Java 1.8.0_20-b26, Mac OSX 10.9.5.)

sipa commented at 0:34 am on February 13, 2015: contributor

@theuni @TheBlueMatt Any opinions on how this should be done after #208? Reintroduce a global in the JNI C code, or have the Java side hold a pointer to an initialized context on startup?

theuni commented at 7:07 pm on February 13, 2015: contributor

@sipa holding long-lasting references via jni can be error-prone and problematic due to gc. I think I’d prefer the global in C unfortunately.

sipa commented at 7:30 pm on February 13, 2015: contributor

@theuni The Java GC shouldn’t apply to C-allocated objects, I think?

In any case, if the JNI C side ends up keeping the context object, initialized at loading time, I feel unconfortable with it being the same .so file, as that would mean that every C user of the library ends up allocating and initializing an extra context.

theuni commented at 8:12 pm on February 13, 2015: contributor

@sipa You’re correct of course, brainfart on my part. I’m used to going the other way around with jni (calling java from c) where the gc gets in the way.

Looks like standard practice is to simply return a (jlong)ptr. Sounds good to me. I suppose we’ll need to add a jni function to create/init the context?

sipa commented at 8:32 pm on February 13, 2015: contributor

It’s a bit annoying that we don’t have any tests for the Java code. Just a trivial Java program that verifies a valid and an invalid signature would already be nice.

theuni commented at 3:58 pm on February 14, 2015: contributor

@sipa Indeed. I’ll see if I can throw something together. Would you like me to rebase this on top of #208 while I’m at it?

sipa commented at 7:32 pm on February 16, 2015: contributor

@theuni Sounds good; thanks a lot!

iangfc commented at 3:15 pm on February 25, 2015: contributor

On 13/02/2015 20:32 pm, Pieter Wuille wrote:

It’s a bit annoying that we don’t have any tests for the Java code. Just a trivial Java program that verifies a valid and an invalid signature would already be nice.

Understanding you’re pointing at me, I’m a bit snowed under at the moment and would be very happy if someone else could pick it up.

iang

ghost commented at 0:27 am on April 26, 2015: none

i realize this is lagging behind the current -master, but i was looking for a reason to explore this and ended up writing a simple testcase to maybe help things along- fwiw, this ran with no problems on my machine

test: https://github.com/faizkhan00/secp256k1/blob/jni-addverifytest/src/java/org/bitcoin/NativeSecp256k1Test.java

ghost commented at 5:30 am on April 28, 2015: none

Not sure if this is going anywhere, but I did the rebase anyway : https://github.com/faizkhan00/secp256k1/tree/jni-rebase

Tests are passing , compiler seems happy*

Diff: https://github.com/bitcoin/secp256k1/compare/master...faizkhan00:jni-rebase

Happy to take care of that nit if this has a chance of going upstream.

*EDIT: Fixed compiler nit.

ghost commented at 6:12 pm on April 28, 2015: none

@theuni If you have a chance, do you mind looking over my rebase attempt and considering it for inclusion in this PR? I realize everybody is a bit snowed under ATM, but I believe the issues brought up prior in this thread have been addressed (and if something hasn’t been, I can make changes - or if the PR is just not worth merging anymore I’d be interested to know this as well)

sipa commented at 6:23 pm on April 28, 2015: contributor

I will definitely merge it if it works and has tests.

theuni commented at 6:36 pm on April 28, 2015: contributor

@faizkhan00 Thanks for reviving this. Last time I rebased and was adding tests, I had a brief discussion about it on IRC and it sounded as though this wasn’t wanted anymore. If @sipa says he still wants it, I’ll have another look.

Thanks again!

ghost commented at 7:27 pm on April 28, 2015: none

Sure thing @theuni. While working on this I noticed the JNI coverage of the library is lacking (only supporting ecdsa_verify); I didn’t dig far enough to understand the intent of the original committer, but if keeping the JNI bindings upstream is the long-term intent, then I’d be interested in expanding the code coverage to include the rest of the secp256k1 API, including testcases, as well.

I think there’s quite a bit of value in that, but I don’t want to get in the way of your impending changes, so if you pushed a WIP branch up sometime I could probably work with your changes in parallel.

sipa commented at 7:30 pm on April 28, 2015: contributor

@faizkhan00 Feel free to expand it.

ghost commented at 8:07 pm on April 28, 2015: none

@sipa OK, should have something to share later this week.

ghost commented at 0:25 am on May 1, 2015: none

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 commented at 10:18 pm on May 3, 2015: none

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 commented at 0:04 am on May 4, 2015: none

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 commented at 3:28 pm on May 4, 2015: contributor

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 commented at 4:54 pm on May 4, 2015: none

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 commented at 5:01 pm on May 4, 2015: contributor

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

ghost commented at 2:43 am on May 5, 2015: none

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 commented at 3:30 am on May 5, 2015: contributor

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 commented at 3:46 am on May 5, 2015: none

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 commented at 3:48 am on May 5, 2015: contributor

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

ghost commented at 3:50 am on May 5, 2015: none

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

theuni commented at 10:25 pm on May 5, 2015: contributor

@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:

0JNIEXPORT jint JNICALL Java_org_bitcoin_NativeSecp256k1_secp256k1_1ecdsa_1verify
1  (JNIEnv* env, jclass classObject, jobject byteBufferObject, jlong ctx_l)
2{
3        secp256k1_context_t *ctx = (secp256k1_context_t*)ctx_l;
4
5       unsigned char* data = (unsigned char*) (*env)->GetDirectBufferAddress(env, byteBufferObject);
6       int sigLen = *((int*)(data + 32));
7       int pubLen = *((int*)(data + 32 + 4));

look more like this:

0JNIEXPORT jint JNICALL Java_org_bitcoin_NativeSecp256k1_secp256k1_1ecdsa_1verify
1  (JNIEnv* env, jclass classObject, jobject byteBufferObject, jlong ctx_l, jint sigLen, jint pubLen)
2{
3       secp256k1_context_t *ctx = (secp256k1_context_t*)ctx_l;
4
5       unsigned char* data = (unsigned char*) (*env)->GetDirectBufferAddress(env, byteBufferObject);

Obviously the function signature would have to be fixed up.

Thoughts?

ghost commented at 11:11 pm on May 5, 2015: none

@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 commented at 11:16 pm on May 5, 2015: contributor

@faizkhan00 pushed a quick change to https://github.com/theuni/secp256k1/commit/f69f4719e6f4337a3e5c2d929afad32fb131a90e.

theuni commented at 11:21 pm on May 5, 2015: contributor

@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 commented at 11:29 pm on May 5, 2015: none

@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 commented at 11:35 pm on May 5, 2015: none

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

theuni commented at 11:36 pm on May 5, 2015: contributor

Whoops! Thanks for catching that!

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

ghost commented at 11:37 pm on May 5, 2015: none

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

ghost commented at 1:45 am on May 7, 2015: none

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?

0    secp256k1_ec_pubkey_decompress
1    secp256k1_ec_privkey_export
2    secp256k1_ec_privkey_import
3    secp256k1_ecdsa_sign_compact
4    secp256k1_ecdsa_recover_compact

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

theuni commented at 1:56 am on May 7, 2015: contributor

@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 commented at 2:18 am on May 7, 2015: none

@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

unknown cross-referenced this on May 8, 2015 from issue libsecp256k1_jni shared lib (continued) by ghost

apoelstra commented at 1:47 am on May 11, 2015: contributor

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

unknown cross-referenced this on May 24, 2015 from issue Bindings for PHP by afk11

gmaxwell added this to the milestone initial release on Aug 31, 2015

sipa commented at 6:26 pm on September 4, 2015: contributor

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

sipa closed this on Sep 4, 2015

greenaddress cross-referenced this on Dec 9, 2015 from issue JNI rebased by greenaddress

real-or-random referenced this in commit 6395289e32 on May 31, 2019

enable libsecp256k1_jni shared lib #64