kernel: improve BITCOINKERNEL_WARN_UNUSED

stickies-v commented at 11:00 AM on May 21, 2026: contributor

Similar to [[nodiscard]], BITCOINKERNEL_WARN_UNUSED_RESULT is used to indicate that ignoring a function's return value is almost certainly a bug.

It is used in cases such as a resource leak (e.g. an owning handle returned by a *_create or *_copy function), or when the returned value is itself an error/status code. It is not used merely because discarding the result is wasteful, e.g. on getters or predicates.

Fix the incorrect usage, and properly document the attribute. Having this clearly documented helps avoid bikeshedding on future PRs.

DrahtBot added the label Validation on May 21, 2026

DrahtBot commented at 11:00 AM on May 21, 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/35344.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	w0xlt, stringintech, sedited
Concept ACK	yuvicc

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

yuvicc commented at 11:48 AM on May 21, 2026: contributor

Concept ACK, will be useful.

DrahtBot added the label CI failed on May 21, 2026

in src/kernel/bitcoinkernel.h:781 in ef4c7c2ab4

 777 | @@ -769,7 +778,7 @@ BITCOINKERNEL_API btck_ScriptPubkey* BITCOINKERNEL_WARN_UNUSED_RESULT btck_scrip
 778 |   * @param[out] status            Nullable, will be set to an error code if the operation fails, or OK otherwise.
 779 |   * @return                       1 if the script is valid, 0 otherwise.
 780 |   */
 781 | -BITCOINKERNEL_API int BITCOINKERNEL_WARN_UNUSED_RESULT btck_script_pubkey_verify(
 782 | +BITCOINKERNEL_API int btck_script_pubkey_verify(

stringintech commented at 3:23 PM on May 21, 2026:

Unlike plain predicates, the result here comes in two parts: status == OK does not imply script validity, and the return value still carries the actual valid/invalid result. Also this seems different from btck_transaction_check() and btck_block_check(), where the full outcome can be deduced from the validation state, so maybe keeping BITCOINKERNEL_WARN_UNUSED_RESULT here could help discourage callers from relying solely on the status out-param?

stickies-v commented at 7:12 PM on May 21, 2026:

This one is a bit of an odd one out. But I agree that the btck_script_pubkey_verify function signature is a bit confusing to use, and the annotation might help to prevent wrong usage (until ideally, we eventually clean up the function signature). So I've reverted this change. Thanks!

in src/kernel/bitcoinkernel.h:802 in ef4c7c2ab4

stringintech commented at 3:32 PM on May 21, 2026:

It seems to me that writer-based serialization functions could benefit from the macro, since a nonzero return can happen after the sink has already consumed part of the serialization, and checking the result helps avoid treating a partial output as complete.

stickies-v commented at 7:13 PM on May 21, 2026:

Good point. I've gone the opposite direction and added the annotation to all *_to_bytes() functions with an error code. Thanks for catching this!

in src/kernel/bitcoinkernel.h:1954 in ef4c7c2ab4

1950 | @@ -1943,7 +1951,7 @@ BITCOINKERNEL_API uint32_t BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_ge
1951 |   * @param[out] output   The serialized block header (80 bytes).
1952 |   * @return              0 on success.
1953 |   */
1954 | -BITCOINKERNEL_API int BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_to_bytes(
1955 | +BITCOINKERNEL_API int btck_block_header_to_bytes(

stringintech commented at 3:33 PM on May 21, 2026:

Also here, since checking the result might still be necessary before relying on the validity of the output buffer, maybe keeping BITCOINKERNEL_WARN_UNUSED_RESULT helps.

stringintech commented at 3:41 PM on May 21, 2026: contributor

Concept ACK on the general cleanup. GCC documents warn_unused_result as useful for functions where not checking the result is “either a security problem or always a bug.” So it makes sense to remove the annotation from plain getters and predicates. Left comments on the ones I wasn’t fully sure about.

DrahtBot removed the label CI failed on May 21, 2026

kernel: improve BITCOINKERNEL_WARN_UNUSED_RESULT usage

Similar to [[nodiscard]], BITCOINKERNEL_WARN_UNUSED_RESULT is used
to indicate that ignoring a function's return value is almost
certainly a bug.

It is used in cases such as a resource leak (e.g. an owning handle returned
by a *_create or *_copy function), or when the returned value is itself an
error/status code. It is not used merely because discarding the result is
wasteful, e.g. on getters or predicates.

Fix the incorrect usage, and properly document the attribute.

18c1cc65e9

stickies-v force-pushed on May 21, 2026

stickies-v commented at 7:14 PM on May 21, 2026: contributor

Force pushed to address review comments from @stringintech.

Added the annotation to all *_to_bytes() functions with error codes, and re-added it to the btck_script_pubkey_verify function as it has quite a confusing function signature.

w0xlt commented at 12:11 AM on May 22, 2026: contributor

ACK 18c1cc65e947c37584e886bede2cb5969a7b722d

DrahtBot requested review from stringintech on May 22, 2026

stringintech commented at 5:57 AM on May 22, 2026: contributor

ACK 18c1cc65

sedited added this to a project on May 22, 2026

github-project-automation[bot] changed the project status on May 22, 2026

sedited changed the project status on May 22, 2026

sedited approved

sedited commented at 7:38 AM on May 22, 2026: contributor

Thank you @stickies-v, the criteria look sane to me.

ACK 18c1cc65e947c37584e886bede2cb5969a7b722d

sedited merged this on May 22, 2026

sedited closed this on May 22, 2026

github-project-automation[bot] changed the project status on May 22, 2026

stickies-v deleted the branch on May 22, 2026

kernel: improve BITCOINKERNEL_WARN_UNUSED_RESULT usage #35344

Code Coverage & Benchmarks

Reviews