Skip to content

Conversation

alexanderwiederin
Copy link
Contributor

@alexanderwiederin alexanderwiederin commented Aug 21, 2025

Refactor: adapt to btck_api changes and reorganize codebase

Adapts to C API changes from bitcoin/bitcoin@bce88ae

API Changes

  • Update enum variants to PascalCase (REGTEST -> Regtest)
  • Remove UnownedBlock, replace with Block in validation callbacks
  • Add user_data_destroy callbacks for proper memory cleanup - Rust now transfers ownership of callback data to C library

Code Organization

  • Add constants.rs with btck_* constant definitions
  • Move enums and verify logic to separate modules
  • Improve type conversions and error handling

Platform Support

  • Add Windows-specific library linking (bcrypt, shell32)

Next steps as follow-up to this change

  • Further breakdown of lib.rs into separate files

Note: Changes should come more piecemeal from now hopefully

…88ae28a

bce88ae28a kernel: Fix bitcoin-chainstate for windows
3a7e9f0eaf kernel: Add Purpose section to header documentation
5bae79ace5 kernel: Allowing reducing exports
d0308a2489 kernel: Add pure kernel bitcoin-chainstate
05a569070c kernel: Add functions to get the block hash from a block
8566ec6e83 kernel: Add block index utility functions to C header
b4d0e80f84 kernel: Add function to read block undo data from disk to C header
488999ac77 kernel: Add functions to read block from disk to C header
3dc76bb7f7 kernel: Add function for copying block data to C header
6151b45a42 kernel: Add functions for the block validation state to C header
5d00432f27 kernel: Add validation interface to C header
facf209aee kernel: Add interrupt function to C header
129f553e4e kernel: Add import blocks function to C header
f7ed7b944d kernel: Add chainstate load options for in-memory dbs in C header
67d9f53a98 kernel: Add options for reindexing in C header
ebc826319f kernel: Add block validation to C header
511a1c8a78 kernel: Add chainstate loading when instantiating a ChainstateManager
aad295899e kernel: Add chainstate manager option for setting worker threads
c701cb2405 kernel: Add chainstate manager object to C header
1df8b87602 kernel: Add notifications context option to C header
571c1a2acb kernel: Add chain params context option to C header
a2cab9f1cd kernel: Add kernel library context object
944ef6b630 kernel: Add logging to kernel library C header
d0cb841fba kernel: Introduce initial kernel C header API
04c115dfde Merge bitcoin/bitcoin#33078: kernel: improve BlockChecked ownership semantics
bc797d2271 Merge bitcoin/bitcoin#33154: test: use local `CBlockIndex` in block read hash mismatch check
d3c58a5be9 Merge bitcoin/bitcoin#33193: Release: Prepare "Translation string freeze" step
9cf7b3d90c Merge bitcoin/bitcoin#33211: test: modify logging_filesize_rate_limit params
f5f853d952 Merge bitcoin/bitcoin#32878: index: fix wrong assert of current_tip == m_best_block_index
5dda364c4b test: modify logging_filesize_rate_limit params
0df2c3c42e qt: Update `src/qt/locale/bitcoin_en.xlf` translation source file
22e689587a Merge bitcoin/bitcoin#33209: cmake: Drop python dependency for translate
be356fc49b Merge bitcoin/bitcoin#32896: wallet, rpc: add v3 transaction creation and wallet support
3c4a109aa8 cmake: Drop python dependency for translate
f58de8749e Merge bitcoin/bitcoin#32345: ipc: Handle unclean shutdowns better
d31dc8f818 Merge bitcoin/bitcoin#33200: cmake: Introduce translate.cmake script for translate target
05255d5d1e cmake: Drop dependency on sed for translate target
d5054beca5 cmake: Introduce translate.cmake script for translate target
57e8f34fe2 Merge bitcoin/bitcoin#32977: wallet: Remove wallet version and several legacy related functions
97593c1fd3 Merge bitcoin/bitcoin#32975: assumevalid: log every script validation state change
5c8bf7b39e doc: add release notes for version 3 transactions
4ef8065a5e test: add truc wallet tests
5d932e14db test: extract `bulk_vout` from `bulk_tx` so it can be used by wallet tests
2cb473d9f2 rpc: Support version 3 transaction creation
4c20343b4d rpc: Add transaction min standard version parameter
c5a2d08011 wallet: don't return utxos from multiple truc txs in AvailableCoins
da8748ad62 wallet: limit v3 tx weight in coin selection
85c5410615 wallet: mark unconfirmed v3 siblings as mempool conflicts
0804fc3cb1 wallet: throw error at conflicting tx versions in pre-selected inputs
cc155226fe wallet: set m_version in coin control to default value
2e9617664e  wallet: don't include unconfirmed v3 txs with children in available coins
ec2676becd wallet: unconfirmed ancestors and descendants are always truc
7b4a1350df Merge bitcoin/bitcoin#33183: validation: rename block script verification error from "mandatory" to "block"
c99f5c5e1b Merge bitcoin/bitcoin#33106: policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee
578b512bdd Merge bitcoin/bitcoin#33011: log: rate limiting followups
8405fdb06e Merge bitcoin/bitcoin#33169: interfaces, chain, refactor: Remove unused getTipLocator and incaccurate getActiveChainLocator
c0d91fc69c Add release note for #33050 and #33183 error string changes
e17b5da0d6 Merge bitcoin/bitcoin#33179: doc: update wallet build instruction
9b1a7c3e8d Merge bitcoin/bitcoin#33116: refactor: Convert uint256 to Txid
b3f781a0ef contrib: adapt max reject string size in tracing demo
9a04635432 scripted-diff: validation: rename mandatory errors into block errors
dbf8b0980b Merge bitcoin/bitcoin#33171: ci: Update `actions/checkout` version
d6887f0cec Merge bitcoin/bitcoin#33178: guix: increase maximum allowed (runtime) GCC to 7
2b00030af8 interfaces, chain, refactor: Remove inaccurate getActiveChainLocator
110a0f405c interfaces, chain, refactor: Remove unused getTipLocator
dadf15f88c Merge bitcoin/bitcoin#33050: net, validation: don't punish peers for consensus-invalid txs
cb173b8e93 test: use local `CBlockIndex` in block read hash mismatch test to avoid data race
73972d5617 Merge bitcoin/bitcoin#31296: wallet: Translate [default wallet] string in progress messages
67e186deb0 doc: update wallet build instruction
5c74a0b397 config: add DEBUG_ONLY -logratelimit
9f3b017bcc test: logging_filesize_rate_limit improvements
350193e5e2 test: don't leak log category mask across tests
05d7c22479 test: add ReadDebugLogLines helper function
3d630c2544 log: make m_limiter a shared_ptr
ec484bd5ce Merge bitcoin/bitcoin#31453: util: detect and warn when using exFAT on MacOS
776a163374 guix: increase maximum allowed (runtime) GCC to 7
ba84a25dee [doc] update mempool-replacements.md for incremental relay feerate change
273e600e65 Merge bitcoin/bitcoin#33021: test/refactor: revive test verifying that `GetCoinsCacheSizeState` switches from OK→LARGE→CRITICAL
18720bc5d5 [doc] release note for min feerate changes
6da5de58ca [policy] lower default minrelaytxfee and incrementalrelayfee to 100sat/kvB
2e515d2897 [prep/test] make wallet_fundrawtransaction's minrelaytxfee assumption explicit
457cfb61b5 [prep/util] help MockMempoolMinFee handle more precise feerates
3eab8b7240 [prep/test] replace magic number 1000 with respective feerate vars
5f2df0ef78 [miner] lower default -blockmintxfee to 1sat/kvB
d6213d6aa1 [doc] assert that default min relay feerate and incremental are the same
1fbee5d7b6 [test] explicitly check default -minrelaytxfee and -incrementalrelayfee
72dc18467d [test] RBF rule 4 for various incrementalrelayfee settings
85f498893f [test] check bypass of minrelay for various minrelaytxfee settings
e5f896bb1f [test] check miner doesn't select 0fee transactions
de0675f9de refactor: Move `transaction_identifier.h` to primitives
6f068f65de Remove implicit uint256 conversion and comparison
9c24cda72e refactor: Convert remaining instances from uint256 to Txid
d2ecd6815d policy, refactor: Convert uint256 to Txid
f6c0d1d231 mempool, refactor: Convert uint256 to Txid
aeb0f78330 refactor: Convert `mini_miner` from uint256 to Txid
326f244724 refactor: Convert RPCs and `merkleblock` from uint256 to Txid
41642d43b3 Merge bitcoin/bitcoin#33162: test: fix scripts in `blockfilter_basic_test`
f83c01d882 ci: Update `actions/checkout` version
a27430e259 Merge bitcoin/bitcoin#32473: Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts
34b366fa2c Merge bitcoin/bitcoin#33155: contrib: drop `bitcoin-util` exception from FORTIFY check
ca64b71ed5 test: fix scripts in `blockfilter_basic_test`
fab2980bdc assumevalid: log every script validation state change
e8f9c37a3b log: clean up LogPrintStr_ and Reset, prefix all logs with "[*]" when there are suppressions
3c7cae49b6 log: change LogLimitStats to struct LogRateLimiter::Stats
876dbdfb47 tests: drop expect_disconnect behaviour for tx relay
b29ae9efdf validation: only check input scripts once
266dd0e10d net_processing: drop MaybePunishNodeForTx
db3228042b util: detect and warn when using exFAT on macOS
4bff4ce561 contrib: drop bitcoin-util exception from FORTIFY check
83950275ed qa: unit test sighash caching
b221aa80a0 qa: simple differential fuzzing for sighash with/without caching
92af9f74d7 script: (optimization) introduce sighash midstate caching
8f3ddb0bcc script: (refactor) prepare for introducing sighash midstate cache
9014d4016a tests: add sighash caching tests to feature_taproot
49b3d3a92a Clean up `FindTxForGetData`
2581258ec2 ipc: Handle bitcoin-wallet disconnections
2160995916 ipc: Add Ctrl-C handler for spawned subprocesses
0c28068ceb doc: Improve IPC interface comments
7f65aac78b ipc: Avoid waiting for clients to disconnect when shutting down
6eb09fd614 test: Add unit test coverage for Init and Shutdown code
9a9fb19536 ipc: Use EventLoopRef instead of addClient/removeClient
5c45bc989b Merge commit 'e886c65b6b37aaaf5d22ca68bc14e55d8ec78212' into pr/ipc-stop-base
e886c65b6b Squashed 'src/ipc/libmultiprocess/' changes from 27c7e8e5a581..b4120d34bad2
3aef38f44b test: exercise index reorg assertion failure
acf50233cd index: fix wrong assert of current_tip == m_best_block_index
1d9f1cb4bd kernel: improve BlockChecked ownership semantics
554befd873 test: revive `getcoinscachesizestate`
64ed0fa6b7 refactor: modernize `LargeCoinsCacheThreshold`
1b40dc02a6 refactor: extract `LargeCoinsCacheThreshold` from `GetCoinsCacheSizeState`
8319a13468 log: clarify RATELIMIT_MAX_BYTES comment, use RATELIMIT_WINDOW
5f70bc80df log: remove const qualifier from arguments in LogPrintFormatInternal
b8e92fb3d4 log: avoid double hashing in SourceLocationHasher
616bc22f13 test: remove noexcept(false) comment in ~DebugLogHelper
9ba1fff29e kernel: refactor: ConnectTip to pass block pointer by value
60d1042b9a wallet: Remove unused `WalletFeature` enums
66de58208a wallet: Remove `CWallet::nWalletVersion` and related functions
7cda3d0f5b wallet: Remove `IsFeatureSupported()` and `CanSupportFeature()`
ba01585229 wallet: `MigrateToDescriptor` no longer calls `CanSupportFeature`
63acee2797 wallet: Remove `GetClosestWalletFeature()`
e27da3150b wallet: Remove `GetVersion()`
db225cea56 wallet, refactor: Replace GetDisplayName() with LogName()
01737883b3 wallet: Translate [default wallet] string in progress messages
REVERT: 35fced5df7 kernel: Fix bitcoin-chainstate for windows
REVERT: c164264fd8 kernel: Add Purpose section to header documentation
REVERT: 9096c35c05 kernel: Allowing reducing exports
REVERT: 6d0d4b2507 kernel: Add pure kernel bitcoin-chainstate
REVERT: ccd85f0333 kernel: Add functions to get the block hash from a block
REVERT: 925f21c37b kernel: Add block index utility functions to C header
REVERT: b7441841c9 kernel: Add function to read block undo data from disk to C header
REVERT: bc0e6f098e kernel: Add functions to read block from disk to C header
REVERT: 5120302f96 kernel: Add function for copying block data to C header
REVERT: 718ccee732 kernel: Add functions for the block validation state to C header
REVERT: eb363ab30e kernel: Add validation interface to C header
REVERT: 246886c6ea kernel: Add interrupt function to C header
REVERT: f3b34ca457 kernel: Add import blocks function to C header
REVERT: fe08857d52 kernel: Add chainstate load options for in-memory dbs in C header
REVERT: f93f171e01 kernel: Add options for reindexing in C header
REVERT: dca7b4c26e kernel: Add block validation to C header
REVERT: f031e9ce03 kernel: Add chainstate loading when instantiating a ChainstateManager
REVERT: 3cb99f73ec kernel: Add chainstate manager option for setting worker threads
REVERT: 9454ad8512 kernel: Add chainstate manager object to C header
REVERT: 3bead9ebdd kernel: Add notifications context option to C header
REVERT: dda805dfb6 kernel: Add chain params context option to C header
REVERT: ea5334925d kernel: Add kernel library context object
REVERT: 36fafbaef9 kernel: Add logging to kernel library C header
REVERT: d28eef5cf2 kernel: Introduce initial kernel C header API

git-subtree-dir: libbitcoinkernel-sys/bitcoin
git-subtree-split: bce88ae28ab2cd12f32aead1fbf47153c50c3b05
@alexanderwiederin alexanderwiederin marked this pull request as ready for review August 21, 2025 14:32
Copy link
Owner

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer if we split the modules by their meaning, and not their type. In rust I would expect a module called "enums" to not necessarily hold enums, but define common traits for working with our enums. Is your plan to just park them there for now and then split them out into more files? I saw that you made an exception for the verification and think that would be a more coherent approach.

The callbacks will also require more work eventually. I wonder if passing in this single struct is really the best way to achieve this, or if we can come up with a clever set of traits that we can define for it.

@@ -93,8 +93,14 @@ fn main() {
.expect("Couldn't write bindings!");

let compiler = cc::Build::new().get_compiler();
let target_os = std::env::var("CARGO_CFG_TARGET_OS").unwrap();

if target_os == "windows" {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did the windows CI pass without these present so far?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into this.

Copy link
Contributor Author

@alexanderwiederin alexanderwiederin Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know of any changes to how library dependencies are resolved in the build environment?

The library has always used Windows APIs that require these libraries (evidenced by bcrypt already being linked in the kernel's CMakeLists.txt), but something changed in how static library dependencies are handled between kernelApi_56 and kernelApi_60.

Any ideas? I couldn't find anything.

@alexanderwiederin
Copy link
Contributor Author

alexanderwiederin commented Aug 25, 2025

I was unsure of how to deal with the enums, since they really are just thin wrappers around what is defined in the C API.

Anyways, had another thought about it and agree we should strive for a more purely domain-oriented structure (which I think you alluded to):

src/
├── lib.rs
├── ffi/                      # Foreign Function Interface / C bindings
│   ├── mod.rs
│   ├── c_helpers.rs          # Utilities for C callback patterns (incl. c_serialize, cast_string)
│   └── constants.rs          # C-side constants
├── core/                     # Core Bitcoin data types and consensus logic
│   ├── mod.rs
│   ├── block.rs              # Block, BlockHash, BlockSpentOutputs, TransactionSpentOutputs
│   ├── transaction.rs        # Transaction, TxOut, Coin
│   ├── script.rs             # ScriptPubkey 
│   └── verify.rs             # Script verification logic, flags, ScriptVerifyStatus ScriptVerifyError
├── state/                    # Chainstate & contextual info
│   ├── mod.rs
│   ├── chainstate.rs         # ChainstateManager
│   ├── context.rs            # Context, ContextBuilder, ChainType
│   └── chain.rs              # Chain, ChainIterator
├── notifications/            # Callback interfaces for notifications
│   ├── mod.rs
│   ├── kernel.rs             # KernelNotificationInterface callbacks
│   ├── validation.rs         # ValidationInterface callbacks
│   └── types.rs              # SynchronizationState, ValidationMode, BlockValidationResult, Warning
└── util/                     # Utility modules
    ├── mod.rs
    └── logging.rs            # Logger, LogLevel, LogCatgeory and Logging helpers (e.g. disable_logging)

The callback traits you mentioned could live in the respective files under /notifications.

Open to feedback on this approach. What do you think?

@alexanderwiederin alexanderwiederin force-pushed the btck_enum branch 4 times, most recently from 6e6e43b to 0ddf679 Compare August 25, 2025 14:55
- Update enum variants to PascalCase(REGTEST -> Regtest)
- Add constants.rs with btck_* constant definitions
- Move enums and verify logic to separate modules
- Add Windows-specific library linking (bcrypt, shell32)
- Update callback signatures and memory management
- Remove UnownedBlock, replace with Block in validation callbacks
- Improve type conversions and error handling
- Implement non-generic Logger with `destroy_log_callback` and pass
  ownership to C library
@TheCharlatan
Copy link
Owner

The layout you proposed looks closer to what I had in mind. I would call util/ just log.

@alexanderwiederin alexanderwiederin force-pushed the btck_enum branch 6 times, most recently from 7af8edb to b221f7f Compare September 2, 2025 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants