Skip to content

Rexport base64 as a feature from bitcoin crate - #102

Merged
DanGould merged 3 commits into
payjoin:masterfrom
jbesraa:rexport-base64-from-bitcoin-crate
Aug 27, 2023
Merged

Rexport base64 as a feature from bitcoin crate#102
DanGould merged 3 commits into
payjoin:masterfrom
jbesraa:rexport-base64-from-bitcoin-crate

Conversation

@jbesraa

@jbesraa jbesraa commented Aug 25, 2023

Copy link
Copy Markdown
Contributor

resolves #30

@jbesraa
jbesraa force-pushed the rexport-base64-from-bitcoin-crate branch from fc24550 to 8f43c73 Compare August 25, 2023 08:54
@jbesraa

jbesraa commented Aug 25, 2023

Copy link
Copy Markdown
Contributor Author

@DanGould
Tried to pin rustls to 0.21.1 but apparently bitcoind is using a package that requires 0.21.6
bitcoind is only used for testing i think.. how to continue with this?

 error: failed to select a version for the requirement `rustls = "^0.21.6"`
candidate versions found which didn't match: 0.21.1
location searched: crates.io index
required by package `minreq v2.9.0`
    ... which satisfies dependency `minreq = "^2.6.0"` (locked to 2.9.0) of package `bitcoind v0.31.1`
    ... which satisfies dependency `bitcoind = "^0.31.1"` (locked to 0.31.1) of package `payjoin v0.9.0 (/home/runner/work/rust-payjoin/rust-payjoin/payjoin)`

@jbesraa jbesraa changed the title Rexport base64 from bitcoin crate Rexport base64 as a feature from bitcoin crate Aug 25, 2023
@jbesraa
jbesraa force-pushed the rexport-base64-from-bitcoin-crate branch 3 times, most recently from 05afe1a to d09a994 Compare August 25, 2023 15:15
@DanGould

DanGould commented Aug 25, 2023

Copy link
Copy Markdown
Member

@notmandatory Would you consider this [dev-dependencies] failure an actual failure to comply with msrv? The package builds on 1.57.0 but the tests require 1.63 or newer. Looks like rustls msrv is 1.60 and tls is a dependency for payjoin integration.

@notmandatory

Copy link
Copy Markdown
Contributor

@notmandatory Would you consider this [dev-dependencies] failure an actual failure to comply with msrv? The package builds on 1.57.0 but the tests require 1.63 or newer.

Strictly speaking it's enough to build with the MSRV in CI since that also verifies MSRV version pinning cargo update commands are correct. For BDK projects we also have CI run all our tests with stable and MSRV versions of rust because it hasn't caused any problems, but if it does we'd probably disable the re-testing for MSRV too.

@notmandatory

Copy link
Copy Markdown
Contributor

Try adding this line to the rust.yml CI for testing with 1.57.0 (and update the corresponding section of the README):

cargo update -p minreq --precise 2.8.0

This should fix the CI. Looks like latest bitcoind is using 2.9.0 which brings in some TLS code that's not happy with 1.57.

@jbesraa
jbesraa force-pushed the rexport-base64-from-bitcoin-crate branch from d09a994 to eb01765 Compare August 27, 2023 17:39
bitcoind crate is using minreq 2.9.0 which requires 1.60 MSRV and we want
to support 1.57 MSRV
@jbesraa
jbesraa force-pushed the rexport-base64-from-bitcoin-crate branch 2 times, most recently from 46fac76 to 51607fc Compare August 27, 2023 18:02
@jbesraa
jbesraa force-pushed the rexport-base64-from-bitcoin-crate branch from 51607fc to fb341d7 Compare August 27, 2023 18:14
@jbesraa

jbesraa commented Aug 27, 2023

Copy link
Copy Markdown
Contributor Author

thank you @notmandatory

@DanGould

Copy link
Copy Markdown
Member

Beautiful PR @jbesraa with well ordered, named, and separated commits. Efforts making big progress.

@notmandatory thanks a ton for the quick help. How did you identify the minreq version to be pinned? Just intuit that that's where the tls problem came from and check crates.io for recent changes?

@DanGould
DanGould merged commit d0cde61 into payjoin:master Aug 27, 2023
@notmandatory

notmandatory commented Aug 27, 2023

Copy link
Copy Markdown
Contributor

Beautiful PR @jbesraa with well ordered, named, and separated commits. Efforts making big progress.

@notmandatory thanks a ton for the quick help. How did you identify the minreq version to be pinned? Just intuit that that's where the tls problem came from and check crates.io for recent changes?

My go-to rust command is cargo tree -i <whatever broke ci> then go look things up on crates.io to see what their MSRVs are, then rerun the tests with 1.57 until it works. So a little science and a little trial and error.

@DanGould DanGould mentioned this pull request Sep 30, 2023
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.

Pack base64 as a feature

3 participants