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

Make request to esplora in parallel #178

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

RCasatta
Copy link
Member

At the moment the Esplora blockchain source, used in our playground, is making non-blocking requests but they are not parallelized.
This allows specifying the desired concurrency level when using this blockchain source.
I went for a default of 4, which is pretty conservative but library users and our repl could obviously use a custom value.

Test with --esplora_concurrency X

RUST_LOG=info cargo run --release --example repl --features cli-utils,electrum,esplora -- --esplora https://blockstream.info/testnet/api --esplora_concurrency 10 --wallet change_deep --descriptor "wpkh(tpubD6NzVbkrYhZ4WrmVQXmpVcPoh1LUtuBaVLJ3PDPNEaFpw5iiBW9GwkMmtkpT3ucpN5G64Tfj9EAGVGkXrYFrpdBdvp8RMDaKhtoEp5cBDWY/0/*)" --change_descriptor "wpkh(tpubD6NzVbkrYhZ4WrmVQXmpVcPoh1LUtuBaVLJ3PDPNEaFpw5iiBW9GwkMmtkpT3ucpN5G64Tfj9EAGVGkXrYFrpdBdvp8RMDaKhtoEp5cBDWY/1/*)" sync  --max_addresses 200
concurrency time for first sync
1 59s
4(default) 16s
10 8s 

.await?)
let mut results = vec![];
for chunk in ChunksIterator::new(scripts.into_iter(), self.concurrency as usize) {
let mut futs = FuturesOrdered::new();
Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, FuturesUnordered could squeeze performance a little bit, but I thought the increased code complexity wasn't worth the effort

@@ -52,7 +52,7 @@
//!
//! # #[cfg(feature = "esplora")]
//! # {
//! let esplora_blockchain = EsploraBlockchain::new("...");
//! let esplora_blockchain = EsploraBlockchain::new("...", None);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a good idea to just put "https://blockstream.info/testnet/api" as first argument so people can copy-paste from the documentation.

@justinmoon
Copy link
Contributor

justinmoon commented Nov 18, 2020

I'm getting this error when I try to run the example command you provided:

error: Found argument '--esplora_concurrency' which wasn't expected, or isn't valid in this context
        Did you mean --esplora?

It works if I update cli.rs like this:

    if cfg!(feature = "esplora") {
        app = app
            .arg(
                Arg::with_name("esplora")
                    .short("e")
                    .long("esplora")
                    .value_name("ESPLORA")
                    .help("Use the esplora server if given as parameter")
                    .takes_value(true),
            )
            // BELOW THIS IS NEW
            .arg(
                Arg::with_name("esplora_concurrency")
                    .long("esplora_concurrency")
                    .takes_value(true)
                    .help("Concurrent requests to esplora backend"),
            );
            // ABOVE THIS IS NEW
    }

Also seems like CI is stuck ...

@RCasatta
Copy link
Member Author

Oh thanks, it looks I forgot the last push :)

Copy link
Contributor

@justinmoon justinmoon left a comment

Choose a reason for hiding this comment

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

Looks good to me. With cli update it compiles and I see a similar speedup.

Copy link
Member

@afilini afilini left a comment

Choose a reason for hiding this comment

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

ACK c9079a7

@afilini afilini merged commit c9079a7 into bitcoindevkit:master Nov 19, 2020
nickfarrow pushed a commit to nickfarrow/bdk that referenced this pull request Feb 21, 2022
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.

3 participants