-
Notifications
You must be signed in to change notification settings - Fork 79
feat(chain): Replace bitcoinconsensus with bitcoinkernel #456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(chain): Replace bitcoinconsensus with bitcoinkernel #456
Conversation
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. |
b0ff8ca to
8c373f9
Compare
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
8c373f9 to
6e82385
Compare
|
Only for Windows - 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)
EOFAs only Windows requires the |
25abb14 to
f9f791d
Compare
|
|
f9f791d to
570d592
Compare
Fixed this in the latest push, there was a dev dependency of |
|
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. |
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? |
|
Yes! Now Windows CI successfully compiles (without the ugly workaround), thanks @TheCharlatan |
jaoleal
left a comment
There was a problem hiding this 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
38197e7 to
33ca6c6
Compare
90cf9fd to
81e7c78
Compare
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
37f1451 to
19403b3
Compare
|
Updated docs and |
19403b3 to
4cfef7b
Compare
|
Pushed 4cfef7b:
|
|
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)
4cfef7b to
9b64255
Compare
|
Applied minor changes suggested by @JoseSK999 |
|
ACK 9b64255 |
luisschwab
left a comment
There was a problem hiding this 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
JoseSK999
left a comment
There was a problem hiding this 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 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. |
|
Oh wow, that's an insane gain, especially for block 367891. |
…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

What is the purpose of this pull request?
Which crates are being modified?
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.