kernel: Add function for creating chainparams with a signet challenge #35464

sedited commented at 2:43 PM on June 4, 2026: contributor

Adds a function for creating a chainparams with a signet challenge to the kernel API. This was requested in issue #35362.

Also takes this opportunity to de-noise the test kernel binary log output a bit.

DrahtBot added the label Validation on Jun 4, 2026

DrahtBot commented at 2:43 PM on June 4, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35464.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v
Concept ACK	w0xlt
Stale ACK	oleonardolima, musaHaruna, edilmedeiros

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

sedited added this to a project on Jun 4, 2026

github-project-automation[bot] changed the project status on Jun 4, 2026

sedited changed the project status on Jun 4, 2026

stickies-v commented at 10:44 AM on June 5, 2026: contributor

Concept ACK

edilmedeiros commented at 12:58 PM on June 5, 2026: contributor

Concept ACK

A better design would be a function btck_chain_parameters_set_signet_challenge that modifies an existing btck_ChainParameters created with btck_chain_parameters_create if it is indeed a signet structure.

I don't like having two functions in the API that can be used to initialize btck_ChainParameters. That might lead to API creeping just like adding parameters to btck_chain_parameters_create to account for test networks as mentioned in #35362 (comment) (e.g. adding support for something like #35335 for testing as well).

What I'm suggesting is probably harder to implement, though. Will put some time to test.

sedited commented at 1:46 PM on June 5, 2026: contributor

A better design would be a function btck_chain_parameters_set_signet_challenge that modifies an existing btck_ChainParameters created with btck_chain_parameters_create if it is indeed a signet structure.

That works too, but it seemed more confusing to allow setting a signet challenge for a non-signet chain.

I don't like having two functions in the API that can be used to initialize btck_ChainParameters. That might lead to API creeping just like adding parameters to btck_chain_parameters_create to account for test networks as mentioned in #35362 (comment) (e.g. adding support for something like #35335 for testing as well).

I don't think this will end up being very noisy. Any further parameters, like deployment bits, can just be added to these chain-type specific functions. Another reason I didn't implement it that way is that we instantiate the chainparams internally as const.

oleonardolima commented at 2:40 PM on June 5, 2026: none

cACK 2ea7ac9aabe6ce4c42fd45c841bf615464b558cc

I've added new draft methods to rust-bitcoinkernel to use the new btck_chain_parameters_create_signet, see: https://github.com/sedited/rust-bitcoinkernel/pull/192/changes/3b5017e41fc5b29fc2ac65410d348c371ce8c099.

I tested with this custom signet and it worked well, though the new APIs I added feel a bit odd.

cc @edilmedeiros @johnnyasantoss

DrahtBot requested review from stickies-v on Jun 5, 2026

edilmedeiros commented at 10:34 PM on June 5, 2026: contributor

That works too, but it seemed more confusing to allow setting a signet challenge for a non-signet chain.

Yeah, that would involve deciding what to do for error handling (perform no side effect, probably).

Another reason I didn't implement it that way is that we instantiate the chainparams internally as const.

I tried to implement an alternative method like I said, but it would involve exposing more methods on CChainParams, but I'm not sure that is the best approach.

w0xlt commented at 1:35 AM on June 6, 2026: contributor

Concept ACK

edilmedeiros commented at 4:13 PM on June 6, 2026: contributor

tACK 2ea7ac9aabe6ce4c42fd45c841bf615464b558cc with a small C program and this datadir.

By the way, we need more examples of how to use this library.

It still feels odd to me to have something like btck_context_options_set_chainparams that modifies a structure and a different pattern with btck_chain_parameters_create_signet. Not a blocker for me, though, as I'm not totally familiar with the design discussions so far.

sedited commented at 9:12 PM on June 6, 2026: contributor

By the way, we need more examples of how to use this library.

I host the header's documentation with a few C examples on https://thecharlatan.ch/kernel-docs/. The tracking issue #27587 links this and lists all the language bindings that I know of. The docs for the Rust crate for example are fairly complete: https://docs.rs/bitcoinkernel/latest/bitcoinkernel/ . The tracking issue also lists and contextualizes most of the larger work going on at the moment. I realize this is a bit all over the place, but I don't really have a better solution for organizing these things into a single visible place. I used to link the tracking issue and project board in every kernel-related PR, maybe that would be helpful to do again?

edilmedeiros commented at 2:28 PM on June 8, 2026: contributor

I host the header's documentation with a few C examples on https://thecharlatan.ch/kernel-docs.

What about bringing them upstream: https://github.com/edilmedeiros/bitcoin/pull/2/changes?

Better if the CI could check if these examples build against changes in the library. I'll check how that could be done.

musaHaruna commented at 3:54 PM on June 23, 2026: contributor

Concept ACK on adding support for creating chain parameters with a custom Signet challenge.

I'm still fairly new to the kernel project and learning my way around the C API/FFI boundaries, so I may be missing some context here.

I have a question about a couple of edge cases around the challenge pointer and length. As I understand it, CChainParams::SigNetOptions::challenge is an std::optional<std::vector<uint8_t>>, where leaving it unset (std::nullopt) causes the default public Signet challenge to be used.

Given:

CChainParams::SigNetOptions options{
    .challenge = std::vector<uint8_t>{p, p + challenge_len},
};

would it be worth defensively handling invalid inputs from downstream FFI callers?

In particular:

If challenge == nullptr and challenge_len > 0, it looks like p + challenge_len could result in undefined behavior.
If challenge == nullptr and challenge_len == 0, would this construct an empty vector and populate the std::optional rather than leaving it unset? If so, is that behavior intentional, or could it prevent the default Signet challenge from being selected?

Would it make sense to guard against these cases and fail early, e.g.

if (challenge == nullptr || challenge_len == 0) {
    return nullptr;
}

or is the intended API behavior to allow these inputs and let validation happen elsewhere?

sedited force-pushed on Jun 24, 2026

sedited commented at 8:28 AM on June 24, 2026: contributor

Thank you for the review @musaHaruna,

Updated 2ea7ac9aabe6ce4c42fd45c841bf615464b558cc -> 012369da708c1a6a3300e877c051f7fc0af37f09 (kernel_signet_challenge_0 -> kernel_signet_challenge_1, compare)

Added nullptr implying zero length assertion, as done with other functions that take a user provided value.

If challenge == nullptr and challenge_len == 0, would this construct an empty vector and populate the std::optional rather than leaving it unset? If so, is that behavior intentional, or could it prevent the default Signet challenge from being selected?

Yes, that was my intention. I think defaulting on null is confusing when using this function. Developers that want the default signet can create it with btck_chain_parameters_create.

musaHaruna commented at 7:29 PM on June 24, 2026: contributor

ACK 012369d

in src/kernel/bitcoinkernel.h:965 in 012369da70

 957 | @@ -958,6 +958,17 @@ BITCOINKERNEL_API void btck_logging_connection_destroy(btck_LoggingConnection* l
 958 |  BITCOINKERNEL_API btck_ChainParameters* BITCOINKERNEL_WARN_UNUSED_RESULT btck_chain_parameters_create(
 959 |      btck_ChainType chain_type);
 960 |  
 961 | +/**
 962 | + * @brief Create a signet chain parameters struct with a user-provided
 963 | + * challenge.
 964 | + *
 965 | + * @param[in] challenge The signet challenge value. Blocks must satisfy it in

edilmedeiros commented at 8:29 PM on June 30, 2026:

I would add to the docstring that the function may break in case of a nullptr or challenge_len = 0

sedited commented at 10:17 AM on July 1, 2026:

We are not asserting on null here. We are merely asserting that when it is null, the length has to be zero. This pattern was introduced here: #35312 , so this just applies the existing pattern. I think this is fine to be honest. The only way I see this being triggered is on a serious programmer error, or on memory corruption. In both cases I prefer an immediate crash over an error code. If you have a different philosophy here, maybe post it in the other pull request, or open an issue?

edilmedeiros commented at 12:15 PM on July 1, 2026:

Not exactly an issue for me as the lib is intended to be consumed in bindings (that are supposed to sanitize inputs and deal with all unsafety).

stickies-v commented at 8:40 PM on July 1, 2026:

nit: currently we document the _len parameters in most places, and I think it's a useful habit

edilmedeiros commented at 8:30 PM on June 30, 2026: contributor

I don't like assertions used like that, but seems to be prevalent in Core's code base.

DrahtBot requested review from edilmedeiros on Jun 30, 2026

edilmedeiros commented at 12:32 PM on July 1, 2026: contributor

reACK 012369da708c1a6a3300e877c051f7fc0af37f09

in src/kernel/bitcoinkernel.h:970 in 012369da70

 965 | + * @param[in] challenge The signet challenge value. Blocks must satisfy it in
 966 | + *                      order to be valid.
 967 | + * @return              An allocated chain parameters opaque struct.
 968 | + */
 969 | +BITCOINKERNEL_API btck_ChainParameters* BITCOINKERNEL_WARN_UNUSED_RESULT btck_chain_parameters_create_signet(
 970 | +    const void* challenge, size_t challenge_len) BITCOINKERNEL_ARG_NONNULL(1);

stickies-v commented at 8:34 PM on July 1, 2026:

I don't think you meant to add BITCOINKERNEL_ARG_NONNULL(1) here, as this can be hit with an empty span?

in src/kernel/bitcoinkernel_wrapper.h:1062 in 012369da70

1058 | @@ -1059,6 +1059,9 @@ class ChainParams : public Handle<btck_ChainParameters, btck_chain_parameters_co
1059 |      ChainParams(ChainType chain_type)
1060 |          : Handle{btck_chain_parameters_create(static_cast<btck_ChainType>(chain_type))} {}
1061 |  
1062 | +    ChainParams(std::span<const std::byte> signet_challenge)

stickies-v commented at 8:36 PM on July 1, 2026:

We should probably make both ctors explicit?

in src/test/kernel/test_kernel.cpp:658 in 012369da70

 650 | @@ -651,6 +651,13 @@ BOOST_AUTO_TEST_CASE(logging_tests)
 651 |      Logger logger{std::make_unique<TestLog>()};
 652 |  }
 653 |  
 654 | +BOOST_AUTO_TEST_CASE(btck_chainparams_tests)
 655 | +{
 656 | +    ChainParams params_regtest{ChainType::REGTEST};
 657 | +    ChainParams params_signet{hex_string_to_byte_vec("51")};
 658 | +    CheckHandle(params_regtest, params_signet);

stickies-v commented at 8:45 PM on July 1, 2026:

nit: find it a bit confusing why we're comparing regtest with signet here, wouldn't signet <-> signet_challenge be more logical? either way, I think the CheckHandle helper is weird (or abused), but that's out of scope for this PR so also happy to leave as-is. The test case just confused me.

diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index eb4f9e6f47..ef65a25889 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -653,9 +653,9 @@ BOOST_AUTO_TEST_CASE(logging_tests)
 
 BOOST_AUTO_TEST_CASE(btck_chainparams_tests)
 {
-    ChainParams params_regtest{ChainType::REGTEST};
-    ChainParams params_signet{hex_string_to_byte_vec("51")};
-    CheckHandle(params_regtest, params_signet);
+    ChainParams params_signet{ChainType::SIGNET};
+    ChainParams params_signet_chlng{hex_string_to_byte_vec("51")};
+    CheckHandle(params_signet, params_signet_chlng);
 }
 
 BOOST_AUTO_TEST_CASE(btck_context_tests)

</details>

in src/test/kernel/test_kernel.cpp:1 in 012369da70 outdated

stickies-v commented at 8:56 PM on July 1, 2026:

I agree that not logging these expected messages is good, but these assertions seem weird to me. I think they only hold because of the current test cases that exist, and are not a propery of TestKernelNotifications behaving as expected?

If certain warnings are (not) expected in certain scenarios, they should be tested in the test case. If/when we need that we can improve TestKernelNotifications to customize notification behaviour, but for now I think they should just be no-op?

stickies-v commented at 8:56 PM on July 1, 2026: contributor

Approach ACK

kernel: Generate a signet with a challenge 5b4fd284f4

test kernel: Don't log on warnings change

Remove the overrides, since they didn't test anything meaningful.

a99148d576

sedited force-pushed on Jul 1, 2026

sedited commented at 9:31 PM on July 1, 2026: contributor

Updated 012369da708c1a6a3300e877c051f7fc0af37f09 -> a99148d576df36e0a2abc047a8757ab8f58b3b6b (kernel_signet_challenge_1 -> kernel_signet_challenge_2, compare)

Addressed @stickies-v's comment, remove BITCOINKERNEL_ARG_NONNULL.
Addressed @stickies-v's comment, mark constructors as explicit.
Addressed @stickies-v's comment, document len param.
Addressed @stickies-v's comment, create another signet handle when comparing handles.
Addressed @stickies-v's comment, removed warning override notifications.

stickies-v commented at 10:27 PM on July 1, 2026: contributor

ACK a99148d576df36e0a2abc047a8757ab8f58b3b6b

DrahtBot requested review from musaHaruna on Jul 1, 2026