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

Add Mempool indexer #50

Merged
merged 9 commits into from
Jul 16, 2024
Merged

Add Mempool indexer #50

merged 9 commits into from
Jul 16, 2024

Conversation

will-bitlight
Copy link
Contributor

This PR adapts bp-wallet to the changes made in bp-invoice for Testnet4 support.

Additionally, it introduces support for the mempool Indexer (or Resolver), ensuring compatibility with the any-network.

If you have any suggestions, please leave your valuable comments, I will actively cooperate with the revision, thank you very much

@dr-orlovsky dr-orlovsky self-requested a review July 13, 2024 09:02
@dr-orlovsky dr-orlovsky added the enhancement New feature or request label Jul 13, 2024
@dr-orlovsky dr-orlovsky added this to the v0.11.0 milestone Jul 13, 2024
@dr-orlovsky
Copy link
Member

Thank you!

One small thing: can you pls here and in the other PRs use [patch.crates-io] section in the Cargo.toml instead of changing the dependencies right in place?

Copy link

codecov bot commented Jul 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 62 lines in your changes missing coverage. Please review.

Project coverage is 0.0%. Comparing base (0a81832) to head (07bf47f).
Report is 42 commits behind head on master.

Files Patch % Lines
src/indexers/mempool.rs 0.0% 21 Missing ⚠️
src/indexers/esplora.rs 0.0% 16 Missing ⚠️
src/cli/args.rs 0.0% 13 Missing ⚠️
src/indexers/any.rs 0.0% 12 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master    #50    +/-   ##
======================================
  Coverage     0.0%   0.0%            
======================================
  Files          11     16     +5     
  Lines        1165   1436   +271     
======================================
- Misses       1165   1436   +271     
Flag Coverage Δ
rust 0.0% <0.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Since 80% of the code is duplicated with esplora, I think this should be re-factored just as a wrapper around explora client re-using it

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@will-bitlight
Copy link
Contributor Author

Thank you!

One small thing: can you pls here and in the other PRs use [patch.crates-io] section in the Cargo.toml instead of changing the dependencies right in place?

Thank you for your advice. I will make timely changes through [patch.crates-io]

@will-bitlight
Copy link
Contributor Author

Since 80% of the code is duplicated with esplora, I think this should be re-factored just as a wrapper around explora client re-using it

Understood, the code looks like it can be reused a lot, I try to make a wrapper, streamline the code

@will-bitlight
Copy link
Contributor Author

The refactored code now reuses most of the esplora code and removes unnecessary dependencies. Additionally, I've updated the [patch.crates-io] section. If have any further suggestions, please let me know : )

@dr-orlovsky dr-orlovsky requested a review from zoedberg July 15, 2024 10:14
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

I like it! One small addition - and from my side we are good to go.

Added @zoedberg to review as well

PS. Can you also pls run cargo +nightly fmt --all?

src/cli/args.rs Outdated Show resolved Hide resolved
@dr-orlovsky dr-orlovsky changed the title Support Testnet4 Network Add Mempool indexer Jul 15, 2024
@dr-orlovsky
Copy link
Member

We also need to fix broken cli feature build: https://github.com/BP-WG/bp-wallet/actions/runs/9937214077/job/27448310034?pr=50

@will-bitlight
Copy link
Contributor Author

I like it! One small addition - and from my side we are good to go.

Added @zoedberg to review as well

PS. Can you also pls run cargo +nightly fmt --all?

Thanks for your suggestion, I will execute 'fmt' simultaneously.

When I execute 'fmt' during development, it will automatically 'fmt' other code, I am worried that it will cause inconvenience to the repo, so fmt is very cautious

@will-bitlight
Copy link
Contributor Author

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK 07bf47f

@dr-orlovsky
Copy link
Member

dr-orlovsky commented Jul 15, 2024

When I try to use either explore or mempool I somehow get no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point which wasn't happening before. Any idea why this happens?

For instance, it happens when running

$ bp create -n testnet4 --mempool --tr-key-only "[a181fad6/86h/1h/0h]tpubDC8SnPn5p7kso1Wp2wYyziUgisECwdFQQyVUu2viNttcxUFzuHcW1E1UXiETHBCD3YKmoB2oXMkGUnmKsehWtzTK1DhQXmyQ8RoaCDjW3Zc/<0;1>/*" testTrKeyonly

@will-bitlight
Copy link
Contributor Author

will-bitlight commented Jul 15, 2024

no process-level CryptoProvider available

I tried to run the following command in my local, and the output is as follows

Loading descriptor from command-line argument
SyncingSyncing keychain 0 .......... keychain 1 .......... success
Saving the wallet as 'testTrKeyonly' ... success

I see that this error is coming from rustls ,
Sorry, I found that ureq also has rustls , I tried to re-examine the problem. Currently my local can use the normal command, I tried to look up rustls issues for clues

@will-bitlight
Copy link
Contributor Author

rustls/rustls#1938 (comment)

Find a solution because a local library is missing, usually due to developers using a new test environment.

I'll try and see what else I can find

@dr-orlovsky
Copy link
Member

dr-orlovsky commented Jul 15, 2024

Thank you for your help. I know it is unrelated to your PR. Strange it wasn't happening before with esplora...

@will-bitlight
Copy link
Contributor Author

will-bitlight commented Jul 15, 2024

Thank you for your help. I know it is unrelated to your PR. Strange it wasn't happening before with esplora...

I think I may have found the reason, because bp-esplora's dependency ureq was upgraded 9 days ago. After its upgrade it relies on rustls 0.23, currently only rustls 0.23 has this error.

And before we rely on ureq relies on the rustls https://github.com/algesten/ureq/blob/2.9.7/Cargo.lock#L562 0.22, will not appear this error.

all cause by rustls 0.23:
rustls/rustls#1938
https://mygit.osfipin.com/release/144217327

@dr-orlovsky
Copy link
Member

Perfect, thank you! After cargo update -p ureq --precise 2.9.7 everything Is back to normal

@will-bitlight
Copy link
Contributor Author

Perfect, thank you! After cargo update -p ureq --precise 2.9.7 everything Is back to normal

Very excited, it's all coming back! :)

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

tACK - I was able to use all these changes with actual wallet

Cargo.toml Outdated
Comment on lines 72 to 73
all = ["electrum", "esplora", "fs", "cli", "clap", "log"]
cli = ["base64", "env_logger", "clap", "shellexpand", "fs", "serde", "electrum", "esplora", "log"]
all = ["electrum", "mempool", "fs", "cli", "clap", "log"]
cli = ["base64", "env_logger", "clap", "shellexpand", "fs", "serde", "electrum", "mempool", "log"]
Copy link
Contributor

Choose a reason for hiding this comment

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

For now this is correct because mempool is just an alias for esplora, but one day the mempool feature could change (it could drop esplora) and by mistake we might forget to re-include esplora to all/cli features. Therefore I suggest adding mempool to all and cli instead of replacing esplora with mempool

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

tACK 1604ca1

I've added few improvements, including @zoedberg suggestion. As we discussed with her I am merging this now

@dr-orlovsky dr-orlovsky merged commit b63b7ac into BP-WG:master Jul 16, 2024
19 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants