refactor(chain): rework all chain class bindings - #5
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (45)
📝 WalkthroughWalkthroughReworks Python C-extension bindings to use PyCapsule opaque handles for chain primitives; expands and keyword-enables many chain_* APIs (headers/blocks/points/scripts/outputs/transactions and list types); adds per-type PyMethodDef registration in module init; regenerates type stubs; adds tests; adjusts setup sources; bumps Conan Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Python test / client
participant PyExt as kth_native Python binding
participant Wrapper as C++ wrapper (capsule layer)
participant Kth as underlying kth C API
Test->>PyExt: chain_header_construct(version, prev_hash, merkle, timestamp, bits, nonce)
PyExt->>Wrapper: parse kwargs, validate byte sizes
Wrapper->>Kth: kth_chain_header_construct(...)
Kth-->>Wrapper: header*
Wrapper-->>PyExt: PyCapsule(header*) ("kth.chain.header")
PyExt-->>Test: capsule
Test->>PyExt: chain_header_to_data(header_capsule, wire=True)
PyExt->>Wrapper: PyCapsule_GetPointer(header_capsule)
Wrapper->>Kth: kth_chain_header_to_data(header*)
Kth-->>Wrapper: data_ptr, size / NULL
Wrapper-->>PyExt: bytes (frees native array) or raise RuntimeError
PyExt-->>Test: bytes / exception
Test->>PyExt: chain_header_accept(header_capsule, state_capsule)
PyExt->>Wrapper: PyCapsule_GetPointer(header_capsule), PyCapsule_GetPointer(state_capsule)
Wrapper->>Kth: kth_chain_header_accept(header*, state*)
Kth-->>Wrapper: int result
Wrapper-->>PyExt: PyLong(result)
PyExt-->>Test: result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/chain/header.cpp (1)
67-70: Consider wrapping capsule extraction to reduce duplication.The file has 26 nearly identical
PyCapsule_GetPointercalls, each followed by a null check and return. Extracting this pattern into a helper function inutils.hwould improve maintainability and consistency across the codebase.PyCapsule_GetPointerraisesValueErrorfor both invalid capsules and name mismatches, so a wrapper need not change exception handling—just centralize the extraction logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/header.cpp` around lines 67 - 70, Multiple functions (e.g., kth_py_native_chain_header_copy) repeat the pattern of calling PyCapsule_GetPointer and checking for NULL; add a small helper in utils.h (and implement in utils.cpp) like a typed wrapper that takes a PyObject* and the expected capsule name and returns the cast pointer or NULL after handling the PyCapsule_GetPointer result; replace the repeated lines in kth_py_native_chain_header_copy and the other ~25 functions that use kth_header_const_t / kth_header_t to call the helper (e.g., utils::get_capsule_ptr<kth_header_const_t>(py_self, "kth.chain.header")) so the null-check and error behavior are centralized and duplicated code is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@conanfile.py`:
- Around line 42-43: The requirements() call currently pins an unreleased Conan
package version via self.requires("kth/0.80.0", ...), causing fresh conan
install to fail; change this to reference a published version or a version
range/fallback: update the self.requires invocation in requirements() to use a
known released package (e.g., the latest published kth version) or a compatible
version range (or make the version configurable via an environment
variable/option) so CI and local builds succeed without a patched environment.
In `@kth_native.pyi`:
- Around line 494-527: Remove the duplicated class Header declaration and
instead ensure the stub uses the existing Header alias (Header = object) by
deleting the auto-generated "class Header" block; additionally declare a minimal
ChainState type (e.g., ChainState = object or a Protocol) so the forward
reference in chain_header_accept(self: Header, state: "ChainState") resolves for
type checkers. Update references to Header and ChainState in the stub so there
are no duplicate definitions or unresolved forward references.
In `@setup.py`:
- Around line 187-202: The setup change comments out C++ units causing the
Python extension to lose bindings (chain_point_*, chain_script_*,
chain_output_point_*) while kth_native.pyi still declares them; restore
consistency by either re-enabling the source files in setup.py (e.g.,
'src/chain/point.cpp', 'src/chain/script.cpp', 'src/chain/output_point.cpp') so
the bindings remain available, or remove/deprecate the corresponding entries in
kth_native.pyi and update any exported Python API docs/tests to match the new
surface; ensure references to the specific symbols (chain_point_*,
chain_script_*, chain_output_point_*) and the kth_native.pyi declarations are
updated together to avoid runtime AttributeError.
In `@src/chain/header.cpp`:
- Around line 15-19: The constructor wrapper
kth_py_native_chain_header_construct_default currently maps a NULL C result to
Py_RETURN_NONE; instead detect when kth_chain_header_construct_default returns
NULL, call PyErr_SetString with an appropriate exception (e.g.,
PyExc_MemoryError or PyExc_RuntimeError) and return NULL so Python sees an
exception, not None; do the same fix for the other header constructor/copy
wrappers referenced (the functions that call kth_chain_header_construct_copy /
similar and create a PyCapsule via PyCapsule_New), and include a concise error
message mentioning the failing C function to aid debugging.
- Around line 76-82: In kth_py_native_chain_header_destruct, after calling
kth_chain_header_destruct(self_handle) invalidate the Python capsule by calling
PyCapsule_SetPointer(py_self, NULL) so the PyCapsule no longer exposes the freed
pointer (this prevents subsequent PyCapsule_GetPointer calls in getters like
version/equals/timestamp from returning a dangling pointer); ensure you still
return Py_None after the capsule is cleared in
kth_py_native_chain_header_destruct.
---
Nitpick comments:
In `@src/chain/header.cpp`:
- Around line 67-70: Multiple functions (e.g., kth_py_native_chain_header_copy)
repeat the pattern of calling PyCapsule_GetPointer and checking for NULL; add a
small helper in utils.h (and implement in utils.cpp) like a typed wrapper that
takes a PyObject* and the expected capsule name and returns the cast pointer or
NULL after handling the PyCapsule_GetPointer result; replace the repeated lines
in kth_py_native_chain_header_copy and the other ~25 functions that use
kth_header_const_t / kth_header_t to call the helper (e.g.,
utils::get_capsule_ptr<kth_header_const_t>(py_self, "kth.chain.header")) so the
null-check and error behavior are centralized and duplicated code is removed.
🪄 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: 478603fc-8cb2-4826-91a1-4e1f1dec5464
📒 Files selected for processing (7)
conanfile.pyinclude/kth/py-native/chain/header.hkth_native.pyisetup.pysrc/chain/header.cppsrc/module.ctests/test_chain_header.py
fpelliccioni
force-pushed
the
refactor/migrate-chain-header
branch
from
April 9, 2026 15:17
6e04512 to
f854b86
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
kth_native.pyi (1)
337-339: Minor clarification needed in comment.The comment mentions
chain_script,chain_output_point, andchain_pointstubs being "auto-generated below the AUTO-GENERATED block", but currently onlychain_headerstubs are present. This could be misleading until the other classes are migrated.Consider updating to reflect current state:
-# chain_header / chain_script / chain_output_point / chain_point stubs are -# auto-generated below the AUTO-GENERATED block once each class is migrated. +# chain_header stubs are auto-generated below. chain_script / chain_output_point / +# chain_point stubs will be added once each class is migrated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kth_native.pyi` around lines 337 - 339, Update the comment that currently reads "# chain_header / chain_script / chain_output_point / chain_point stubs are auto-generated below the AUTO-GENERATED block once each class is migrated." to accurately reflect the current state by referencing the AUTO-GENERATED block and stating that only chain_header stubs are present now and the other stubs (chain_script, chain_output_point, chain_point) will be added as those classes are migrated; keep references to the AUTO-GENERATED marker and the class names (chain_header, chain_script, chain_output_point, chain_point) so future migrations remain clear.src/chain/header.cpp (1)
263-277: Consider defining a macro for the chain_state capsule name.The
"kth.chain.chain_state"string on line 273 is hardcoded while other capsule names useKTH_PY_CAPSULE_NAME. For consistency and to prevent typos when chain_state bindings are added, consider defining a macro:// In a shared header or when chain_state bindings are added: `#define` KTH_PY_CAPSULE_NAME_CHAIN_STATE "kth.chain.chain_state"This is a minor consistency improvement and can be deferred to the chain_state migration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/header.cpp` around lines 263 - 277, Replace the hardcoded capsule name "kth.chain.chain_state" in kth_py_native_chain_header_accept with a symbolic macro for consistency: define KTH_PY_CAPSULE_NAME_CHAIN_STATE (e.g. `#define` KTH_PY_CAPSULE_NAME_CHAIN_STATE "kth.chain.chain_state") in the shared header where other capsule names (KTH_PY_CAPSULE_NAME) live, then use that macro when calling PyCapsule_GetPointer for py_state (kth_chain_state_const_t state_handle) so the capsule name is centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@kth_native.pyi`:
- Around line 337-339: Update the comment that currently reads "# chain_header /
chain_script / chain_output_point / chain_point stubs are auto-generated below
the AUTO-GENERATED block once each class is migrated." to accurately reflect the
current state by referencing the AUTO-GENERATED block and stating that only
chain_header stubs are present now and the other stubs (chain_script,
chain_output_point, chain_point) will be added as those classes are migrated;
keep references to the AUTO-GENERATED marker and the class names (chain_header,
chain_script, chain_output_point, chain_point) so future migrations remain
clear.
In `@src/chain/header.cpp`:
- Around line 263-277: Replace the hardcoded capsule name
"kth.chain.chain_state" in kth_py_native_chain_header_accept with a symbolic
macro for consistency: define KTH_PY_CAPSULE_NAME_CHAIN_STATE (e.g. `#define`
KTH_PY_CAPSULE_NAME_CHAIN_STATE "kth.chain.chain_state") in the shared header
where other capsule names (KTH_PY_CAPSULE_NAME) live, then use that macro when
calling PyCapsule_GetPointer for py_state (kth_chain_state_const_t state_handle)
so the capsule name is centralized and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d042951f-2038-45a2-9a19-bf85dfea634f
📒 Files selected for processing (7)
conanfile.pyinclude/kth/py-native/chain/header.hkth_native.pyisetup.pysrc/chain/header.cppsrc/module.ctests/test_chain_header.py
🚧 Files skipped from review as they are similar to previous changes (3)
- conanfile.py
- src/module.c
- tests/test_chain_header.py
fpelliccioni
force-pushed
the
refactor/migrate-chain-header
branch
2 times, most recently
from
April 10, 2026 22:51
7fd7428 to
b64b497
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/chain/chain.cpp (2)
151-160:⚠️ Potential issue | 🟠 MajorMissing GIL handling in callback.
Unlike
chain_fetch_block_handler(Line 22-36) andchain_fetch_last_height_handler(Line 212-224), this callback handler does not acquire the GIL before calling Python APIs. Since this is invoked from a C thread, Python calls without the GIL can cause crashes or data corruption.🐛 Proposed fix to add GIL handling
void chain_fetch_block_header_handler(kth_chain_t chain, void* ctx, kth_error_code_t error , kth_header_mut_t header, kth_size_t h) { + PyGILState_STATE gstate; + gstate = PyGILState_Ensure(); + PyObject* py_callback = (PyObject*)ctx; PyObject* py_header = to_py_obj(header); PyObject* arglist = Py_BuildValue("(iOK)", error, py_header, h); PyObject_CallObject(py_callback, arglist); Py_DECREF(arglist); Py_XDECREF(py_callback); // Dispose of the call + + PyGILState_Release(gstate); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/chain.cpp` around lines 151 - 160, chain_fetch_block_header_handler is calling Python C-API functions from a C thread without holding the GIL; modify it to acquire the GIL before any Python API use and release it afterwards: call PyGILState_Ensure() at the start (before to_py_obj, Py_BuildValue, PyObject_CallObject), perform the existing calls (to_py_obj(header), Py_BuildValue("(iOK)", ...), PyObject_CallObject, Py_DECREF/Py_XDECREF on arglist and py_callback), then call PyGILState_Release(gstate) before returning; keep the rest of the logic and reference symbols chain_fetch_block_header_handler, to_py_obj, Py_BuildValue, PyObject_CallObject, Py_DECREF, Py_XDECREF, PyGILState_Ensure, and PyGILState_Release.
90-99:⚠️ Potential issue | 🟠 MajorMissing GIL handling in callback.
Similar to
chain_fetch_block_header_handler, this handler also lacks GIL acquisition before calling Python APIs.🐛 Proposed fix to add GIL handling
void chain_fetch_merkle_block_handler(kth_chain_t chain, void* ctx, kth_error_code_t error, kth_merkleblock_t merkle, kth_size_t h) { + PyGILState_STATE gstate; + gstate = PyGILState_Ensure(); + PyObject* py_callback = (PyObject*)ctx; PyObject* py_merkle = to_py_obj(merkle); PyObject* arglist = Py_BuildValue("(iOK)", error, py_merkle, h); PyObject_CallObject(py_callback, arglist); Py_DECREF(arglist); Py_XDECREF(py_callback); // Dispose of the call + + PyGILState_Release(gstate); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/chain.cpp` around lines 90 - 99, chain_fetch_merkle_block_handler calls Python C-API functions without holding the GIL; wrap the Python interactions by acquiring the GIL with PyGILState_Ensure() at the start of the Python API section and release it with PyGILState_Release(gstate) after all Python calls and DECREFs complete. Specifically, around the calls that use to_py_obj(merkle), Py_BuildValue("(iOK)", ...), PyObject_CallObject(py_callback, arglist), Py_DECREF(arglist) and Py_XDECREF(py_callback) ensure the GIL is held; use PyGILState_Ensure()/PyGILState_Release to match the pattern used in chain_fetch_block_header_handler.src/chain/compact_block.cpp (1)
56-67:⚠️ Potential issue | 🟠 MajorFormat string mismatch for
py_n.The variable
py_nis declared asuint64_t(Line 58), but the format specifier"OI"usesIwhich corresponds tounsigned int(typically 32-bit). This can cause truncation on 64-bit values or undefined behavior due to argument size mismatch.🐛 Proposed fix for format string
PyObject* kth_py_native_chain_compact_block_transaction_nth(PyObject* self, PyObject* args){ PyObject* py_compact_block; uint64_t py_n; - if ( ! PyArg_ParseTuple(args, "OI", &py_compact_block, &py_n)) { + if ( ! PyArg_ParseTuple(args, "OK", &py_compact_block, &py_n)) { return NULL; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/compact_block.cpp` around lines 56 - 67, The format string for PyArg_ParseTuple in kth_py_native_chain_compact_block_transaction_nth is incorrect for the 64-bit py_n; change the parse to use the unsigned long long specifier by replacing "OI" with "OK" and make py_n an unsigned long long (or parse into unsigned long long and cast to uint64_t) so the call to kth_chain_compact_block_transaction_nth receives a correctly-sized 64-bit value.
🧹 Nitpick comments (2)
src/module.c (1)
20-35: Minor: Duplicate includes in AUTO-GENERATED block.The auto-generated block re-includes headers that are already present above:
block.hat line 19 and line 21point.hat line 16 and line 24While harmless due to include guards, removing the duplicates would clean up the file.
♻️ Suggested cleanup
`#include` <kth/py-native/chain/block.h> // ── AUTO-GENERATED INCLUDES START ───────────────────────────────────── -#include <kth/py-native/chain/block.h> `#include` <kth/py-native/chain/block_list.h> `#include` <kth/py-native/chain/header.h> -#include <kth/py-native/chain/point.h> `#include` <kth/py-native/chain/point_list.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module.c` around lines 20 - 35, The AUTO-GENERATED INCLUDES block contains duplicate header entries (e.g., kth/py-native/chain/block.h and kth/py-native/chain/point.h appear twice); remove the duplicate include lines so each unique header is included only once in the AUTO-GENERATED INCLUDES block (ensure the entries like "block.h" and "point.h" are present a single time), and if this file is produced by a generator, update the generator logic that emits the AUTO-GENERATED INCLUDES to de-duplicate headers before writing them.kth_native.pyi (1)
225-237: Type alias conflicts with auto-generated class definitions.The opaque handle aliases (e.g.,
Block = objectat line 228) conflict with the fully-typedclass Blockdefinition in the AUTO-GENERATED block (line 472). Type checkers may flag these as redefinitions. Consider removing or commenting out the aliases that are now superseded by proper class stubs.Affected aliases:
Block,Transaction,Input,Output,Script♻️ Proposed fix
# Type aliases for documentation only — they all resolve to `object` so # the type-checker doesn't reject passing them around. Node = object Chain = object P2P = object -Block = object # `Header` is defined as a real class in the AUTO-GENERATED STUBS block below. ChainState = object -Transaction = object -Input = object -Output = object -Script = object +# Block, Transaction, Input, Output, Script are defined as classes in the AUTO-GENERATED STUBS block below. PaymentAddress = object OutputPoint = object Hash = bytes # always 32 bytes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kth_native.pyi` around lines 225 - 237, Remove or comment out the opaque handle type aliases that conflict with the auto-generated class stubs: specifically remove/comment the lines defining Block = object, Transaction = object, Input = object, Output = object, and Script = object in kth_native.pyi so the later fully-typed class definitions (e.g., class Block) are not shadowed or redefined; ensure only the proper class stubs remain and keep Hash = bytes and other non-conflicting aliases intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@conanfile.py`:
- Line 43: The Conan requirement is pinned to kth/0.79.0 but the PR uses the new
header API (kth_header_const_t, kth_header_mut_t and
kth_chain_header_construct_from_data), so update the conan dependency pin in
conanfile.py from "kth/0.79.0" to "kth/0.80.0" to match the migration target;
alternatively, if you intend to keep 0.79.0, add a short comment in conanfile.py
explaining why the older pin is compatible with the new header types and
functions (mentioning kth_header_const_t, kth_header_mut_t,
kth_chain_header_construct_from_data) so reviewers understand the rationale.
In `@src/chain/input.cpp`:
- Around line 304-316: The function
kth_py_native_chain_input_extract_embedded_script wraps the returned
kth_script_mut_t in the wrong capsule name (uses KTH_PY_CAPSULE_NAME for
"kth.chain.input"); update the PyCapsule_New call to use the script capsule
identifier (replace KTH_PY_CAPSULE_NAME with the script capsule name, e.g.
KTH_PY_CAPSULE_SCRIPT_NAME or the literal "kth.chain.script" if no constant
exists) so the created capsule matches the kth_script_mut_t type expected by
script-handling functions.
In `@src/chain/output.cpp`:
- Around line 81-86: The current binding around kth_chain_output_to_data in
src/chain/output.cpp returns Python None when the native call returns NULL;
change it to raise a Python exception instead: call
kth_chain_output_to_data(self_handle, (kth_bool_t)wire, &out_size), and if
result == NULL set an appropriate Python error (e.g., PyErr_NoMemory() or
PyErr_SetString(PyExc_RuntimeError, "chain_output_to_data serialization
failed")) and return NULL; if result != NULL build the bytes object with
Py_BuildValue("y#", result, (Py_ssize_t)out_size) (which will produce b"" when
out_size==0), call kth_core_destruct_array(result) afterward, and do not
INCREF/return Py_None in the error path so callers consistently get an exception
on failure.
In `@src/chain/point.cpp`:
- Around line 139-144: The code currently treats a NULL result from
kth_chain_point_to_data as Py_None; instead, when
kth_chain_point_to_data(self_handle, (kth_bool_t)wire, &out_size) returns NULL
you must raise a Python exception (e.g., PyErr_SetString(PyExc_RuntimeError,
"chain_point serialization failed") or PyErr_NoMemory on allocation failure) and
return NULL so the Python caller sees the error; if the call succeeds but
out_size==0 return an empty bytes object (use the same Py_BuildValue("y#",
result, 0) path) and still call kth_core_destruct_array(result) after building
the Python bytes; update the branch handling result==NULL to set the error and
return NULL instead of returning Py_None and keep destruct logic tied to
non-NULL result.
---
Outside diff comments:
In `@src/chain/chain.cpp`:
- Around line 151-160: chain_fetch_block_header_handler is calling Python C-API
functions from a C thread without holding the GIL; modify it to acquire the GIL
before any Python API use and release it afterwards: call PyGILState_Ensure() at
the start (before to_py_obj, Py_BuildValue, PyObject_CallObject), perform the
existing calls (to_py_obj(header), Py_BuildValue("(iOK)", ...),
PyObject_CallObject, Py_DECREF/Py_XDECREF on arglist and py_callback), then call
PyGILState_Release(gstate) before returning; keep the rest of the logic and
reference symbols chain_fetch_block_header_handler, to_py_obj, Py_BuildValue,
PyObject_CallObject, Py_DECREF, Py_XDECREF, PyGILState_Ensure, and
PyGILState_Release.
- Around line 90-99: chain_fetch_merkle_block_handler calls Python C-API
functions without holding the GIL; wrap the Python interactions by acquiring the
GIL with PyGILState_Ensure() at the start of the Python API section and release
it with PyGILState_Release(gstate) after all Python calls and DECREFs complete.
Specifically, around the calls that use to_py_obj(merkle),
Py_BuildValue("(iOK)", ...), PyObject_CallObject(py_callback, arglist),
Py_DECREF(arglist) and Py_XDECREF(py_callback) ensure the GIL is held; use
PyGILState_Ensure()/PyGILState_Release to match the pattern used in
chain_fetch_block_header_handler.
In `@src/chain/compact_block.cpp`:
- Around line 56-67: The format string for PyArg_ParseTuple in
kth_py_native_chain_compact_block_transaction_nth is incorrect for the 64-bit
py_n; change the parse to use the unsigned long long specifier by replacing "OI"
with "OK" and make py_n an unsigned long long (or parse into unsigned long long
and cast to uint64_t) so the call to kth_chain_compact_block_transaction_nth
receives a correctly-sized 64-bit value.
---
Nitpick comments:
In `@kth_native.pyi`:
- Around line 225-237: Remove or comment out the opaque handle type aliases that
conflict with the auto-generated class stubs: specifically remove/comment the
lines defining Block = object, Transaction = object, Input = object, Output =
object, and Script = object in kth_native.pyi so the later fully-typed class
definitions (e.g., class Block) are not shadowed or redefined; ensure only the
proper class stubs remain and keep Hash = bytes and other non-conflicting
aliases intact.
In `@src/module.c`:
- Around line 20-35: The AUTO-GENERATED INCLUDES block contains duplicate header
entries (e.g., kth/py-native/chain/block.h and kth/py-native/chain/point.h
appear twice); remove the duplicate include lines so each unique header is
included only once in the AUTO-GENERATED INCLUDES block (ensure the entries like
"block.h" and "point.h" are present a single time), and if this file is produced
by a generator, update the generator logic that emits the AUTO-GENERATED
INCLUDES to de-duplicate headers before writing them.
🪄 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: 9d53cfaa-36c1-4d1c-a842-25853d501b5e
📒 Files selected for processing (37)
conanfile.pyinclude/kth/py-native/chain/block.hinclude/kth/py-native/chain/block_list.hinclude/kth/py-native/chain/header.hinclude/kth/py-native/chain/input.hinclude/kth/py-native/chain/input_list.hinclude/kth/py-native/chain/output.hinclude/kth/py-native/chain/output_list.hinclude/kth/py-native/chain/output_point.hinclude/kth/py-native/chain/output_point_list.hinclude/kth/py-native/chain/point.hinclude/kth/py-native/chain/point_list.hinclude/kth/py-native/chain/script.hinclude/kth/py-native/chain/transaction.hinclude/kth/py-native/chain/transaction_list.hkth_native.pyisetup.pysrc/chain/block.cppsrc/chain/block_list.cppsrc/chain/chain.cppsrc/chain/compact_block.cppsrc/chain/header.cppsrc/chain/history.cppsrc/chain/input.cppsrc/chain/input_list.cppsrc/chain/merkle_block.cppsrc/chain/output.cppsrc/chain/output_list.cppsrc/chain/output_point.cppsrc/chain/output_point_list.cppsrc/chain/point.cppsrc/chain/point_list.cppsrc/chain/script.cppsrc/chain/transaction.cppsrc/chain/transaction_list.cppsrc/module.ctests/test_chain_header.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_chain_header.py
🚧 Files skipped from review as they are similar to previous changes (3)
- setup.py
- include/kth/py-native/chain/header.h
- src/chain/header.cpp
fpelliccioni
force-pushed
the
refactor/migrate-chain-header
branch
from
April 11, 2026 07:10
b64b497 to
b4d92d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/chain/output.cpp (1)
67-84:⚠️ Potential issue | 🟠 MajorRaise on serialization failure instead of returning
None.
kth_chain_output_to_data()returningNULLis an error path, but this wrapper currently converts it intoNone. That hides the failure and breaks the bytes-only API contract; zero-length success should still come back asb"".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/output.cpp` around lines 67 - 84, The wrapper kth_py_native_chain_output_to_data currently maps a NULL result from kth_chain_output_to_data to Py_None; instead detect result == NULL as an error, call PyErr_SetString (e.g. PyExc_RuntimeError) or PyErr_Format with a clear message, and return NULL so Python raises; keep successful zero-length buffers as b"" by using Py_BuildValue("y#", result, out_size) when result is non-NULL; ensure kth_core_destruct_array(result) is only called when result != NULL and still call it before returning the bytes object.
🧹 Nitpick comments (4)
src/chain/chain.cpp (1)
151-160: Type change looks correct; consider GIL consistency as a follow-up.The signature change to
kth_header_mut_tis correct for the updated C-API.As an optional note for future improvement: this handler calls Python C-API functions (e.g.,
to_py_obj,Py_BuildValue,PyObject_CallObject) without acquiring the GIL, unlike similar handlers such aschain_fetch_block_handler(line 23) andchain_fetch_transaction_handler(line 372). If this callback is invoked from a background thread, the missing GIL could cause undefined behavior. The same pattern exists in other unchanged handlers (chain_fetch_merkle_block_handler,chain_fetch_spend_handler). This is pre-existing behavior, so it's fine to address separately.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/chain.cpp` around lines 151 - 160, chain_fetch_block_header_handler calls Python C-API functions (to_py_obj, Py_BuildValue, PyObject_CallObject, Py_DECREF, Py_XDECREF) without holding the Python GIL; update the function to acquire the GIL at its start (e.g., PyGILState_Ensure or PyEval_SaveThread counterpart), perform the to_py_obj, Py_BuildValue, PyObject_CallObject and DECREF operations while the GIL is held, then release the GIL before returning (e.g., PyGILState_Release), ensuring GIL consistency with the other handlers like chain_fetch_block_handler and chain_fetch_transaction_handler.src/chain/script.cpp (1)
199-223: Consider documenting behavior on empty scripts.The
frontandbackfunctions correctly check forNULLreturns and raiseRuntimeError. However, calling these on an empty script may yield undefined behavior in the underlying C API. Users should callchain_script_emptyfirst. This is more of an API documentation concern than a code issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/script.cpp` around lines 199 - 223, Document that kth_py_native_chain_script_front and kth_py_native_chain_script_back require a non-empty script: reference chain_script_empty and instruct callers to check it before invoking front/back; update the API docs (or add docstrings/comments near kth_py_native_chain_script_front and kth_py_native_chain_script_back) to explicitly state that calling front/back on an empty script is undefined by the underlying C API and that callers should call chain_script_empty(self) first, or handle the RuntimeError raised when a NULL handle is returned.src/chain/point_list.cpp (1)
54-65: Bounds checking innthis not required if the C API contract guarantees safe behavior for out-of-bounds access.While the function doesn't validate
index < countbefore calling the native function, this pattern is consistent across all list implementations in this PR. Importantly, if the C API returnsNULLfor out-of-bounds access,PyCapsule_Newwill immediately fail with aValueError—not defer the error to a subsequent call. This means the code relies on the C API's contract to handle out-of-bounds safely.Adding optional bounds checking at the Python level would fail fast and provide clearer error messages, but it is not required for correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/point_list.cpp` around lines 54 - 65, The function kth_py_native_chain_point_list_nth currently relies on the C API to handle out-of-bounds access; add an explicit bounds check before calling kth_chain_point_list_nth by querying the list size with kth_chain_point_list_count(list_handle) and comparing the provided index, and if index >= count set a Python IndexError (via PyErr_SetString(PyExc_IndexError, "...")) and return NULL; keep using the existing capsule handling (KTH_PY_CAPSULE_CHAIN_POINT_LIST and KTH_PY_CAPSULE_CHAIN_POINT) and only call kth_chain_point_list_nth when the index is valid.src/module.c (1)
19-48: Consider consolidating duplicate includes.The file now has duplicate includes due to the auto-generated block overlapping with pre-existing manual includes:
chain/block.hat lines 19 and 21chain/point.hat lines 16 and 24chain/transaction.hat lines 33 and 38chain/output.h,chain/output_list.hat lines 29-30 and 39-42chain/input.h,chain/input_list.hat lines 31-32 and 41-42chain/script.hat lines 28 and 43chain/output_point.hat lines 26-27 and 44chain/block_list.hat lines 22 and 47chain/transaction_list.hat lines 34 and 48While include guards prevent compilation issues, removing the duplicates would improve maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module.c` around lines 19 - 48, Remove duplicate `#include` lines so each header appears only once; keep either the auto-generated block or the manual includes but not both. Specifically deduplicate the listed headers (e.g. kth/py-native/chain/block.h, chain/block_list.h, chain/point.h, chain/transaction.h, chain/output.h, chain/output_list.h, chain/input.h, chain/input_list.h, chain/script.h, chain/output_point.h, chain/transaction_list.h) by removing the redundant occurrences outside the AUTO-GENERATED INCLUDES block (or remove the duplicate entries inside the auto-generated block if you prefer to preserve the manual list), ensuring only one `#include` exists per unique header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kth_native.pyi`:
- Around line 684-688: The stub references an undeclared forward type
"Operation" used by chain_script_front, chain_script_back, and
chain_script_first_operation; add a simple opaque alias declaration (e.g.,
Operation = object) alongside other opaque handles (the same area where
ChainState is declared) so the forward reference resolves for type checkers,
then update the file to include that alias.
In `@src/chain/header.cpp`:
- Around line 101-118: The wrapper kth_py_native_chain_header_to_data currently
maps a NULL return from kth_chain_header_to_data to Python None; instead detect
when result == NULL and raise a Python exception (e.g.,
PyErr_SetString(PyExc_RuntimeError, "...serialization failed for chain header")
or a more specific error) and return NULL so the Python caller sees the failure;
keep the existing behavior of calling kth_core_destruct_array(result) only when
result != NULL and return a bytes object (Py_BuildValue("y#", ...)) on success;
update kth_py_native_chain_header_to_data accordingly to set the error before
returning NULL.
In `@src/chain/transaction.cpp`:
- Around line 114-131: The current kth_py_native_chain_transaction_to_data
returns None when kth_chain_transaction_to_data returns NULL, hiding the native
failure; instead, detect result == NULL and raise a Python exception (e.g.,
PyErr_SetString or PyErr_Format with RuntimeError) with a descriptive message
before returning NULL. Update kth_py_native_chain_transaction_to_data to call
PyErr_SetString/PyErr_Format when result is NULL (include context such as
"kth_chain_transaction_to_data failed" and the wire parameter if useful), and
only build and return the bytes object when result != NULL, ensuring
kth_core_destruct_array is still called for non-NULL results. Ensure you still
validate the capsule via KTH_PY_CAPSULE_CHAIN_TRANSACTION and return NULL after
setting the Python error.
- Around line 307-319: The wrapper accepts signed Python integers (using "n")
and casts them to kth_size_t without validating they are non-negative, causing
negative values like max_block_size=-1 to wrap to huge unsigned numbers; update
kth_py_native_chain_transaction_check to check the parsed Py_ssize_t
max_block_size >= 0 before casting and raise a Python ValueError (via
PyErr_SetString and return NULL) if negative, then cast to (kth_size_t) safely;
apply the same non-negative validation pattern to other wrappers that parse
signed size-like args (e.g., the functions handling height, max_sigops,
input_index, block_height) so all such parameters are validated before casting
to kth_size_t.
- Around line 16-23: The PyCapsule_New calls (e.g., in
kth_py_native_chain_transaction_construct_default returning a capsule with
KTH_PY_CAPSULE_CHAIN_TRANSACTION) must register a destructor instead of passing
NULL to avoid leaks; add a capsule destructor function (e.g.,
transaction_capsule_destructor) that calls the native free function
(kth_chain_transaction_destruct or the appropriate kth_chain_*_destruct) and
pass that function as the fourth argument to PyCapsule_New in
kth_py_native_chain_transaction_construct_default and in the analogous
list/hash-list constructor functions in this file so native handles are freed
automatically on GC.
---
Duplicate comments:
In `@src/chain/output.cpp`:
- Around line 67-84: The wrapper kth_py_native_chain_output_to_data currently
maps a NULL result from kth_chain_output_to_data to Py_None; instead detect
result == NULL as an error, call PyErr_SetString (e.g. PyExc_RuntimeError) or
PyErr_Format with a clear message, and return NULL so Python raises; keep
successful zero-length buffers as b"" by using Py_BuildValue("y#", result,
out_size) when result is non-NULL; ensure kth_core_destruct_array(result) is
only called when result != NULL and still call it before returning the bytes
object.
---
Nitpick comments:
In `@src/chain/chain.cpp`:
- Around line 151-160: chain_fetch_block_header_handler calls Python C-API
functions (to_py_obj, Py_BuildValue, PyObject_CallObject, Py_DECREF, Py_XDECREF)
without holding the Python GIL; update the function to acquire the GIL at its
start (e.g., PyGILState_Ensure or PyEval_SaveThread counterpart), perform the
to_py_obj, Py_BuildValue, PyObject_CallObject and DECREF operations while the
GIL is held, then release the GIL before returning (e.g., PyGILState_Release),
ensuring GIL consistency with the other handlers like chain_fetch_block_handler
and chain_fetch_transaction_handler.
In `@src/chain/point_list.cpp`:
- Around line 54-65: The function kth_py_native_chain_point_list_nth currently
relies on the C API to handle out-of-bounds access; add an explicit bounds check
before calling kth_chain_point_list_nth by querying the list size with
kth_chain_point_list_count(list_handle) and comparing the provided index, and if
index >= count set a Python IndexError (via PyErr_SetString(PyExc_IndexError,
"...")) and return NULL; keep using the existing capsule handling
(KTH_PY_CAPSULE_CHAIN_POINT_LIST and KTH_PY_CAPSULE_CHAIN_POINT) and only call
kth_chain_point_list_nth when the index is valid.
In `@src/chain/script.cpp`:
- Around line 199-223: Document that kth_py_native_chain_script_front and
kth_py_native_chain_script_back require a non-empty script: reference
chain_script_empty and instruct callers to check it before invoking front/back;
update the API docs (or add docstrings/comments near
kth_py_native_chain_script_front and kth_py_native_chain_script_back) to
explicitly state that calling front/back on an empty script is undefined by the
underlying C API and that callers should call chain_script_empty(self) first, or
handle the RuntimeError raised when a NULL handle is returned.
In `@src/module.c`:
- Around line 19-48: Remove duplicate `#include` lines so each header appears only
once; keep either the auto-generated block or the manual includes but not both.
Specifically deduplicate the listed headers (e.g. kth/py-native/chain/block.h,
chain/block_list.h, chain/point.h, chain/transaction.h, chain/output.h,
chain/output_list.h, chain/input.h, chain/input_list.h, chain/script.h,
chain/output_point.h, chain/transaction_list.h) by removing the redundant
occurrences outside the AUTO-GENERATED INCLUDES block (or remove the duplicate
entries inside the auto-generated block if you prefer to preserve the manual
list), ensuring only one `#include` exists per unique header.
🪄 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: c6db7aad-9bda-44da-85ae-01febd58d667
📒 Files selected for processing (39)
conanfile.pyinclude/kth/py-native/capsule_names.hinclude/kth/py-native/chain/block.hinclude/kth/py-native/chain/block_list.hinclude/kth/py-native/chain/header.hinclude/kth/py-native/chain/input.hinclude/kth/py-native/chain/input_list.hinclude/kth/py-native/chain/output.hinclude/kth/py-native/chain/output_list.hinclude/kth/py-native/chain/output_point.hinclude/kth/py-native/chain/output_point_list.hinclude/kth/py-native/chain/point.hinclude/kth/py-native/chain/point_list.hinclude/kth/py-native/chain/script.hinclude/kth/py-native/chain/transaction.hinclude/kth/py-native/chain/transaction_list.hkth_native.pyisetup.pysrc/chain/block.cppsrc/chain/block_list.cppsrc/chain/chain.cppsrc/chain/compact_block.cppsrc/chain/header.cppsrc/chain/history.cppsrc/chain/input.cppsrc/chain/input_list.cppsrc/chain/merkle_block.cppsrc/chain/output.cppsrc/chain/output_list.cppsrc/chain/output_point.cppsrc/chain/output_point_list.cppsrc/chain/point.cppsrc/chain/point_list.cppsrc/chain/script.cppsrc/chain/transaction.cppsrc/chain/transaction_list.cppsrc/module.ctests/test_chain_header.pytests/test_chain_input.py
✅ Files skipped from review due to trivial changes (2)
- conanfile.py
- include/kth/py-native/capsule_names.h
🚧 Files skipped from review as they are similar to previous changes (12)
- src/chain/history.cpp
- src/chain/merkle_block.cpp
- src/chain/compact_block.cpp
- tests/test_chain_header.py
- include/kth/py-native/chain/transaction_list.h
- setup.py
- src/chain/output_point_list.cpp
- include/kth/py-native/chain/output_list.h
- src/chain/input.cpp
- src/chain/block.cpp
- include/kth/py-native/chain/script.h
- include/kth/py-native/chain/transaction.h
fpelliccioni
force-pushed
the
refactor/migrate-chain-header
branch
2 times, most recently
from
April 11, 2026 07:58
7664ef9 to
b452d9b
Compare
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 (1)
kth_native.pyi (1)
228-239:⚠️ Potential issue | 🟡 MinorPotential type alias/class conflict for
Block,Transaction,Input,Output,Script,OutputPoint.The opaque handles section (lines 225-238) declares these as
objectaliases:
Block = object(line 228)Transaction = object(line 231)Input = object(line 232)Output = object(line 233)Script = object(line 234)OutputPoint = object(line 237)However, the auto-generated block below (lines 473, 545, 582, etc.) defines these as actual
classdeclarations. This creates the same duplication issue that was previously flagged and fixed forHeader.Type checkers may resolve these inconsistently. Consider removing the
= objectaliases for types that are now defined as classes in the auto-generated block.Proposed fix
# Type aliases for documentation only — they all resolve to `object` so # the type-checker doesn't reject passing them around. Node = object Chain = object P2P = object -Block = object # `Header` is defined as a real class in the AUTO-GENERATED STUBS block below. ChainState = object -Transaction = object -Input = object -Output = object -Script = object Operation = object PaymentAddress = object -OutputPoint = object Hash = bytes # always 32 bytes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kth_native.pyi` around lines 228 - 239, Remove the opaque handle aliases that conflict with later autogenerated class declarations: delete the "Block = object", "Transaction = object", "Input = object", "Output = object", "Script = object", and "OutputPoint = object" alias lines in the opaque handles section so the later class definitions for Block, Transaction, Input, Output, Script, and OutputPoint in the AUTO-GENERATED STUBS are the single canonical definitions; ensure no other code relies on those aliases and leave other aliases (e.g. ChainState, PaymentAddress, Hash) intact if they do not have class counterparts.
🧹 Nitpick comments (5)
tests/test_chain_output.py (1)
37-43: Assert the actual script payload here.
chain_output_script(out)can return the wrong non-empty script and this test still passes. Compareviewwithsso the setter/getter roundtrip validates content, not just truthiness.♻️ Proposed change
def test_script_setter_roundtrip(): out = nat.chain_output_construct_default() s = nat.chain_script_construct_from_data(SCRIPT_BODY, True) nat.chain_output_set_script(out, s) - # The returned view must be a valid Script capsule. view = nat.chain_output_script(out) - assert nat.chain_script_empty(view) is False + assert nat.chain_script_equals(view, s) is True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_chain_output.py` around lines 37 - 43, The test currently only checks truthiness of the returned script view; change it to assert the actual payload equals the original script by comparing the view to s after the setter/getter roundtrip—use nat.chain_script_equals(view, s) if that helper exists, otherwise compare serialized/byte data via nat.chain_script_data(view) == nat.chain_script_data(s) (or nat.chain_script_serialize(view) == nat.chain_script_serialize(s)) so the test validates content, not just non-emptiness.tests/test_chain_script.py (1)
31-33: Strengthen the raw-constructor oracle.A decoder that returns the wrong non-empty script still passes here. Compare this result with the prefixed constructor result so the raw path is locked to the same script.
♻️ Proposed change
def test_from_data_raw_roundtrip(): s = nat.chain_script_construct_from_encoded_prefix(RAW_BODY, False) assert nat.chain_script_empty(s) is False + expected = nat.chain_script_construct_from_data(PREFIXED_BODY, True) + assert nat.chain_script_equals(s, expected) is True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_chain_script.py` around lines 31 - 33, The test currently only checks nat.chain_script_empty for the raw constructor which can miss incorrect non-empty values; instead construct the prefixed-version via nat.chain_script_construct_from_encoded_prefix(RAW_BODY, True) and assert the raw result equals the prefixed result (use nat.chain_script_equal(s, spref) if available, otherwise assert s == spref) so the raw path is locked to the same script as the prefixed constructor.tests/test_chain_output_point.py (1)
71-78: Add a negative capsule-type assertion to match the docstring.This only proves a valid
chain_pointworks.src/chain/output_point.cpp:77-88also has aPyCapsule_GetPointer(..., KTH_PY_CAPSULE_CHAIN_POINT)rejection path, so add a non-Point capsule case here and pin the exception it surfaces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_chain_output_point.py` around lines 71 - 78, Add a negative test case to test_construct_from_point_preserves_fields that constructs a non-Point PyCapsule (use an existing capsule-producing helper such as nat.chain_transaction_construct or another capsule factory) and pass it to nat.chain_output_point_construct_from_point, then assert that the call raises the exact exception type emitted by the C++ rejection path (the PyCapsule_GetPointer(..., KTH_PY_CAPSULE_CHAIN_POINT) branch) so the test pins that failure mode; keep the original positive assertions intact.include/kth/py-native/capsule_names.h (1)
28-65: Promote the destroyed-capsule tag into this header too.This file is the single source of truth for live capsule names, but the invalidation name still has to be repeated manually in each destructor. Adding a shared
KTH_PY_CAPSULE_DESTROYEDmacro here will keep every migrated wrapper on the same post-free sentinel and reduce typo-driven lifetime bugs.♻️ Proposed change
`#define` KTH_PY_CAPSULE_WALLET_EC_COMPRESSED_LIST "kth.wallet.ec_compressed_list" `#define` KTH_PY_CAPSULE_WALLET_PAYMENT_ADDRESS "kth.wallet.payment_address" `#define` KTH_PY_CAPSULE_WALLET_PAYMENT_ADDRESS_LIST "kth.wallet.payment_address_list" +#define KTH_PY_CAPSULE_DESTROYED "kth.destroyed"Based on learnings,
PyCapsule_SetPointer(capsule, NULL)is rejected by CPython, andPyCapsule_SetName(capsule, "kth.destroyed")is the supported invalidation path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/kth/py-native/capsule_names.h` around lines 28 - 65, Add a shared invalidation name macro KTH_PY_CAPSULE_DESTROYED, e.g. `#define` KTH_PY_CAPSULE_DESTROYED "kth.destroyed", to this header and replace any hardcoded destruction sentinel usage in the capsule destructors with that macro; update each destructor function referenced here (kth_py_native_chain_block_capsule_dtor, kth_py_native_chain_block_list_capsule_dtor, kth_py_native_chain_header_capsule_dtor, kth_py_native_chain_input_capsule_dtor, kth_py_native_chain_input_list_capsule_dtor, kth_py_native_chain_output_capsule_dtor, kth_py_native_chain_output_list_capsule_dtor, kth_py_native_chain_output_point_capsule_dtor, kth_py_native_chain_output_point_list_capsule_dtor, kth_py_native_chain_point_capsule_dtor, kth_py_native_chain_point_list_capsule_dtor, kth_py_native_chain_script_capsule_dtor, kth_py_native_chain_transaction_capsule_dtor, kth_py_native_chain_transaction_list_capsule_dtor) to call PyCapsule_SetName(capsule, KTH_PY_CAPSULE_DESTROYED) as the post-free sentinel (do not use PyCapsule_SetPointer(capsule, NULL) which CPython rejects).src/module.c (1)
20-35: Minor: Duplicate includes in the auto-generated block.The auto-generated includes block duplicates headers that are already included earlier:
block.his included at line 19 and again at line 21point.his included at line 16 and again at line 24This is harmless due to header guards, but creates unnecessary noise.
Proposed fix to remove duplicates
-// ── AUTO-GENERATED INCLUDES START ───────────────────────────────────── -#include <kth/py-native/chain/block.h> `#include` <kth/py-native/chain/block_list.h> -#include <kth/py-native/chain/header.h> -#include <kth/py-native/chain/point.h> `#include` <kth/py-native/chain/point_list.h> `#include` <kth/py-native/chain/output_point.h> `#include` <kth/py-native/chain/output_point_list.h> -#include <kth/py-native/chain/script.h> -#include <kth/py-native/chain/output.h> -#include <kth/py-native/chain/output_list.h> -#include <kth/py-native/chain/input.h> -#include <kth/py-native/chain/input_list.h> -#include <kth/py-native/chain/transaction.h> -#include <kth/py-native/chain/transaction_list.h> -// ── AUTO-GENERATED INCLUDES END ───────────────────────────────────────Or adjust the generator to avoid emitting includes that already exist in the hand-written section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module.c` around lines 20 - 35, The auto-generated include block in module.c contains duplicate headers (e.g., kth/py-native/chain/block.h and kth/py-native/chain/point.h) already present in the hand-written includes; remove the duplicate include lines from the AUTO-GENERATED INCLUDES section (or update the generator) so each header like block.h and point.h appears only once, ensuring the auto-generated block only emits headers not already included.
🤖 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 `@kth_native.pyi`:
- Around line 228-239: Remove the opaque handle aliases that conflict with later
autogenerated class declarations: delete the "Block = object", "Transaction =
object", "Input = object", "Output = object", "Script = object", and
"OutputPoint = object" alias lines in the opaque handles section so the later
class definitions for Block, Transaction, Input, Output, Script, and OutputPoint
in the AUTO-GENERATED STUBS are the single canonical definitions; ensure no
other code relies on those aliases and leave other aliases (e.g. ChainState,
PaymentAddress, Hash) intact if they do not have class counterparts.
---
Nitpick comments:
In `@include/kth/py-native/capsule_names.h`:
- Around line 28-65: Add a shared invalidation name macro
KTH_PY_CAPSULE_DESTROYED, e.g. `#define` KTH_PY_CAPSULE_DESTROYED "kth.destroyed",
to this header and replace any hardcoded destruction sentinel usage in the
capsule destructors with that macro; update each destructor function referenced
here (kth_py_native_chain_block_capsule_dtor,
kth_py_native_chain_block_list_capsule_dtor,
kth_py_native_chain_header_capsule_dtor, kth_py_native_chain_input_capsule_dtor,
kth_py_native_chain_input_list_capsule_dtor,
kth_py_native_chain_output_capsule_dtor,
kth_py_native_chain_output_list_capsule_dtor,
kth_py_native_chain_output_point_capsule_dtor,
kth_py_native_chain_output_point_list_capsule_dtor,
kth_py_native_chain_point_capsule_dtor,
kth_py_native_chain_point_list_capsule_dtor,
kth_py_native_chain_script_capsule_dtor,
kth_py_native_chain_transaction_capsule_dtor,
kth_py_native_chain_transaction_list_capsule_dtor) to call
PyCapsule_SetName(capsule, KTH_PY_CAPSULE_DESTROYED) as the post-free sentinel
(do not use PyCapsule_SetPointer(capsule, NULL) which CPython rejects).
In `@src/module.c`:
- Around line 20-35: The auto-generated include block in module.c contains
duplicate headers (e.g., kth/py-native/chain/block.h and
kth/py-native/chain/point.h) already present in the hand-written includes;
remove the duplicate include lines from the AUTO-GENERATED INCLUDES section (or
update the generator) so each header like block.h and point.h appears only once,
ensuring the auto-generated block only emits headers not already included.
In `@tests/test_chain_output_point.py`:
- Around line 71-78: Add a negative test case to
test_construct_from_point_preserves_fields that constructs a non-Point PyCapsule
(use an existing capsule-producing helper such as
nat.chain_transaction_construct or another capsule factory) and pass it to
nat.chain_output_point_construct_from_point, then assert that the call raises
the exact exception type emitted by the C++ rejection path (the
PyCapsule_GetPointer(..., KTH_PY_CAPSULE_CHAIN_POINT) branch) so the test pins
that failure mode; keep the original positive assertions intact.
In `@tests/test_chain_output.py`:
- Around line 37-43: The test currently only checks truthiness of the returned
script view; change it to assert the actual payload equals the original script
by comparing the view to s after the setter/getter roundtrip—use
nat.chain_script_equals(view, s) if that helper exists, otherwise compare
serialized/byte data via nat.chain_script_data(view) == nat.chain_script_data(s)
(or nat.chain_script_serialize(view) == nat.chain_script_serialize(s)) so the
test validates content, not just non-emptiness.
In `@tests/test_chain_script.py`:
- Around line 31-33: The test currently only checks nat.chain_script_empty for
the raw constructor which can miss incorrect non-empty values; instead construct
the prefixed-version via
nat.chain_script_construct_from_encoded_prefix(RAW_BODY, True) and assert the
raw result equals the prefixed result (use nat.chain_script_equal(s, spref) if
available, otherwise assert s == spref) so the raw path is locked to the same
script as the prefixed constructor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61733b45-c0da-4e6a-ab32-c6e5a5987184
📒 Files selected for processing (45)
conanfile.pyinclude/kth/py-native/capsule_names.hinclude/kth/py-native/chain/block.hinclude/kth/py-native/chain/block_list.hinclude/kth/py-native/chain/header.hinclude/kth/py-native/chain/input.hinclude/kth/py-native/chain/input_list.hinclude/kth/py-native/chain/output.hinclude/kth/py-native/chain/output_list.hinclude/kth/py-native/chain/output_point.hinclude/kth/py-native/chain/output_point_list.hinclude/kth/py-native/chain/point.hinclude/kth/py-native/chain/point_list.hinclude/kth/py-native/chain/script.hinclude/kth/py-native/chain/transaction.hinclude/kth/py-native/chain/transaction_list.hkth_native.pyisetup.pysrc/chain/block.cppsrc/chain/block_list.cppsrc/chain/chain.cppsrc/chain/compact_block.cppsrc/chain/header.cppsrc/chain/history.cppsrc/chain/input.cppsrc/chain/input_list.cppsrc/chain/merkle_block.cppsrc/chain/output.cppsrc/chain/output_list.cppsrc/chain/output_point.cppsrc/chain/output_point_list.cppsrc/chain/point.cppsrc/chain/point_list.cppsrc/chain/script.cppsrc/chain/transaction.cppsrc/chain/transaction_list.cppsrc/module.ctests/test_chain_block.pytests/test_chain_header.pytests/test_chain_input.pytests/test_chain_output.pytests/test_chain_output_point.pytests/test_chain_point.pytests/test_chain_script.pytests/test_chain_transaction.py
✅ Files skipped from review due to trivial changes (2)
- conanfile.py
- src/chain/script.cpp
🚧 Files skipped from review as they are similar to previous changes (21)
- src/chain/merkle_block.cpp
- src/chain/history.cpp
- setup.py
- tests/test_chain_input.py
- src/chain/compact_block.cpp
- include/kth/py-native/chain/input_list.h
- include/kth/py-native/chain/block_list.h
- src/chain/chain.cpp
- include/kth/py-native/chain/transaction_list.h
- src/chain/output_point_list.cpp
- src/chain/point_list.cpp
- src/chain/input_list.cpp
- include/kth/py-native/chain/output_point.h
- include/kth/py-native/chain/input.h
- src/chain/output.cpp
- src/chain/transaction_list.cpp
- include/kth/py-native/chain/header.h
- src/chain/block.cpp
- src/chain/transaction.cpp
- include/kth/py-native/chain/script.h
- include/kth/py-native/chain/transaction.h
Rework the py-native bindings for every chain class against the new const/mut C-API surface in kth/0.79.0 and add opaque-handle list bindings for the same set of classes. - Bump conanfile.py to kth/0.79.0 - Rewrite py-native wrappers for: header, block, point, output_point, script, output, input, transaction - Add py-native list wrappers for: block_list, point_list, output_point_list, output_list, input_list, transaction_list - Each class gets a per-class PyMethodDef[] table; module.c picks them up via PyModule_AddFunctions calls inside an AUTO-GENERATED REGISTER block - kth_native.pyi gains typed stubs for every class and list (auto- spliced into the AUTO-GENERATED STUBS block) - Add a shared include/kth/py-native/capsule_names.h with one KTH_PY_CAPSULE_<GROUP>_<CLASS> macro per class and list, plus forward declarations for every PyCapsule destructor. Each per-class .cpp includes the header and references macros by name, so cross-class PyCapsule_New / PyCapsule_GetPointer calls stay in sync - PyCapsule destructors for owned handles: every per-class .cpp defines kth_py_native_<group>_<class>_capsule_dtor so owned handles free their native resources on GC. Borrowed views (const reference returns) keep NULL. Explicit destruct() sets the capsule name to "kth.destroyed" so the dtor becomes a no-op afterwards — no double- free - chain_*_to_data now raises RuntimeError on NULL instead of returning None, matching the typed `bytes` contract in the stubs - size_t parameters reject negative values before casting to kth_size_t (prevents -1 → SIZE_MAX) - value_struct params copy bytes into a local kth_<struct>_t and forward by value (matches the C-API safe variants) - setup.py recompiles every chain wrapper from the reworked sources - Drop legacy hand-written entries from KnuthNativeMethods that are now served by the per-class tables - Fix hand-written wrappers in chain/chain.cpp and friends that still used the legacy void* typedefs (kth_block_t -> kth_block_mut_t, etc.) - Add tests for every class: test_chain_header (22), test_chain_block (12), test_chain_point (10), test_chain_output_point (9), test_chain_script (8), test_chain_output (5), test_chain_input (8), test_chain_transaction (9) — 83 chain tests, 89 total green locally
fpelliccioni
force-pushed
the
refactor/migrate-chain-header
branch
from
April 11, 2026 10:50
b452d9b to
04ed7bf
Compare
3 tasks
2 tasks
fpelliccioni
added a commit
that referenced
this pull request
Apr 21, 2026
Adds pytest coverage for the ~20 chain / wallet classes that landed as generator-driven bindings in the 0.80.0 sync PR. The existing suite exercised the classes migrated under PR #5 (block, header, point, output_point, output, input, script, transaction plus their lists) — the regen brought in a bunch more, and this fills the gap. Twelve new test files, 75 new cases (suite goes from 89 → 164): ### Chain - `test_chain_utxo.py` — construct_default, height / amount / point round-trips, copy, equals; list push_back / nth / erase. - `test_chain_operation.py` — construct_default, construct_from_code, from_data → to_data round-trip, copy, equals; static opcode probes (`is_push`, `is_counted`, `opcode_from_positive` ↔ `_to_positive`); list lifecycle. - `test_chain_history_compact.py` — list lifecycle + symbol-presence smoke on the element accessors. `history_compact` has no public constructor on either the C-API or py-native side (instances only arrive through the async `fetch_history` callback), so per-element unit tests aren't practical without a running chain. - `test_chain_get_blocks.py` — covers both `get_blocks` and `get_headers`: construct_default, stop_hash round-trip, copy / equals, `to_data` ↔ `construct_from_data` round-trip at the wire level, reset. - `test_chain_prefilled_transaction.py` — construct_default, index / transaction round-trips, copy, equals; list push_back / nth. - `test_chain_double_spend_proof.py` — DSP construct_default, copy / equals, hash is 32 bytes, serialized_size matches to_data length, reset; element-level `spender` accessors covered by a symbol-presence smoke test (no public ctor from Python). - `test_chain_token_data.py` — each `make_*` factory (`make_fungible`, `make_non_fungible`, `make_both`), accessors (`get_amount`, `has_nft`, `get_nft_capability`, `get_nft_commitment`, `is_mutable_nft`, `is_immutable_nft`), copy, equals, `to_data` ↔ `construct_from_data` round-trip. - `test_chain_payment_address_list.py` — list lifecycle using real `wallet_payment_address_construct_from_address` elements, with a round-trip through `wallet_payment_address_encoded_legacy` to verify `nth` returns a usable handle. ### Wallet - `test_wallet_ec_public.py` — construct_default invalid, base16 / compressed-point round-trips, copy / equals, `to_uncompressed` is 65 bytes with `0x04` sentinel, `to_payment_address` derives a valid address. - `test_wallet_ec_private.py` — construct_default invalid, secret round-trip, WIF encoded ↔ `construct_from_wif_version` round-trip, copy / equals. - `test_wallet_hd_private.py` — BIP32 test-vector-1 seed construction, secret / chain_code sizes, encoded ↔ `construct_from_encoded_prefixes` round-trip, `derive_private` child has distinct secret, `to_public` cross-check on chain_code, plus HdPublic default-invalid, `derive_public`, copy / equals. - `test_wallet_wallet_data.py` — `create(password, passphrase)` exposure, encrypted_seed is non-empty bytes, xpub is a valid HdPublic, copy preserves encrypted_seed, `mnemonics` call doesn't raise (cross-group capsule reference, one of the five generator bugs #6 fixed). ### Intentional scope No end-to-end validation against real block data — these are smoke + structural round-trip tests meant to catch regen drift (missing symbols, wrong arities, segfaults on common call patterns, breakages of the handle lifecycle). Deeper fidelity tests against known test-vectors land in later PRs as the bindings see actual consumer use.
2 tasks
4 tasks
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
Rework py-native for every chain class against the new const/mut C-API surface in
kth/0.79.0, add opaque-handle list bindings for the same set, and fix a pile of review findings across capsule type safety, GC cleanup, error reporting, and parameter validation.Scope (was: just
chain_header)header,block,point,output_point,script,output,input,transactionblock_list,point_list,output_point_list,output_list,input_list,transaction_listWhat changed
conanfile.pybumped tokth/0.79.0PyMethodDef[]table;src/module.cpicks them up viaPyModule_AddFunctionscalls inside anAUTO-GENERATED REGISTERblockkth_native.pyigains typed stubs for every class and list (auto-spliced)setup.pyrecompiles every chain wrapper from the reworked sourcesKnuthNativeMethods(now served by the per-class tables)chain/chain.cpp/merkle_block.cpp/compact_block.cpp/history.cppupdated to consume the new const/mut typedefs (kth_block_t→kth_block_mut_t, etc.)Capsule type safety
include/kth/py-native/capsule_names.hcentralises oneKTH_PY_CAPSULE_<GROUP>_<CLASS>macro per class and per list, plus forward declarations for every PyCapsule destructor..cppincludes that header and references macros by name — no per-file#define KTH_PY_CAPSULE_NAME "..."and no inline string literals inPyCapsule_New/PyCapsule_GetPointer.input::extract_embedded_scriptreturning aScript) now tag the returned capsule with the target class's macro, not the owning class's.GC-driven cleanup
_mut_treturn attaches a PyCapsule destructor:kth_py_native_<group>_<class>_capsule_dtor. GC calls it automatically, so forgetting the explicit*_destruct()call no longer leaks native memory._const_tby-reference returns,list.nth) keepNULLdestructors — the parent still owns the memory, no double-free.destruct()calls set the capsule name to"kth.destroyed", so the GC destructor becomes a no-op afterwards. No double-free when both paths run.Error handling & validation
chain_*_to_datanow raisesRuntimeError("kth: serialization failed")on a NULL native buffer instead of returningNone. The.pyireturn type is nowbytes(notbytes | None), so the stub matches the runtime contract.size_tparameters reject negative values before casting tokth_size_t.height = -1used to silently becomeSIZE_MAX; now it raisesValueError("height must be non-negative, got -1").value_structparams (hashes) copy bytes into a localkth_<struct>_tand forward by value, matching the C-API safe variants.Tests
test_chain_header.py(22),test_chain_block.py(12),test_chain_point.py(10),test_chain_output_point.py(9),test_chain_script.py(8),test_chain_output.py(5),test_chain_input.py(8),test_chain_transaction.py(9)test_config,test_node,test_node_async) = 89/89 green locally.test_chain_blockexercises the GC destructor path by dropping 100 blocks without callingdestructexplicitly and runninggc.collect().Out of scope for this PR (tracked for follow-up)
chain_fetch_merkle_block_handler/chain_fetch_block_header_handler.chain_compact_block_transaction_nthparses index as 32-bit.Block = object/ etc. aliases shadow generated class stubs inkth_native.pyi.#includes inmodule.c's AUTO-GENERATED INCLUDES block.Test plan
pip install --no-build-isolation -e .— build cleanpytest tests/ -v— 89/89 pass locallySummary by CodeRabbit
Chores
Tests
New Features
Refactor