Page MenuHomePhabricator

[secp256k1] add schnorr sign jni binding

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


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

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

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

floreslorca created this revision.Mar 30 2019, 04:39
Owners added a reviewer: Restricted Owners Package.Mar 30 2019, 04:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 30 2019, 04:39
Herald added a subscriber: schancel. · View Herald Transcript
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.

427 ↗(On Diff #7874)

Is an ECDSA buffer appropriated for Schnorr signature ?

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 updated this revision to Diff 7880.Mar 30 2019, 21:41
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.
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

360 ↗(On Diff #7880)

I could only find the Java doc for this function: 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
jasonbcox accepted this revision.Apr 10 2019, 06:18

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

435 ↗(On Diff #7880)

Created a task for this since it's not necessary to block this diff for it:

360 ↗(On Diff #7880)

After more digging, @floreslorca pointed me to 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 accepted this revision.Apr 15 2019, 18:11
Fabien added inline comments.
321 ↗(On Diff #8061)

Nit: move the comment above

This revision is now accepted and ready to land.Apr 15 2019, 18:11
floreslorca updated this revision to Diff 8062.Apr 15 2019, 18:34

fix comment

This revision was landed with ongoing or failed builds.Apr 15 2019, 18:35
This revision was automatically updated to reflect the committed changes.