Update secp256k1 to 0.3.2 #4653
Conversation
The new code was dropped directly in `src/secp256k1` without changes. We can consider changing to a Git submodule, though that will require changes to the build instructions because we are not using submodules anywhere else.
|
In your PR description, can you link to where you got the secp256k1 source code from, and (if possible) the commit hash of the code used? |
There was a problem hiding this comment.
I checked out b9b7a71 and confirmed that src/secp256k1 is identical to bitcoin-core/secp256k1@acf5c55 (v0.3.2)
| set(SECP256K1_INSTALL TRUE) | ||
| add_subdirectory(src/secp256k1) | ||
| add_library(secp256k1::secp256k1 ALIAS secp256k1) |
There was a problem hiding this comment.
Why were these two lines added?
There was a problem hiding this comment.
The previous CMakeLists.txt in src/secp256k1 was written by us. The new CMakeLists.txt was written by the Bitcoin developers. The SECP256K1_INSTALL option is necessary to get it to include install instructions, and they don't define the ALIAS library (a CMake best practice) like we were.
|
Hey John, If we adopt a git submodule approach and if there is a mistake in the bitcoin-core's implementation of the secp256k1, isn't that a risk for rippled? |
|
We don't have to update to every new version of any dependency. Our update cadence is up to us. We always manually update dependencies. A mistake can be found in the implementation whether or not we use submodules. If a mistake is found in a dependency, the most common fix is to switch to a different version. |
|
@thejohnfreeman Okay. What are the tradeoffs between manually copying the code versus performing an import using In an ideal world, if this piece of code were exported as a library by the bitcoin-core developers, we could import it. That is a better solution right? |
|
@ckeshava I don't understand your questions. Manually copy the code where? We are |
|
ok, I misunderstood that using git submodules has an impact on the size of the rippled binary. thanks for the clarification. |
intelliot
added
Passed
|
note: confirmed with @thejohnfreeman that this looks good to go |
Copy the new code to `src/secp256k1` without changes: `src/secp256k1` is identical to bitcoin-core/secp256k1@acf5c55 (v0.3.2). We could consider changing to a Git submodule, though that would require changes to the build instructions because we are not using submodules anywhere else.
Copy the new code to `src/secp256k1` without changes: `src/secp256k1` is identical to bitcoin-core/secp256k1@acf5c55 (v0.3.2). We could consider changing to a Git submodule, though that would require changes to the build instructions because we are not using submodules anywhere else.
|
The new code was dropped directly in
src/secp256k1without changes. We can consider changing to a Git submodule, though that will require changes to the build instructions because we are not using submodules anywhere else.