Implement decompress function in JNI
#508
Conversation
This function was already exported but not implemented.
It seemed a little overkill given that we only use one trivial function from this library.
|
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
changed the title
parse function in JNIdecompress function in JNI
|
@Christewart Only partly, it doesn't include the Apart from that, attempting to decompress an invalid public key will result in an exception been thrown, so that's functionally equivalent to Also I somehow missed the file |
|
@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.NativeSecp256k1TestIf 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 |
* `Commitments()` renamed to `commitments()`. * `test` chain renamed to `testnet` * Updated `NativeSecp256k1` files as of commit a3d5dda of PR #508 of bitcoin-core/secp256k1 - awaiting for merge. See bitcoin-core/secp256k1#508
|
ping @greenaddress @sipa Thoughts on merging? |
|
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:
|
|
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
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 |
642cd06 Remove Java Native Interface (Jonas Nick) Pull request description: This was discussed in #508. The main reasons are that the existing Java Native Interface (JNI) bindings would need way more work to remain useful to Java developers but the maintainers and regular contributors of libsecp are not very familiar with Java (and evidently are motivated enough to improve the situation). We don't know who relies on these bindings with the exception of ACINQ who have their own fork at https://github.com/ACINQ/secp256k1/tree/jni-embed/src/java (@sstone). Bitcoinj can optionally use the libsecp bindings. Ideally, there would be a separate repository owned by Java developers with just the bindings. Until this exists, Java developers relying on libsecp can use ACINQs fork or an older commit of libsecp. ACKs for top commit: real-or-random: ACK 642cd06 I read the diff real-or-random: ACK 642cd06 I read the diff, and I verified that the diff to 7d9066a, which has been ACKed by sipa, is only the additonal removal of ax_jni_include_dir.m4 Tree-SHA512: 9e573f2b01897bd5f301707062b41de53424517b537ce0834d9049d003cfd73fa1bcc024b543256016e4c9a1126f7c7fef559b84dc4914083b5a2d0ad5e57ea8
|
Closing this because we removed the JNI bindings in #682. |
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.