libsecp256k1_jni shared lib (continued)

[ghost]

This is a continuation of #64 where @theuni proposed expansions to the JNI shared lib, several improvements have been added to the PR. With @theuni 's efforts, we've expanded the original featureset to include:

*Makefile support for the JNI library
*Tests + harness
*sign/verify/create support
*Travis buildbot support

A note: I'm still working on expanding this, with several function implementations listed TODO and support for thread synchronization needed as well. I'm opening this here to keep the discussion moving from #64 and to be able to close that stale thread.

From the latter part of #64, I was addressing concerns on ignoring the retValue which I believe are now fixed by passing a jObjectArray (a 2d byte array) into the Java side, extracting the hex value and performing a final assert (never exposing this to the user) @ 3ce6181

edit: I haven't squashed this yet since its a WIP, technically, but I could do that once things settle a bit more and we get closer to a merge.

[ghost]

Chatting with @apoelstra a bit on IRC, it seems that the JNI is a pretty low level interface, and to help keep that layer free of bugs, we were discussing possibly using rust-libsecp256k1 as the native interface, instead of JNI.

I didn't want to pull in a whole bunch of new dependencies, but its starting to feel like this PR is getting rather large. I'm not exactly sure what makes sense to do, since calling Rust from Java entails using JNA (which itself has a really good mapping from native types to Java types) which would be yet another dependency to grab. (However, we might also be okay with just using JNA, and relying on their mappings)

I suppose I'm looking for a bit of direction here, since I'd like to avoid pointer/low-level errors, while also providing a high-quality interface for Java users, and so I'm very open to doing things in the most sensible way with minimal upkeep.

[ghost]

[wip]

Added support for

 secp256k1_ec_pubkey_decompress
    secp256k1_ec_privkey_export
    secp256k1_ec_privkey_import
    secp256k1_ecdsa_sign_compact
    secp256k1_ecdsa_recover_compact

will be shooting for reviewable later this week as i have still to implement tweak_* , context copy and synch locking.

[ghost]

I've added support for the rest of the library including synchronized access for context_destroy and randomize(), and I think I'm comfortable seeing what others think about this now that it's further along than it was the other week. There should be some more updates periodically while new PRs are merged in (#250) and rebases happen, but nothing major now.

[mikehearn]

I'm all for using JNA. That would simplify things for bitcoinj users quite a bit, I think. JNA is pretty similar to what the Java team want to integrate directly into the platform, so it's clearly the way to go. And it's available on Android so there should be no issue there if the ARM port is committed.

[ghost]

I have a rebase scheduled to keep up with API changes soon. I still am not sure if this would be better maintained downstream in a private repo, but for now its here. A JNA refactor might very well be something that I might do after the API changes are merged in, I think JNA's automatic type conversions are what I'm looking to include, I trust my untested JNI code less than what JNA probably does out-of-the-box and with tests.

[sipa]

@faizkhan00 The synchronization you're doing is not enough. You can't allow a call to randomize to happen at the same time as one to verify. The recommended model is a read/write lock: destroy and randomize then acquire a write lock, all the rest acquires a read lock.

An alternative is only calling randomize once at creation, and only making the context object available afterwards.

[sipa]

See http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReadWriteLock.html

[ghost]

@sipa Understood. Thanks for looking this over; synchronized allows one thread to execute the function at a time, providing safety in the case randomize is called separately from two different threads. I can see how the synchronization applied here wouldn't provide enough safety for verify and randomize to not be called simultaneously.

I'll be working on a patch over the next few days as I do have a few other updates to catch up on from the newer API changes, hopefully including r/w lock support to that list.

[sipa]

the synchronized mechanism works well, but you should apply it to every function that touches the shared state, including sign/verify.

Since that would prevent all multithreading whatsoever, you want another mechanism. Go have a look at that readwrite lock.

[sipa]

@faizkhan00 Still interested in working on this?

[ghost]

Yes, I'm running a bit behind on this, with most of the breaking API changes updated in a local tree. I've just got the locking mechnism to finish up, and with that conceptually understood, I just need to make those changes, which really should not take long.

EDIT: Locking mechanism, and possibly more testing and updating the pending API-BREAK PRs, too.

[instagibbs]

@faizkhan00 I'm useless at figuring out how to rebase this, but I may be able to help complete this post-rebase. I'm hoping the api changes have slowed down enough to get this finished.

[ghost]

@instagibbs Yeah, my thoughts as well, I was waiting for release/api changes to slow a bit but i can take a look soon about getting it up-to-date. There are still a few rough edges in this, I left a few comments as TODO if you're interested, or I just might later.

[afk11]

It looks like it's steadying a bit. I usually wait for dependent tests to crash and burn - until then, there's nothing to worry about - and it's been some time since the last break!

[ghost]

@instagibbs I've updated the API to match the latest code (haven't rebased, maybe when we're closer to merging) and added a few new TODO's and stubs (mainly for schnorr support) that I plan to look at soon (maybe today even) if you're interested in taking a look there

@afk11 Yup, although I wonder if there will be an api break for handling secret keys, which seem to be the only data not encapsulated by the api quite yet

[afk11]

@faizkhan00 Indeed, perhaps msg32's as well! I'm curious about the safest way of handling these and keys, I've been rejecting input where len != 32. Might be worth opening an issue..

[ghost]

superceded by #364, which I tested and can confirm is passing (with schnorr/ecdh tests as well)