refactor(c-api): value_struct params by const* in the safe variant - #285
Merged
Conversation
📝 WalkthroughWalkthroughRefactored many C-API headers and implementations to take hash/fixed-size struct parameters as non-owning Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Secure as kth_core_secure_zero
participant OS as Platform API
Caller->>Secure: kth_core_secure_zero(void* p, size_t n)
Note right of Secure: if p==NULL or n==0 -> return
Secure->>Secure: choose mechanism by platform/build
alt Windows
Secure->>OS: SecureZeroMemory(p, n)
else C11 Annex K
Secure->>OS: memset_s(p, n, 0, n)
else explicit_bzero available
Secure->>OS: explicit_bzero(p, n)
else
Secure->>OS: volatile-store loop writing zeros
end
Secure-->>Caller: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
src/c-api/include/kth/capi/wallet/ec_private.h (1)
30-59:⚠️ Potential issue | 🟡 MinorUpdate the
_unsafedocs to match the newconst*safe API.The warnings still describe the safe variant as requiring by-value struct passing. After this change, callers need to pass a pointer to the typed struct.
📝 Proposed doc update
- * `@warning` `wif_compressed` MUST point to a buffer of at least 38 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + * `@warning` `wif_compressed` MUST point to a buffer of at least 38 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_wif_compressed_t`. @@ - * `@warning` `wif_uncompressed` MUST point to a buffer of at least 37 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + * `@warning` `wif_uncompressed` MUST point to a buffer of at least 37 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_wif_uncompressed_t`. @@ - * `@warning` `secret` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + * `@warning` `secret` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_hash_t`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/wallet/ec_private.h` around lines 30 - 59, Update the documentation comments for the `_unsafe` functions (kth_wallet_ec_private_construct_from_wif_compressed_version_unsafe, kth_wallet_ec_private_construct_from_wif_uncompressed_version_unsafe, kth_wallet_ec_private_construct_from_secret_version_compress_unsafe) to reflect that the API now accepts pointers to const buffers/typed structs (e.g., `uint8_t const*`), not by-value structs; change language that currently says "Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value" to instead state that callers must pass a pointer to a buffer/typed struct of the required minimum size (38, 37, and 32 bytes respectively) and that passing a shorter buffer is undefined behavior.src/c-api/include/kth/capi/chain/token_data.h (1)
30-59:⚠️ Potential issue | 🟡 MinorFix stale
_unsafeguidance after switching safe APIs to pointers.The warnings still tell callers to prefer the safe variant when their language can pass a C struct by value. The safe variant now requires a pointer to
kth_hash_t.📝 Proposed doc update
- * `@warning` `id` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + * `@warning` `id` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_hash_t`. @@ - * `@warning` `id` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + * `@warning` `id` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_hash_t`. @@ - * `@warning` `id` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + * `@warning` `id` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_hash_t`. @@ -/** `@warning` `value` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. */ +/** `@warning` `value` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_hash_t`. */ KTH_EXPORT void kth_chain_token_data_set_id_unsafe(kth_token_data_mut_t self, uint8_t const* value);Also applies to: 117-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/chain/token_data.h` around lines 30 - 59, Update the outdated warning text that tells callers to prefer the safe variant "when your language can pass a C struct by value"—the safe APIs now take a pointer to kth_hash_t, so change the warning for kth_chain_token_make_fungible_unsafe, kth_chain_token_make_non_fungible_unsafe, and kth_chain_token_make_both_unsafe (and the duplicate block around lines 117-121) to state that the safe variant accepts a kth_hash_t pointer (kth_hash_t const* id) and remove the advice about passing a C struct by value; keep the note that id must point to at least 32 bytes and that callers must release non-NULL results with kth_chain_token_data_destruct.src/c-api/include/kth/capi/chain/stealth_compact.h (1)
46-64:⚠️ Potential issue | 🟡 MinorRefresh the
_unsafewarning text for pointer-based safe variants.These warnings still say the safe variant is preferred when the caller can pass a C struct “by value”, but the safe variants now take
const*parameters. This can mislead FFI consumers toward the raw-buffer APIs.📝 Proposed doc update
-/** `@warning` `value` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. */ +/** `@warning` `value` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_hash_t`. */ KTH_EXPORT void kth_chain_stealth_compact_set_ephemeral_public_key_hash_unsafe(kth_stealth_compact_mut_t self, uint8_t const* value); @@ -/** `@warning` `value` MUST point to a buffer of at least 20 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. */ +/** `@warning` `value` MUST point to a buffer of at least 20 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_shorthash_t`. */ KTH_EXPORT void kth_chain_stealth_compact_set_public_key_hash_unsafe(kth_stealth_compact_mut_t self, uint8_t const* value); @@ -/** `@warning` `value` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. */ +/** `@warning` `value` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_hash_t`. */ KTH_EXPORT void kth_chain_stealth_compact_set_transaction_hash_unsafe(kth_stealth_compact_mut_t self, uint8_t const* value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/chain/stealth_compact.h` around lines 46 - 64, The warning comments for the `_unsafe` setters (kth_chain_stealth_compact_set_ephemeral_public_key_hash_unsafe, kth_chain_stealth_compact_set_public_key_hash_unsafe, kth_chain_stealth_compact_set_transaction_hash_unsafe) are outdated: they tell callers to prefer the “safe variant when your language can pass a C struct by value” but the safe variants now take typed pointers (kth_hash_t const*, kth_shorthash_t const*). Update each warning to clearly state the safe (non-_unsafe) variant expects a pointer to the corresponding typed buffer (e.g. kth_hash_t const* or kth_shorthash_t const*) of the required length, and advise FFI consumers to use those typed pointer APIs when their language can pass/represent the fixed-size struct or typed buffer safely rather than raw uint8_t buffers; keep the explicit byte-length requirement (32 or 20 bytes) for the `_unsafe` raw-buffer overloads.src/c-api/test/chain/point.cpp (1)
206-230:⚠️ Potential issue | 🟡 MinorUpdate the precondition docs/tests for the safe pointer APIs.
These comments still say safe calls take hashes by value, but
kth_chain_point_constructandkth_chain_point_set_hashnow acceptkth_hash_t const*, soNULLcompiles and should hit the new runtime precondition. Please add safe-variant death tests alongside the_unsafeones.Suggested test coverage update
-// Safe `kth_chain_point_construct` takes `kth_hash_t` by value: passing -// NULL is a compile error. The runtime precondition still applies on the -// `_unsafe` companion. +TEST_CASE("C-API Point - construct null hash aborts", + "[C-API Point][precondition]") { + KTH_EXPECT_ABORT(kth_chain_point_construct(NULL, 0)); +} + TEST_CASE("C-API Point - construct_unsafe null hash aborts", "[C-API Point][precondition]") { KTH_EXPECT_ABORT(kth_chain_point_construct_unsafe(NULL, 0)); } @@ +TEST_CASE("C-API Point - set_hash null aborts", + "[C-API Point][precondition]") { + kth_point_mut_t point = kth_chain_point_construct_default(); + KTH_EXPECT_ABORT(kth_chain_point_set_hash(point, NULL)); + kth_chain_point_destruct(point); +} + TEST_CASE("C-API Point - set_hash_unsafe null aborts", "[C-API Point][precondition]") { kth_point_mut_t point = kth_chain_point_construct_default();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/test/chain/point.cpp` around lines 206 - 230, The tests and comments assume the safe APIs take hashes by value, but kth_chain_point_construct and kth_chain_point_set_hash now accept kth_hash_t const* and should abort on NULL at runtime; add death tests mirroring the existing unsafe checks: add TEST_CASEs that call kth_chain_point_construct(NULL, 0) and kth_chain_point_set_hash(point, NULL) wrapped with KTH_EXPECT_ABORT, create/destroy a default point as needed (use kth_chain_point_construct_default() and kth_chain_point_destruct()), and update the nearby comments to remove the "by value" claim so the tests and docs reflect the new pointer precondition.src/c-api/test/chain/double_spend_proof.cpp (1)
307-314:⚠️ Potential issue | 🟡 MinorAdd safe null-pointer death coverage for the new hash setter signatures.
The PR adds pointer-based safe setters, but this precondition section still only validates the
_unsafenull path. A regression inkth_chain_double_spend_proof_spender_set_prev_outs_hash(sp, NULL)/set_sequence_hash/set_outputs_hashwould not be caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/test/chain/double_spend_proof.cpp` around lines 307 - 314, Add tests to cover the safe pointer-based hash setters for null-pointer preconditions: call kth_chain_double_spend_proof_spender_set_prev_outs_hash(sp, NULL), kth_chain_double_spend_proof_spender_set_sequence_hash(sp, NULL), and kth_chain_double_spend_proof_spender_set_outputs_hash(sp, NULL) (in addition to the existing _unsafe test) and assert they abort/trigger the same death behavior (use KTH_EXPECT_ABORT or equivalent). Locate these new checks near the existing DspSpender precondition tests that call kth_chain_double_spend_proof_spender_construct_from_data and kth_chain_double_spend_proof_spender_destruct so they run with a valid sp instance.src/c-api/test/chain/output_point.cpp (1)
247-275:⚠️ Potential issue | 🟡 MinorRefresh safe constructor/setter null precondition coverage.
The comment still says
kth_chain_output_point_construct_from_hash_indextakes the hash by value, but it now acceptskth_hash_t const*;NULLis no longer a compile error. Please add safe-variant death tests for constructor and setter null hashes in addition to the_unsafecases.Suggested coverage update
-// Safe `kth_chain_output_point_construct_from_hash_index` takes the hash -// by value: passing NULL is a compile error. The runtime precondition -// still applies on the `_unsafe` companion. +TEST_CASE("C-API OutputPoint - construct_from_hash_index null hash aborts", + "[C-API OutputPoint][precondition]") { + KTH_EXPECT_ABORT(kth_chain_output_point_construct_from_hash_index(NULL, 0)); +} + TEST_CASE("C-API OutputPoint - construct_from_hash_index_unsafe null hash aborts", "[C-API OutputPoint][precondition]") { KTH_EXPECT_ABORT(kth_chain_output_point_construct_from_hash_index_unsafe(NULL, 0)); } @@ +TEST_CASE("C-API OutputPoint - set_hash null aborts", + "[C-API OutputPoint][precondition]") { + kth_output_point_mut_t op = kth_chain_output_point_construct_default(); + KTH_EXPECT_ABORT(kth_chain_output_point_set_hash(op, NULL)); + kth_chain_output_point_destruct(op); +} + TEST_CASE("C-API OutputPoint - set_hash_unsafe null aborts", "[C-API OutputPoint][precondition]") { kth_output_point_mut_t op = kth_chain_output_point_construct_default();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/test/chain/output_point.cpp` around lines 247 - 275, The test coverage must be extended to include death tests for the safe variants that now accept a kth_hash_t const* pointer: add KTH_EXPECT_ABORT calls for kth_chain_output_point_construct_from_hash_index(NULL, 0) and for kth_chain_output_point_set_hash(op, NULL) alongside the existing _unsafe tests; locate the construction test using kth_chain_output_point_construct_from_hash_index/_unsafe and the setter tests using kth_chain_output_point_set_hash_unsafe to mirror their structure and ensure you construct a default kth_output_point_mut_t (via kth_chain_output_point_construct_default()) and destruct it after the test where needed.src/c-api/test/chain/get_headers.cpp (1)
250-258:⚠️ Potential issue | 🟡 MinorCover the safe null
stop_hashpreconditions too.The new safe APIs accept
kth_hash_t const*, sokth_chain_get_headers_construct(starts, NULL)andkth_chain_get_headers_set_stop_hash(gh, NULL)now compile and should abort via precondition. Current tests only cover the_unsafenull stop paths.Also applies to: 318-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/test/chain/get_headers.cpp` around lines 250 - 258, Add tests that assert the safe APIs abort on a null stop_hash: call kth_chain_get_headers_construct(starts, NULL) (using make_hash_list_of_two() to produce 'starts' and kth_hash_list_mut_t) and assert abort via KTH_EXPECT_ABORT, and likewise create a valid kth_chain_get_headers_t (kth_chain_get_headers_construct or construct_unsafe), then call kth_chain_get_headers_set_stop_hash(gh, NULL) and assert abort with KTH_EXPECT_ABORT; mirror the existing unsafe test patterns but target the safe functions kth_chain_get_headers_construct and kth_chain_get_headers_set_stop_hash so the precondition for NULL stop_hash is covered.src/c-api/test/chain/header.cpp (1)
266-301:⚠️ Potential issue | 🟡 MinorUpdate safe hash precondition docs/tests.
This comment still describes the safe constructor as by-value, but the provided header now declares
kth_hash_t const*. Please add safe null death tests for constructor previous-hash/merkle and safe hash setters, not just the_unsafevariants.Suggested coverage outline
-// The safe `kth_chain_header_construct` takes `kth_hash_t` by value, so -// passing NULL is a compile error rather than a runtime abort. The -// corresponding precondition still exists on the `_unsafe` companion, -// where the C type system can no longer enforce the buffer length. +TEST_CASE("C-API Header - construct null previous_block_hash aborts", + "[C-API Header][precondition]") { + KTH_EXPECT_ABORT( + kth_chain_header_construct(kVersion, NULL, &kMerkle, + kTimestamp, kBits, kNonce)); +} + +TEST_CASE("C-API Header - construct null merkle aborts", + "[C-API Header][precondition]") { + KTH_EXPECT_ABORT( + kth_chain_header_construct(kVersion, &kPrevHash, NULL, + kTimestamp, kBits, kNonce)); +} + TEST_CASE("C-API Header - construct_unsafe null previous_block_hash aborts", "[C-API Header][precondition]") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/test/chain/header.cpp` around lines 266 - 301, The tests only cover null-pointer preconditions for the _unsafe constructors/setters but the safe APIs now take kth_hash_t const* (not by-value), so add matching death tests that assert aborts when passing NULL to kth_chain_header_construct (safe), kth_chain_header_set_previous_block_hash (safe), and kth_chain_header_set_merkle (safe), similar to the existing KTH_EXPECT_ABORT cases for the _unsafe variants; also update the test comments above those blocks to mention the safe APIs accept pointers and require non-NULL, and ensure you still call kth_chain_header_destruct(header) after each test that constructs a header and that kth_chain_header_to_data already has a null out_size test—add a test to KTH_EXPECT_ABORT(kth_chain_header_to_data(header, 1, NULL)) if missing for safe API coverage.src/c-api/test/chain/token_data.cpp (1)
376-379:⚠️ Potential issue | 🟡 MinorAdd safe null-ID precondition tests.
This only checks
kth_chain_token_make_fungible_unsafe(NULL, ...); the newly pointer-based safe APIs also need coverage formake_fungible(NULL, ...),make_non_fungible(NULL, ...),make_both(NULL, ...), andkth_chain_token_data_set_id(td, NULL).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/test/chain/token_data.cpp` around lines 376 - 379, Add unit tests in the same test suite to cover null-ID preconditions for the pointer-based safe APIs: assert that kth_chain_token_make_fungible(NULL, ...), kth_chain_token_make_non_fungible(NULL, ...), kth_chain_token_make_both(NULL, ...), and kth_chain_token_data_set_id(td, NULL) abort or fail the same way as the existing kth_chain_token_make_fungible_unsafe(NULL, ... ) test; create TEST_CASE entries mirroring the existing pattern in token_data.cpp and use the same expectation macro (KTH_EXPECT_ABORT or the appropriate failure macro) for each function to ensure null id handling is tested.src/c-api/test/chain/script.cpp (1)
404-410:⚠️ Potential issue | 🟡 MinorUpdate null-precondition tests for safe pointer inputs.
The safe pattern factory no longer takes the short hash by value, and
check_signaturenow acceptskth_longhash_t const*;NULLcompiles for both. Please replace the stale comment and add safe null death tests so the new preconditions are covered.Also applies to: 434-445
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/test/chain/script.cpp` around lines 404 - 410, Replace the stale comment about the safe factory taking the short hash by value and add null-precondition death tests for the safe pointer-based APIs: update the comment near the existing test to reflect that kth_chain_script_to_pay_script_hash_pattern now accepts a pointer and therefore you must add a KTH_EXPECT_ABORT invoking kth_chain_script_to_pay_script_hash_pattern(NULL) (and likewise add a KTH_EXPECT_ABORT for kth_chain_script_check_signature(NULL) since check_signature now accepts kth_longhash_t const*), keeping the existing unsafe tests (kth_chain_script_to_pay_script_hash_pattern_unsafe and any _unsafe check_signature) unchanged so both safe and unsafe null-precondition behaviors are covered.src/c-api/include/kth/capi/wallet/hd_public.h (1)
27-42:⚠️ Potential issue | 🟡 MinorUpdate the stale safe-variant wording.
Lines 27 and 38 now take
kth_hd_key_t const*, but the adjacent_unsafewarnings still describe the safe variant as passing a C struct “by value”.📝 Proposed wording update
- * `@warning` `public_key` MUST point to a buffer of at least 82 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + * `@warning` `public_key` MUST point to a buffer of at least 82 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can provide a `kth_hd_key_t` and pass its address.Apply the same wording adjustment to both
_unsafeconstructor warnings in this range.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/wallet/hd_public.h` around lines 27 - 42, Update the outdated warning text for the two `_unsafe` constructors so it no longer tells users to "Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value"; instead, clarify that the non-`_unsafe` safe variants accept `kth_hd_key_t` by value (not pointer) and avoid requiring the caller to supply an 82-byte buffer. Specifically edit the warning comments for kth_wallet_hd_public_construct_from_public_key_unsafe and the other `_unsafe` constructor in this block to state that the safe (non-`_unsafe`) overload takes a `kth_hd_key_t` value and that the `_unsafe` version expects a pointer to an 82-byte buffer, applying the same wording to both `_unsafe` constructor warnings.src/c-api/include/kth/capi/wallet/ec_public.h (1)
41-56:⚠️ Potential issue | 🟡 MinorUpdate the
_unsafeguidance after switching safe variants toconst*.These safe constructors no longer take point structs by value, so the warning text should tell callers to pass the address of the fixed-size struct.
📝 Proposed wording update
- * `@warning` `compressed_point` MUST point to a buffer of at least 33 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + * `@warning` `compressed_point` MUST point to a buffer of at least 33 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can provide a `kth_ec_compressed_t` and pass its address.- * `@warning` `uncompressed_point` MUST point to a buffer of at least 65 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + * `@warning` `uncompressed_point` MUST point to a buffer of at least 65 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can provide a `kth_ec_uncompressed_t` and pass its address.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/wallet/ec_public.h` around lines 41 - 56, Update the warning text for the `_unsafe` constructors to reflect that the safe variants now take pointer arguments: change the messages for kth_wallet_ec_public_construct_from_compressed_point_compress_unsafe and kth_wallet_ec_public_construct_from_uncompressed_point_compress_unsafe (and any corresponding safe-variant comments) to instruct callers to pass the address of the fixed-size struct (e.g., a pointer to a 33-byte buffer for compressed points and a pointer to a 65-byte buffer for uncompressed points) rather than implying passing by value.src/c-api/include/kth/capi/wallet/hd_private.h (1)
31-57:⚠️ Potential issue | 🟡 MinorAdjust the HD-key
_unsafewarnings to match the new safe signatures.The safe variants now take
kth_hd_key_t const*; the “pass a C struct by value” wording is stale across these three warnings.📝 Proposed wording update
- * `@warning` `private_key` MUST point to a buffer of at least 82 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + * `@warning` `private_key` MUST point to a buffer of at least 82 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can provide a `kth_hd_key_t` and pass its address.Apply the same replacement to all three
_unsafeconstructor warnings in this range.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/wallet/hd_private.h` around lines 31 - 57, Update the three `_unsafe` constructor doc warnings to reflect the new safe signatures: for kth_wallet_hd_private_construct_from_private_key_unsafe, kth_wallet_hd_private_construct_from_private_key_prefixes_unsafe, and the corresponding `_unsafe` with prefix, replace the stale "Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value." text with wording that matches the new safe-signature semantics, e.g. "Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by pointer (i.e. a kth_hd_key_t pointer)." Ensure all three `_unsafe` functions have the same updated warning and keep the buffer-size caution unchanged.src/c-api/include/kth/capi/chain/get_headers.h (1)
32-40:⚠️ Potential issue | 🟡 MinorRefresh the
_unsafewarning text for pointer-based safe variants.The safe variants now take
kth_hash_t const*, so the “pass a C struct by value” guidance is no longer accurate.📝 Proposed wording update
- * `@warning` `stop` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + * `@warning` `stop` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can provide a `kth_hash_t` and pass its address.-/** `@warning` `value` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. */ +/** `@warning` `value` MUST point to a buffer of at least 32 bytes. Passing a shorter buffer is undefined behavior. Prefer the safe variant (without the `_unsafe` suffix) when your language can provide a `kth_hash_t` and pass its address. */Also applies to: 90-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/chain/get_headers.h` around lines 32 - 40, The warning for kth_chain_get_headers_construct_unsafe is outdated because the safe variant now accepts a pointer (kth_hash_t const*); update the comment to state that `stop` MUST point to a buffer of at least 32 bytes and change the guidance to prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to a 32‑byte buffer (and use the `_unsafe` variant only when you cannot); apply the same wording change to the duplicate comment for the other occurrence (lines 90-94) so both kth_chain_get_headers_construct and kth_chain_get_headers_construct_unsafe comments are consistent.src/c-api/include/kth/capi/chain/header.h (1)
29-37:⚠️ Potential issue | 🟡 MinorUpdate
_unsafeguidance for the new pointer-based safe APIs.The
_unsafewarnings still tell consumers to prefer the safe variant when their language can pass a C struct “by value”, but these safe signatures now takekth_hash_t const*. That stale wording can mislead FFI consumers during migration.📝 Suggested wording update
- Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_hash_t`.Apply the same wording to the
previous_block_hash,merkle, and setter warnings in this header.Also applies to: 99-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/chain/header.h` around lines 29 - 37, Update the `_unsafe` warning text for kth_chain_header_construct_unsafe (and the similar setter functions) to reflect that the safe APIs now take pointers to kth_hash_t (kth_hash_t const* previous_block_hash / merkle) instead of C structs passed “by value”; change the guidance to instruct FFI consumers to prefer the safe variant when their language can pass/handle a kth_hash_t pointer safely (or otherwise manage the 32‑byte buffer), and apply the same revised wording to the previous_block_hash, merkle, and corresponding setter warnings elsewhere in the header.src/c-api/include/kth/capi/chain/script.h (1)
209-220:⚠️ Potential issue | 🟡 MinorUpdate
_unsafeguidance after switching safe variants toconst*.The warnings still say the safe API is preferred when a language can pass a C struct by value, but these declarations now take pointers such as
kth_longhash_t const*,kth_hash_t const*, andkth_shorthash_t const*.📝 Suggested wording update
- Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to the corresponding `kth_*_t` struct.Also applies to: 230-239, 253-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/chain/script.h` around lines 209 - 220, Update the warning text for all *_unsafe functions (e.g., kth_chain_script_check_signature_unsafe, kth_chain_script_create_endorsement_unsafe and other _unsafe declarations around the same area) to reflect that the "safe" variants now take const pointer arguments (for example kth_longhash_t const*, kth_hash_t const*, kth_shorthash_t const*), so the guidance should no longer say "when your language can pass a C struct by value"; instead instruct callers that the non-unsafe variant is preferred when their language can pass immutable/const pointers or safely handle the corresponding const* types, and adjust the sentence about required buffer sizes to remain accurate for the pointer-based API.src/c-api/include/kth/capi/chain/double_spend_proof_spender.h (1)
87-105:⚠️ Potential issue | 🟡 MinorRefresh
_unsafedocs to matchkth_hash_t const*safe setters.These warnings still describe the safe setters as suitable when a language can pass a C struct by value. The safe setters now require a pointer to
kth_hash_t, so the migration guidance should be updated.📝 Suggested wording update
- Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a C struct by value. + Prefer the safe variant (without the `_unsafe` suffix) when your language can pass a pointer to `kth_hash_t`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/chain/double_spend_proof_spender.h` around lines 87 - 105, The _unsafe setter warnings are outdated: they still advise preferring the safe variant when a language can pass a C struct by value, but the safe setters (kth_chain_double_spend_proof_spender_set_prev_outs_hash, kth_chain_double_spend_proof_spender_set_sequence_hash, kth_chain_double_spend_proof_spender_set_outputs_hash) now take kth_hash_t const*; update the comment blocks above the corresponding _unsafe functions (kth_chain_double_spend_proof_spender_set_prev_outs_hash_unsafe, kth_chain_double_spend_proof_spender_set_sequence_hash_unsafe, kth_chain_double_spend_proof_spender_set_outputs_hash_unsafe) to reflect that the safe variant expects a pointer to kth_hash_t (or a language binding that can provide/allocate a kth_hash_t), and remove the "pass by value" guidance so the migration guidance is accurate.
🧹 Nitpick comments (5)
src/c-api/include/kth/capi/wallet/wallet_data.h (1)
56-57: Missing Doxygen@paramcomment for the safe setter.The other setters in this header (
set_mnemonicsat line 50,set_xpubat line 54) documentvalueas/**@PARAMvalue Borrowed input. Copied by value into the resulting object; ownership ofvaluestays with the caller. */, and the_unsafevariant at line 59 carries its own warning. The new pointer-based safe variant has no doc comment, making the borrowed/non-null contract implicit.📝 Proposed doc comment
+/** `@param` value Borrowed input; must be non-null. Copied by value into the resulting object; ownership of `value` stays with the caller. */ KTH_EXPORT void kth_wallet_wallet_data_set_encrypted_seed(kth_wallet_data_mut_t self, kth_encrypted_seed_t const* value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/wallet/wallet_data.h` around lines 56 - 57, Add a Doxygen `@param` comment to the safe pointer-based setter kth_wallet_wallet_data_set_encrypted_seed matching the style used by set_mnemonics and set_xpub: document that the parameter value is a borrowed input, copied into the resulting object, and ownership remains with the caller (i.e. non-owning/borrowed contract); keep the unsafe variant's separate warning unchanged. This ensures the pointer/non-null and ownership contract is explicit for kth_wallet_wallet_data_set_encrypted_seed.src/c-api/include/kth/capi/wallet/payment_address.h (1)
24-26: Consider documenting the borrow/non-null contract on the new pointer parameters.The
_unsafeoverloads carry an@warningdescribing buffer-size requirements, and other pointer-taking constructors in this header (e.g.,construct_from_ec_private,construct_from_ec_public_version,construct_from_script_version) include an@param ... Borrowed input. Copied by value ...note. The three updated safe variants now takekth_payment_t const*/kth_shorthash_t const*/kth_hash_t const*but don't document that the pointer is borrowed and must be non-null (the implementation enforcesKTH_PRECONDITION(... != nullptr)). Adding a short@paramline would keep the doc style consistent across the header and make the non-null requirement explicit for C / FFI consumers.Also applies to: 50-52, 61-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/wallet/payment_address.h` around lines 24 - 26, Add a short `@param` doc line to each of the new safe constructors that take pointer inputs (the functions taking `kth_payment_t const*`, `kth_shorthash_t const*`, and `kth_hash_t const*`) stating that the pointer is a borrowed input and must be non-NULL (e.g. "Borrowed input. Caller retains ownership; must be non-NULL."). This mirrors the existing `_unsafe` and other constructors' style and matches the implementation `KTH_PRECONDITION(... != nullptr)`, making the non-null/borrow contract explicit for C/FFI callers.src/c-api/test/wallet/hd_private.cpp (1)
96-97: LGTM — pointer-based call-site change is consistent with the PR-wide refactor. Note: sincekth_hd_key_tholds sensitive material, consider a follow-up addingexplicit_bzero(&hd_key, sizeof(hd_key))after use in tests that mirror consumer usage patterns — this is exactly the caller-side scrubbing the PR motivation cites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/test/wallet/hd_private.cpp` around lines 96 - 97, Tests call kth_wallet_hd_private_to_hd_key to produce a kth_hd_key_t and then pass it to kth_wallet_hd_private_construct_from_private_key, but the sensitive kth_hd_key_t is not scrubbed; after reconstructing (i.e., after the call to kth_wallet_hd_private_construct_from_private_key), explicitly wipe the stack copy by calling explicit_bzero(&hd_key, sizeof(hd_key)) so the test mirrors consumer-side scrubbing of private material and avoids leaving secrets in memory.src/c-api/test/chain/get_blocks.cpp (1)
73-252: LGTM — all safe-variant call sites (kth_chain_get_blocks_construct,kth_chain_get_blocks_set_stop_hash) consistently updated to&kStopHash, while*_unsafevariants correctly retainkStopHash.hash. The precondition test at line 252 still exercises the NULL-start-hashes path with a valid stop pointer, as intended.Minor gap (optional): there is no precondition test asserting that
kth_chain_get_blocks_construct(starts, NULL)andkth_chain_get_blocks_set_stop_hash(gb, NULL)abort. Since this PR introduces those null-checks on the safe variants, a death test for each would lock in the new contract symmetrically with the existingconstruct_unsafenull-stop test on line 258.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/test/chain/get_blocks.cpp` around lines 73 - 252, Add death tests to assert the new safe-variant null-stop preconditions: add a test that calls kth_chain_get_blocks_construct(starts, NULL) and expects abort, and another that constructs a default kth_get_blocks_mut_t then calls kth_chain_get_blocks_set_stop_hash(gb, NULL) and expects abort; reference the existing precondition style (KTH_EXPECT_ABORT) used for kth_chain_get_blocks_construct(NULL, &kStopHash) and place them alongside the other precondition tests so the contract for kth_chain_get_blocks_construct and kth_chain_get_blocks_set_stop_hash is symmetrically enforced.src/c-api/include/kth/capi/chain/point.h (1)
27-36: Optional: document the null-pointer precondition on the safe variants.The
_unsafevariants carry an explicit@warningfor their buffer contract, but the safe variants (lines 29, 91 here, and the parallel signatures intransaction.h,get_blocks.h,output_point.h, etc.) gained an implicithash != nullptr/value != nullptrprecondition (seesrc/c-api/src/chain/point.cpplines 42, 124) that will abort. Consider a brief@paramnote like "hashBorrowed, must be non-null; points to akth_hash_towned by the caller." so that C consumers don't have to read the.cppto discover the abort contract. Same note applies PR-wide; feel free to address in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/include/kth/capi/chain/point.h` around lines 27 - 36, Add explicit non-null parameter documentation for the "safe" constructors to document the abort precondition: update the comment for kth_chain_point_construct (and analogous safe variants such as the value-taking signatures in transaction.h, get_blocks.h, output_point.h) to include a `@param` note like "hash Borrowed, must be non-null; points to a kth_hash_t owned by the caller." so C callers know the pointer must not be NULL (the _unsafe variants keep their existing buffer-size warning). Locate the safe function comments (e.g., kth_chain_point_construct) and add the single-line `@param` description for the pointer parameter consistently across the PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/c-api/include/kth/capi/chain/double_spend_proof_spender.h`:
- Around line 87-105: The _unsafe setter warnings are outdated: they still
advise preferring the safe variant when a language can pass a C struct by value,
but the safe setters (kth_chain_double_spend_proof_spender_set_prev_outs_hash,
kth_chain_double_spend_proof_spender_set_sequence_hash,
kth_chain_double_spend_proof_spender_set_outputs_hash) now take kth_hash_t
const*; update the comment blocks above the corresponding _unsafe functions
(kth_chain_double_spend_proof_spender_set_prev_outs_hash_unsafe,
kth_chain_double_spend_proof_spender_set_sequence_hash_unsafe,
kth_chain_double_spend_proof_spender_set_outputs_hash_unsafe) to reflect that
the safe variant expects a pointer to kth_hash_t (or a language binding that can
provide/allocate a kth_hash_t), and remove the "pass by value" guidance so the
migration guidance is accurate.
In `@src/c-api/include/kth/capi/chain/get_headers.h`:
- Around line 32-40: The warning for kth_chain_get_headers_construct_unsafe is
outdated because the safe variant now accepts a pointer (kth_hash_t const*);
update the comment to state that `stop` MUST point to a buffer of at least 32
bytes and change the guidance to prefer the safe variant (without the `_unsafe`
suffix) when your language can pass a pointer to a 32‑byte buffer (and use the
`_unsafe` variant only when you cannot); apply the same wording change to the
duplicate comment for the other occurrence (lines 90-94) so both
kth_chain_get_headers_construct and kth_chain_get_headers_construct_unsafe
comments are consistent.
In `@src/c-api/include/kth/capi/chain/header.h`:
- Around line 29-37: Update the `_unsafe` warning text for
kth_chain_header_construct_unsafe (and the similar setter functions) to reflect
that the safe APIs now take pointers to kth_hash_t (kth_hash_t const*
previous_block_hash / merkle) instead of C structs passed “by value”; change the
guidance to instruct FFI consumers to prefer the safe variant when their
language can pass/handle a kth_hash_t pointer safely (or otherwise manage the
32‑byte buffer), and apply the same revised wording to the previous_block_hash,
merkle, and corresponding setter warnings elsewhere in the header.
In `@src/c-api/include/kth/capi/chain/script.h`:
- Around line 209-220: Update the warning text for all *_unsafe functions (e.g.,
kth_chain_script_check_signature_unsafe,
kth_chain_script_create_endorsement_unsafe and other _unsafe declarations around
the same area) to reflect that the "safe" variants now take const pointer
arguments (for example kth_longhash_t const*, kth_hash_t const*, kth_shorthash_t
const*), so the guidance should no longer say "when your language can pass a C
struct by value"; instead instruct callers that the non-unsafe variant is
preferred when their language can pass immutable/const pointers or safely handle
the corresponding const* types, and adjust the sentence about required buffer
sizes to remain accurate for the pointer-based API.
In `@src/c-api/include/kth/capi/chain/stealth_compact.h`:
- Around line 46-64: The warning comments for the `_unsafe` setters
(kth_chain_stealth_compact_set_ephemeral_public_key_hash_unsafe,
kth_chain_stealth_compact_set_public_key_hash_unsafe,
kth_chain_stealth_compact_set_transaction_hash_unsafe) are outdated: they tell
callers to prefer the “safe variant when your language can pass a C struct by
value” but the safe variants now take typed pointers (kth_hash_t const*,
kth_shorthash_t const*). Update each warning to clearly state the safe
(non-_unsafe) variant expects a pointer to the corresponding typed buffer (e.g.
kth_hash_t const* or kth_shorthash_t const*) of the required length, and advise
FFI consumers to use those typed pointer APIs when their language can
pass/represent the fixed-size struct or typed buffer safely rather than raw
uint8_t buffers; keep the explicit byte-length requirement (32 or 20 bytes) for
the `_unsafe` raw-buffer overloads.
In `@src/c-api/include/kth/capi/chain/token_data.h`:
- Around line 30-59: Update the outdated warning text that tells callers to
prefer the safe variant "when your language can pass a C struct by value"—the
safe APIs now take a pointer to kth_hash_t, so change the warning for
kth_chain_token_make_fungible_unsafe, kth_chain_token_make_non_fungible_unsafe,
and kth_chain_token_make_both_unsafe (and the duplicate block around lines
117-121) to state that the safe variant accepts a kth_hash_t pointer (kth_hash_t
const* id) and remove the advice about passing a C struct by value; keep the
note that id must point to at least 32 bytes and that callers must release
non-NULL results with kth_chain_token_data_destruct.
In `@src/c-api/include/kth/capi/wallet/ec_private.h`:
- Around line 30-59: Update the documentation comments for the `_unsafe`
functions (kth_wallet_ec_private_construct_from_wif_compressed_version_unsafe,
kth_wallet_ec_private_construct_from_wif_uncompressed_version_unsafe,
kth_wallet_ec_private_construct_from_secret_version_compress_unsafe) to reflect
that the API now accepts pointers to const buffers/typed structs (e.g., `uint8_t
const*`), not by-value structs; change language that currently says "Prefer the
safe variant (without the `_unsafe` suffix) when your language can pass a C
struct by value" to instead state that callers must pass a pointer to a
buffer/typed struct of the required minimum size (38, 37, and 32 bytes
respectively) and that passing a shorter buffer is undefined behavior.
In `@src/c-api/include/kth/capi/wallet/ec_public.h`:
- Around line 41-56: Update the warning text for the `_unsafe` constructors to
reflect that the safe variants now take pointer arguments: change the messages
for kth_wallet_ec_public_construct_from_compressed_point_compress_unsafe and
kth_wallet_ec_public_construct_from_uncompressed_point_compress_unsafe (and any
corresponding safe-variant comments) to instruct callers to pass the address of
the fixed-size struct (e.g., a pointer to a 33-byte buffer for compressed points
and a pointer to a 65-byte buffer for uncompressed points) rather than implying
passing by value.
In `@src/c-api/include/kth/capi/wallet/hd_private.h`:
- Around line 31-57: Update the three `_unsafe` constructor doc warnings to
reflect the new safe signatures: for
kth_wallet_hd_private_construct_from_private_key_unsafe,
kth_wallet_hd_private_construct_from_private_key_prefixes_unsafe, and the
corresponding `_unsafe` with prefix, replace the stale "Prefer the safe variant
(without the `_unsafe` suffix) when your language can pass a C struct by value."
text with wording that matches the new safe-signature semantics, e.g. "Prefer
the safe variant (without the `_unsafe` suffix) when your language can pass a C
struct by pointer (i.e. a kth_hd_key_t pointer)." Ensure all three `_unsafe`
functions have the same updated warning and keep the buffer-size caution
unchanged.
In `@src/c-api/include/kth/capi/wallet/hd_public.h`:
- Around line 27-42: Update the outdated warning text for the two `_unsafe`
constructors so it no longer tells users to "Prefer the safe variant (without
the `_unsafe` suffix) when your language can pass a C struct by value"; instead,
clarify that the non-`_unsafe` safe variants accept `kth_hd_key_t` by value (not
pointer) and avoid requiring the caller to supply an 82-byte buffer.
Specifically edit the warning comments for
kth_wallet_hd_public_construct_from_public_key_unsafe and the other `_unsafe`
constructor in this block to state that the safe (non-`_unsafe`) overload takes
a `kth_hd_key_t` value and that the `_unsafe` version expects a pointer to an
82-byte buffer, applying the same wording to both `_unsafe` constructor
warnings.
In `@src/c-api/test/chain/double_spend_proof.cpp`:
- Around line 307-314: Add tests to cover the safe pointer-based hash setters
for null-pointer preconditions: call
kth_chain_double_spend_proof_spender_set_prev_outs_hash(sp, NULL),
kth_chain_double_spend_proof_spender_set_sequence_hash(sp, NULL), and
kth_chain_double_spend_proof_spender_set_outputs_hash(sp, NULL) (in addition to
the existing _unsafe test) and assert they abort/trigger the same death behavior
(use KTH_EXPECT_ABORT or equivalent). Locate these new checks near the existing
DspSpender precondition tests that call
kth_chain_double_spend_proof_spender_construct_from_data and
kth_chain_double_spend_proof_spender_destruct so they run with a valid sp
instance.
In `@src/c-api/test/chain/get_headers.cpp`:
- Around line 250-258: Add tests that assert the safe APIs abort on a null
stop_hash: call kth_chain_get_headers_construct(starts, NULL) (using
make_hash_list_of_two() to produce 'starts' and kth_hash_list_mut_t) and assert
abort via KTH_EXPECT_ABORT, and likewise create a valid kth_chain_get_headers_t
(kth_chain_get_headers_construct or construct_unsafe), then call
kth_chain_get_headers_set_stop_hash(gh, NULL) and assert abort with
KTH_EXPECT_ABORT; mirror the existing unsafe test patterns but target the safe
functions kth_chain_get_headers_construct and
kth_chain_get_headers_set_stop_hash so the precondition for NULL stop_hash is
covered.
In `@src/c-api/test/chain/header.cpp`:
- Around line 266-301: The tests only cover null-pointer preconditions for the
_unsafe constructors/setters but the safe APIs now take kth_hash_t const* (not
by-value), so add matching death tests that assert aborts when passing NULL to
kth_chain_header_construct (safe), kth_chain_header_set_previous_block_hash
(safe), and kth_chain_header_set_merkle (safe), similar to the existing
KTH_EXPECT_ABORT cases for the _unsafe variants; also update the test comments
above those blocks to mention the safe APIs accept pointers and require
non-NULL, and ensure you still call kth_chain_header_destruct(header) after each
test that constructs a header and that kth_chain_header_to_data already has a
null out_size test—add a test to
KTH_EXPECT_ABORT(kth_chain_header_to_data(header, 1, NULL)) if missing for safe
API coverage.
In `@src/c-api/test/chain/output_point.cpp`:
- Around line 247-275: The test coverage must be extended to include death tests
for the safe variants that now accept a kth_hash_t const* pointer: add
KTH_EXPECT_ABORT calls for
kth_chain_output_point_construct_from_hash_index(NULL, 0) and for
kth_chain_output_point_set_hash(op, NULL) alongside the existing _unsafe tests;
locate the construction test using
kth_chain_output_point_construct_from_hash_index/_unsafe and the setter tests
using kth_chain_output_point_set_hash_unsafe to mirror their structure and
ensure you construct a default kth_output_point_mut_t (via
kth_chain_output_point_construct_default()) and destruct it after the test where
needed.
In `@src/c-api/test/chain/point.cpp`:
- Around line 206-230: The tests and comments assume the safe APIs take hashes
by value, but kth_chain_point_construct and kth_chain_point_set_hash now accept
kth_hash_t const* and should abort on NULL at runtime; add death tests mirroring
the existing unsafe checks: add TEST_CASEs that call
kth_chain_point_construct(NULL, 0) and kth_chain_point_set_hash(point, NULL)
wrapped with KTH_EXPECT_ABORT, create/destroy a default point as needed (use
kth_chain_point_construct_default() and kth_chain_point_destruct()), and update
the nearby comments to remove the "by value" claim so the tests and docs reflect
the new pointer precondition.
In `@src/c-api/test/chain/script.cpp`:
- Around line 404-410: Replace the stale comment about the safe factory taking
the short hash by value and add null-precondition death tests for the safe
pointer-based APIs: update the comment near the existing test to reflect that
kth_chain_script_to_pay_script_hash_pattern now accepts a pointer and therefore
you must add a KTH_EXPECT_ABORT invoking
kth_chain_script_to_pay_script_hash_pattern(NULL) (and likewise add a
KTH_EXPECT_ABORT for kth_chain_script_check_signature(NULL) since
check_signature now accepts kth_longhash_t const*), keeping the existing unsafe
tests (kth_chain_script_to_pay_script_hash_pattern_unsafe and any _unsafe
check_signature) unchanged so both safe and unsafe null-precondition behaviors
are covered.
In `@src/c-api/test/chain/token_data.cpp`:
- Around line 376-379: Add unit tests in the same test suite to cover null-ID
preconditions for the pointer-based safe APIs: assert that
kth_chain_token_make_fungible(NULL, ...),
kth_chain_token_make_non_fungible(NULL, ...), kth_chain_token_make_both(NULL,
...), and kth_chain_token_data_set_id(td, NULL) abort or fail the same way as
the existing kth_chain_token_make_fungible_unsafe(NULL, ... ) test; create
TEST_CASE entries mirroring the existing pattern in token_data.cpp and use the
same expectation macro (KTH_EXPECT_ABORT or the appropriate failure macro) for
each function to ensure null id handling is tested.
---
Nitpick comments:
In `@src/c-api/include/kth/capi/chain/point.h`:
- Around line 27-36: Add explicit non-null parameter documentation for the
"safe" constructors to document the abort precondition: update the comment for
kth_chain_point_construct (and analogous safe variants such as the value-taking
signatures in transaction.h, get_blocks.h, output_point.h) to include a `@param`
note like "hash Borrowed, must be non-null; points to a kth_hash_t owned by the
caller." so C callers know the pointer must not be NULL (the _unsafe variants
keep their existing buffer-size warning). Locate the safe function comments
(e.g., kth_chain_point_construct) and add the single-line `@param` description for
the pointer parameter consistently across the PR.
In `@src/c-api/include/kth/capi/wallet/payment_address.h`:
- Around line 24-26: Add a short `@param` doc line to each of the new safe
constructors that take pointer inputs (the functions taking `kth_payment_t
const*`, `kth_shorthash_t const*`, and `kth_hash_t const*`) stating that the
pointer is a borrowed input and must be non-NULL (e.g. "Borrowed input. Caller
retains ownership; must be non-NULL."). This mirrors the existing `_unsafe` and
other constructors' style and matches the implementation `KTH_PRECONDITION(...
!= nullptr)`, making the non-null/borrow contract explicit for C/FFI callers.
In `@src/c-api/include/kth/capi/wallet/wallet_data.h`:
- Around line 56-57: Add a Doxygen `@param` comment to the safe pointer-based
setter kth_wallet_wallet_data_set_encrypted_seed matching the style used by
set_mnemonics and set_xpub: document that the parameter value is a borrowed
input, copied into the resulting object, and ownership remains with the caller
(i.e. non-owning/borrowed contract); keep the unsafe variant's separate warning
unchanged. This ensures the pointer/non-null and ownership contract is explicit
for kth_wallet_wallet_data_set_encrypted_seed.
In `@src/c-api/test/chain/get_blocks.cpp`:
- Around line 73-252: Add death tests to assert the new safe-variant null-stop
preconditions: add a test that calls kth_chain_get_blocks_construct(starts,
NULL) and expects abort, and another that constructs a default
kth_get_blocks_mut_t then calls kth_chain_get_blocks_set_stop_hash(gb, NULL) and
expects abort; reference the existing precondition style (KTH_EXPECT_ABORT) used
for kth_chain_get_blocks_construct(NULL, &kStopHash) and place them alongside
the other precondition tests so the contract for kth_chain_get_blocks_construct
and kth_chain_get_blocks_set_stop_hash is symmetrically enforced.
In `@src/c-api/test/wallet/hd_private.cpp`:
- Around line 96-97: Tests call kth_wallet_hd_private_to_hd_key to produce a
kth_hd_key_t and then pass it to
kth_wallet_hd_private_construct_from_private_key, but the sensitive kth_hd_key_t
is not scrubbed; after reconstructing (i.e., after the call to
kth_wallet_hd_private_construct_from_private_key), explicitly wipe the stack
copy by calling explicit_bzero(&hd_key, sizeof(hd_key)) so the test mirrors
consumer-side scrubbing of private material and avoids leaving secrets in
memory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4cb7e17f-da73-460f-a799-763c9f72e1b8
📒 Files selected for processing (50)
src/c-api/include/kth/capi/chain/double_spend_proof_spender.hsrc/c-api/include/kth/capi/chain/get_blocks.hsrc/c-api/include/kth/capi/chain/get_headers.hsrc/c-api/include/kth/capi/chain/header.hsrc/c-api/include/kth/capi/chain/output_point.hsrc/c-api/include/kth/capi/chain/point.hsrc/c-api/include/kth/capi/chain/script.hsrc/c-api/include/kth/capi/chain/stealth_compact.hsrc/c-api/include/kth/capi/chain/token_data.hsrc/c-api/include/kth/capi/chain/transaction.hsrc/c-api/include/kth/capi/wallet/ec_private.hsrc/c-api/include/kth/capi/wallet/ec_public.hsrc/c-api/include/kth/capi/wallet/hd_private.hsrc/c-api/include/kth/capi/wallet/hd_public.hsrc/c-api/include/kth/capi/wallet/payment_address.hsrc/c-api/include/kth/capi/wallet/wallet_data.hsrc/c-api/src/chain/double_spend_proof_spender.cppsrc/c-api/src/chain/get_blocks.cppsrc/c-api/src/chain/get_headers.cppsrc/c-api/src/chain/header.cppsrc/c-api/src/chain/output_point.cppsrc/c-api/src/chain/point.cppsrc/c-api/src/chain/script.cppsrc/c-api/src/chain/stealth_compact.cppsrc/c-api/src/chain/token_data.cppsrc/c-api/src/chain/transaction.cppsrc/c-api/src/wallet/ec_private.cppsrc/c-api/src/wallet/ec_public.cppsrc/c-api/src/wallet/hd_private.cppsrc/c-api/src/wallet/hd_public.cppsrc/c-api/src/wallet/payment_address.cppsrc/c-api/src/wallet/wallet_data.cppsrc/c-api/test/chain/compact_block.cppsrc/c-api/test/chain/double_spend_proof.cppsrc/c-api/test/chain/get_blocks.cppsrc/c-api/test/chain/get_headers.cppsrc/c-api/test/chain/header.cppsrc/c-api/test/chain/input.cppsrc/c-api/test/chain/input_list.cppsrc/c-api/test/chain/merkle_block.cppsrc/c-api/test/chain/output_point.cppsrc/c-api/test/chain/output_point_list.cppsrc/c-api/test/chain/point.cppsrc/c-api/test/chain/point_list.cppsrc/c-api/test/chain/script.cppsrc/c-api/test/chain/token_data.cppsrc/c-api/test/chain/transaction.cppsrc/c-api/test/chain/utxo.cppsrc/c-api/test/wallet/hd_private.cppsrc/c-api/test/wallet/hd_public.cpp
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #285 +/- ##
==========================================
- Coverage 48.96% 0.25% -48.71%
==========================================
Files 325 324 -1
Lines 16207 16160 -47
Branches 5911 5883 -28
==========================================
- Hits 7936 42 -7894
- Misses 5970 16093 +10123
+ Partials 2301 25 -2276 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fpelliccioni
force-pushed
the
feature/sensitive-params-const-ptr
branch
from
April 21, 2026 16:29
79abb7a to
2e5d12f
Compare
2 tasks
fpelliccioni
force-pushed
the
feature/sensitive-params-const-ptr
branch
2 times, most recently
from
April 21, 2026 17:14
92ae004 to
9c7980e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/c-api/src/chain/script.cpp (1)
370-385:⚠️ Potential issue | 🟠 MajorAdd
kth_core_secure_zero()calls to wipesecret_cppbefore returning.The function materializes a sensitive 32-byte private key hash on the stack at line 377 but never erases it. Both the error and success return paths leave it exposed. Use
kth_core_secure_zero(&secret_cpp, sizeof(secret_cpp));immediately before each return statement:if ( ! result) { kth_core_secure_zero(&secret_cpp, sizeof(secret_cpp)); return kth::to_c_err(result.error()); } *out = kth::create_c_array(*result, *out_size); kth_core_secure_zero(&secret_cpp, sizeof(secret_cpp)); return kth_ec_success;Apply the same fix to
kth_chain_script_create_endorsement_unsafe()at lines 387–405.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/src/chain/script.cpp` around lines 370 - 385, The function kth_chain_script_create_endorsement materializes a sensitive secret_cpp on the stack and does not wipe it on either the error or success return paths; before every return from kth_chain_script_create_endorsement call kth_core_secure_zero(&secret_cpp, sizeof(secret_cpp)) to securely erase the secret, i.e. insert the secure-zero call immediately before the early return when result is false and immediately before the final return after creating *out; apply the identical change to kth_chain_script_create_endorsement_unsafe so secret_cpp is always wiped on both error and success exits (reference symbols: secret_cpp, result, kth_core_secure_zero, kth_chain_script_create_endorsement, kth_chain_script_create_endorsement_unsafe).
🧹 Nitpick comments (2)
src/c-api/src/secure_memory.cpp (2)
32-33: Fallback is correct; consider a compiler barrier for extra insurance.The
volatile unsigned char*loop is the standard portable scrub and is sufficient. For belt-and-suspenders against LTO/whole-program inliners that may reason across thevolatileboundary, you could add an asm memory clobber after the loop on GCC/Clang (asm volatile("" : : "r"(p) : "memory");). Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/src/secure_memory.cpp` around lines 32 - 33, Add a compiler barrier after the volatile byte-wise scrub loop to guard against LTO/whole-program optimizations: after the loop that writes through volatile unsigned char* vp (the loop using "while (n--) *vp++ = 0;"), insert an architecture-conditional compiler fence for GCC/Clang (use an asm volatile("" : : "r"(p) : "memory") style memory clobber) so the scrub cannot be reordered or elided; keep the existing volatile loop intact and only add the barrier, gated by appropriate compiler checks.
20-23:memset_sbranch is effectively dead in a C++ TU.
__STDC_LIB_EXT1__andmemset_sare C11 Annex K features not part of C++, and most C++ standard libraries (libstdc++, libc++, MSVC STL) do not expose them. Additionally,__STDC_WANT_LIB_EXT1__must be defined before<string.h>is included to enable Annex K support—it isn't defined before the include at line 8. This branch will never execute. Not a correctness issue—WindowsSecureZeroMemory,explicit_bzero(glibc ≥ 2.25, BSD, macOS), and the fallback volatile-pointer loop cover all real targets—but worth dropping the branch or adding a comment documenting that it's aspirational.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/c-api/src/secure_memory.cpp` around lines 20 - 23, The memset_s branch in secure_memory.cpp (the `#elif` using __STDC_LIB_EXT1__ / __STDC_WANT_LIB_EXT1__ and call to memset_s) is effectively dead in a C++ TU because Annex K isn’t part of C++ and __STDC_WANT_LIB_EXT1__ must be defined before <string.h>; either remove this branch entirely or replace it with a short explanatory comment stating it’s aspirational/kept for reference and that real targets use SecureZeroMemory, explicit_bzero, or the volatile-pointer fallback; update or delete the block that references memset_s so readers don’t expect it to ever be selected in C++ builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/c-api/include/kth/capi/chain/script.h`:
- Around line 208-210: Update the parameter documentation to stop implying
creation/copying of caller-owned buffers: for kth_chain_script_check_signature
(and the similar docs at lines 216-218 such as create_endorsement), replace
"Borrowed input; must be non-null. Copied into the resulting object; ownership
of `signature` stays with the caller." with wording that says the pointer is a
borrowed, non-null input that is read during the call and that ownership remains
with the caller; likewise change any wording for `secret`/endorsement to state
the secret is read during the call and not copied into or transferred to the
returned endorsement.
In `@src/c-api/src/secure_memory.cpp`:
- Around line 24-28: The preprocessor branch that calls explicit_bzero(p, n)
incorrectly includes macOS and lacks the proper header for BSD/Linux; change the
condition so macOS is removed from the explicit_bzero branch (letting it use the
C11 memset_s fallback), and ensure <strings.h> is included for platforms that
actually provide explicit_bzero; specifically, update the preprocessor check
around explicit_bzero to exclude (defined(__APPLE__) && defined(__MACH__)), and
add inclusion of strings.h when the explicit_bzero branch is selected so
explicit_bzero is properly declared.
---
Outside diff comments:
In `@src/c-api/src/chain/script.cpp`:
- Around line 370-385: The function kth_chain_script_create_endorsement
materializes a sensitive secret_cpp on the stack and does not wipe it on either
the error or success return paths; before every return from
kth_chain_script_create_endorsement call kth_core_secure_zero(&secret_cpp,
sizeof(secret_cpp)) to securely erase the secret, i.e. insert the secure-zero
call immediately before the early return when result is false and immediately
before the final return after creating *out; apply the identical change to
kth_chain_script_create_endorsement_unsafe so secret_cpp is always wiped on both
error and success exits (reference symbols: secret_cpp, result,
kth_core_secure_zero, kth_chain_script_create_endorsement,
kth_chain_script_create_endorsement_unsafe).
---
Nitpick comments:
In `@src/c-api/src/secure_memory.cpp`:
- Around line 32-33: Add a compiler barrier after the volatile byte-wise scrub
loop to guard against LTO/whole-program optimizations: after the loop that
writes through volatile unsigned char* vp (the loop using "while (n--) *vp++ =
0;"), insert an architecture-conditional compiler fence for GCC/Clang (use an
asm volatile("" : : "r"(p) : "memory") style memory clobber) so the scrub cannot
be reordered or elided; keep the existing volatile loop intact and only add the
barrier, gated by appropriate compiler checks.
- Around line 20-23: The memset_s branch in secure_memory.cpp (the `#elif` using
__STDC_LIB_EXT1__ / __STDC_WANT_LIB_EXT1__ and call to memset_s) is effectively
dead in a C++ TU because Annex K isn’t part of C++ and __STDC_WANT_LIB_EXT1__
must be defined before <string.h>; either remove this branch entirely or replace
it with a short explanatory comment stating it’s aspirational/kept for reference
and that real targets use SecureZeroMemory, explicit_bzero, or the
volatile-pointer fallback; update or delete the block that references memset_s
so readers don’t expect it to ever be selected in C++ builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aff62cc7-b14c-4625-bc6c-87d14a5dfc69
📒 Files selected for processing (55)
src/c-api/CMakeLists.txtsrc/c-api/include/kth/capi/capi.hsrc/c-api/include/kth/capi/chain/double_spend_proof_spender.hsrc/c-api/include/kth/capi/chain/get_blocks.hsrc/c-api/include/kth/capi/chain/get_headers.hsrc/c-api/include/kth/capi/chain/header.hsrc/c-api/include/kth/capi/chain/output_point.hsrc/c-api/include/kth/capi/chain/point.hsrc/c-api/include/kth/capi/chain/script.hsrc/c-api/include/kth/capi/chain/stealth_compact.hsrc/c-api/include/kth/capi/chain/token_data.hsrc/c-api/include/kth/capi/chain/transaction.hsrc/c-api/include/kth/capi/secure_memory.hsrc/c-api/include/kth/capi/wallet/ec_private.hsrc/c-api/include/kth/capi/wallet/ec_public.hsrc/c-api/include/kth/capi/wallet/hd_private.hsrc/c-api/include/kth/capi/wallet/hd_public.hsrc/c-api/include/kth/capi/wallet/payment_address.hsrc/c-api/include/kth/capi/wallet/wallet_data.hsrc/c-api/src/chain/double_spend_proof_spender.cppsrc/c-api/src/chain/get_blocks.cppsrc/c-api/src/chain/get_headers.cppsrc/c-api/src/chain/header.cppsrc/c-api/src/chain/output_point.cppsrc/c-api/src/chain/point.cppsrc/c-api/src/chain/script.cppsrc/c-api/src/chain/stealth_compact.cppsrc/c-api/src/chain/token_data.cppsrc/c-api/src/chain/transaction.cppsrc/c-api/src/secure_memory.cppsrc/c-api/src/wallet/ec_private.cppsrc/c-api/src/wallet/ec_public.cppsrc/c-api/src/wallet/hd_private.cppsrc/c-api/src/wallet/hd_public.cppsrc/c-api/src/wallet/payment_address.cppsrc/c-api/src/wallet/wallet_data.cppsrc/c-api/test/chain/compact_block.cppsrc/c-api/test/chain/double_spend_proof.cppsrc/c-api/test/chain/get_blocks.cppsrc/c-api/test/chain/get_headers.cppsrc/c-api/test/chain/header.cppsrc/c-api/test/chain/input.cppsrc/c-api/test/chain/input_list.cppsrc/c-api/test/chain/merkle_block.cppsrc/c-api/test/chain/output_point.cppsrc/c-api/test/chain/output_point_list.cppsrc/c-api/test/chain/point.cppsrc/c-api/test/chain/point_list.cppsrc/c-api/test/chain/script.cppsrc/c-api/test/chain/token_data.cppsrc/c-api/test/chain/transaction.cppsrc/c-api/test/chain/utxo.cppsrc/c-api/test/wallet/hd_private.cppsrc/c-api/test/wallet/hd_public.cppsrc/domain/include/kth/domain/version.hpp
✅ Files skipped from review due to trivial changes (6)
- src/c-api/include/kth/capi/capi.h
- src/domain/include/kth/domain/version.hpp
- src/c-api/test/chain/input_list.cpp
- src/c-api/test/chain/transaction.cpp
- src/c-api/test/wallet/hd_public.cpp
- src/c-api/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (32)
- src/c-api/test/chain/compact_block.cpp
- src/c-api/test/chain/output_point_list.cpp
- src/c-api/test/wallet/hd_private.cpp
- src/c-api/test/chain/utxo.cpp
- src/c-api/include/kth/capi/chain/transaction.h
- src/c-api/test/chain/point_list.cpp
- src/c-api/test/chain/script.cpp
- src/c-api/test/chain/get_blocks.cpp
- src/c-api/test/chain/get_headers.cpp
- src/c-api/src/chain/get_headers.cpp
- src/c-api/include/kth/capi/wallet/wallet_data.h
- src/c-api/include/kth/capi/chain/get_blocks.h
- src/c-api/src/chain/output_point.cpp
- src/c-api/include/kth/capi/chain/output_point.h
- src/c-api/src/wallet/ec_public.cpp
- src/c-api/include/kth/capi/wallet/ec_public.h
- src/c-api/test/chain/output_point.cpp
- src/c-api/src/wallet/wallet_data.cpp
- src/c-api/test/chain/token_data.cpp
- src/c-api/test/chain/header.cpp
- src/c-api/src/wallet/hd_private.cpp
- src/c-api/include/kth/capi/wallet/payment_address.h
- src/c-api/include/kth/capi/chain/double_spend_proof_spender.h
- src/c-api/include/kth/capi/chain/header.h
- src/c-api/include/kth/capi/chain/token_data.h
- src/c-api/include/kth/capi/chain/get_headers.h
- src/c-api/src/chain/stealth_compact.cpp
- src/c-api/src/wallet/ec_private.cpp
- src/c-api/src/wallet/payment_address.cpp
- src/c-api/src/chain/token_data.cpp
- src/c-api/include/kth/capi/chain/stealth_compact.h
- src/c-api/include/kth/capi/chain/point.h
fpelliccioni
force-pushed
the
feature/sensitive-params-const-ptr
branch
from
April 21, 2026 17:46
9c7980e to
0ef12d9
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0ef12d9. Configure here.
Two related C-API changes, bundled because high-level bindings need
both together:
1. Every C-API function with a fixed-size value_struct parameter now
takes that parameter as `kth_xxx_t const*` in the non-`_unsafe`
variant (hash, short_hash, payment, ec_compressed/uncompressed,
wif_compressed/uncompressed, hd_key, encrypted_seed, long_hash).
The `_unsafe` companions stay on `uint8_t const*`.
Motivation:
- Performance: every value_struct we expose is > 16 bytes, so the
x86_64 SysV ABI spills the whole blob onto the callee's stack
frame on by-value calls. `const*` keeps the bytes in the
caller's buffer and hands the callee a register-sized pointer.
- Security: for crypto material (secret / WIF / HD private key /
encrypted seed) the callee-stack copy was a second scrub target
a caller-side `kth_core_secure_zero` could not reach. `const*`
eliminates it.
ABI break for external consumers of the affected `construct_from_*`,
`set_*`, and `extract_*` entry points. Hand-written C-API tests
were swept to pass `&var` at every call site and to add matching
safe-variant death tests (`KTH_EXPECT_ABORT(fn(NULL))`) alongside
the existing `_unsafe` coverage.
2. New `kth_core_secure_zero(void*, kth_size_t)` in
`kth/capi/secure_memory.h`. Portable, non-optimizable zero wipe
routed per-platform (explicit_bzero on glibc/BSD/macOS,
SecureZeroMemory on Windows, volatile-pointer fallback
elsewhere). Every high-level binding — py-native today, cs-api
and wasm tomorrow — can share the same primitive for scrubbing
stack-local key material on the way out of sensitive wrappers,
instead of each binding duplicating the shim.
fpelliccioni
force-pushed
the
feature/sensitive-params-const-ptr
branch
from
April 21, 2026 18:07
0ef12d9 to
6be75ca
Compare
2 tasks
fpelliccioni
added a commit
that referenced
this pull request
Apr 22, 2026
`src/c-api/include/kth/capi/capi.h` is the public API entry point
that consumers are told to include ("API Users: Include only this
header"), but it was missing `debug_snapshot.h` and
`debug_snapshot_list.h` — headers that have been part of the C-API
since #275 but only reachable by directly including the
`kth/capi/vm/debug_snapshot*.h` paths.
This bit py-native's VM binding work (post-#285, post-#287): the
generator emits `#include <kth/capi.h>` in each wrapper, and the
resulting `kth_vm_debug_snapshot_*` calls failed to resolve until
the aggregator was fixed. The py-native generator side now also
includes the per-class header directly as a defense-in-depth, but
the aggregator should still be authoritative for external
consumers.
fpelliccioni
added a commit
that referenced
this pull request
Apr 22, 2026
`src/c-api/include/kth/capi/capi.h` is the public API entry point
that consumers are told to include ("API Users: Include only this
header"), but it was missing `debug_snapshot.h` and
`debug_snapshot_list.h` — headers that have been part of the C-API
since #275 but only reachable by directly including the
`kth/capi/vm/debug_snapshot*.h` paths.
This bit py-native's VM binding work (post-#285, post-#287): the
generator emits `#include <kth/capi.h>` in each wrapper, and the
resulting `kth_vm_debug_snapshot_*` calls failed to resolve until
the aggregator was fixed. The py-native generator side now also
includes the per-class header directly as a defense-in-depth, but
the aggregator should still be authoritative for external
consumers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
kth_xxx_t const*in the non-_unsafevariant (hash, short_hash, payment, ec_compressed/uncompressed, wif_compressed/uncompressed, hd_key, encrypted_seed, long_hash)._unsafecompanions (takinguint8_t const*) are unchanged — they were already pointer-based for FFI consumers that cannot pass C structs by value.construct_from_*,set_*, andextract_*entry points.Motivation
Performance. Every value_struct we expose is > 16 bytes. On x86_64 SysV the ABI spills the whole blob onto the callee's stack frame on by-value calls.
const*keeps the bytes in the caller's buffer and hands the callee a register-sized pointer.Security. For crypto material (secret, WIF, HD private key, encrypted seed) the callee-stack copy was a second scrub target that a caller-side
explicit_bzeroon its own local could not reach. Withconst*the C-API's stack frame only holds a pointer — scrubbing the caller's buffer is sufficient.What changed
Before:
After:
Caller side:
Scope
16 C-API headers, 16 C-API impls (regenerated), 16 hand-written test files (swept to pass
&var) — ~93 call-site edits total.Hand-written free functions in
capi/hash.h(kth_hash_equal,kth_hash_is_null,kth_hash_to_str) andcapi/hash_list.h(kth_core_hash_list_push_back,_nth) still takekth_hash_tby value — those live outside the generator and the same conversion can be folded in as a follow-up.Consumer impact
forward_args=[&name]) so the Python wrappers consume the new C-API signatures after a regen. py-native migration lands in a separate PR once this merges and kth 0.81.0 is released.&at call sites.Test plan
src/c-api/test/updated at every affected call site.Note
Medium Risk
Medium risk due to a broad ABI-breaking signature change across the C API (many call sites and downstream bindings must update), plus sensitive-memory scrubbing changes that touch wallet/crypto-related entry points.
Overview
This PR makes the C API’s safe entry points accept fixed-size
value_structinputs (hashes, keys, WIF/payment structs, etc.) askth_xxx_t const*instead of passing them by value, while keeping the existing_unsaferaw-buffer variants unchanged.It also adds a new cross-platform
kth_core_secure_zero()API plus an internal RAIIsecure_scrubguard, and uses it to scrub stack-local copies of sensitive material in wallet/script-related functions. Tests are updated to pass pointers and expanded with new death tests to enforce non-null preconditions for the newly-pointer-based parameters.Reviewed by Cursor Bugbot for commit 6be75ca. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit