Update BDK version to 0.12.0 by rajarshimaitra · Pull Request #41 · bitcoindevkit/bdk-cli

rajarshimaitra · 2021-08-26T09:56:24Z

Description

This PR updates the upstream BDK version to latest release (v0.10.0).

Notes to the reviewers

The latest release of BDK includes some breaking changes, some of which are handled here. A previously available function in BDK, maintain_single_recipient() is changed into allow_shrinking() which requires a scriptpubkey to be specified. So far I couldn't find any public API in BDK using which a scriptpubkey (or the transaction) can be retrieved from the wallet given a TxID. Marked the code with a TODO. It probably needs some additional API in BDK. Made an issue (bitcoindevkit/bdk#427) to track this.

BDK esplora blockchain module has been divided into two types using-ureq and using-reqwest. reqwest is an async feature in BDK, which produces Future related errors in bdk-cli (#36 (comment)). Trying to solve that error creates more complexity (async related weridness). Thus decided to scrap reqwest and go with blocking ureq version, which I think is adequate for the usecases of bdk-cli.

Updated electrum cli args.

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

Updating the BDK version creates some breaking change. One of such change probably requires modification of BDK to expose a new function to fetch scriptpubkey from a txid. Used ureq version of esplora to remove async related complexities.

notmandatory · 2021-08-27T13:11:07Z


 [dependencies]
-bdk = { version = "^0.7", default-features = false, features = ["all-keys"]}
+bdk = { version = "0.10.0", default-features = false, features = ["all-keys"]}


I wasn't able to connect to the blockstream esplora server via https (and http redirects to https) because bdk disables ureq default features. @tcharding was disabling the tls feature intentional?
@rajarshimaitra If having the ureqtls feature disabled by default is intentional I think it makes sense to add a ureq-tls = [ "ureq/tls" ] feature to bdk so we can enable it from the bdk-cli project (or for others who need it).

Good Idea. Better to enable tls in ureq if we use it. IMO connecting https will be quite a generic requirement. Will open a PR if we all agree the same.
We don't need another feature flag for this, just add tls in ureq's feature list.

True, maybe best of have tls feature enabled by default in bdk, but first I'd like to get @tcharding's input since he originally added the ureq dependency and may have an argument for leaving tls off by default.

I have no problem with enabling TLS by default. As for why I disabled it, I can't remember exactly why, I probably just disabled default features during a debugging session and never turned TLS back on. Thanks for checking in with me and thanks for giving it the thought to realize we should enable it by default!

afilini · 2021-09-09T09:35:10Z

 repl = ["regex", "rustyline"]
 electrum = ["bdk/electrum"]
-esplora = ["bdk/esplora"]
+esplora = ["bdk/use-esplora-ureq"]


I'm trying to use this to test bitcoindevkit/bdk#429: to get bdk-cli to work in the browser we need a way to enable bdk/use-esplora-reqwest. I would suggest maybe adding another feature esplora-reqwest that we can use from wasm32

Or maybe even better: async-esplora

I think this PR may be blocked until @tcharding and @LLFourn work out things out with bitcoindevkit/bdk#433.

Yes. I am not really sure how big the httpclient fix would be. If it takes much longer, can we consider merging bitcoindevkit/bdk#430 in the mean time? #36 is also pending on this one.
And I think that will fix @afilini 's issue too.

In the meantime I made this branch just to test the playground: https://github.com/afilini/bdk-cli/tree/update-with-reqwest

rajarshimaitra · 2021-09-14T10:08:47Z

Added esplora-reqwest into bdk-cli.

@notmandatory we have to use bdk-master for now, until the esplora-fix is released upstream. This can be changed in next bdk release.

@afilini please check if this meets your requirement, or something more to be done? To use request compile with esplora-reqwest feature.

notmandatory · 2021-09-17T18:00:37Z

When bdk release 0.12.0 is out ~Sept-29 I think this one will be ready to merge. I've updated the title. 😉

rajarshimaitra · 2021-09-19T06:14:50Z

@notmandatory just FYI, the rest of the open PRs builds on top of this. So this needs to go first before they can be merged.. :)

notmandatory · 2021-10-01T22:49:29Z

I just made the bdk 0.12.0 release, so you're good to go on this one.

BDK v0.10.0 adds a stop_gap parameter to electrum config.

Esplora ureq version requires having new cli args.

Adds esplora-reqwest capability from bdk. Allows to use async https connections with esplora from bdk-cli.

Recent update in BDK esplora backend allows it to connect with SOCKS5 proxy. This change adds proxy option to esplora configuration. Updates Proxy_Opts feature guard to enable it for esplora feature.

rajarshimaitra · 2021-10-03T10:02:31Z

Rebased with bdk v0.12.

FYI, I am not updating #42 and #36 right now. For simplicity of merging this needs to be merged first. Then the rest two can be rebased on top of updated master. I think that will simplify the review process as all these PRs are interdependent.

This one is the biggest change among all other as it handles all the update from bdk v0.10 to v0.12. So keeping this one separate from review of the rest.

notmandatory

re-ACK f9a9606

I manually tested and works fine as long as correct esplora server URL is used. Either README needs to be updated, defaults need to be changed for testnet or maybe something else. Will merge this now so other PRs can proceed and we can fix the esplora server URL issue in a subsequent PR.

rajarshimaitra · 2021-08-26T09:56:24Z

Description

This PR updates the upstream BDK version to latest release (v0.10.0).

Notes to the reviewers

The latest release of BDK includes some breaking changes, some of which are handled here. A previously available function in BDK, maintain_single_recipient() is changed into allow_shrinking() which requires a scriptpubkey to be specified. So far I couldn't find any public API in BDK using which a scriptpubkey (or the transaction) can be retrieved from the wallet given a TxID. Marked the code with a TODO. It probably needs some additional API in BDK. Made an issue (bitcoindevkit/bdk#427) to track this.

BDK esplora blockchain module has been divided into two types using-ureq and using-reqwest. reqwest is an async feature in BDK, which produces Future related errors in bdk-cli (#36 (comment)). Trying to solve that error creates more complexity (async related weridness). Thus decided to scrap reqwest and go with blocking ureq version, which I think is adequate for the usecases of bdk-cli.

Updated electrum cli args.

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

Updating the BDK version creates some breaking change. One of such change probably requires modification of BDK to expose a new function to fetch scriptpubkey from a txid. Used ureq version of esplora to remove async related complexities.

notmandatory · 2021-08-27T13:11:07Z


 [dependencies]
-bdk = { version = "^0.7", default-features = false, features = ["all-keys"]}
+bdk = { version = "0.10.0", default-features = false, features = ["all-keys"]}


I wasn't able to connect to the blockstream esplora server via https (and http redirects to https) because bdk disables ureq default features. @tcharding was disabling the tls feature intentional?
@rajarshimaitra If having the ureqtls feature disabled by default is intentional I think it makes sense to add a ureq-tls = [ "ureq/tls" ] feature to bdk so we can enable it from the bdk-cli project (or for others who need it).

Good Idea. Better to enable tls in ureq if we use it. IMO connecting https will be quite a generic requirement. Will open a PR if we all agree the same.
We don't need another feature flag for this, just add tls in ureq's feature list.

True, maybe best of have tls feature enabled by default in bdk, but first I'd like to get @tcharding's input since he originally added the ureq dependency and may have an argument for leaving tls off by default.

I have no problem with enabling TLS by default. As for why I disabled it, I can't remember exactly why, I probably just disabled default features during a debugging session and never turned TLS back on. Thanks for checking in with me and thanks for giving it the thought to realize we should enable it by default!

afilini · 2021-09-09T09:35:10Z

 repl = ["regex", "rustyline"]
 electrum = ["bdk/electrum"]
-esplora = ["bdk/esplora"]
+esplora = ["bdk/use-esplora-ureq"]


I'm trying to use this to test bitcoindevkit/bdk#429: to get bdk-cli to work in the browser we need a way to enable bdk/use-esplora-reqwest. I would suggest maybe adding another feature esplora-reqwest that we can use from wasm32

Or maybe even better: async-esplora

I think this PR may be blocked until @tcharding and @LLFourn work out things out with bitcoindevkit/bdk#433.

Yes. I am not really sure how big the httpclient fix would be. If it takes much longer, can we consider merging bitcoindevkit/bdk#430 in the mean time? #36 is also pending on this one.
And I think that will fix @afilini 's issue too.

In the meantime I made this branch just to test the playground: https://github.com/afilini/bdk-cli/tree/update-with-reqwest

rajarshimaitra · 2021-09-14T10:08:47Z

Added esplora-reqwest into bdk-cli.

@notmandatory we have to use bdk-master for now, until the esplora-fix is released upstream. This can be changed in next bdk release.

@afilini please check if this meets your requirement, or something more to be done? To use request compile with esplora-reqwest feature.

notmandatory · 2021-09-17T18:00:37Z

When bdk release 0.12.0 is out ~Sept-29 I think this one will be ready to merge. I've updated the title. 😉

rajarshimaitra · 2021-09-19T06:14:50Z

@notmandatory just FYI, the rest of the open PRs builds on top of this. So this needs to go first before they can be merged.. :)

notmandatory · 2021-10-01T22:49:29Z

I just made the bdk 0.12.0 release, so you're good to go on this one.

BDK v0.10.0 adds a stop_gap parameter to electrum config.

Esplora ureq version requires having new cli args.

Adds esplora-reqwest capability from bdk. Allows to use async https connections with esplora from bdk-cli.

Recent update in BDK esplora backend allows it to connect with SOCKS5 proxy. This change adds proxy option to esplora configuration. Updates Proxy_Opts feature guard to enable it for esplora feature.

rajarshimaitra · 2021-10-03T10:02:31Z

Rebased with bdk v0.12.

FYI, I am not updating #42 and #36 right now. For simplicity of merging this needs to be merged first. Then the rest two can be rebased on top of updated master. I think that will simplify the review process as all these PRs are interdependent.

This one is the biggest change among all other as it handles all the update from bdk v0.10 to v0.12. So keeping this one separate from review of the rest.

notmandatory

re-ACK f9a9606

I manually tested and works fine as long as correct esplora server URL is used. Either README needs to be updated, defaults need to be changed for testnet or maybe something else. Will merge this now so other PRs can proceed and we can fix the esplora server URL issue in a subsequent PR.

rajarshimaitra · 2021-08-26T09:56:24Z

Description

This PR updates the upstream BDK version to latest release (v0.10.0).

Notes to the reviewers

The latest release of BDK includes some breaking changes, some of which are handled here. A previously available function in BDK, maintain_single_recipient() is changed into allow_shrinking() which requires a scriptpubkey to be specified. So far I couldn't find any public API in BDK using which a scriptpubkey (or the transaction) can be retrieved from the wallet given a TxID. Marked the code with a TODO. It probably needs some additional API in BDK. Made an issue (bitcoindevkit/bdk#427) to track this.

BDK esplora blockchain module has been divided into two types using-ureq and using-reqwest. reqwest is an async feature in BDK, which produces Future related errors in bdk-cli (#36 (comment)). Trying to solve that error creates more complexity (async related weridness). Thus decided to scrap reqwest and go with blocking ureq version, which I think is adequate for the usecases of bdk-cli.

Updated electrum cli args.

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

Updating the BDK version creates some breaking change. One of such change probably requires modification of BDK to expose a new function to fetch scriptpubkey from a txid. Used ureq version of esplora to remove async related complexities.

notmandatory · 2021-08-27T13:11:07Z


 [dependencies]
-bdk = { version = "^0.7", default-features = false, features = ["all-keys"]}
+bdk = { version = "0.10.0", default-features = false, features = ["all-keys"]}


I wasn't able to connect to the blockstream esplora server via https (and http redirects to https) because bdk disables ureq default features. @tcharding was disabling the tls feature intentional?
@rajarshimaitra If having the ureqtls feature disabled by default is intentional I think it makes sense to add a ureq-tls = [ "ureq/tls" ] feature to bdk so we can enable it from the bdk-cli project (or for others who need it).

Good Idea. Better to enable tls in ureq if we use it. IMO connecting https will be quite a generic requirement. Will open a PR if we all agree the same.
We don't need another feature flag for this, just add tls in ureq's feature list.

True, maybe best of have tls feature enabled by default in bdk, but first I'd like to get @tcharding's input since he originally added the ureq dependency and may have an argument for leaving tls off by default.

I have no problem with enabling TLS by default. As for why I disabled it, I can't remember exactly why, I probably just disabled default features during a debugging session and never turned TLS back on. Thanks for checking in with me and thanks for giving it the thought to realize we should enable it by default!

afilini · 2021-09-09T09:35:10Z

 repl = ["regex", "rustyline"]
 electrum = ["bdk/electrum"]
-esplora = ["bdk/esplora"]
+esplora = ["bdk/use-esplora-ureq"]


I'm trying to use this to test bitcoindevkit/bdk#429: to get bdk-cli to work in the browser we need a way to enable bdk/use-esplora-reqwest. I would suggest maybe adding another feature esplora-reqwest that we can use from wasm32

Or maybe even better: async-esplora

I think this PR may be blocked until @tcharding and @LLFourn work out things out with bitcoindevkit/bdk#433.

Yes. I am not really sure how big the httpclient fix would be. If it takes much longer, can we consider merging bitcoindevkit/bdk#430 in the mean time? #36 is also pending on this one.
And I think that will fix @afilini 's issue too.

In the meantime I made this branch just to test the playground: https://github.com/afilini/bdk-cli/tree/update-with-reqwest

rajarshimaitra · 2021-09-14T10:08:47Z

Added esplora-reqwest into bdk-cli.

@notmandatory we have to use bdk-master for now, until the esplora-fix is released upstream. This can be changed in next bdk release.

@afilini please check if this meets your requirement, or something more to be done? To use request compile with esplora-reqwest feature.

notmandatory · 2021-09-17T18:00:37Z

When bdk release 0.12.0 is out ~Sept-29 I think this one will be ready to merge. I've updated the title. 😉

rajarshimaitra · 2021-09-19T06:14:50Z

@notmandatory just FYI, the rest of the open PRs builds on top of this. So this needs to go first before they can be merged.. :)

notmandatory · 2021-10-01T22:49:29Z

I just made the bdk 0.12.0 release, so you're good to go on this one.

BDK v0.10.0 adds a stop_gap parameter to electrum config.

Esplora ureq version requires having new cli args.

Adds esplora-reqwest capability from bdk. Allows to use async https connections with esplora from bdk-cli.

Recent update in BDK esplora backend allows it to connect with SOCKS5 proxy. This change adds proxy option to esplora configuration. Updates Proxy_Opts feature guard to enable it for esplora feature.

rajarshimaitra · 2021-10-03T10:02:31Z

Rebased with bdk v0.12.

FYI, I am not updating #42 and #36 right now. For simplicity of merging this needs to be merged first. Then the rest two can be rebased on top of updated master. I think that will simplify the review process as all these PRs are interdependent.

This one is the biggest change among all other as it handles all the update from bdk v0.10 to v0.12. So keeping this one separate from review of the rest.

notmandatory

re-ACK f9a9606

I manually tested and works fine as long as correct esplora server URL is used. Either README needs to be updated, defaults need to be changed for testnet or maybe something else. Will merge this now so other PRs can proceed and we can fix the esplora server URL issue in a subsequent PR.

rajarshimaitra · 2021-08-26T09:56:24Z

Description

This PR updates the upstream BDK version to latest release (v0.10.0).

Notes to the reviewers

The latest release of BDK includes some breaking changes, some of which are handled here. A previously available function in BDK, maintain_single_recipient() is changed into allow_shrinking() which requires a scriptpubkey to be specified. So far I couldn't find any public API in BDK using which a scriptpubkey (or the transaction) can be retrieved from the wallet given a TxID. Marked the code with a TODO. It probably needs some additional API in BDK. Made an issue (bitcoindevkit/bdk#427) to track this.

BDK esplora blockchain module has been divided into two types using-ureq and using-reqwest. reqwest is an async feature in BDK, which produces Future related errors in bdk-cli (#36 (comment)). Trying to solve that error creates more complexity (async related weridness). Thus decided to scrap reqwest and go with blocking ureq version, which I think is adequate for the usecases of bdk-cli.

Updated electrum cli args.

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

Updating the BDK version creates some breaking change. One of such change probably requires modification of BDK to expose a new function to fetch scriptpubkey from a txid. Used ureq version of esplora to remove async related complexities.

notmandatory · 2021-08-27T13:11:07Z


 [dependencies]
-bdk = { version = "^0.7", default-features = false, features = ["all-keys"]}
+bdk = { version = "0.10.0", default-features = false, features = ["all-keys"]}


I wasn't able to connect to the blockstream esplora server via https (and http redirects to https) because bdk disables ureq default features. @tcharding was disabling the tls feature intentional?
@rajarshimaitra If having the ureqtls feature disabled by default is intentional I think it makes sense to add a ureq-tls = [ "ureq/tls" ] feature to bdk so we can enable it from the bdk-cli project (or for others who need it).

Good Idea. Better to enable tls in ureq if we use it. IMO connecting https will be quite a generic requirement. Will open a PR if we all agree the same.
We don't need another feature flag for this, just add tls in ureq's feature list.

True, maybe best of have tls feature enabled by default in bdk, but first I'd like to get @tcharding's input since he originally added the ureq dependency and may have an argument for leaving tls off by default.

I have no problem with enabling TLS by default. As for why I disabled it, I can't remember exactly why, I probably just disabled default features during a debugging session and never turned TLS back on. Thanks for checking in with me and thanks for giving it the thought to realize we should enable it by default!

afilini · 2021-09-09T09:35:10Z

 repl = ["regex", "rustyline"]
 electrum = ["bdk/electrum"]
-esplora = ["bdk/esplora"]
+esplora = ["bdk/use-esplora-ureq"]


I'm trying to use this to test bitcoindevkit/bdk#429: to get bdk-cli to work in the browser we need a way to enable bdk/use-esplora-reqwest. I would suggest maybe adding another feature esplora-reqwest that we can use from wasm32

Or maybe even better: async-esplora

I think this PR may be blocked until @tcharding and @LLFourn work out things out with bitcoindevkit/bdk#433.

Yes. I am not really sure how big the httpclient fix would be. If it takes much longer, can we consider merging bitcoindevkit/bdk#430 in the mean time? #36 is also pending on this one.
And I think that will fix @afilini 's issue too.

In the meantime I made this branch just to test the playground: https://github.com/afilini/bdk-cli/tree/update-with-reqwest

rajarshimaitra · 2021-09-14T10:08:47Z

Added esplora-reqwest into bdk-cli.

@notmandatory we have to use bdk-master for now, until the esplora-fix is released upstream. This can be changed in next bdk release.

@afilini please check if this meets your requirement, or something more to be done? To use request compile with esplora-reqwest feature.

notmandatory · 2021-09-17T18:00:37Z

When bdk release 0.12.0 is out ~Sept-29 I think this one will be ready to merge. I've updated the title. 😉

rajarshimaitra · 2021-09-19T06:14:50Z

@notmandatory just FYI, the rest of the open PRs builds on top of this. So this needs to go first before they can be merged.. :)

notmandatory · 2021-10-01T22:49:29Z

I just made the bdk 0.12.0 release, so you're good to go on this one.

BDK v0.10.0 adds a stop_gap parameter to electrum config.

Esplora ureq version requires having new cli args.

Adds esplora-reqwest capability from bdk. Allows to use async https connections with esplora from bdk-cli.

Recent update in BDK esplora backend allows it to connect with SOCKS5 proxy. This change adds proxy option to esplora configuration. Updates Proxy_Opts feature guard to enable it for esplora feature.

rajarshimaitra · 2021-10-03T10:02:31Z

Rebased with bdk v0.12.

FYI, I am not updating #42 and #36 right now. For simplicity of merging this needs to be merged first. Then the rest two can be rebased on top of updated master. I think that will simplify the review process as all these PRs are interdependent.

This one is the biggest change among all other as it handles all the update from bdk v0.10 to v0.12. So keeping this one separate from review of the rest.

notmandatory

re-ACK f9a9606

I manually tested and works fine as long as correct esplora server URL is used. Either README needs to be updated, defaults need to be changed for testnet or maybe something else. Will merge this now so other PRs can proceed and we can fix the esplora server URL issue in a subsequent PR.

rajarshimaitra · 2021-08-26T09:56:24Z

Description

This PR updates the upstream BDK version to latest release (v0.10.0).

Notes to the reviewers

The latest release of BDK includes some breaking changes, some of which are handled here. A previously available function in BDK, maintain_single_recipient() is changed into allow_shrinking() which requires a scriptpubkey to be specified. So far I couldn't find any public API in BDK using which a scriptpubkey (or the transaction) can be retrieved from the wallet given a TxID. Marked the code with a TODO. It probably needs some additional API in BDK. Made an issue (bitcoindevkit/bdk#427) to track this.

BDK esplora blockchain module has been divided into two types using-ureq and using-reqwest. reqwest is an async feature in BDK, which produces Future related errors in bdk-cli (#36 (comment)). Trying to solve that error creates more complexity (async related weridness). Thus decided to scrap reqwest and go with blocking ureq version, which I think is adequate for the usecases of bdk-cli.

Updated electrum cli args.

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

Updating the BDK version creates some breaking change. One of such change probably requires modification of BDK to expose a new function to fetch scriptpubkey from a txid. Used ureq version of esplora to remove async related complexities.

notmandatory · 2021-08-27T13:11:07Z


 [dependencies]
-bdk = { version = "^0.7", default-features = false, features = ["all-keys"]}
+bdk = { version = "0.10.0", default-features = false, features = ["all-keys"]}


I wasn't able to connect to the blockstream esplora server via https (and http redirects to https) because bdk disables ureq default features. @tcharding was disabling the tls feature intentional?
@rajarshimaitra If having the ureqtls feature disabled by default is intentional I think it makes sense to add a ureq-tls = [ "ureq/tls" ] feature to bdk so we can enable it from the bdk-cli project (or for others who need it).

Good Idea. Better to enable tls in ureq if we use it. IMO connecting https will be quite a generic requirement. Will open a PR if we all agree the same.
We don't need another feature flag for this, just add tls in ureq's feature list.

True, maybe best of have tls feature enabled by default in bdk, but first I'd like to get @tcharding's input since he originally added the ureq dependency and may have an argument for leaving tls off by default.

I have no problem with enabling TLS by default. As for why I disabled it, I can't remember exactly why, I probably just disabled default features during a debugging session and never turned TLS back on. Thanks for checking in with me and thanks for giving it the thought to realize we should enable it by default!

afilini · 2021-09-09T09:35:10Z

 repl = ["regex", "rustyline"]
 electrum = ["bdk/electrum"]
-esplora = ["bdk/esplora"]
+esplora = ["bdk/use-esplora-ureq"]


I'm trying to use this to test bitcoindevkit/bdk#429: to get bdk-cli to work in the browser we need a way to enable bdk/use-esplora-reqwest. I would suggest maybe adding another feature esplora-reqwest that we can use from wasm32

Or maybe even better: async-esplora

I think this PR may be blocked until @tcharding and @LLFourn work out things out with bitcoindevkit/bdk#433.

Yes. I am not really sure how big the httpclient fix would be. If it takes much longer, can we consider merging bitcoindevkit/bdk#430 in the mean time? #36 is also pending on this one.
And I think that will fix @afilini 's issue too.

In the meantime I made this branch just to test the playground: https://github.com/afilini/bdk-cli/tree/update-with-reqwest

rajarshimaitra · 2021-09-14T10:08:47Z

Added esplora-reqwest into bdk-cli.

@notmandatory we have to use bdk-master for now, until the esplora-fix is released upstream. This can be changed in next bdk release.

@afilini please check if this meets your requirement, or something more to be done? To use request compile with esplora-reqwest feature.

notmandatory · 2021-09-17T18:00:37Z

When bdk release 0.12.0 is out ~Sept-29 I think this one will be ready to merge. I've updated the title. 😉

rajarshimaitra · 2021-09-19T06:14:50Z

@notmandatory just FYI, the rest of the open PRs builds on top of this. So this needs to go first before they can be merged.. :)

notmandatory · 2021-10-01T22:49:29Z

I just made the bdk 0.12.0 release, so you're good to go on this one.

BDK v0.10.0 adds a stop_gap parameter to electrum config.

Esplora ureq version requires having new cli args.

Adds esplora-reqwest capability from bdk. Allows to use async https connections with esplora from bdk-cli.

Recent update in BDK esplora backend allows it to connect with SOCKS5 proxy. This change adds proxy option to esplora configuration. Updates Proxy_Opts feature guard to enable it for esplora feature.

rajarshimaitra · 2021-10-03T10:02:31Z

Rebased with bdk v0.12.

FYI, I am not updating #42 and #36 right now. For simplicity of merging this needs to be merged first. Then the rest two can be rebased on top of updated master. I think that will simplify the review process as all these PRs are interdependent.

This one is the biggest change among all other as it handles all the update from bdk v0.10 to v0.12. So keeping this one separate from review of the rest.

notmandatory

re-ACK f9a9606

I manually tested and works fine as long as correct esplora server URL is used. Either README needs to be updated, defaults need to be changed for testnet or maybe something else. Will merge this now so other PRs can proceed and we can fix the esplora server URL issue in a subsequent PR.

rajarshimaitra · 2021-08-26T09:56:24Z

Description

This PR updates the upstream BDK version to latest release (v0.10.0).

Notes to the reviewers

The latest release of BDK includes some breaking changes, some of which are handled here. A previously available function in BDK, maintain_single_recipient() is changed into allow_shrinking() which requires a scriptpubkey to be specified. So far I couldn't find any public API in BDK using which a scriptpubkey (or the transaction) can be retrieved from the wallet given a TxID. Marked the code with a TODO. It probably needs some additional API in BDK. Made an issue (bitcoindevkit/bdk#427) to track this.

BDK esplora blockchain module has been divided into two types using-ureq and using-reqwest. reqwest is an async feature in BDK, which produces Future related errors in bdk-cli (#36 (comment)). Trying to solve that error creates more complexity (async related weridness). Thus decided to scrap reqwest and go with blocking ureq version, which I think is adequate for the usecases of bdk-cli.

Updated electrum cli args.

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

Updating the BDK version creates some breaking change. One of such change probably requires modification of BDK to expose a new function to fetch scriptpubkey from a txid. Used ureq version of esplora to remove async related complexities.

notmandatory · 2021-08-27T13:11:07Z


 [dependencies]
-bdk = { version = "^0.7", default-features = false, features = ["all-keys"]}
+bdk = { version = "0.10.0", default-features = false, features = ["all-keys"]}


I wasn't able to connect to the blockstream esplora server via https (and http redirects to https) because bdk disables ureq default features. @tcharding was disabling the tls feature intentional?
@rajarshimaitra If having the ureqtls feature disabled by default is intentional I think it makes sense to add a ureq-tls = [ "ureq/tls" ] feature to bdk so we can enable it from the bdk-cli project (or for others who need it).

Good Idea. Better to enable tls in ureq if we use it. IMO connecting https will be quite a generic requirement. Will open a PR if we all agree the same.
We don't need another feature flag for this, just add tls in ureq's feature list.

True, maybe best of have tls feature enabled by default in bdk, but first I'd like to get @tcharding's input since he originally added the ureq dependency and may have an argument for leaving tls off by default.

I have no problem with enabling TLS by default. As for why I disabled it, I can't remember exactly why, I probably just disabled default features during a debugging session and never turned TLS back on. Thanks for checking in with me and thanks for giving it the thought to realize we should enable it by default!

afilini · 2021-09-09T09:35:10Z

 repl = ["regex", "rustyline"]
 electrum = ["bdk/electrum"]
-esplora = ["bdk/esplora"]
+esplora = ["bdk/use-esplora-ureq"]


I'm trying to use this to test bitcoindevkit/bdk#429: to get bdk-cli to work in the browser we need a way to enable bdk/use-esplora-reqwest. I would suggest maybe adding another feature esplora-reqwest that we can use from wasm32

Or maybe even better: async-esplora

I think this PR may be blocked until @tcharding and @LLFourn work out things out with bitcoindevkit/bdk#433.

Yes. I am not really sure how big the httpclient fix would be. If it takes much longer, can we consider merging bitcoindevkit/bdk#430 in the mean time? #36 is also pending on this one.
And I think that will fix @afilini 's issue too.

In the meantime I made this branch just to test the playground: https://github.com/afilini/bdk-cli/tree/update-with-reqwest

rajarshimaitra · 2021-09-14T10:08:47Z

Added esplora-reqwest into bdk-cli.

@notmandatory we have to use bdk-master for now, until the esplora-fix is released upstream. This can be changed in next bdk release.

@afilini please check if this meets your requirement, or something more to be done? To use request compile with esplora-reqwest feature.

notmandatory · 2021-09-17T18:00:37Z

When bdk release 0.12.0 is out ~Sept-29 I think this one will be ready to merge. I've updated the title. 😉

rajarshimaitra · 2021-09-19T06:14:50Z

@notmandatory just FYI, the rest of the open PRs builds on top of this. So this needs to go first before they can be merged.. :)

notmandatory · 2021-10-01T22:49:29Z

I just made the bdk 0.12.0 release, so you're good to go on this one.

BDK v0.10.0 adds a stop_gap parameter to electrum config.

Esplora ureq version requires having new cli args.

Adds esplora-reqwest capability from bdk. Allows to use async https connections with esplora from bdk-cli.

Recent update in BDK esplora backend allows it to connect with SOCKS5 proxy. This change adds proxy option to esplora configuration. Updates Proxy_Opts feature guard to enable it for esplora feature.

rajarshimaitra · 2021-10-03T10:02:31Z

Rebased with bdk v0.12.

FYI, I am not updating #42 and #36 right now. For simplicity of merging this needs to be merged first. Then the rest two can be rebased on top of updated master. I think that will simplify the review process as all these PRs are interdependent.

This one is the biggest change among all other as it handles all the update from bdk v0.10 to v0.12. So keeping this one separate from review of the rest.

notmandatory

re-ACK f9a9606

I manually tested and works fine as long as correct esplora server URL is used. Either README needs to be updated, defaults need to be changed for testnet or maybe something else. Will merge this now so other PRs can proceed and we can fix the esplora server URL issue in a subsequent PR.

rajarshimaitra · 2021-08-26T09:56:24Z

Description

This PR updates the upstream BDK version to latest release (v0.10.0).

Notes to the reviewers

The latest release of BDK includes some breaking changes, some of which are handled here. A previously available function in BDK, maintain_single_recipient() is changed into allow_shrinking() which requires a scriptpubkey to be specified. So far I couldn't find any public API in BDK using which a scriptpubkey (or the transaction) can be retrieved from the wallet given a TxID. Marked the code with a TODO. It probably needs some additional API in BDK. Made an issue (bitcoindevkit/bdk#427) to track this.

BDK esplora blockchain module has been divided into two types using-ureq and using-reqwest. reqwest is an async feature in BDK, which produces Future related errors in bdk-cli (#36 (comment)). Trying to solve that error creates more complexity (async related weridness). Thus decided to scrap reqwest and go with blocking ureq version, which I think is adequate for the usecases of bdk-cli.

Updated electrum cli args.

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

Updating the BDK version creates some breaking change. One of such change probably requires modification of BDK to expose a new function to fetch scriptpubkey from a txid. Used ureq version of esplora to remove async related complexities.

notmandatory · 2021-08-27T13:11:07Z


 [dependencies]
-bdk = { version = "^0.7", default-features = false, features = ["all-keys"]}
+bdk = { version = "0.10.0", default-features = false, features = ["all-keys"]}


I wasn't able to connect to the blockstream esplora server via https (and http redirects to https) because bdk disables ureq default features. @tcharding was disabling the tls feature intentional?
@rajarshimaitra If having the ureqtls feature disabled by default is intentional I think it makes sense to add a ureq-tls = [ "ureq/tls" ] feature to bdk so we can enable it from the bdk-cli project (or for others who need it).

Good Idea. Better to enable tls in ureq if we use it. IMO connecting https will be quite a generic requirement. Will open a PR if we all agree the same.
We don't need another feature flag for this, just add tls in ureq's feature list.

True, maybe best of have tls feature enabled by default in bdk, but first I'd like to get @tcharding's input since he originally added the ureq dependency and may have an argument for leaving tls off by default.

I have no problem with enabling TLS by default. As for why I disabled it, I can't remember exactly why, I probably just disabled default features during a debugging session and never turned TLS back on. Thanks for checking in with me and thanks for giving it the thought to realize we should enable it by default!

afilini · 2021-09-09T09:35:10Z

 repl = ["regex", "rustyline"]
 electrum = ["bdk/electrum"]
-esplora = ["bdk/esplora"]
+esplora = ["bdk/use-esplora-ureq"]


I'm trying to use this to test bitcoindevkit/bdk#429: to get bdk-cli to work in the browser we need a way to enable bdk/use-esplora-reqwest. I would suggest maybe adding another feature esplora-reqwest that we can use from wasm32

Or maybe even better: async-esplora

I think this PR may be blocked until @tcharding and @LLFourn work out things out with bitcoindevkit/bdk#433.

Yes. I am not really sure how big the httpclient fix would be. If it takes much longer, can we consider merging bitcoindevkit/bdk#430 in the mean time? #36 is also pending on this one.
And I think that will fix @afilini 's issue too.

In the meantime I made this branch just to test the playground: https://github.com/afilini/bdk-cli/tree/update-with-reqwest

rajarshimaitra · 2021-09-14T10:08:47Z

Added esplora-reqwest into bdk-cli.

@notmandatory we have to use bdk-master for now, until the esplora-fix is released upstream. This can be changed in next bdk release.

@afilini please check if this meets your requirement, or something more to be done? To use request compile with esplora-reqwest feature.

notmandatory · 2021-09-17T18:00:37Z

When bdk release 0.12.0 is out ~Sept-29 I think this one will be ready to merge. I've updated the title. 😉

rajarshimaitra · 2021-09-19T06:14:50Z

@notmandatory just FYI, the rest of the open PRs builds on top of this. So this needs to go first before they can be merged.. :)

notmandatory · 2021-10-01T22:49:29Z

I just made the bdk 0.12.0 release, so you're good to go on this one.

BDK v0.10.0 adds a stop_gap parameter to electrum config.

Esplora ureq version requires having new cli args.

Adds esplora-reqwest capability from bdk. Allows to use async https connections with esplora from bdk-cli.

Recent update in BDK esplora backend allows it to connect with SOCKS5 proxy. This change adds proxy option to esplora configuration. Updates Proxy_Opts feature guard to enable it for esplora feature.

rajarshimaitra · 2021-10-03T10:02:31Z

Rebased with bdk v0.12.

FYI, I am not updating #42 and #36 right now. For simplicity of merging this needs to be merged first. Then the rest two can be rebased on top of updated master. I think that will simplify the review process as all these PRs are interdependent.

This one is the biggest change among all other as it handles all the update from bdk v0.10 to v0.12. So keeping this one separate from review of the rest.

notmandatory

re-ACK f9a9606

I manually tested and works fine as long as correct esplora server URL is used. Either README needs to be updated, defaults need to be changed for testnet or maybe something else. Will merge this now so other PRs can proceed and we can fix the esplora server URL issue in a subsequent PR.

rajarshimaitra · 2021-08-26T09:56:24Z

Description

This PR updates the upstream BDK version to latest release (v0.10.0).

Notes to the reviewers

The latest release of BDK includes some breaking changes, some of which are handled here. A previously available function in BDK, maintain_single_recipient() is changed into allow_shrinking() which requires a scriptpubkey to be specified. So far I couldn't find any public API in BDK using which a scriptpubkey (or the transaction) can be retrieved from the wallet given a TxID. Marked the code with a TODO. It probably needs some additional API in BDK. Made an issue (bitcoindevkit/bdk#427) to track this.

BDK esplora blockchain module has been divided into two types using-ureq and using-reqwest. reqwest is an async feature in BDK, which produces Future related errors in bdk-cli (#36 (comment)). Trying to solve that error creates more complexity (async related weridness). Thus decided to scrap reqwest and go with blocking ureq version, which I think is adequate for the usecases of bdk-cli.

Updated electrum cli args.

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

Updating the BDK version creates some breaking change. One of such change probably requires modification of BDK to expose a new function to fetch scriptpubkey from a txid. Used ureq version of esplora to remove async related complexities.

notmandatory · 2021-08-27T13:11:07Z


 [dependencies]
-bdk = { version = "^0.7", default-features = false, features = ["all-keys"]}
+bdk = { version = "0.10.0", default-features = false, features = ["all-keys"]}


I wasn't able to connect to the blockstream esplora server via https (and http redirects to https) because bdk disables ureq default features. @tcharding was disabling the tls feature intentional?
@rajarshimaitra If having the ureqtls feature disabled by default is intentional I think it makes sense to add a ureq-tls = [ "ureq/tls" ] feature to bdk so we can enable it from the bdk-cli project (or for others who need it).

Good Idea. Better to enable tls in ureq if we use it. IMO connecting https will be quite a generic requirement. Will open a PR if we all agree the same.
We don't need another feature flag for this, just add tls in ureq's feature list.

True, maybe best of have tls feature enabled by default in bdk, but first I'd like to get @tcharding's input since he originally added the ureq dependency and may have an argument for leaving tls off by default.

I have no problem with enabling TLS by default. As for why I disabled it, I can't remember exactly why, I probably just disabled default features during a debugging session and never turned TLS back on. Thanks for checking in with me and thanks for giving it the thought to realize we should enable it by default!

afilini · 2021-09-09T09:35:10Z

 repl = ["regex", "rustyline"]
 electrum = ["bdk/electrum"]
-esplora = ["bdk/esplora"]
+esplora = ["bdk/use-esplora-ureq"]


I'm trying to use this to test bitcoindevkit/bdk#429: to get bdk-cli to work in the browser we need a way to enable bdk/use-esplora-reqwest. I would suggest maybe adding another feature esplora-reqwest that we can use from wasm32

Or maybe even better: async-esplora

I think this PR may be blocked until @tcharding and @LLFourn work out things out with bitcoindevkit/bdk#433.

Yes. I am not really sure how big the httpclient fix would be. If it takes much longer, can we consider merging bitcoindevkit/bdk#430 in the mean time? #36 is also pending on this one.
And I think that will fix @afilini 's issue too.

In the meantime I made this branch just to test the playground: https://github.com/afilini/bdk-cli/tree/update-with-reqwest

rajarshimaitra · 2021-09-14T10:08:47Z

Added esplora-reqwest into bdk-cli.

@notmandatory we have to use bdk-master for now, until the esplora-fix is released upstream. This can be changed in next bdk release.

@afilini please check if this meets your requirement, or something more to be done? To use request compile with esplora-reqwest feature.

notmandatory · 2021-09-17T18:00:37Z

When bdk release 0.12.0 is out ~Sept-29 I think this one will be ready to merge. I've updated the title. 😉

rajarshimaitra · 2021-09-19T06:14:50Z

@notmandatory just FYI, the rest of the open PRs builds on top of this. So this needs to go first before they can be merged.. :)

notmandatory · 2021-10-01T22:49:29Z

I just made the bdk 0.12.0 release, so you're good to go on this one.

BDK v0.10.0 adds a stop_gap parameter to electrum config.

Esplora ureq version requires having new cli args.

Adds esplora-reqwest capability from bdk. Allows to use async https connections with esplora from bdk-cli.

Recent update in BDK esplora backend allows it to connect with SOCKS5 proxy. This change adds proxy option to esplora configuration. Updates Proxy_Opts feature guard to enable it for esplora feature.

rajarshimaitra · 2021-10-03T10:02:31Z

Rebased with bdk v0.12.

FYI, I am not updating #42 and #36 right now. For simplicity of merging this needs to be merged first. Then the rest two can be rebased on top of updated master. I think that will simplify the review process as all these PRs are interdependent.

This one is the biggest change among all other as it handles all the update from bdk v0.10 to v0.12. So keeping this one separate from review of the rest.

notmandatory

re-ACK f9a9606

I manually tested and works fine as long as correct esplora server URL is used. Either README needs to be updated, defaults need to be changed for testnet or maybe something else. Will merge this now so other PRs can proceed and we can fix the esplora server URL issue in a subsequent PR.

Update BDK dependency to v0.10.0
cdf05f1
Updating the BDK version creates some breaking change. One of such change probably requires modification of BDK to expose a new function to fetch scriptpubkey from a txid. Used ureq version of esplora to remove async related complexities.

rajarshimaitra changed the title ~~Dep update~~Update BDK version to 0.10.0Aug 26, 2021

notmandatory reviewed Aug 27, 2021
View reviewed changes

rajarshimaitra mentioned this pull request Aug 30, 2021
[CI] Fixed esplora blockchain tests in CI bitcoindevkit/bdk#430
Merged
4 tasks

afilini reviewed Sep 9, 2021
View reviewed changes

rajarshimaitra force-pushed the dep-update branch from 88621be to dbf3a1fCompare September 17, 2021 13:33

rajarshimaitra mentioned this pull request Sep 17, 2021
RPC backend implementation #36
Merged
6 tasks

notmandatory changed the title ~~Update BDK version to 0.10.0~~Update BDK version to 0.12.0Sep 17, 2021

rajarshimaitra added 7 commits October 3, 2021 15:21

Update electrum cli args
1c8127f
BDK v0.10.0 adds a stop_gap parameter to electrum config.

Update Esplora cli args
fe8742c
Esplora ureq version requires having new cli args.

Add esplora-reqwest from bdk
07a4287
Adds esplora-reqwest capability from bdk. Allows to use async https connections with esplora from bdk-cli.

Add proxy option to esplora
279fec1
Recent update in BDK esplora backend allows it to connect with SOCKS5 proxy. This change adds proxy option to esplora configuration. Updates Proxy_Opts feature guard to enable it for esplora feature.

Update cargo.toml
c347ef4

Update CI
d25c708

Update Cargo.lock
f9a9606

rajarshimaitra force-pushed the dep-update branch from dbf3a1f to f9a9606Compare October 3, 2021 09:59

notmandatory approved these changes Oct 4, 2021
View reviewed changes

notmandatory merged commit f9a9606 into bitcoindevkit:masterOct 4, 2021

notmandatory added this to the Release 0.3.0 Feature Freeze milestone Oct 5, 2021