Skip to content

[WIP] 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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[WIP] chain: Replace bitcoinconsensus with bitcoinkernel #456

wants to merge 1 commit into from
+216 −78

Conversation

Davidson-Souza
Copy link
Member

@Davidson-Souza Davidson-Souza commented Apr 22, 2025 •
edited
Loading

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.

Notes to the reviewers

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 cstr_count_bytes 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.

Checklist

  • I've signed all my commits
  • I ran just lint
  • I ran cargo test
  • I've checked the integration tests
  • I've followed the contribution guidelines
  • I'm linking the issue being fixed by this PR (if any)

@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 😂

@JoseSK999 JoseSK999 mentioned this pull request Apr 23, 2025
Draft
21 tasks
@TheCharlatan TheCharlatan mentioned this pull request Apr 24, 2025
Open
@TheCharlatan
Copy link

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 •
edited
Loading

@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 3 times, most recently from 254707f to b0ff8ca Compare May 7, 2025 17:50
@Davidson-Souza
chain: Replace bitcoinconsensus with bitcoinkernel
8c373f9 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Davidson-Souza @TheCharlatan