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

Speed up ledger-from-archive #818

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sugargoat
Copy link
Contributor

@sugargoat sugargoat commented May 13, 2021

Soundtrack of this PR: I love it

Motivation

Ledger-from-archive is painfully slow, especially on TestNet at 160K blocks currently

In this PR

  • Borrows structure from ledger_sync_service to pull blocks in multi-threaded fashion

Future Work

  • Watcher may also benefit from this

@sugargoat sugargoat force-pushed the ledger-from-archive-speedup branch from 86c1432 to d19db7e Compare May 13, 2021 04:57
@sugargoat sugargoat force-pushed the ledger-from-archive-speedup branch from d19db7e to c98ff92 Compare May 13, 2021 04:59
joekottke
joekottke previously approved these changes May 13, 2021
Copy link
Contributor

@joekottke joekottke left a comment

Choose a reason for hiding this comment

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

LGTM, based on discussions. Not checking for Rust completeness.

loop {
let end = start + config.block_chunk_size;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be min(start + config.block_chunk_size, config.num_blocks.unwrap_or(u64::maxint))?

err
);
return;
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

.expect

///
/// Peers are queried concurrently. Currently, this method will run indefinitely
/// until all transactions have been retrieved.
fn get_block_contents(
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to copy this entire thing instead of making the one in ledger_sync_service pub?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I see that it is less strict so using the one in ledger_sync_service would be difficult.

);
match transactions_fetcher.block_from_url(&url) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Replacing this call with transactions_fetcher.get_block_data_by_index(block_index, None) should be sufficient for benefiting from the merged blocks and getting the speed increase you are looking for.

@remoun
Copy link
Contributor

remoun commented Apr 12, 2022

This still seems useful; do we want to merge it?

@jcape jcape added the help wanted Extra attention is needed label Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

None yet

5 participants