Page MenuHomePhabricator

[secp256k1] add schnorr sign jni binding
ClosedPublic

Authored by floreslorca on Mar 30 2019, 04:39.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGING60e30bca748a: [secp256k1] add schnorr sign jni binding
rABC60e30bca748a: [secp256k1] add schnorr sign jni binding
Summary

add the jni binding for schnorr sign

Test Plan

run the single test but ill add more tests. any suggestions where i can take failing and positive tests is welcome

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ssign2
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5459
Build 8980: Bitcoin ABC Buildbot (legacy)
Build 8979: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Mar 30 2019, 04:39
Fabien requested changes to this revision.Mar 30 2019, 10:14
Fabien added a subscriber: Fabien.

The diff title is is about schnorr_verify but the content is about schnorr_sig.
Maybe you can look at the tests from libsecp256k1 to add the missing failing cases.

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
427 ↗(On Diff #7874)

Is an ECDSA buffer appropriated for Schnorr signature ?

src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.c
370 ↗(On Diff #7874)

Why not intsarray[0] = secp256k1_schnorr_sign(ctx, sig, data, secKey, NULL, NULL); ?

This revision now requires changes to proceed.Mar 30 2019, 10:14
floreslorca retitled this revision from [secp256k1] add schnorr verify jni binding to [secp256k1] add schnorr sign jni binding.Mar 30 2019, 19:16
floreslorca edited the summary of this revision. (Show Details)
floreslorca marked an inline comment as done.

make revision changes

jasonbcox requested changes to this revision.Apr 2 2019, 01:27
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
435 ↗(On Diff #7880)

Just from looking at this diff, it looks like`byteBuff` and seckey are not being explicitly cleaned up before this function exits. Given that Java's GC is "at will" the secret key could be floating around in memory.

If this memory is not being cleaned up somewhere, we may have to zero them out and clear them before exiting. The exact behavior may be impacted by my other question below about memory leaking. If a new object is NOT created, then we need to know the consequences of altering that memory.

Note that this issue may also impact other ECDSA functions in this file as well.

427 ↗(On Diff #7874)

Changes to this will likely depend on D2756

src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.c
360 ↗(On Diff #7880)

I could only find the Java doc for this function: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/jni-14.html#GetDirectBufferAddress it seems to indicate that a new array is allocated. Does that mean this is leaking? I don't see data being cleaned up.

This revision now requires changes to proceed.Apr 2 2019, 01:27

Accepted, but it will need to be rebased on master.

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
435 ↗(On Diff #7880)

Created a task for this since it's not necessary to block this diff for it: https://reviews.bitcoinabc.org/T587

src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.c
360 ↗(On Diff #7880)

After more digging, @floreslorca pointed me to https://www.ibm.com/support/knowledgecenter/en/SSYKE2_8.0.0/com.ibm.java.vm.80.doc/docs/jni_refs.html which indicates that objects created by the JNI will be cleaned up when the native function returns to the calling JVM. Assuming this is right, there should be no leak here.

This revision now requires review to proceed.Apr 10 2019, 06:18
Fabien added inline comments.
src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Test.java
321

Nit: move the comment above

This revision is now accepted and ready to land.Apr 15 2019, 18:11
This revision was landed with ongoing or failed builds.Apr 15 2019, 18:35
This revision was automatically updated to reflect the committed changes.