Chore: remove `bindgen` and replace with hand-rolled FFI bindings by alexanderwiederin · Pull Request #163 · sedited/rust-bitcoinkernel

alexanderwiederin · 2026-04-24T11:38:28Z

Closes: #157

Summary

Replaces the bindgen-generated FFI bindings with a hand-written lib.rs in libbitcoinkernel-sys. As a follow-up the constants.rs in the wrapper crate is removed and call sites are updated to use the btck_* names from the sys crate.

Motivation

Auditability
No build-time dependency

Changes

libbitcoinkernel-sys/src/lib.rs: Hand-written FFI bindings replacing the bindgen-generated bindings.rs.
std::os::raw type aliases replaced with core::ffi equivalents.
libbitcoinkernel-sys/build.rs: Bindgen block removed.
libbitcoinkernel-sys/Cargo.toml: bindgen removed from build-dependencies.
src/ffi/constants.rs: Deleted. Constants now imported from libbitcoinkernel-sys using btck_* names.
Cargo-minimal.lock, Cargo-recent.lock: Updated to reflect removed bindgen transitive dependencies.

Question to Reviewers: Is it the right moment to do this?

alexanderwiederin · 2026-04-24T12:05:27Z

~~Will take another look later to ensure the constants have been migrated accurately.~~

sedited · 2026-04-24T16:39:24Z

Question to Reviewers: Is it the right moment to do this?

Yes!

jaoleal · 2026-04-24T17:23:38Z

Question to Reviewers: Is it the right moment to do this?

Yeah, this pr will help me a lot and ill be glad to help you guys to ship it.

alexanderwiederin · 2026-04-25T14:08:29Z

Good to hear!

I think we are practically good to go here. One thing to note is that opaque types and grouped definitions are ordered alphabetically rather than following the C API order, as I felt it would scale easiest. The extern "C" declarations have the C API ordering as interdependencies require it.

Correction: On second thought, I don't think the extern "C" declarations need to be in the order of the C API either.

sedited · 2026-04-29T12:00:49Z

+    _unused: [u8; 0],
+}
+#[repr(C)]
+pub struct btck_ChainstateManagerOptions {


I wonder if we should also derive Debug, Copy, and Clone for these types. Do you think there could be a downside?

I removed them to keep the -sys crate minimal. openssl-sys takes the same approach, while secp256k1-sys does derive them - so there's no universal convention. For me it was mostly about keeping the surface area small.

Do you have a preference?

sedited

Looks good. The only drawback I see here is that we need to be careful with copying over the correct constant values.

jaoleal

ACK 9975909

Only nits and docs suggestions, perhaps the second commit thats solely to move constants could be included in CHANGELOG.md ?

Also, the README.md under libbitcoinkernel-sys is pretty simple, we could include some sayings about the hand written bindings

jaoleal · 2026-04-30T13:24:09Z

+// Primitive type aliases - alphabetical order
+
+pub type btck_BlockCheckFlags = u32;
+pub type btck_BlockValidationResult = u32;


Reading this and seeing that auditability is one of the purposes for this, perhaps we could work out a little more docs here ? A good example that I saw was secp256k1 bindings.

For this case i suggest some simple docs such as

Suggested change

pub type btck_BlockValidationResult = u32;

/// Represents a granular reason for block validation.

pub type btck_BlockValidationResult = u32;

Good suggestion, but I'd like to keep the scope focused on the bindings as much as possible. I'll add a top-level doc comment and update the README pointing to the C header for now.

jaoleal · 2026-04-30T16:03:23Z

 pub type btck_ValidationMode = u8;
 pub type btck_Warning = u8;

+// btck_BlockValidationResult


The constants looks fine, cant we maintain the constants.rs ? That would leave this lib.rs only for declarations right ?

I would prefer to keep everything in lib.rs and mirror the structure of the C header. This makes auditing easier, especially when updating to newer versions. The file is organised with the section comments to make navigation straightforward. Having said that, if the API grows significantly we can revisit splitting it up.

jaoleal · 2026-04-30T17:57:04Z

Question: will this trigger a new release ?

sedited · 2026-04-30T18:05:26Z

Yes :)

jaoleal · 2026-04-30T20:21:34Z

+    assert!(core::mem::size_of::<btck_NotificationInterfaceCallbacks>() == 72);
+    assert!(core::mem::align_of::<btck_NotificationInterfaceCallbacks>() == 8);
+    assert!(core::mem::size_of::<btck_ValidationInterfaceCallbacks>() == 48);
+    assert!(core::mem::align_of::<btck_ValidationInterfaceCallbacks>() == 8);


Suggested change

assert!(core::mem::size_of::<btck_NotificationInterfaceCallbacks>() == 72);

assert!(core::mem::align_of::<btck_NotificationInterfaceCallbacks>() == 8);

assert!(core::mem::size_of::<btck_ValidationInterfaceCallbacks>() == 48);

assert!(core::mem::align_of::<btck_ValidationInterfaceCallbacks>() == 8);

assert!(

core::mem::size_of::<btck_NotificationInterfaceCallbacks>()

== 9 * core::mem::size_of::<*const ()>()

);

assert!(

core::mem::align_of::<btck_NotificationInterfaceCallbacks>()

== core::mem::align_of::<*const ()>()

);

assert!(

core::mem::size_of::<btck_ValidationInterfaceCallbacks>()

== 6 * core::mem::size_of::<*const ()>()

);

assert!(

core::mem::align_of::<btck_ValidationInterfaceCallbacks>()

== core::mem::align_of::<*const ()>()

);

After rebasing #158 on top of this to see if anything breaks, i found a problem regarding pointers size on armv7-linux-androideabi since pointers are 4 bytes on 32-bit targets.

the above suggestion fixed the problem and i was able to successfully build this lib to armv7-linux-androideabi. It multiplies the size of pointers at compile time but im not totally sure if its the best fix for this case.

Good catch! The hardcoded values were assuming 64-but pointer sizes. Your suggestion make the assertions valid on both 32-bit and 64-bit targets.

Applied - thanks!

Added #175 to formally support 32-bit targets.

alexanderwiederin · 2026-05-01T10:57:12Z

Only nits and docs suggestions, perhaps the second commit thats solely to move constants could be included in CHANGELOG.md ?

Done.

Also, the README.md under libbitcoinkernel-sys is pretty simple, we could include some sayings about the hand written bindings

Done.

Thanks for the feedback!

jaoleal

ACK

… FFI bindings Replace the bindgen-generated bindings.rs with a hand-written lib.rs. The bindgen build dependency is removed. The hand-rolled bindings are verified against bindgen output and match the types. Layout assertions enforce ABI correctness for three structs passed by value across the FFI boundary at compile time. Unnecessary derives (Debug, Copy, Clone) are removed from all types. Cargo-minimal.lock and Cargo-recent.lock have been updated to remove bindgen dependency. libbitcoinkernel-sys/CHANGELOG.md updated to reflect bindgen removal.

…om sys crate Delete the constants.rs module and update all call sites to import btck_* constants directly from libbitcoinkernel-sys. The BTCK_* aliases are no longer needed.

Add #[allow(non_upper_case_globals)] to the From impl blocks that match on btck_* constants. These names intentionally mirror the C header naming convention.

jaoleal · 2026-05-04T19:40:41Z

ACK ccb3542

alexanderwiederin force-pushed the ffi branch from b680870 to 99e9076 Compare April 24, 2026 11:47

alexanderwiederin marked this pull request as draft April 24, 2026 12:07

alexanderwiederin changed the title ~~Chore: remove Bindgen and replace with hand-rolled FFI bindings~~ Chore: remove bindgen and replace with hand-rolled FFI bindings Apr 24, 2026

alexanderwiederin force-pushed the ffi branch from 99e9076 to e420147 Compare April 24, 2026 14:43

alexanderwiederin marked this pull request as ready for review April 24, 2026 14:46

alexanderwiederin force-pushed the ffi branch from e420147 to 6bd305f Compare April 24, 2026 14:50

alexanderwiederin force-pushed the ffi branch from 6bd305f to e0127f9 Compare April 25, 2026 13:49

alexanderwiederin force-pushed the ffi branch 2 times, most recently from e49a09d to 0917624 Compare April 25, 2026 15:06

sedited reviewed Apr 29, 2026

View reviewed changes

Comment thread libbitcoinkernel-sys/src/lib.rs Outdated

alexanderwiederin force-pushed the ffi branch 3 times, most recently from 5d80502 to 9975909 Compare April 29, 2026 18:50

jaoleal approved these changes Apr 30, 2026

View reviewed changes

jaoleal reviewed Apr 30, 2026

View reviewed changes

jaoleal mentioned this pull request Apr 30, 2026

Feat: Android support #158

Open

alexanderwiederin force-pushed the ffi branch 4 times, most recently from ff4de75 to 81bd88d Compare May 1, 2026 10:56

alexanderwiederin mentioned this pull request May 1, 2026

ci: add cross-compilation job for 32-bit/Android targets #175

Open

jaoleal approved these changes May 1, 2026

View reviewed changes

alexanderwiederin added 4 commits May 2, 2026 12:35

docs(libbitcoinkernel-sys): improve README and crate description

fc426fe

chore(libbitcoinkernel-sys): remove constants.rs, use btck_* names fr…

4f23ecf

…om sys crate Delete the constants.rs module and update all call sites to import btck_* constants directly from libbitcoinkernel-sys. The BTCK_* aliases are no longer needed.

chore(bitcoinkernel): suppress non_upper_case_globals at call sites

ccb3542

Add #[allow(non_upper_case_globals)] to the From impl blocks that match on btck_* constants. These names intentionally mirror the C header naming convention.

alexanderwiederin force-pushed the ffi branch from 81bd88d to ccb3542 Compare May 2, 2026 10:35

	pub type btck_BlockValidationResult = u32;
	/// Represents a granular reason for block validation.
	pub type btck_BlockValidationResult = u32;

Conversation

alexanderwiederin commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Changes

Uh oh!

alexanderwiederin commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sedited commented Apr 24, 2026

Uh oh!

jaoleal commented Apr 24, 2026

Uh oh!

alexanderwiederin commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sedited Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaoleal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaoleal commented Apr 30, 2026

Uh oh!

sedited commented Apr 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderwiederin commented May 1, 2026

Uh oh!

jaoleal left a comment

Choose a reason for hiding this comment

Uh oh!

jaoleal commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexanderwiederin commented Apr 24, 2026 •

edited

Loading

alexanderwiederin commented Apr 24, 2026 •

edited

Loading

alexanderwiederin commented Apr 25, 2026 •

edited

Loading

sedited Apr 29, 2026 •

edited

Loading