Implement `decompress` function in JNI

pm47 commented at 10:28 am on February 21, 2018: none

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.

implement exported `parse` function in JNI

This function was already exported but not implemented.

287634c955

renamed `parse` to `decompress` f4e2e2738c

a little more consistency in method comments a0ce5d04c4

removed dependency on google guava library

It seemed a little overkill given that we only use one trivial function
from this library.

2c7977620f

pm47 commented at 2:29 pm on March 6, 2018: none

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 renamed this:
~~Implement exported `parse` function in JNI~~
Implement `decompress` function in JNI
on Mar 6, 2018

Christewart commented at 6:46 pm on March 8, 2018: none

@pm47 Does this supersede #443?

pm47 commented at 7:10 pm on March 8, 2018: none

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

Christewart commented at 7:16 pm on March 8, 2018: none

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

pm47 commented at 7:18 pm on March 8, 2018: none

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

fixed github link bitcoin->bitcoin-core b5eb51226c

tests: removed guava dependency + cleanup 71e0f84591

added pub key parsing tests ded99ddc42

add `compressed` arg to methods returning a pubkey 6bde3f14e7

removed unreachable null check 3c4009e0a9

fixup: typo in tests 2e16ac7d6c

pm47 commented at 6:03 pm on March 11, 2018: none

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

0$ ./autogen.sh
1$ ./configure --enable-jni --enable-experimental --enable-module-ecdh
2$ make
3$ javac src/java/org/bitcoin/*
4$ 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.

Christewart commented at 6:05 pm on March 11, 2018: none

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

Christewart cross-referenced this on Mar 11, 2018 from issue Adding compressed public key flag to java api by Christewart

sipa commented at 6:09 pm on March 11, 2018: contributor

@greenaddress Feel like reviewing this?

Christewart cross-referenced this on Mar 18, 2018 from issue Pm47 secp256k1 by Christewart

pm47 cross-referenced this on Mar 18, 2018 from issue Use a submodule for secp256k1 instead of copying files by pm47

Christewart commented at 0:11 am on March 20, 2018: none

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

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

Christewart cross-referenced this on Mar 21, 2018 from issue Submodule secp256k1 instead of copying files by mecampbellsoup

removed UnsatisfiedLinkError printed to stdout a3d5ddad8d

dpad85 referenced this in commit aa95d0038c on Mar 29, 2018

Christewart commented at 6:56 pm on April 18, 2018: none

ping @greenaddress @sipa Thoughts on merging?

greenaddress commented at 4:56 pm on August 27, 2018: contributor

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

removed download of guava lib in travis build f1597d6ccf

removed useless 'throws' fa63f54b71

re-implemented 'isValidPubKey' by calling 'decompress' 9294796595

pm47 commented at 3:28 pm on August 29, 2018: none

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

Merge branch 'master' into parse 4005555f51

tentatively remove guava from makefile.am 23819923ef

ghost commented at 8:41 pm on March 6, 2019: none

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

Christewart cross-referenced this on Jul 21, 2019 from issue JNI: add compressed arg by fingera

Christewart commented at 1:26 am on July 21, 2019: none

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

gmaxwell commented at 8:12 pm on July 26, 2019: contributor

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

sipa commented at 10:27 pm on August 6, 2019: contributor

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

real-or-random commented at 10:32 pm on August 6, 2019: contributor

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

ysangkok commented at 11:33 pm on August 6, 2019: none

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

sstone commented at 5:00 pm on August 7, 2019: none

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 ?

elichai commented at 5:19 pm on August 7, 2019: contributor

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

apoelstra commented at 6:14 pm on August 7, 2019: contributor

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.

real-or-random commented at 12:24 pm on August 8, 2019: contributor

(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

schildbach commented at 2:00 pm on August 8, 2019: none

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.

schildbach commented at 2:01 pm on August 8, 2019: none

Maybe ask on the bitcoinj mailing list?

real-or-random commented at 2:49 pm on August 8, 2019: contributor

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

sstone commented at 3:38 pm on August 8, 2019: none

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

schildbach commented at 4:31 pm on August 8, 2019: none

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

sipa commented at 0:39 am on August 9, 2019: contributor

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.

greenaddress commented at 3:10 pm on August 20, 2019: contributor

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

jonasnick cross-referenced this on Oct 29, 2019 from issue Remove Java Native Interface by jonasnick

real-or-random referenced this in commit d72b9e2483 on Feb 10, 2020

real-or-random commented at 11:12 am on February 10, 2020: contributor

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

real-or-random closed this on Feb 10, 2020

Implement decompress function in JNI #508

Implement `decompress` function in JNI #508