JNI rebased #364

pull greenaddress wants to merge 2 commits into bitcoin-core:master from greenaddress:jni-rebased changing 13 files +1490 −46
  1. greenaddress commented at 8:04 am on December 9, 2015: contributor

    This is another continuation of #64 and #248 implementing JNI library for secp256k1, rebased to master.

    Also it includes a fix for a bug we noticed while including it in GreenBits - buffers were overflowing if two different functions were called consecutively with the second one needing more buffer space.

  2. ghost commented at 9:04 pm on December 9, 2015: none
    Thanks for pulling this forward and testing. It looks like Travis is still having a few problems in picking up the jni object, but I’m going to close my original PR in favor of this, as its rebased and tests are passing (running make check-java) locally at least
  3. unknown cross-referenced this on Dec 9, 2015 from issue libsecp256k1_jni shared lib (continued) by ghost
  4. greenaddress commented at 10:20 pm on December 12, 2015: contributor

    @faizkhan00 thank you and @theuni (and anyone else involved I missed) for the work done thus far.

    tests are passing locally but there seems to be a race condition in make where by the shared library is not fully linked by the time the test starts (but strangely only in travis, we can’t reproduce locally) - looks like we need to fix the dependency

    we tried to fix that and failed (we are not too strong on makefiles) so we asked @theuni and he suggested he may have a look at this at some point but anyone with any input is welcome

  5. theuni commented at 0:56 am on December 15, 2015: contributor

    @greenaddress Sorry for not getting to this at the meetup as promised, I kept getting pulled away.

    Anyway, that’s probably for the best. This turned out to be non-trivial to track down, and not build-system related at all.

    See the top few commits here for the fix: https://github.com/theuni/secp256k1/commits/jni-test

    tl;dr: JNIEXPORT on some Java versions (the one Travis uses by default, at least) is busted wrt symbol visibility.

  6. greenaddress commented at 10:15 am on December 15, 2015: contributor
    @theuni no worries! Cheers for finding the actual source of the problem!
  7. sipa commented at 9:44 pm on December 15, 2015: contributor
    Concept ACK, but can you rebase and squash the fixups?
  8. greenaddress force-pushed on Dec 16, 2015
  9. in src/java/org/bitcoin/NativeSecp256k1.java: in 0a30103b1e outdated
     98+        int pubLen = new BigInteger(new byte[] { retByteArray[1][0] }).intValue();
     99+        int retVal = new BigInteger(new byte[] { retByteArray[1][1] }).intValue();
    100+
    101+        assertEquals(pubArr.length, pubLen, "Got bad signature length.");
    102+
    103+        assertEquals(retVal, 1, "Failed return value check.");
    


    sipa commented at 8:11 pm on December 16, 2015:
    This function can legitimately return 0, if the passed signature’s R coordinate does not correspond to a point on the curve (around 50% chance for arbitrary byte arrays).
  10. in src/java/org/bitcoin/NativeSecp256k1.java: in 0a30103b1e outdated
    175+
    176+    /**
    177+     * libsecp256k1 Compute Pubkey - computes public key from secret key
    178+     *
    179+     * @param seckey ECDSA Secret key, 32 bytes
    180+     * @param compressed 1 to return compressed key, 0 for uncompressed
    


    sipa commented at 8:13 pm on December 16, 2015:
    Seems unimplemented.
  11. in src/java/org/bitcoin/NativeSecp256k1.java: in 0a30103b1e outdated
    246+
    247+        byte[][] retByteArray;
    248+
    249+        r.lock();
    250+        try {
    251+          retByteArray = secp256k1_ec_privkey_import(byteBuff,Secp256k1Context.getContext(), seckey.length);
    


    sipa commented at 8:14 pm on December 16, 2015:
    I don’t think that function still exists.
  12. in src/java/org/bitcoin/NativeSecp256k1.java: in 0a30103b1e outdated
    381+        int privLen = (byte) new BigInteger(new byte[] { retByteArray[1][0] }).intValue() & 0xFF;
    382+        int retVal = new BigInteger(new byte[] { retByteArray[1][1] }).intValue();
    383+
    384+        assertEquals(privArr.length, privLen, "Got bad pubkey length.");
    385+
    386+        assertEquals(retVal, 1, "Failed return value check.");
    


    sipa commented at 8:15 pm on December 16, 2015:
    This function can legitimately fail (though only in adverserial or very unlikely conditions).
  13. in src/java/org/bitcoin/NativeSecp256k1.java: in 0a30103b1e outdated
    465+
    466+        return pubArr;
    467+    }
    468+
    469+    /**
    470+     * libsecp256k1 create ECDH secret - constant time ECDH calculation
    


    sipa commented at 8:16 pm on December 16, 2015:
    Does not match the name of the function below.
  14. in src/java/org/bitcoin/NativeSecp256k1.java: in 0a30103b1e outdated
    471+     *
    472+     * @param seckey byte array of secret key used in exponentiaion
    473+     * @param pubkey byte array of public key used in exponentiaion
    474+     */
    475+    //TODO schnorrSign, schnoorVerify, schnorrRecover()
    476+    public static byte[] schnoorOps(byte[] pubkey, byte[] msg32) throws AssertFailException{
    


    sipa commented at 8:16 pm on December 16, 2015:
    Name is wrong (Schnorr, nor Schnoor).
  15. in src/java/org/bitcoin/NativeSecp256k1.java: in 0a30103b1e outdated
    487+        byteBuff.put(pubkey);
    488+
    489+        byte[][] retByteArray;
    490+        r.lock();
    491+        try {
    492+          retByteArray = secp256k1_ecdsa_recover(byteBuff,Secp256k1Context.getContext(), pubkey.length);
    


    sipa commented at 8:17 pm on December 16, 2015:
    Why all this commented-out code?

    unknown commented at 9:38 pm on December 16, 2015:
    When @greenaddress opened the PR, I definitely can say it was unfinished (not covering 100% of the secp256k1 headers) but I had intended to cover that. Depending on if @greenaddress was planning on completeing the coverage, I’d be happy to contribute the rest of the code as well (as a patch if desired)

    greenaddress commented at 10:11 pm on December 16, 2015:

    @sipa as you say the work is not complete, it doesn’t cover all functions yet but the aim is to have all of them.

    we plan to do more towards completing coverage but would be great to work together with @faizkhan00 (and @theuni) to finish it.

    we currently have pedersen_blind_sum, pedersen_commit and rangeproof_sign in the works


    sipa commented at 8:00 pm on January 27, 2016:
    @greenaddress I’m very interested in that work, but it shouldn’t go into the main library’s commit history. Please clean things up.
  16. JNI library
    Squashed and rebased. Thanks to @theuni and @faizkhan00 for doing
    the majority of work here! Also thanks to @btchip for help with debugging
    and review.
    3093576aa4
  17. JNI library: cleanup, removed unimplemented code 86e2d07e4c
  18. in src/java/org/bitcoin/NativeSecp256k1.java: in 0a30103b1e outdated
    76+     * @param data The data which was signed, must be exactly 32 bytes
    77+     * @param signature The signature
    78+     * @param compressed whether to recover a compressed pubkey
    79+     * @param pub The public key which did the signing
    80+     */
    81+    //TODO recoverCompact()
    


    sipa commented at 8:05 pm on January 27, 2016:
    What’s TODO about it?
  19. greenaddress force-pushed on Feb 1, 2016
  20. greenaddress commented at 1:10 pm on February 1, 2016: contributor

    secp256k1_ecdsa_recover was not implemented in org_bitcoin_NativeSecp256k1.c, hence the TODO and commented out code.

    I’ve now removed the unimplemented and commented parts and cleaned it up in general. I think it’s reasonable to keep these two commits for future reference, in case someone wants to look up the incomplete implementations in git history.

  21. sipa merged this on Feb 16, 2016
  22. sipa closed this on Feb 16, 2016

  23. sipa referenced this in commit e4570184ff on Feb 16, 2016
  24. afk11 cross-referenced this on Mar 2, 2016 from issue Minor issue in automatic enabling of JNI build by afk11
  25. sipa cross-referenced this on Nov 28, 2016 from issue Is there an option to create a shared library for Java applications? by fatefree

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-22 14:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me