Feedback on 0.0.1

[craigraw]

To investigate the library in order to provide feedback, I attempted an integration into Sparrow (actually, drongo) in which I replaced the following functions with equivalents from secp256k1-jdk, using the ffm implementation:

1. Deriving a public point from a private key
2. Signing ECDSA
3. Verifying ECDSA signatures
4. Signing Schnorr
5. Verifying Schnorr signatures
6. Creating an ECDH secret from a public and private key

I was able to achieve the first 5 items, with some caveats which I'll list below. I've put the changes in a branch. The secp256k1_ecdh function was not present in secp256k1-jdk, so I wasn't able to implement the 6th item. Here are the issues I encountered:

• When distributing a client application, one cannot rely on the presence of any external libraries (like secp256k1). Generally, the approach is to package these libraries in the application jar. When the library is used, it is first extracted to a temporary location on the user's drive and loaded directly from there using System.load(filepath). However, the approach used by jextract is different - it expects to be able to find the library on java.library.path which must be specified on application startup, since it is only read once. The problem is in the static initializer: https://github.com/bitcoinj/secp256k1-jdk/blob/453da0b4bdeb41fe4b4f3942c10bb3f617e676eb/secp-ffm/src/main/java/org/bitcoinj/secp/ffm/jextract/secp256k1_h.java#L71-L73 I was able to get it working by commenting this block out and performing the library loading manually.
• Grinding ECDSA signatures for low R is important, not just to reduce transaction size but to create the same signatures across different wallets (all using RFC6979). secp256k1-jdk currently has no ability to specify the RFC6979 extra ndata in secp.ecdsaSign. Currently, it's set to nullPointer.
• I encountered the following warning when running the application - I'm not sure if anything can be done about it given it's code created by jextract, but I mention it since it's obviously not ideal to need to access to potentially unsafe operations.

  WARNING: A restricted method in java.lang.foreign.AddressLayout has been called
  WARNING: java.lang.foreign.AddressLayout::withTargetLayout has been called by org.bitcoinj.secp.ffm.jextract.secp256k1_h in module org.bitcoinj.secp.ffm
  WARNING: Use --enable-native-access=org.bitcoinj.secp.ffm to avoid a warning for callers in this module
  WARNING: Restricted methods will be blocked in a future release unless native access is enabled

Apart from these issues, everything went well. The client code was relatively easy to read/write. I did need to create two implementations (ByteArrayP256K1XOnlyPubKey and BigIntegerP256k1PrivKey) of the provided interfaces, which could be considered to be included in the api as I imagine they will be often required. I tested with secp256k1 v0.5.1.

[msgilligan]

Thanks, Craig! This is great news!

The secp256k1_ecdh function was not present in secp256k1-jdk, so I wasn't able to implement the 6th item.

For v0.0.1/v0.0.2 I've been focused on just Schnorr and Ecdsa signing/verification. I will add secp256k1_ecdh.

Generally, the approach is to package external libraries (like secp256k1) in the application jar.

I have done this many times in the past, but for secp-jdk I've been ignoring that issue until now. During development, especially when using Nix it has been convenient to access the library with java.library.path. I will open an issue for this and we will figure out a way to support both use cases.

Are you using jpackage to build the Sparrow distribution? I wonder if there is support in jpackage for setting java.library.path and having native binaries as files in the distribution.

secp256k1-jdk currently has no ability to specify the RFC6979 extra ndata in secp.ecdsaSign.

I will add this.

I encountered the following warning … WARNING: Use --enable-native-access=org.bitcoinj.secp.ffm

This is part of an effort called "Integrity by Default". See draft JEP and this video for more information. This change will affect JNI as well.

You should be able to add --enable-native-access=org.bitcoinj.secp.ffm to the startup configuration for your application. If your application is not running as a module app add --enable-native-access=ALL-UNNAMED

I like this approach, especially with the module system. This means the person building the app can control which modules are allowed native access and this helps protect against supply chain attacks.

ByteArrayP256K1XOnlyPubKey and BigIntegerP256k1PrivKey … will often be required.

I will look at them and add them or the equivalent functionality.

I'm not sure if anything can be done about it given it's code created by jextract.

I'm not sure we will always be using jextract to generate the FFM adapter files. Currently they are generated by jextract but checked in to Git. As I (we) learn more about FFM we might decide to manually improve these files. The secp256k1 library seems fairly stable with most changes being the addition of new functions, so maybe we could use jextract to generate support for new functions when they are added, but then go manual after that.

But for now, I do plan to continue with jextract unless there is some otherwise insurmountable issue.

Thanks again for doing this, this is really helpful feedback. I would have expected you to find more serious issues, so to me this is a very encouraging report.

[msgilligan]

Also, I have one comment on your drongo changes. Although Secp256k1 is marked as Closeable it doesn't have to be used in a try-with-resources. Since calling Secp256k1.get() initializes the secp256k1 context, you might consider modifying your Secp256k1Context.getContext() method to return a Secp256k1 instance rather than a long.

[craigraw]

Thanks - I agree, it is encouraging.

Are you using jpackage to build the Sparrow distribution? I wonder if there is support in jpackage for setting java.library.path and having native binaries as files in the distribution.

Yes I am. I did see this this answer but there would need to be another solution for development/testing to avoid having to run jpackage every time.

You might consider modifying your Secp256k1Context.getContext() method to return a Secp256k1 instance rather than a long.

Yes, good point.

[msgilligan]

I wonder if there is support in jpackage for setting java.library.path and having native binaries as files in the distribution.

there would need to be another solution for development/testing to avoid having to run jpackage every time.

Take a look at how I am setting it in Gradle. I'm assuming it was installed with Nix, but you don't have to use Nix. You could just set the path in Gradle in a similar way. (I think I even allow overriding the Nix location)

[msgilligan]

But again, I am open to supporting the old method of loading it out of a JAR if that is what is needed.

[craigraw]

Yes, it's certainly possible - my point was more that it means a different code path is used between development and production, which means double testing across all platforms.