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

Replace ureq with minreq for blocking #75

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Jan 31, 2024

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
Copy link
Member

@tnull
Copy link
Contributor Author

tnull commented Feb 1, 2024

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
Copy link
Member

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 (bitcoindevkit/bdk#1257). Once we get that PR into BDK we should be able to do something similar for this project with @storopoli 's help.

@storopoli
Copy link

Yeah count me in. Propagating Nix CI PRs throughout BDK's ecosystem should be a breeze after bitcoindevkit/bdk#1257

@notmandatory
Copy link
Member

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
Copy link
Contributor Author

tnull commented Feb 5, 2024

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.

notmandatory added a commit that referenced this pull request Feb 7, 2024
ac2fd86 ci: limit test threads to 1 (Steve Myers)
19bdeb1 replace temporary dependency on bitcoin-internals with hex-conservative (Steve Myers)
c99118e bump electrsd to 0.26 + bitcoind_25_0 and fix msrv errors (Steve Myers)

Pull request description:

  This PR bumps electrsd to 0.26 and replaces temporary dependency on `bitcoin-internals` with `hex-conservative` as done in #75. Fixed CI MSRV issues with dev dependencies blocking #75 and #74.

ACKs for top commit:
  evanlinjin:
    ACK ac2fd86
  tcharding:
    ACK ac2fd86

Tree-SHA512: 37d825aae78d1ca89806870455b97a48e5a69bbdd78adadb16f506fb4df0fe98dc3a262b8fb74b68f5c44031e76c8fa0b5304c8592bf332356a0215843a52c33
@tnull
Copy link
Contributor Author

tnull commented Feb 8, 2024

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

@coveralls
Copy link

coveralls commented Feb 8, 2024

Pull Request Test Coverage Report for Build 8104429681

Details

  • 111 of 148 (75.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+5.9%) to 84.837%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/blocking.rs 109 146 74.66%
Totals Coverage Status
Change from base Build 8096806272: 5.9%
Covered Lines: 912
Relevant Lines: 1075

💛 - Coveralls

@notmandatory notmandatory added this to the Release 0.7.0 milestone Feb 14, 2024
@notmandatory
Copy link
Member

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

@tnull
Copy link
Contributor Author

tnull commented Feb 29, 2024

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

Rebased.

@notmandatory
Copy link
Member

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).

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 6a598b2

@notmandatory notmandatory merged commit c487b31 into bitcoindevkit:master Mar 6, 2024
23 checks passed
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.

None yet

4 participants