Skip to content
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

Batch tx requests #1759

Merged
merged 11 commits into from
Feb 20, 2023
Merged

Batch tx requests #1759

merged 11 commits into from
Feb 20, 2023

Conversation

andrewtoth
Copy link
Contributor

@andrewtoth andrewtoth commented Feb 15, 2023

Fixes https://github.com/casey/ord/issues/1364 by reverting #1357.

This PR uses hyper to fetch raw transactions in the background with batched JSON-RPC requests sent 12 at a time concurrently. This saturates the default 4 rpc threads but doesn't exceed the default rpcworkqueue of 16. I managed to sync to block 776774 in 41 minutes.

This does not require any configuration changes. It uses the json-rpc endpoint, but more efficiently. All missing inputs are sent to a separate thread before indexing, and the resulting input values are sent back via a channel in order.

Copy link
Collaborator

@veryordinally veryordinally left a comment

Choose a reason for hiding this comment

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

@raphjaph @casey Reviewed conceptually and talked to @andrewtoth - this approach looks consistent with what we discussed yesterday. Does not introduce new communication protocol, and massively speeds up the indexing process for when we have inscriptions (from a day+ to less than an hour).

Let's discuss and see if you also ACK this approach, then @andrewtoth can complete this PR.

@raphjaph
Copy link
Collaborator

Concept ACK from @veryordinally, @casey, and @raphjaph (reviewing together on a Discord call).

This seems like the right approach. It doesn't require any config changes and doesn't change our security model.

Would be great to get the MVP in! Should be as simple as possible, with no configuration or additional knobs. Do not worry about polishing too much. Just get full test coverage, undraft, and request review.

@andrewtoth andrewtoth marked this pull request as ready for review February 16, 2023 22:33
@andrewtoth
Copy link
Contributor Author

@veryordinally @raphjaph @casey fixed broken test, ready for review.

Also, #1637 and #1731 are relevant to this as well. Since we're using hyper, the json-rpc index.client that is created at the start times out and fails when trying to fetch block count again. Same with json-rpc clients created in subcommands before a long update.

Copy link
Collaborator

@raphjaph raphjaph left a comment

Choose a reason for hiding this comment

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

Reviewed by the peanut gallery in Discord: We are confident this fixes the issue but we would love any comments that will help us understand and maintain this code. If you could do that in addition to our review comments (if they are easy), we can smash merge ❤️

Cargo.toml Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index/tx_fetcher.rs Outdated Show resolved Hide resolved
src/index/tx_fetcher.rs Outdated Show resolved Hide resolved
src/index/tx_fetcher.rs Outdated Show resolved Hide resolved
src/index/tx_fetcher.rs Outdated Show resolved Hide resolved
.unwrap()
.cmp(&b.id.parse::<usize>().unwrap())
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

add an assert if it's critical that it's sorted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not sure what I would be asserting here? It needs to be sorted before we return, but this is sorting it so we know it's sorted?

src/index/tx_fetcher.rs Outdated Show resolved Hide resolved
src/index/tx_fetcher.rs Outdated Show resolved Hide resolved
src/index/updater.rs Outdated Show resolved Hide resolved
@andrewtoth
Copy link
Contributor Author

andrewtoth commented Feb 18, 2023

@raphjaph addressed most of your feedback, asked some questions for clarification on some things.
I added comments to parts of the code that I think could use them. Let me know if there are any other places that could benefit from comments.

@andrewtoth andrewtoth requested review from veryordinally and raphjaph and removed request for raphjaph and veryordinally February 18, 2023 21:10
@kvnn
Copy link

kvnn commented Feb 20, 2023

I managed to sync to block 776774 in 41 minutes.
@andrewtoth Nice! Do you mind sharing the machine specs?

@andrewtoth
Copy link
Contributor Author

andrewtoth commented Feb 20, 2023

@kvnn running pop_os with Intel® Core™ i7-10870H CPU @ 2.20GHz × 16, with nvme drive. Bitcoind was running locally on the same machine.

Did a test with the same machine but connecting over wifi to a bitcoind on a separate machine in the local network. Took 178 minutes to get to the same block.

src/index.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@raphjaph raphjaph left a comment

Choose a reason for hiding this comment

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

Thanks for addressing our comments! Merging this now.

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.

Re-enable skipping transactions below first inscription height
5 participants