bitcoindevkit / rust-esplora-client

Bitcoin Esplora API client library. Supports plaintext, TLS and Onion servers. Blocking or async.
MIT License
28 stars 44 forks source link

Replace `ureq` with `minreq` for `blocking` #75

Closed tnull closed 6 months ago

tnull commented 7 months ago

Taking this over from #57.

So far just a few minor adjustments on top of #57. Tested that syncing LDK works with the updated blocking client (in addition to the tests here).

notmandatory commented 7 months ago

You may need to pin some dependencies to get CI to build, see https://github.com/bitcoindevkit/bdk/blob/f099b42005b5310cee51fa2eaa29f94d76753bd7/.github/workflows/cont_integration.yml#L30C7-L36C49

tnull commented 7 months ago

You may need to pin some dependencies to get CI to build, see https://github.com/bitcoindevkit/bdk/blob/f099b42005b5310cee51fa2eaa29f94d76753bd7/.github/workflows/cont_integration.yml#L30C7-L36C49

Yes, however, I'm not sure if indiscriminately pinning (and advise the user to pin) potentially security-relevant libraries such as zstd is the best way forward, as it's really only test dependency but we would pin it for the entirety of the MSRV build, i.e., also for the entirety of the production build.

Maybe this would be the right time to fix this by downloading electrs/bitcoind via a shell script in CI rather than using electrsd's download feature which introduces a significant dependency tree? We did so when bumping LDK to 1.63 and it works like a charm. Would you accept such a change if I openend another PR to that end?

notmandatory commented 7 months ago

I'm much less concerned about pinning dev only dependencies so I don't see a reason to redo the CI to make progress on this PR. For BDK there's a PR to change the CI to use NIX which would also remove the auto-downloaded bitcoind / electrsd issue that required these broken for msrv dev dependencies (https://github.com/bitcoindevkit/bdk/pull/1257). Once we get that PR into BDK we should be able to do something similar for this project with @storopoli 's help.

storopoli commented 7 months ago

Yeah count me in. Propagating Nix CI PRs throughout BDK's ecosystem should be a breeze after https://github.com/bitcoindevkit/bdk/pull/1257

notmandatory commented 7 months ago

Rather than mixing the discussion and fixes for the MSRV issue into this PR I've create a stand-alone PR #76 for it.

tnull commented 7 months ago

I'm much less concerned about pinning dev only dependencies so I don't see a reason to redo the CI to make progress on this PR.

They may be dev dependencies for BDK, but you pin them for the entirety of the cargo build, and also advise users to do so. But can continue that discussion on #76.

tnull commented 7 months ago

Rebased after #76 landed and squashed fixup commits while I was at it.

coveralls commented 7 months ago

Pull Request Test Coverage Report for Build 8104429681

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/blocking.rs 109 146 74.66%
<!-- Total: 111 148 75.0% -->
Totals Coverage Status
Change from base Build 8096806272: 5.9%
Covered Lines: 912
Relevant Lines: 1075

💛 - Coveralls
notmandatory commented 6 months ago

I merged #79 so this PR should be re-based again.

tnull commented 6 months ago

I merged #79 so this PR should be re-based again.

Rebased.

notmandatory commented 6 months ago

I had to push another commit to fix the CI. Apparently the latest version of jobserver has an MSRV of 1.63 so we do not need to pin it anymore (and our old pinned version 1.26 does not work).