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

Auto deploy regtest nodes in background #65

Merged
merged 3 commits into from
Apr 28, 2022

Conversation

rajarshimaitra
Copy link
Contributor

@rajarshimaitra rajarshimaitra commented Dec 20, 2021

Description

Fixes #55. I decided to go for one feature flag for each backends as they made dependency management easier. Because different backend will have different dependency, and we don't wanna have redundant ones. Also it makes the commands a little shorter.

Notes to the reviewers

Right now its not that useful because the backend APIs are not exposed. So I can't really do much other than sync/send in the command line. (unlike testnet we cannot send coins to bdk from outside of local regtest node). So to make it useful in repl mode as described in the issue, we also have to expose the blockchain apis in bdk-cli command in some way (at least the basic stuffs, address, send, generate). Looking for suggestions on this.

usage looks like this:

cargo run --features regtest-rpc -- wallet --descriptor "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)" sync
{}

Although I am having trouble connecting to the esplora end of electrs. Not sure whats the issue, will have to look more.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@rajarshimaitra
Copy link
Contributor Author

Fixed merge conflict with some enhancements.

@rajarshimaitra rajarshimaitra force-pushed the regtest-node branch 5 times, most recently from f0332f1 to ac2a6c2 Compare January 9, 2022 17:34
@rajarshimaitra
Copy link
Contributor Author

Updated the commit history for better structuring.

@notmandatory notmandatory added good first issue Good for newcomers and removed good first issue Good for newcomers labels Feb 1, 2022
@notmandatory
Copy link
Member

@rajarshimaitra Looks like this one needs a rebase or if you're tied up I can do the rebase but would have to rewrite your commits. Once that's done I'll do some testing and see if I can figure out how to expose the core rpc in repl mode.

@rajarshimaitra
Copy link
Contributor Author

Rebased and resolved conflicts..

@notmandatory
Copy link
Member

notmandatory commented Feb 13, 2022

I spent some time today testing and reviewing and this is a great start! There are still the two (related) issues you found:

  • 1. no way to control the bitcoind node (ie. create new blocks, get regtest coins)
  • 2. sync doesn't work for esplora

I believe issue 2 is because electrsd won't sync until at least one block is created by bitcoind; so if we fix 1 we should also fix 2. My idea for fixing 1 is to change the bitcoind lib to allow us to set a reusable (not randomly generated tmp) directory for the bitcoind data dir; and maybe the same for electrsd. Then we can add some simple RPC commands to bdk-cli to update the bitcoind (and electrsd) database and that data will persist for future invocations of bdk-cli.

But for now as long as we add the comment that the regtest-* features are experimental this should be OK to merge. Then we have more time to to play with and get merged any PRs to bitcoind (and electrsd) that we need to have it all working end to end.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

Add some little experimental warnings and this is good to go.

CHANGELOG.md Outdated Show resolved Hide resolved
.github/workflows/cont_integration.yml Show resolved Hide resolved
@rajarshimaitra
Copy link
Contributor Author

Thanks @notmandatory .. Updated as per suggested and rebased..

@rajarshimaitra
Copy link
Contributor Author

Rebased on latest master.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

Also need to do a cargo fmt.

src/lib.rs Outdated Show resolved Hide resolved
This adds bitcoind and electrsd deployment and management for each kind
of feature flags. The wallet then gets connected to the backend. All the
backend related args for bdk-cli can be omitted in regtest-* mode.
@rajarshimaitra
Copy link
Contributor Author

  • Reverted back default network to testnet
  • Fixed test failure

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 930b0c4

@notmandatory notmandatory merged commit bd510e2 into bitcoindevkit:master Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add options to start regtest bitcoind and electrsd
2 participants