Implement `decompress` function in JNI #508

This function was already exported but not implemented.

Being able to efficiently decompress a public key is useful for implementing LN on JVM-based languages, mainly because network announcements use the 33-bytes representation.

Last 2 commits are trivial but not related to the topic, please let me know if I should put them in a separate PR.

@pm47 Does this supersede #443?

@Christewart Only partly, it doesn't include the fCompressed flag.

Apart from that, attempting to decompress an invalid public key will result in an exception been thrown, so that's functionally equivalent to isValidPubKey. And we both removed the guava dependency ;-)

Also I somehow missed the file NativeSecp256k1Test.java, I'm going to add a test on decompress.

@pm47 Maybe you want to rip my code from #443 to have isValidPubKey that doesn't throw and then add a fCompressed flag? Then I'll close #443 and we can go with this one.

@Christewart Sounds good! I'll work on that this weekend

@Christewart done, I have taken your other changes and added more tests.

Guava dependency has been completely removed, the JNI part can be built and tested by simply doing:

$ ./autogen.sh
$ ./configure --enable-jni --enable-experimental --enable-module-ecdh
$ make
$ javac src/java/org/bitcoin/*
$ java -Djava.library.path=.libs -cp src/java org.bitcoin.NativeSecp256k1Test

If this gets approved, I'd also like to reformat java files before merging, they are quite messy currently.

Great, I'll test with bitcoin-s and make sure everything is working smoothly. Getting rid of the guava dependency is nice. Thanks for integrating! I'll close the other PR now

@greenaddress Feel like reviewing this?

Confirming these changes work with bitcoin-s. They are implemented in

https://github.com/bitcoin-s/bitcoin-s-core/pull/120

ping @greenaddress @sipa Thoughts on merging?

@pm47 @Christewart

nice changes!

sorry this took long.

(1) GUAVA_URL=https://search.maven.org/remotecontent?filepath=com/google/guava/guava/18.0/guava-18.0.jar GUAVA_JAR=src/java/guava/guava-18.0.jar

in .travis.yml

This should be removed since you removed GUAVA

(2) AssertFailException should also be removed from 465 (randomize)

(3) isValidPubKey is missing some checks that are in its sister method decompress. Maybe would be better if isValidPubKey was implemented in terms of calling decompress and returning false on failure.

Good catch re: guava in .travis.yml!

I'm not too sure about (3), but it is indeed simpler.

If that's ok with you I'd like to reformat before merging

bump this. we are also interested in getting this merged @greenaddress

It's pretty ridiculous this hasn't been merged yet. @sipa @gmaxwell @greenaddress

Do not ping me. I no longer work on this project.

Sorry for the slow answers here; there hasn't been that much activity in this project lately, and even less from people familiar with Java. I'll try to improve upon that, but it may be better if the JNI bindings move to a different project eventually, as it's hard to guarantee the same review standards as the rest of the project by the current maintainers. However, as long as that hasn't happened, it shouldn't stand in the way of merging useful improvements to the JNI code.

Overall, concept ACK to the code changes, but a few comments:

• This will need (probably trivial) rebase on top of #651.
• This PR is doing a lot more than adding a JNI decompress function, perhaps it should be renamed.
• I can't run the tests; I get src/java/org/bitcoin/NativeSecp256k1Test.java:206: cannot find symbol ... DataTypeConverter. What am I missing? (given that it works in Travis, I probably am).
• Please provide a clean commit history (no merges, no intermediary commits where tests fail).

@sipa The DataTypeConverter error should be gone after the rebase on #651; usage of this class has been removed therein.

Just FYI, the DataTypeConverter problem is due to JEP 320. I guess @sipa must be on OpenJDK 11+.

We started this PR and eventually decided to fork secp256k1 and modified the JNI bindings quite a bit, see https://github.com/ACINQ/secp256k1/tree/jni-embed

We did this because we wanted some changes, including this one, which just impacted the JNI bindings (we did not change anything in the core library), but also because it made things a lot simpler.

For example, we wanted to use a proper java build/dependency management tool (yes, maven, I can hear you all scream from where I am :)), organise source files in a more standard way, package the native library into the jar which means changing how it is loaded, and publish the jar to maven central which is just impossible (for us) if we keep it under org.bitcoin...

This is probably the reason why keeping JNI bindings here is a bit awkward: using java sources as they are now is clumsy because of how they are packaged (that's simple to fix though: move bindings to src/main/java and unit tests to src/test/java), and more importantly most JVM users will want something that integrates with their build tool, is deployment friendly which means embedding the native library into the JAR (this is what surebits and us have done independently for example) and available on maven central (or another widely used repo) and all that is really hard to do now.

There are also build issues: to cross-build you need JNI headers for the * target * OS which a standard JDK install won't give you, on Android you need to use a specific toolchain and cannot embed native libs into jars, ...

A first step towards this would be to re-organise java sources to match the maven convention (which is also used by sbt/gradle/...), which would easily let people add their own build files. But I think that the biggest issues are embedding native libs into the jar, and publishing the jar, is it something that secp256k1 maintainers would be willing to do ?

@sstone Maybe you can start a dedicated jni bindings repository, and the support will be removed from the core library? (just like https://github.com/rust-bitcoin/rust-secp256k1 for rust)

For my part I can say no, I am not willing or able to reorganize the Java sources. I don't know Java, don't know Maven, don't understand the org structure, and have never looked at the JNI parts of libsecp.

I'm curious if the story is different for any of the libsecp maintainers. If not, we should probably do what we can to separate the JNI bindings from the lib.

(Not a maintainer but) I think separating the bindings is a good idea. There is no reason to believe that they will get more love in the future.

Who else are large users? Certainly bitcoinj but I don't know any other project. /cc @schildbach

I'm not aware of any projects using it. Theoretically the bindings are a nice thing to have, but in reality signing is an extremely rare operation.

Maybe ask on the bitcoinj mailing list?

@schildbach Just for clarification: You're saying you're not aware of any projects besides bitcoinj? Or doesn't even bitcoinj use it? I'm asking because it seems to me that bitcoinj uses it: https://github.com/bitcoinj/bitcoinj/blob/master/core/src/main/java/org/bitcoin/NativeSecp256k1.java

Theoretically the bindings are a nice thing to have, but in reality signing is an extremely rare operation.

Yes, but we provide verification, too.

@elichai @real-or-random users can have a look at https://github.com/ACINQ/secp256k1/tree/jni-embed/src/java, and I believe that @Christewart has a similar project. Of course if we're the only users of the JNI bindings I guess the problem is solved :)

@real-or-random Bitcoinj provides the bindings, but they're optional to use. By default, BouncyCastle is used for signing and verification.

I think it's become clear that building and maintaining proper Java integration, including the requirements for build systems used in that community, will need work by people familiar with the language. I've personally not used Java since 2005, and I believe other regular contributors also don't have much experience.

If there is a minimal set of improvements we can make to the JNI bindings to keep them usable, that would be great. But if the result is going to involve other projects having their own packages/modification on top, I believe the best action is probably to remove it entirely. As the JNI bindings internally only use the public C API, there is no actual technical reason why it needs to be part of the same library anyway.

I'm no longer using the Java secp JNI bindings so I am unlikely to do more development or testing around it - I am supportive of moving things out, that being said I reviewed the above changes, they seem fine but it would need some rebase and some squashing I think

Closing this because we removed the JNI bindings in #682.