kernel: Remove key module from kernel library #29252

TheCharlatan · 2024-01-15T17:44:09Z

The key module's functionality is not used by the kernel library, but currently kernel users are still required to initialize the key module's secp256k1_context_sign global as part of the kernel::Context through ECC_Start. So move the ECC_Start call to the NodeContext ctor instead to completely remove the key module from the kernel library.

The gui tests currently keep multiple NodeContext objects in memory, so call ECC_Stop manually to avoid triggering an assertion on ECC_Start.

This PR is part of the libbitcoinkernel project. It removes a module from the kernel library.

DrahtBot · 2024-01-15T17:44:12Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, theuni, achow101
Stale ACK	maflcko, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

maflcko · 2024-01-16T09:10:05Z

The gui tests currently keep multiple NodeContext objects in memory, so relax the secp256k1_context_sign check in ECC_Start to instead return early if it has already been initialized.

An alternative would be to call ECC_Stop in the gui tests instead of modifying key code?

TheCharlatan · 2024-01-16T15:42:20Z

Updated 67058d4 -> 2cb38d9 (kernelRmKey_0 -> kernelRmKey_1, compare)

Implemented @maflcko's suggestion, calling ECC_Stop instead of relaxing the ECC_Start check.

src/kernel/context.h

src/kernel/checks.cpp

maflcko · 2024-01-16T16:02:25Z

lgtm ACK 2cb38d9 🍂

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 2cb38d93a8fea16bf84b072a90ac023ef345134d 🍂
3Z6w9mvDbuK4T+f2+g2HGbUkY+ETKts/CGAUhdhsu/MjBFRcdyVX4GOLme1SMwENKvYIENkhNym7luWlGcHRAw==

TheCharlatan · 2024-01-16T22:09:39Z

Updated 2cb38d9 -> a885a16 (kernelRmKey_1 -> kernelRmKey_2, compare)

Addressed @maflcko's comment, re-added explicit context destructor.
Addressed @maflcko's comment, removing dangling include for key.h.

maflcko · 2024-01-17T09:01:35Z

lgtm ACK a885a16 🔀

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK a885a166cec6d84d08600f12b25d912bd28af80e 🔀
MgtL4Gf030oUpUZQHa33bgM/KAIA6X7ogn4k7avoYVc87vxEv1qrxN9wxdvRTYiOZ1Hd8faqpxGbrRFpJIA9DQ==

hebasto · 2024-05-06T07:38:59Z

Concept ACK.

hebasto

ACK a885a16.

src/qt/test/rpcnestedtests.cpp

theuni

Very nice change. ACK a885a16

src/qt/test/rpcnestedtests.cpp

ryanofsky

Code review a885a16. This is a good change, but I think it has a few issues pointed out in comments below. At a high level I think it makes sense to decouple ECC initialization from the kernel context struct, but I don't think it makes sense to couple it so tightly with the node context struct.

Would suggest an approach more like this branch: pr/ecc with commits:

c6b4b7c common: Add ECC_Context RAII wrapper for ECC_Start/ECC_Stop
7184d36 test: Use ECC_Context helper in bench and fuzz tests
668e2f8 tools: Use ECC_Context helper in bitcoin-tx and bitcoin-wallet tools
7719001 kernel: Remove key module from kernel library

Feel free to take any of this or do something different.

src/kernel/context.h

src/qt/test/rpcnestedtests.cpp

src/node/context.cpp

theuni · 2024-05-08T18:17:12Z

@TheCharlatan Mind adding a commit to fixup the includes as per this conversation?

TheCharlatan · 2024-05-09T12:51:12Z

Thank you for the review @ryanofsky!

a885a16 -> 7dc7938 (kernelRmKey_2 -> kernelRmKey_3, compare)

Addressed @ryanofsky's comment, cherry-picked the mentioned commits. Also added another commit for removing the now unused ECC_Start and ECC_Stop from the header.
Addressed @theuni's comment, cleaning up some of the includes.

TheCharlatan · 2024-05-09T13:35:35Z

Rebased 7dc7938 -> 65a2051 (kernelRmKey_3 -> kernelRmKey_4, compare)

ryanofsky

Code review ACK 65a2051

src/key.h

The key module's functionality is not used by the kernel library, but currently kernel users are still required to initialize the key module's `secp256k1_context_sign` global as part of the `kernel::Context` through `ECC_Start`.

They are unused outside of the key module now.

TheCharlatan · 2024-05-09T14:01:36Z

Updated 65a2051 -> 96378fe (kernelRmKey_4 -> kernelRmKey_5, compare)

Addressed @ryanofsky's comment, moving comments and improving description of ECC_Context.

ryanofsky

Code review ACK 96378fe. Just suggested comment changes since last review.

theuni

Very nice improvements since my last review. The RAII context is an obvious improvement, thanks @ryanofsky.

I looked for a way to add a virtual lifetime class with on_startup/on_shutdown to replace the .init in fuzz.h, but didn't have any luck because of a (afacs?) missing teardown callback. It seems the static context approach used here is actually what's recommended.

utACK 96378fe

achow101 · 2024-05-10T17:07:29Z

ACK 96378fe

hebasto · 2024-05-20T23:03:55Z

Ported to the CMake-based build system in hebasto#202.

DrahtBot added the Validation label Jan 15, 2024

DrahtBot mentioned this pull request Jan 15, 2024

kernel: Run sanity checks on context construction #28228

Open

TheCharlatan force-pushed the kernelRmKey branch from 67058d4 to 2cb38d9 Compare January 16, 2024 15:41

maflcko reviewed Jan 16, 2024

View reviewed changes

src/kernel/context.h Show resolved Hide resolved

maflcko reviewed Jan 16, 2024

View reviewed changes

src/kernel/checks.cpp Outdated Show resolved Hide resolved

TheCharlatan force-pushed the kernelRmKey branch from 2cb38d9 to a885a16 Compare January 16, 2024 22:09

fanquake requested a review from theuni January 17, 2024 10:51

fanquake added this to the 28.0 milestone Feb 27, 2024

achow101 requested a review from darosior April 9, 2024 15:24

DrahtBot mentioned this pull request Apr 29, 2024

[WIP] Cluster mempool implementation #28676

Draft

8 tasks

hebasto approved these changes May 6, 2024

View reviewed changes

src/qt/test/rpcnestedtests.cpp Outdated Show resolved Hide resolved

theuni approved these changes May 6, 2024

View reviewed changes

src/qt/test/rpcnestedtests.cpp Outdated Show resolved Hide resolved

theuni mentioned this pull request May 6, 2024

Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

Merged

TheCharlatan marked this pull request as draft May 7, 2024 12:02

ryanofsky reviewed May 7, 2024

View reviewed changes

src/kernel/context.h Outdated Show resolved Hide resolved

src/kernel/context.h Show resolved Hide resolved

src/qt/test/rpcnestedtests.cpp Outdated Show resolved Hide resolved

src/node/context.cpp Outdated Show resolved Hide resolved

TheCharlatan marked this pull request as ready for review May 7, 2024 14:53

TheCharlatan force-pushed the kernelRmKey branch from a885a16 to 7dc7938 Compare May 9, 2024 12:50

TheCharlatan force-pushed the kernelRmKey branch from 7dc7938 to 65a2051 Compare May 9, 2024 13:35

ryanofsky approved these changes May 9, 2024

View reviewed changes

src/key.h Show resolved Hide resolved

DrahtBot requested a review from maflcko May 9, 2024 13:42

DrahtBot requested review from theuni and hebasto May 9, 2024 13:42

ryanofsky and others added 5 commits May 9, 2024 15:55

common: Add ECC_Context RAII wrapper for ECC_Start/ECC_Stop

538fedd

test: Use ECC_Context helper in bench and fuzz tests

28905c1

tools: Use ECC_Context helper in bitcoin-tx and bitcoin-wallet tools

a08d2b3

kernel: Remove key module from kernel library

41eba5b

The key module's functionality is not used by the kernel library, but currently kernel users are still required to initialize the key module's `secp256k1_context_sign` global as part of the `kernel::Context` through `ECC_Start`.

Refactor: Remove ECC_Start and ECC_Stop from key header

96378fe

They are unused outside of the key module now.

TheCharlatan force-pushed the kernelRmKey branch from 65a2051 to 96378fe Compare May 9, 2024 14:01

ryanofsky approved these changes May 9, 2024

View reviewed changes

theuni approved these changes May 9, 2024

View reviewed changes

achow101 merged commit 2cedb42 into bitcoin:master May 10, 2024
16 checks passed

theStack mentioned this pull request May 12, 2024

script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks) #28923

Open

hebasto added the Needs CMake port label May 20, 2024

hebasto mentioned this pull request May 20, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#202

Closed

hebasto removed the Needs CMake port label May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kernel: Remove key module from kernel library #29252

kernel: Remove key module from kernel library #29252

TheCharlatan commented Jan 15, 2024 •

edited

DrahtBot commented Jan 15, 2024 •

edited

maflcko commented Jan 16, 2024

TheCharlatan commented Jan 16, 2024

maflcko commented Jan 16, 2024

TheCharlatan commented Jan 16, 2024

maflcko commented Jan 17, 2024

hebasto commented May 6, 2024

hebasto left a comment

theuni left a comment

ryanofsky left a comment •

edited

theuni commented May 8, 2024

TheCharlatan commented May 9, 2024

TheCharlatan commented May 9, 2024

ryanofsky left a comment

TheCharlatan commented May 9, 2024

ryanofsky left a comment

theuni left a comment

achow101 commented May 10, 2024

hebasto commented May 20, 2024

kernel: Remove key module from kernel library #29252

kernel: Remove key module from kernel library #29252

Conversation

TheCharlatan commented Jan 15, 2024 • edited

DrahtBot commented Jan 15, 2024 • edited

Code Coverage

Reviews

Conflicts

maflcko commented Jan 16, 2024

TheCharlatan commented Jan 16, 2024

maflcko commented Jan 16, 2024

TheCharlatan commented Jan 16, 2024

maflcko commented Jan 17, 2024

hebasto commented May 6, 2024

hebasto left a comment

Choose a reason for hiding this comment

theuni left a comment

Choose a reason for hiding this comment

ryanofsky left a comment • edited

Choose a reason for hiding this comment

theuni commented May 8, 2024

TheCharlatan commented May 9, 2024

TheCharlatan commented May 9, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

TheCharlatan commented May 9, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

theuni left a comment

Choose a reason for hiding this comment

achow101 commented May 10, 2024

hebasto commented May 20, 2024

TheCharlatan commented Jan 15, 2024 •

edited

DrahtBot commented Jan 15, 2024 •

edited

ryanofsky left a comment •

edited