kernel: don't use assert to handle invalid user input

stickies-v commented at 3:24 pm on November 25, 2025: contributor

Crashing with assert for invalid user input is problematic for a library, e.g. this is impossible for FFI users to recover from, forcing them to re-implement the error handling logic client-side. See e.g. the go and .net bindings, or the discussion in https://github.com/stringintech/kernel-bindings-tests/issues/5.

This PR changes behaviour by:

updating btck_script_pubkey_verify to return -1 for invalid/malformed requests, 0 for invalid scripts, and 1 for valid scripts. Previously, the program would crash for the -1 cases.
updating 5 getters to return nullptr when the request index is out of bounds. Previously, the program would crash in these cases.

The wrapper is updated to throw std::runtime_error or std::out_of_bounds errors, and the tests are also updated.

By not crashing, we can properly test for these edge cases in kernel-bindings-tests to ensure that wrappers are handling them properly, as opposed to e.g. not differentiating between 0 and -1, which a naive (incorrect) implementation could do.

Note: I’m not sure why btck_ScriptVerifyStatus_ERROR_INVALID_FLAGS_COMBINATION and btck_ScriptVerifyStatus_ERROR_SPENT_OUTPUTS_REQUIRED are currently handled as INVALID_SCRIPT, in my view they should probably be INVALID_REQUEST (which means btck_ScriptVerifyStatus becomes meaningless in its current form), but I’ve not implemented that yet as I’m not 100% sure. Happy to update that here, or can also be done in a separate pull later.

kernel: return -1 on invalid btck_script_pubkey_verify request

Crashing on invalid user input is problematic for a library, e.g.
this is impossible for FFI users to recover from, forcing them
to re-implement the error handling logic client-side.

Improve this by returning a -1 error code instead of crashing.

Updates the C++ wrapper to throw a std::runtime_error for an
invalid request, and adds test coverage.

7bd3cfb840

kernel: return nullptr for out-of-bounds getters

Crashing on invalid user input is problematic for a library, e.g.
this is impossible for FFI users to recover from, forcing them
to re-implement the error handling logic client-side.

Improve this by returning a nullptr  instead of crashing.

Updates the C++ wrapper to throw a std::out_of_bounds error for an
invalid request, and adds test coverage.

5ccd6a1d38

DrahtBot added the label Validation on Nov 25, 2025

DrahtBot commented at 3:24 pm on November 25, 2025: 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/33943.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	stringintech, sedited, yuvicc

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33891 (kernel: Expose reusable PrecomputedTransactionData in script validation by joshdoman)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where named args may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

taproot_spent_script_pubkey.Verify(88480, taproot_tx, spent_outputs, 1, ScriptVerificationFlags::ALL, status) in src/test/kernel/test_kernel.cpp
taproot_spent_script_pubkey.Verify(88480, taproot_tx, wrong_size_spent_outputs, 0, ScriptVerificationFlags::ALL, status) in src/test/kernel/test_kernel.cpp

2025-11-25

?

added_to_project_v2 fanquake

?

project_v2_item_status_changed github-project-automation[bot]

in src/kernel/bitcoinkernel.cpp:628 in 5ccd6a1d38

621@@ -615,33 +622,45 @@ int btck_script_pubkey_verify(const btck_ScriptPubkey* script_pubkey,
622                               const btck_ScriptVerificationFlags flags,
623                               btck_ScriptVerifyStatus* status)
624 {
625-    // Assert that all specified flags are part of the interface before continuing
626-    assert((flags & ~btck_ScriptVerificationFlags_ALL) == 0);
627+    constexpr auto INVALID_REQUEST{-1};
628+    constexpr auto INVALID_SCRIPT{0};
629+    constexpr auto VALID_SCRIPT{1};
630+

janb84 commented at 7:48 pm on November 25, 2025:

I find this a bit inconsistent with this enum :

0typedef uint8_t btck_ValidationMode;
1#define btck_ValidationMode_VALID ((btck_ValidationMode)(0))
2#define btck_ValidationMode_INVALID ((btck_ValidationMode)(1))
3#define btck_ValidationMode_INTERNAL_ERROR ((btck_ValidationMode)(2))

Where validationmode runs from 0 to 2 and 2 is the internal error.

Also isn’t it better to expose this as an ENUM so that it can be re-used by the handlers ? (not sure because of the single use)

Or remove the int and use ScriptValidationFlag as return to be more in line with #33856 https://github.com/bitcoin/bitcoin/pull/33856/files#diff-8e83b7ea1d1a8ee1f496f2fda01436ba62b7ad926d1c61386a049d6ef392f71eR1052

EDIT: Kinda changed my view on this issue, “removed” option 3

stickies-v commented at 11:46 pm on December 3, 2025:

I think you’re right that there is no strict consistency wrt return codes in the API, and that’s also why I defined the status codes inside the function rather than in the header. A function-specific enum like btck_ScriptPubkeyVerifyResult could be an alternative, though. I didn’t do it to minimize overhead, but am open to doing that if preferred.

stringintech commented at 10:52 am on November 26, 2025: contributor

Concept ACK on being more strict with using assert.

I think it makes more sense for a public API to sanitize user input and communicate issues with the input, allowing for recovery and leave usages of assert for internal logical inconsistencies (library programmer errors that must never happen).

Regarding the approach: I like what is suggested in #33943 (review) (exposing the enum and having it as the return value). I’m still thinking about whether to differentiate invalid requests with different error codes or just have one code for all as you did.

Also, I remember at some point we had error codes instead of assertions for all cases of invalid user input in btck_script_pubkey_verify(): for example see https://github.com/TheCharlatan/bitcoin/blob/kernelApi_44/src/kernel/bitcoinkernel.cpp#L473. It would be good to go over/remember the rationale behind replacing some of them with assertions.

sedited commented at 1:05 pm on November 26, 2025: contributor

I’m Concept ACK on improving the behaviour around bad input, but not sure about adding more error handling surface to the API with the current approach. In my opinion the cases addressed are all programming errors, and should be treated as such. The argument that this duplicates the error handling code for the wrappers is not convincing to me and the PR itself as it currently stands provides the counter argument for it - an out of bounds case is still added and handled by the respective language wrapper. This seems sane to me, since every language will have its unique convention and way for handling these cases - from returning a variant to throwing an exception. Along these lines, I also don’t understand why the change here should make cross-bindings tests easier.

Might a better approach be adding a custom argument checker? The secp library has some that do both assertions and allow for a user-defined override. Personally, I think we’d be fine with just assertions, but a macro that can be defined to wrap a user-defined function through a function pointer seems useful for both debugging and providing a way to unify behaviour across tests.

EDIT: This is the secp behaviour I mean: https://github.com/bitcoin-core/secp256k1/blob/master/src/secp256k1.c#L47

yuvicc commented at 3:50 am on November 27, 2025: contributor

Concept ACK on fixing input validation. Agree with @sedited here, not sure if we want to add more error handling to the API. I found the secp256k1 approach using ARG_CHECK macros through callback to be more straightforward and appropriate.

in src/kernel/bitcoinkernel.cpp:636 in 5ccd6a1d38

634+    }
635 
636     if (!is_valid_flag_combination(script_verify_flags::from_int(flags))) {
637         if (status) *status = btck_ScriptVerifyStatus_ERROR_INVALID_FLAGS_COMBINATION;
638-        return 0;
639+        return INVALID_SCRIPT;

billymcbip commented at 7:38 pm on November 30, 2025:

You’re also validating flags here. Wouldn’t INVALID_REQUEST be more appropriate?

stickies-v commented at 11:48 pm on December 3, 2025:

I’m keeping the behaviour as-is as the default stance, but yes, I’m also not 100% sure on the boundary between INVALID_REQUEST and INVALID_SCRIPT here. I mention this at the end of the PR description, too. I’d like to understand that rationale better - perhaps @sedited can chime in?

in src/kernel/bitcoinkernel.cpp:521 in 5ccd6a1d38

515@@ -516,7 +516,10 @@ size_t btck_transaction_count_outputs(const btck_Transaction* transaction)
516 const btck_TransactionOutput* btck_transaction_get_output_at(const btck_Transaction* transaction, size_t output_index)
517 {
518     const CTransaction& tx = *btck_Transaction::get(transaction);
519-    assert(output_index < tx.vout.size());
520+    if (output_index >= tx.vout.size()) {
521+        LogError("output_index is out of bounds.");
522+        return nullptr;

billymcbip commented at 7:47 pm on November 30, 2025:

Noob question: How come you’re not throwing directly in this function (throw std::out_of_range("output_index is out of bounds")?

stickies-v commented at 11:44 pm on December 3, 2025:

The API is in C, and C doesn’t have exceptions, so we have to make sure no exceptions cross the C++/C boundary.

stickies-v commented at 0:03 am on December 4, 2025: contributor

but not sure about adding more error handling surface to the API with the current approach

One design goal of this PR was to minimize increasing the error handling surface while achieving the goal of making these functions more testable, but it seems like the current solution is not striking the right balance. I’m converting this PR to draft while I’m exploring other options, including implementing (global or local) error/illegal callbacks similar to secp256k1 (but that would be a pretty big overhaul). Thank you for the quick reviews and feedback everyone.

the PR itself as it currently stands provides the counter argument for it - an out of bounds case is still added and handled by the respective language wrapper

I see your point, but I think there is a difference between a language wrapper converting a documented return value into an exception and the language wrapper having to duplicate the implementation details.

I also don’t understand why the change here should make cross-bindings tests easier.

The C API itself should also be testable by the cross-bindings test suite, and on master it would just crash with an assertion error. Additionally, the cross-bindings test suite defines expected return values or exceptions (e.g. based on btck_ status codes), but assert is neither. Of course, the test suite could define its own exceptions/codes that are not part of the interface, but I think the tests should only cover the public interface. A language binding that covers the entire interface should be able to pass the tests without implementing additional logic. See also my comment here.

stickies-v marked this as a draft on Dec 4, 2025

?

project_v2_item_status_changed sedited

DrahtBot added the label Needs rebase on Dec 27, 2025

DrahtBot commented at 5:42 pm on December 27, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

kernel: don’t use assert to handle invalid user input #33943

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)