libsecp256k1_jni shared lib (continued)

ghost commented at 6:04 am on May 8, 2015: none

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) @ https://github.com/bitcoin/secp256k1/commit/3ce6181d94bae5984cc53fe40bc4629cb0087ec0

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.

jni: update jni for api change

Also silence an unnecessary warning.

a4bdfa846d

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

Add simple test a340fd63d4

Merge branch 'master' of github.com:bitcoin/secp256k1 into jni-attemptmerge 4830d6d98d

Rebase f1e04f4991

add support for several other secp256k1 functions, as well as testcases 7838a3ea4a

Refactor and pass single Secp256k1context instead of initializing per operation 85c9f197f3

long to jlong -> 64bit 8d7163db9d

Make sure there is a way to cleanup the context, and add comment to explain when it should be used. 4f1b7102f5

travis: add check-java make target and update travis to test it da729b6b0a

jni: pass size parameters rather than embedding them in the bytebuffers 0348e83232

Tabs to spaces 76bd6c2b83

Use 2d byte array to pass back ret 3ce6181d94

ghost commented at 0:40 am on May 11, 2015: none

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.

Add rest of to-be-implemented funcs, and implement pubkey decompression + test 524da6fd40

add support for sign_compact, verify_compact, privkey import/export (der) fbcf6a2f8f

ghost commented at 3:17 am on May 12, 2015: none

[wip]

Added support for

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

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

added tweak implementations, clone and tests 072d19370a

Cleanup, add randomize() and synchronized access 6d3facb433

ghost commented at 2:15 am on May 15, 2015: none

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 (https://github.com/bitcoin/secp256k1/pull/250) and rebases happen, but nothing major now.

mikehearn commented at 2:20 pm on July 31, 2015: none

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 commented at 5:10 pm on July 31, 2015: none

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 commented at 10:06 pm on August 3, 2015: contributor

@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 commented at 10:39 pm on August 3, 2015: contributor

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

ghost commented at 6:42 pm on August 6, 2015: none

@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 commented at 7:17 pm on August 6, 2015: contributor

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.

Merge branch 'master' of github.com:bitcoin/secp256k1 into jni-retvalfix 3476639ff0

Committing temp work 633e0e6b16

Merge branch 'master' of github.com:bitcoin/secp256k1 into jni-retvalfix-temp d2b7b285c9

Update ecdsa_Verify 88d81acc85

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

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

@faizkhan00 Still interested in working on this?

ghost commented at 11:18 pm on September 4, 2015: none

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.

Update test runner and remove deprecated API 769227601d

Add ReadWriteLock to library methods, separate static context from other classes and utils, add comments 8045c2fbc8

Merge branch 'master' of github.com:bitcoin/secp256k1 into jni-retvalfix 994ad95404

Fix warnings and update API with new typenames f5dcc25ecb

Add travis fix for locating secp256k1 libs 77717bd49d

instagibbs commented at 9:10 pm on December 2, 2015: none

@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 commented at 10:31 pm on December 2, 2015: none

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

Merge branch 'master' of github.com:bitcoin/secp256k1 into jni-retvalfix 74b034c586

afk11 commented at 2:22 am on December 3, 2015: contributor

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!

Update API to latest and clean up usage to use serialize/parse() methods for loading native sig/pubkey objects. Add TODO notes and stubs for rest of API addtiions b6dc95844a

ghost commented at 6:51 pm on December 5, 2015: none

@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 commented at 10:53 pm on December 5, 2015: contributor

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

greenaddress referenced this in commit 831c12506c on Dec 9, 2015

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

ghost commented at 9:05 pm on December 9, 2015: none

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

unknown closed this on Dec 9, 2015

libsecp256k1_jni shared lib (continued) #248