Skip to content

Conversation

@schildbach
Copy link
Member

I propose removing support for libsecp256k1 until a clear support path is established. Currently, the removed code is untested and it's not clear to me if it's still compatible.

Also I suspect this native code path stands in the way of implementing Schnorr signatures.

But don't worry: Of course, BouncyCastle is still used.

@msgilligan
Copy link
Member

msgilligan commented Jan 27, 2022

I think it makes sense to remove this code if it is unused and untested. However we should at least consider refactoring it to a separate module rather than removing it entirely. As you know, I recently was looking at this code and made some minor improvements via a PR or two.

One important question is what are the advantages to using this code over Bouncy Castle?

I was going to propose that we create an interface for the core EC (and I suppose Schnorr) functionality needed by bitcoinj. We could then provide one or more of the following implementations:

  1. Bouncy Castle
  2. libsecp256k1 via JNI
  3. libsecp256k1 via a more modern mechanism (perhaps "Panama" which will be final in JDK 22)
  4. Built in JDK cryptography which includes EC with the secp256k1 curve (was removed in JDK 16)

One specific argument for not removing this code is it at least provides an example of an alternate implementation and would allow us to use it as a starting place for the refactoring.

@schildbach
Copy link
Member Author

btw. The JNI bindings were removed in libsecp256k1 too: bitcoin-core/secp256k1#682

@schildbach
Copy link
Member Author

One important question is what are the advantages to using this code over Bouncy Castle?

Good question. If we were aiming to implement a full node, it would probably make sense to use native code from Bitcoin Core, at least for the consensus-critical parts.

But I guess we aren't. BouncyCastle is well supported and widely used. And I don't remember any bug that affected us.

@schildbach
Copy link
Member Author

I went ahead and at least deprecated the classes via 24095fd.

@schildbach schildbach force-pushed the remove-native-secp256k1 branch from ab79e3e to be10cd8 Compare June 15, 2022 08:39
@schildbach
Copy link
Member Author

Rebased this PR.

@schildbach schildbach force-pushed the remove-native-secp256k1 branch from be10cd8 to c9e2054 Compare July 29, 2022 11:40
@msgilligan msgilligan added this to the 0.17 milestone Mar 15, 2023
Of course, BouncyCastle is still used.
@schildbach schildbach force-pushed the remove-native-secp256k1 branch from c9e2054 to cf3c023 Compare September 30, 2023 15:49
@schildbach
Copy link
Member Author

Rebased once more.

@schildbach
Copy link
Member Author

I finally merged this. If we ever have an interface backed by multiple implementations (see #1874), I believe we'd have to rewrite this removed code anyway.

@schildbach schildbach closed this Sep 30, 2023
@msgilligan
Copy link
Member

@schildbach schildbach deleted the remove-native-secp256k1 branch March 10, 2025 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants