Skip to content

Bitcoin slices integration#913

Merged
romanz merged 1 commit into
romanz:masterfrom
RCasatta:poc_bitcoin_slices
Aug 22, 2023
Merged

Bitcoin slices integration#913
romanz merged 1 commit into
romanz:masterfrom
RCasatta:poc_bitcoin_slices

Conversation

@RCasatta
Copy link
Copy Markdown
Contributor

@RCasatta RCasatta commented Jul 26, 2023 •
edited by romanz
Loading

This is a proof of concept using bitcoin_slices since it seems a good fit.

Parsing a block with rust_bitcoin is expensive because of allocating all the needed data, since many times you are interested only in a subset of it, we can skip most of the work, for example, to find a transaction.

UPDATED: added visiting of inputs and outputs via bitcoin_slices since the idea seems interesting for the maintainer

Fixes #864.

RCasatta
RCasatta commented Jul 26, 2023
View reviewed changes
Comment thread src/tracker.rs Outdated
@romanz romanz self-requested a review July 27, 2023 05:15
@romanz
Copy link
Copy Markdown
Owner

romanz commented Jul 27, 2023

Good idea, thanks!

@RCasatta RCasatta force-pushed the poc_bitcoin_slices branch from 4ce50a7 to d7c06e7 Compare July 28, 2023 14:31
@RCasatta
Copy link
Copy Markdown
Contributor Author

RCasatta commented Jul 28, 2023 •
edited
Loading

these are profiles while running ./history.py 1FeexV6bAHb8ybZjqQMjJrcCrHGW9sb6uF

before: https://share.firefox.dev/3KCBdPV

after: https://share.firefox.dev/3KkfHPE

About 3 seconds are shaved off, but most of the time only the main thread is doing something, not sure what. The section finding inputs and outputs looks much faster

@RCasatta RCasatta force-pushed the poc_bitcoin_slices branch from d7c06e7 to 7fdbbf4 Compare July 28, 2023 14:33
@RCasatta
Copy link
Copy Markdown
Contributor Author

RCasatta commented Jul 28, 2023 •
edited
Loading

the main thread is doing something, not sure what.

found out, is getting blocks, for creating merkle proofs

antonilol
antonilol reviewed Jul 29, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@antonilol antonilol left a comment

Choose a reason for hiding this comment

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

always in for performance improvements! here are a few things i found while reading through the code

Comment thread src/p2p.rs Outdated
Comment thread src/status.rs Outdated
@RCasatta RCasatta marked this pull request as ready for review August 3, 2023 11:48
@RCasatta
Copy link
Copy Markdown
Contributor Author

RCasatta commented Aug 3, 2023 •
edited
Loading

needs some squashing, will do it after @romanz review (or before if asked)

@RCasatta RCasatta changed the title Bitcoin slices usage proof of concept Bitcoin slices integration Aug 3, 2023
@RCasatta RCasatta force-pushed the poc_bitcoin_slices branch from e80a963 to e3ad9a1 Compare August 3, 2023 11:57
@romanz
Copy link
Copy Markdown
Owner

romanz commented Aug 3, 2023 •
edited
Loading

Will review this weekend, thanks @RCasatta and @antonilol!

P.S.
Could you please fix the test build issue at https://github.com/romanz/electrs/actions/runs/5750390115/job/15587072304?pr=913?

@RCasatta RCasatta force-pushed the poc_bitcoin_slices branch 2 times, most recently from 8bdaae6 to d0a33bc Compare August 4, 2023 07:53
@RCasatta
Copy link
Copy Markdown
Contributor Author

RCasatta commented Aug 4, 2023

Could you please fix the test build issue at https://github.com/romanz/electrs/actions/runs/5750390115/job/15587072304?pr=913?

Fixed

@romanz romanz mentioned this pull request Aug 5, 2023
Closed
shaurya947
shaurya947 reviewed Aug 5, 2023
View reviewed changes
Comment thread Cargo.toml Outdated
Comment thread src/p2p.rs Outdated
romanz added a commit that referenced this pull request Aug 5, 2023
@romanz
Fixup Cargo.toml
2e6191e 
Following #913 (comment)
romanz
romanz requested changes Aug 6, 2023
View reviewed changes
Copy link
Copy Markdown
Owner

@romanz romanz left a comment

Choose a reason for hiding this comment

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

LGTM, with a few suggested changes:
RCasatta/electrs@poc_bitcoin_slices...romanz:electrs:poc_bitcoin_slices
WDYT?

@antonilol antonilol mentioned this pull request Aug 19, 2023
Open
@RCasatta
Copy link
Copy Markdown
Contributor Author

RCasatta commented Aug 22, 2023

Sorry, for taking some time but I was AFK.
Every change looks good to me.

utACK a716847

@romanz romanz self-requested a review August 22, 2023 16:14
romanz
romanz approved these changes Aug 22, 2023
View reviewed changes
Copy link
Copy Markdown
Owner

@romanz romanz left a comment

Choose a reason for hiding this comment

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

Many thanks @RCasatta!

@romanz romanz force-pushed the poc_bitcoin_slices branch from a716847 to 84f1c3e Compare August 22, 2023 16:46
@RCasatta @romanz
Optimize index querying via bitcoin_slices
2ea0208 
Co-authored-by: Roman Zeyde <me@romanzey.de>
@romanz romanz force-pushed the poc_bitcoin_slices branch from 84f1c3e to 2ea0208 Compare August 22, 2023 17:03
@romanz
Copy link
Copy Markdown
Owner

romanz commented Aug 22, 2023

Squashed and rebased over latest master (due to #922)

@romanz romanz merged commit acb4997 into romanz:master Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romanz romanz romanz approved these changes

+2 more reviewers

@shaurya947 shaurya947 shaurya947 left review comments

@antonilol antonilol antonilol left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature: consider using bitcoin_slices for parsing

4 participants

@RCasatta @romanz @shaurya947 @antonilol