Skip to content

Conversation

@Davidson-Souza
Copy link
Member

@Davidson-Souza Davidson-Souza commented Apr 22, 2025

What is the purpose of this pull request?

  • Bug fix
  • Documentation update
  • New feature
  • Test
  • Other: Updating the consensus code.

Which crates are being modified?

  • floresta-chain
  • floresta-cli
  • floresta-common
  • floresta-compact-filters
  • floresta-electrum
  • floresta-watch-only
  • floresta-wire
  • floresta
  • florestad
  • Other: .

Description

This replaces #418

libbitcoinconsensus is deprecated, and have a sub-optimal performance due to deserializing transactions for every input, even if we are validating them in sequence. Libbitcoinkernel has a wrapper type for transactions, and we pass a & ref to it. We can then create the tx once and pass it as reference to validate each input. This causes a massive perf gain during script validation.

This commit replaces all mentions to libbitcoinconsensus, and replaces with kernel where needed. Notice that we can't use both, because of name clashing on linking time.

@Davidson-Souza Davidson-Souza added dependencies Pull requests that update a dependency file chore Cleaning, refactoring, reducing complexity performance This is a performance-related issue consensus This changes something inside our consensus implementation labels Apr 22, 2025
@Davidson-Souza
Copy link
Member Author

Here's a flamegraph of it validating blocks from our assumeutxo height (about 850k) to the tip.

flamegraph

PSA: the perf.data for this flamegraph has 113G and took 10h to process 😂

@sedited
Copy link

sedited commented Apr 25, 2025

This builds on top of the unmerged bitcoin/bitcoin#30595, using rust-bitcoinkernel. It also can't be built with our MSRV for two reasons:
It uses the rust-lang/rust#114441 feature that was stabilized in 1.81.0.
A build dependency, home, have MSRV of 1.80. Although I did menage to pin this into an older version and it worked.

Thanks for the notice, I pushed a new release to enforce an MSRV of 1.71

@Davidson-Souza
Copy link
Member Author

Davidson-Souza commented Apr 25, 2025

@TheCharlatan

This builds on top of the unmerged bitcoin/bitcoin#30595, using rust-bitcoinkernel. It also can't be built with our MSRV for two reasons:
It uses the rust-lang/rust#114441 feature that was stabilized in 1.81.0.
A build dependency, home, have MSRV of 1.80. Although I did menage to pin this into an older version and it worked.

Thanks for the notice, I pushed a new release to enforce an MSRV of 1.71

Nice! I'll update with the latest version.

I'm gettering some data to make a proper feedback on your PR, should be ready by early next week.

@Davidson-Souza Davidson-Souza force-pushed the libbitcoinkernel branch 4 times, most recently from b0ff8ca to 8c373f9 Compare May 7, 2025 17:55
fanquake added a commit to bitcoin/bitcoin that referenced this pull request Nov 4, 2025
6c7a34f kernel: Add Purpose section to header documentation (TheCharlatan)
7e9f00b kernel: Allowing reducing exports (TheCharlatan)
7990463 kernel: Add pure kernel bitcoin-chainstate (TheCharlatan)
36ec9a3 Kernel: Add functions for working with outpoints (TheCharlatan)
5eec7fa kernel: Add block hash type and block tree utility functions to C header (TheCharlatan)
f5d5d12 kernel: Add function to read block undo data from disk to C header (TheCharlatan)
09d0f62 kernel: Add functions to read block from disk to C header (TheCharlatan)
a263a4c kernel: Add function for copying block data to C header (TheCharlatan)
b30e15f kernel: Add functions for the block validation state to C header (TheCharlatan)
aa262da kernel: Add validation interface to C header (TheCharlatan)
d27e277 kernel: Add interrupt function to C header (TheCharlatan)
1976b13 kernel: Add import blocks function to C header (TheCharlatan)
a747ca1 kernel: Add chainstate load options for in-memory dbs in C header (TheCharlatan)
070e777 kernel: Add options for reindexing in C header (TheCharlatan)
ad80abc kernel: Add block validation to C header (TheCharlatan)
cb1590b kernel: Add chainstate loading when instantiating a ChainstateManager (TheCharlatan)
e2c1bd3 kernel: Add chainstate manager option for setting worker threads (TheCharlatan)
65571c3 kernel: Add chainstate manager object to C header (TheCharlatan)
c62f657 kernel: Add notifications context option to C header (TheCharlatan)
9e1bac4 kernel: Add chain params context option to C header (TheCharlatan)
337ea86 kernel: Add kernel library context object (TheCharlatan)
28d679b kernel: Add logging to kernel library C header (TheCharlatan)
2cf136d kernel: Introduce initial kernel C header API (TheCharlatan)

Pull request description:

  This is a first attempt at introducing a C header for the libbitcoinkernel library that may be used by external applications for interfacing with Bitcoin Core's validation logic. It currently is limited to operations on blocks. This is a conscious choice, since it already offers a lot of powerful functionality, but sits just on the cusp of still being reviewable scope-wise while giving some pointers on how the rest of the API could look like.

  The current design was informed by the development of some tools using the C header:

  * A re-implementation (part of this pull request) of [bitcoin-chainstate](https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-chainstate.cpp).
  * A re-implementation of the python [block linearize](https://github.com/bitcoin/bitcoin/tree/master/contrib/linearize) scripts: https://github.com/TheCharlatan/bitcoin/tree/kernelLinearize
  * A silent payment scanner: https://github.com/josibake/silent-payments-scanner
  * An electrs index builder: https://github.com/josibake/electrs/commits/electrs-kernel-integration
  * A rust bitcoin node: https://github.com/TheCharlatan/kernel-node
  * A reindexer: https://github.com/TheCharlatan/bitcoin/tree/kernelApi_Reindexer

  The library has also been used by other developers already:

  * A historical block analysis tool: https://github.com/ismaelsadeeq/mining-analysis
  * A swiftsync hints generator: https://github.com/theStack/swiftsync-hints-gen
  * Fast script validation in floresta: getfloresta/Floresta#456
  * A swiftsync node implementation: https://github.com/2140-dev/swiftsync/tree/master/node

  Next to the C++ header also made available in this pull request, bindings for other languages are available here:

  * Rust: https://github.com/TheCharlatan/rust-bitcoinkernel
  * Python: https://github.com/stickies-v/py-bitcoinkernel
  * Go: https://github.com/stringintech/go-bitcoinkernel
  * Java: https://github.com/yuvicc/java-bitcoinkernel

  The rust bindings include unit and fuzz tests for the API.

  The header currently exposes logic for enabling the following functionality:
  * Feature-parity with the now deprecated libbitcoin-consensus
  * Optimized sha256 implementations that were not available to previous users of libbitcoin-consensus thanks to a static kernel context
  * Full support for logging as well as control over categories and severity
  * Feature parity with the existing experimental bitcoin-chainstate
  * Traversing the block index as well as using block index entries for reading block and undo data.
  * Running the chainstate in memory
  * Reindexing (both full and chainstate-only)
  * Interrupting long-running functions

  The pull request introduces a new kernel-only test binary that purely relies on the kernel C header and the C++ standard library. This is intentionally done to show its capabilities without relying on other code inside the project. This may be relaxed to include some of the existing utilities, or even be merged into the existing test suite.

  The complete docs for the API as well as some usage examples are hosted on [thecharlatan.ch/kernel-docs](https://thecharlatan.ch/kernel-docs/index.html). The docs are generated from the following repository (which also holds the examples): [github.com/TheCharlatan/kernel-docs](https://github.com/TheCharlatan/kernel-docs).

  #### How can I review this PR?

  Scrutinize the commit messages, run the tests, write your own little applications using the library, let your favorite code sanitizer loose on it, hook it up to your fuzzing infrastructure, profile the difference between the existing bitcoin-chainstate and the bitcoin-chainstate introduced here, be nitty on the documentation, police the C interface, opine on your own API design philosophy.

  To get a feeling for the API, read through the tests, or one of the examples.

  To configure this PR for making the shared library and the bitcoin-chainstate and test_kernel utilities available:
  ```
  cmake -B build -DBUILD_KERNEL_LIB=ON -DBUILD_UTIL_CHAINSTATE=ON
  ```

  Once compiled the library is part of the build artifacts that can be installed with:
  ```
  cmake --install build
  ```

  #### Why a C header (and not a C++ header)

  * Shipping a shared library with a C++ header is hard, because of name mangling and an unstable ABI.
  * Mature and well-supported tooling for integrating C exists for nearly every popular language.
  * C offers a reasonably stable ABI

  Also see #30595 (comment).

  #### What about versioning?

  The header and library are still experimental and I would expect this to remain so for some time, so best not to worry about versioning yet.

  #### Potential future additions

  In future, the C header could be expanded to support (some of these have been roughly implemented):

  * Handling transactions, block headers, coins cache, utxo set, meta data, and the mempool
  * Adapters for an abstract coins store
  * Adapters for an abstract block store
  * Adapters for an abstract block tree store
  * Allocators and buffers for more efficient memory usage
  * An "[io-less](https://sans-io.readthedocs.io/how-to-sans-io.html)" interface
  * Hooks for an external mempool, or external policy rules

  #### Current drawbacks

  * For external applications to read the block index of an existing Bitcoin Core node, Bitcoin Core needs to shut down first, since leveldb does not support reading across multiple processes. Other than migrating away from leveldb, there does not seem to be a solution for this problem. Such a migration is implemented in #32427.
  * The fatal error handling through the notifications is awkward. This is partly improved through #29642.
  * Handling shared pointers in the interfaces is unfortunate. They make ownership and freeing of the resources fuzzy and poison the interfaces with additional types and complexity. However, they seem to be an artifact of the current code that interfaces with the validation engine. The validation engine itself does not seem to make extensive use of these shared pointers.
  * If multiple instances of the same type of objects are used, there is no mechanism for distinguishing the log messages produced by each of them. A potential solution is #30342.
  * The background leveldb compaction thread may not finish in time leading to a non-clean exit. There seems to be nothing we can do about this, outside of patching leveldb.

ACKs for top commit:
  alexanderwiederin:
    re-ACK 6c7a34f
  stringintech:
    re-ACK 6c7a34f
  laanwj:
    Code review ACK 6c7a34f
  ismaelsadeeq:
    reACK 6c7a34f 👾
  fanquake:
    ACK 6c7a34f - soon we'll be running bitcoin (kernel)

Tree-SHA512: ffe7d4581facb7017d06da8b685b81f4b5e4840576e878bb6845595021730eab808d8f9780ed0eb0d2b57f2647c85dcb36b6325180caaac469eaf339f7258030
@JoseSK999
Copy link
Member

JoseSK999 commented Nov 24, 2025

Only for Windows cross-testing, you may need to add a CI job like this after restoring Rust cache (and before building):

      - name: Create stub fuzz/test_runner.py for libbitcoinkernel-sys
        if: runner.os == 'Windows'
        shell: bash
        run: |
          # Ensure the crate is downloaded
          cargo fetch --locked

          REGISTRY_SRC="$HOME/.cargo/registry/src"

          # Find any libbitcoinkernel-sys-* crate
          CRATE_ROOT="$(find "$REGISTRY_SRC" -maxdepth 5 -type d -name 'libbitcoinkernel-sys-*' -print -quit)"

          if [ -z "$CRATE_ROOT" ]; then
            echo "libbitcoinkernel-sys crate not found; skipping stub."
            exit 0
          fi

          mkdir -p "$CRATE_ROOT/bitcoin/test/fuzz"

          cat > "$CRATE_ROOT/bitcoin/test/fuzz/test_runner.py" << 'EOF'
          #!/usr/bin/env python3
          # CI stub to satisfy CMake CREATE_LINK on Windows; not executed by lib build.
          if __name__ == "__main__":
              raise SystemExit(0)
          EOF

As only Windows requires the bitcoin/test/fuzz/test_runner.py file to exist for some reason, and it isn't vendored.

@Davidson-Souza Davidson-Souza force-pushed the libbitcoinkernel branch 2 times, most recently from 25abb14 to f9f791d Compare November 24, 2025 19:39
@JoseSK999
Copy link
Member

libbitcoinconsensus still compiled somehow, we must ensure it's not compiled when kernel is built

@Davidson-Souza
Copy link
Member Author

libbitcoinconsensus still compiled somehow, we must ensure it's not compiled when kernel is built

Fixed this in the latest push, there was a dev dependency of bitcoin with the bitcoinconsensus feature on

@JoseSK999
Copy link
Member

JoseSK999 commented Nov 24, 2025

Need to fix clippy. The windows error is what I expected, easiest solution is my CI job above (create a fake file so Windows CMake shuts up).

  --- stderr
  CMake Error at test/CMakeLists.txt:47 (file):
    file Cannot hard link
    'C:/Users/runneradmin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libbitcoinkernel-sys-0.1.0/bitcoin/test/fuzz/test_runner.py'
    as it does not exist.

@sedited
Copy link

sedited commented Nov 24, 2025

The windows error is what I expected, easiest solution is my CI job above (create a fake file so Windows CMake shuts up).

This seemed like a packaging issue to me. I did a 0.1.1 release of the kernel crate in response to it. Does that fix it?

@JoseSK999
Copy link
Member

Yes! Now Windows CI successfully compiles (without the ugly workaround), thanks @TheCharlatan

Copy link
Collaborator

@jaoleal jaoleal left a comment

Choose a reason for hiding this comment

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

This really needs a rebase

@Davidson-Souza Davidson-Souza force-pushed the libbitcoinkernel branch 4 times, most recently from 38197e7 to 33ca6c6 Compare November 26, 2025 16:59
Davidson-Souza added a commit that referenced this pull request Dec 15, 2025
f25f867 docs(nix): Reflect nix changes on docs. (jaoleal)
19e3929 feat(nix): add commitizen under for git hooks inside nix devshell. (jaoleal)
910f161 chore(nix): remove nix packaging and simplify nix code. (jaoleal)

Pull request description:

  ### Description and Notes

  Part of #686.

  [Packaging should be done apart from this repo](#686 (comment)), im moving the nix packaging to https://github.com/jaoleal/floresta-flake, which already provides the same set of packages we were doing before.

  This should help #456.

  The first commit focuses totally in totally removing any nix native building in what regards our rust code.
  Ill introduce another two that

  [X] - Fixes devshell.
  [ ] - ~Bring rust checks back together~.
  [X] - Checks on git hooks.

  This should superseed #685, its changes will slip together here.

  ### Contributor Checklist

  <!-- Please remove this section once you've confirmed all items -->

  - [ ] I've followed the [contribution guidelines](https://github.com/vinteumorg/Floresta/blob/master/CONTRIBUTING.md)
  - [X] I've verified one of the following:
    - Ran `just pcc` (recommended but slower)
    - Ran `just lint-features '-- -D warnings' && cargo test --release`
    - Confirmed CI passed on my fork
  - [X] I've linked any related issue(s) in the sections above

  Finally, you are encouraged to sign all your commits (it proves authorship and guards against tampering—see [How (and why) to sign Git commits](https://withblue.ink/2020/05/17/how-and-why-to-sign-git-commits.html) and [GitHub's guide to signing commits](https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits)).

ACKs for top commit:
  Davidson-Souza:
    re-ACK f25f867
  JoseSK999:
    re-ACK f25f867
  luisschwab:
    ACK f25f867

Tree-SHA512: 9ed90742b8a02feee751ca22a2e76ed7a1b999fa4e21b69daecb07c6035fd276fb716001211561d0e6b4131872306a2231a10e576d2f1d1e0957e7087bbfc919
@Davidson-Souza Davidson-Souza force-pushed the libbitcoinkernel branch 2 times, most recently from 37f1451 to 19403b3 Compare December 15, 2025 18:07
@Davidson-Souza Davidson-Souza marked this pull request as ready for review December 15, 2025 18:47
@Davidson-Souza
Copy link
Member Author

Updated docs and install.sh with the new dependencies

@Davidson-Souza Davidson-Souza changed the title [WIP] chain: Replace bitcoinconsensus with bitcoinkernel feat(chain): Replace bitcoinconsensus with bitcoinkernel Dec 15, 2025
@Davidson-Souza
Copy link
Member Author

Pushed 4cfef7b:

  • removing space in Cargo.toml
  • updating stale comment
  • Removing duplicated function

@jaoleal
Copy link
Collaborator

jaoleal commented Dec 18, 2025

ACK 4cfef7b

libbitcoinconsensus is deprecated, and have a suboptimal performance due
to deserializing transactions for every input, even if we are validating
them in sequence. Libbitcoinkernel has a wrapper type for transactions,
and we pass a & ref to it. We can then create the tx once and pass it as
reference to validate each input. This causes a massive perf gain during
script validation.

This commit replaces all mentions to libbitcoinconsensus, and replaces
with kernel where neede. Notice that we can't use both, because of name
clashing on linking time.
The debian version we use in our Dockerfile ships with a super old
gcc that doesn't have C++20. This commit updates to the latest
stable release (13.2)
@Davidson-Souza
Copy link
Member Author

Applied minor changes suggested by @JoseSK999

@jaoleal
Copy link
Collaborator

jaoleal commented Dec 23, 2025

ACK 9b64255

Copy link
Member

@luisschwab luisschwab left a comment

Choose a reason for hiding this comment

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

ACK 9b64255

Successfully IBD'd on signet

Copy link
Member

@JoseSK999 JoseSK999 left a comment

Choose a reason for hiding this comment

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

ACK 9b64255; successfully synced with --assume-valid 0

@Davidson-Souza Davidson-Souza merged commit 063f4a8 into getfloresta:master Dec 23, 2025
9 checks passed
@luisschwab
Copy link
Member

BOOM! Nice Xmas gift

image

@luisschwab
Copy link
Member

@Davidson-Souza do you have any performance benchmarks comparing consensus and kernel?

@Davidson-Souza
Copy link
Member Author

@Davidson-Souza do you have any performance benchmarks comparing consensus and kernel?

I wrote some results for the upstream core PR (bitcoin/bitcoin#30595 (comment)). Other than that, I do have some perf records laying around, but never published anything.

@luisschwab
Copy link
Member

Oh wow, that's an insane gain, especially for block 367891.

Davidson-Souza added a commit that referenced this pull request Jan 11, 2026
…oin kernel and re export packages

652a5d4 feat(nixflake): Consume a build expression that supports bitcoin kernel and re export packages. (jaoleal)

Pull request description:

  ### Description and Notes

  we had some previous changes moving the build expressions outside this repo.

  I kept it simple so we could work without worries about packaging dealing but what i did was about to be broken by bitcoin kernel.

  This Pr consumes the build expression from floresta-flake which now supports bitcoin kernel.

  Depends on #456

  ### How to verify the changes you have done?

  This branch builds on top of the #456, check out on it to ensure that it contain the changes that introduced the bitcoin kernel

  nix flake check --all-systems # Will do static evaluation on the flake.

  nix build # will build the default package, which is the same as just `cargo build` on the workspace.

  ### Contributor Checklist

  <!-- Please remove this section once you've confirmed all items -->

  - [X] I've followed the [contribution guidelines](https://github.com/vinteumorg/Floresta/blob/master/CONTRIBUTING.md)
  - [X] I've verified one of the following:
    - Ran `just pcc` (recommended but slower)
    - Ran `just lint-features '-- -D warnings' && cargo test --release`
    - Confirmed CI passed on my fork
  - [X] I've linked any related issue(s) in the sections above

  Finally, you are encouraged to sign all your commits (it proves authorship and guards against tampering—see [How (and why) to sign Git commits](https://withblue.ink/2020/05/17/how-and-why-to-sign-git-commits.html) and [GitHub's guide to signing commits](https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits)).

ACKs for top commit:
  joaozinhom:
    tACK 652a5d4
  moisesPompilio:
    crACK 652a5d4
  JoseSK999:
    re-ACK 652a5d4
  Davidson-Souza:
    re-ACK 652a5d4

Tree-SHA512: 3d00629664b3fd4c6a9fef84d636f3811fa5a0020e2c6bbace009be94a7961e597d6f74b02daecd53f5aa794a637eeb4f3963a065685e28d969902acf1953d6a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Cleaning, refactoring, reducing complexity consensus This changes something inside our consensus implementation dependencies Pull requests that update a dependency file performance This is a performance-related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants