b0a1bfb0cb ci: pin msrv dep version for rustls (Steve Myers)
Pull request description:
I'm not thrilled about having to pin `rustls` to meet our MSRV, but we need to make a new release and can't count on rustls/rustls#2239 being accepted by the `rustls` team.
ACKs for top commit:
luisschwab:
utACK b0a1bfb0cb
ValuedMammal:
utACK b0a1bfb0cb
oleonardolima:
ACK b0a1bfb0cb
Tree-SHA512: 1f37b423f3e10a7e3fa4e2072d5ad38b96341a91f78c1bcffc011dedc08071bd176b556646e40e635c3b558e6b771a4a7baa85b10d14da64d2cd385a69342ee4
d917f891e8 Update URLs in `Cargo.toml` (Elias Rohrer)
Pull request description:
.. while it's still magical, the URLs seem to be outdated ..
ACKs for top commit:
ValuedMammal:
ACK d917f891e8
Tree-SHA512: cf652d6949d8be67f5884457f388e8fb5bc68a54d826e387dcc3542c00c2d71d62bad1dd9cb78a95f55b4f19d1beec2379540b7fea2e610a71817b8cb743f90d
a871b084ec feat: add `id_from_pos` support (Wei Chen)
Pull request description:
This PR introduces the `blockchain.transaction.id_from_pos` feature from Electrum. This functionality is essential for an ongoing implementation in `bdk_electrum`, which requires retrieving the coinbase transaction from a block at a specified `height`. Currently, there is no straightforward method to achieve this. By implementing the `id_from_pos` feature, we will effectively address this gap.
ACKs for top commit:
oleonardolima:
utACK a871b084ec
ValuedMammal:
ACK a871b084ec
evanlinjin:
ACK a871b084ec
Tree-SHA512: e8f1ab44473f67030b4f19e766fcee472b7cfc1dac6df8c9f0e08d966e0ed48289de03768771623b9fc7e7ef15dacdb7e3b26c04f9b0de7566d67cc7585a7fa6
05771a81d7 fix: `NoCertificateVerification` implementation (Leonardo Lima)
Pull request description:
fixes#149https://github.com/bitcoindevkit/bdk/issues/1598
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
<!-- Describe the purpose of this PR, what's being adding and/or fixed -->
It has been noticed some issues by both users and developers, as reported in #149, https://github.com/bitcoindevkit/bdk/issues/1598 and https://github.com/wizardsardine/liana/issues/1300, when using the library with `use-rustls-{ring}` feature to connect to electrum servers that use self-signed certificates, there are even some issues when trying to connect to `ssl://electrum.blockstream.info:50002` server.
To connect in an insecure manner either with `rustls` or `openssl` features, the user can set the `validate_domain` field in the `Config` to false, this will either set the `SslVerifyMode::NONE` when using `openssl`, or use the custom `NoCertificateVerification` for the
`rustls::client::danger::ServerCertVerifier` trait when using `rustls`, that said it should ignore the certificate verification when used.
At the current library state, it's failing because we didn't set up the supported `rustls::SignatureScheme` properly, returning an empty vector at the moment. This PR focuses on fixing this issue by relying on the `CryptoProvider` in usage to get the correct and supported signature schemes.
As part of the research to understand the problem, I've noticed that ideally, we should still use both the `rustls::webpki::verify_tls12_signature` and `rustls::webpki::verify_tls12_signature` and only rely on `rustls::client::danger::ServerCertVerified::assertion()` to ignore the certificate verification, however, it would still fail in scenarios such as https://github.com/bitcoindevkit/bdk/issues/1598 which uses X.509 certificates with any version other than 3 (it uses version 1), see [here](1a0d1646d0/src/cert.rs (L202-L218)).
I kept the current behavior to also ignore the TLS signature, but I still would like to bring this to the discussion, should we validate it properly and update the documentation to mention the `webpki` limitation instead ?
### Notes to the reviewers
I kept the current behavior to also ignore the TLS signature, but I still would like to bring this to the discussion, should we validate it properly and update the documentation to mention the `webpki` limitation instead ?
<!-- In this section you can include notes directed to the reviewers, like explaining why some parts
of the PR were done in a specific way -->
### Changelog notice
- Updates the `NoCertificateVerification` implementation for the
`rustls::client::danger::ServerCertVerifier` to use the `rustls::SignatureScheme` from `CryptoProvider` in use.
<!-- Notice the release manager should include in the release tag message changelog -->
<!-- See https://keepachangelog.com/en/1.0.0/ for examples -->
### Checklists
#### All Submissions:
* [x] I've signed all my commits
* [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
* [x] 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
#### 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
ACKs for top commit:
LLFourn:
ACK 05771a81d7
ValuedMammal:
ACK 05771a81d7
notmandatory:
ACK 05771a81d7
Tree-SHA512: f74dedf458853fb19cd21dedb5b92158acd865ee0ab0fd6bbb2b3e267bac22edc7cf004d2dc0068f66d2665d87e6dd419231710a02317e3b2bfaa9f408b30759
It updates the `NoCertificateVerification` implementation of
`rustls::client::danger::ServerCertVerifier` trait, it keeps the usage
of both `ServerCertVerified::assertion()` and
`HandshakeSignatureValid::assertion()` usage, but now instead of having
an empty vector vector of supported `SignatureScheme`, it uses the ones
supported by the used `CryptoProvider`.
4dd7e2135e Bump version to 0.21.0 and update CHANGELOG.md (Steve Myers)
Pull request description:
Bumped crate version to 0.21.0 and added below to changelog:
## 0.21.0
- Add use-rustls-ring feature #135
- refactor: make validate_merkle_proof more efficient #134
- chore: set rust edition to 2021, fix clippy, add ci fmt and clippy checks #139
## 0.20.0
- Upgrade rustls to 0.23 #132
- chore(deps): upgrade rust-bitcoin to 0.32.0 #133
- ci: add test with MSRV 1.63.0 #128
ACKs for top commit:
oleonardolima:
ACK 4dd7e2135e
ValuedMammal:
ACK 4dd7e2135e
Tree-SHA512: 3fcec2fb437733eac235bccb1b9c8f6b706e7a713c71de85016adc93f7db128ca6eadb5e9d1d44df27f1b49cce139b222aa9c21343afcf25befdf80a47442e51
0dd51408a3 chore(ci): add fmt and clippy checks (Steve Myers)
e7af332684 chore: fix clippy errors and set msrv to 1.63 (Steve Myers)
84f4f609b9 chore: set rust edition to 2021 (Steve Myers)
Pull request description:
A little house keeping to avoid the warning about missing the edition. After setting the rust edition to 2021 I had to fix clippy warnings. I also added CI fmt and clippy checks.
ACKs for top commit:
ValuedMammal:
ACK 0dd51408a3
oleonardolima:
ACK 0dd51408a3
Tree-SHA512: ee37c3ef06e217f76dcb622cdbb53e9b5c94163b789ca584037d5a8bb4bdef13c6cf251b555ac0e0ede309d7436295f1ede4e2ac6c0dc141c9bf2e3d6ecf9238
3a1f1bffb0 refactor: make `validate_merkle_proof` more efficient (志宇)
Pull request description:
ACKs for top commit:
notmandatory:
ACK 3a1f1bffb0
Tree-SHA512: 995d1582bc13d21c13b76dcd5e5edb633a59588e713b1e0aaf33363e17c47aafb22c7f250483e60cdc68e85fc94f041a0c39815160dc2592bedf959679f804dc
8d71f9597c feat: add use-rustls-ring feature (thunderbiscuit)
Pull request description:
This PR adds the ability to build the client using the `ring` dependency for `rustls` instead of the new default `aws-lc-rs`.
As of the [`0.23.0` release](https://github.com/rustls/rustls/releases/tag/v%2F0.23.0), rustls changed its default cryptography provider to [aws-lc-rs](https://crates.io/crates/aws-lc-rs). This new library is actually a set of bindings to a C library maintained by AWS, and they provide prebuilt bindings for [some platforms](https://aws.github.io/aws-lc-rs/platform_support.html) but not all. On these other platforms, the compilation step will attempt to build the bindings, requiring extra dependencies (CMake, libclang and others depending on the platform). This compilation step is what is currently breaking our Android and Swift builds for bdk-ffi. It is certainly possible to build the bindings (and the AWS docs on it are very nice), but for some reason I have not been able to make it work everywhere yet (local, CI, Windows).
This PR enables us to use the previous default `ring` library for rustls. I basically have to turn off the default features on `rustls` and re-enable all of them _except_ for the `aws_lc_rs`. We also have a few feature-gated constructs in the library, for which I needed to add the new proposed `use-rustls-ring` feature in order to make all of this work for us. Let me know if there are maybe better ways to achieve this!
ACKs for top commit:
oleonardolima:
ACK 8d71f9597c
notmandatory:
ACK 8d71f9597c
Tree-SHA512: 5ea8bfac7a18700e32035518e9e8253252c8ff9064b011e14a060ac8ed7b478876ee408ce06a89af9e53de837ffa9a13fbe5030d12b48a76558fd4e8187e5651
4b421084e9 ci: add test with MSRV 1.63.0 (Steve Myers)
Pull request description:
Since the main BDK crates are changing to MSRV 1.63.0 as is LDK the CI for this project should also have an MSRV and test against it in CI.
Also rustc 1.63 is the version shipped with the current debian stable (bookworm): https://packages.debian.org/stable/rust/rustc
ACKs for top commit:
oleonardolima:
utACK 4b421084e9
storopoli:
ACK 4b42108
ValuedMammal:
ACK 4b421084e9 looks good to me
Tree-SHA512: baff75887008a586af0ef0c4f50dd8c7c3de782f2dc6bc7d94c072c95205d4ef1a339860b3674dd781ad5558b98b071ed49b3a48507219b747e14c394dfe0a7b
6cf723504f deps: bump crate version to `0.20.0` (Leonardo Lima)
78cb066966 chore(deps)): upgrade `rust-bitcoin` to `0.32.0` (Leonardo Lima)
Pull request description:
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
partially fixes [#1422](https://github.com/bitcoindevkit/bdk/issues/1422)
### Description
It updates the rust-bitcoin to 0.32.0, the `bitcoin` crate dependency.
_NOTE: The overall BDK update to `0.32.0` still requires and depends on some other crates, please refer to [#1422](https://github.com/bitcoindevkit/bdk/issues/1422)._
<!-- Describe the purpose of this PR, what's being adding and/or fixed -->
### Notes to the reviewers
It's open for any comments.
<!-- In this section you can include notes directed to the reviewers, like explaining why some parts
of the PR were done in a specific way -->
### Changelog notice
- Update the `bitcoin` crate dependency to `0.32.0`
<!-- Notice the release manager should include in the release tag message changelog -->
<!-- See https://keepachangelog.com/en/1.0.0/ for examples -->
### Checklists
#### All Submissions:
* [x] I've signed all my commits
* [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
* [x] I ran `cargo fmt` and `cargo clippy` before committing
ACKs for top commit:
notmandatory:
utACK 6cf723504f
Tree-SHA512: c1e170d8da7687b40916b7c2de48f08ca393a2af79522abc85933bae1da6f79d2aa05d59c73b99dcbd01cc7add4def1b8a14e7858550d4e7a007c07279b45854
28b1aaa0c3 upgrade rustls to 0.23 (Nick Farrow)
Pull request description:
With rustls 0.23 there is no longer a dependency on ring, allowing easier compilation for various targets.
Not super confident with my updates to `ServerCertVerifier` and `Der` of certificates (is this being tested?), needs review.
ACKs for top commit:
notmandatory:
utACK 28b1aaa0c3
Tree-SHA512: 6561c4d20d446d86ca7a6c04ddb5a8acb136756606c82ca00e9b4a1f0eb2a3b00120d6db475f14474a89ebaa2ad600208d51c777cb5aeed0dcf62335a84fee5a
ef9fd6bf50 Bump version to 0.19.0 and add CHANGELOG.md (Steve Myers)
Pull request description:
Besides bumping the version I've also added a simple changelog that list the PRs in this (0.19.0) and the prior (0.18.0) release. The main reason for this release is the bump of the rust-bitcoin version to 0.31.0 which is needed to upgrade dependent projects like bdk to also use rust-bitcoin 0.31.0.
ACKs for top commit:
evanlinjin:
ACK ef9fd6bf50
Tree-SHA512: c18a0312915adfad40ef900ac4f1ee10454863f7a29882f1fa8f967f4c13df34b4e3929703614d0c3e6b8a189338f3a4d15b8d4598f68b87948454184fb13957
d8554fb550 enforce timeout on initial socks5 proxy connection (conduition)
Pull request description:
This PR fixes a bug which excepted initial SOCKS5 proxy connection attempts from the punctual enforcement of timeouts.
Before this change, invoking `Socks5Stream::connect` (or `Socks5Stream::connect_with_password`) could block for much longer than the configured timeout. In practice, this manifested as `Client::from_config` apparently failing to respect the timeout specified in the `Config` passed to it. AFAICT this only applied to SOCKS proxy connections.
## Example
To demonstrate, here is a simple example program which attempts to connect to an unreachable electrum server with a 10 second timeout.
```rust
use electrum_client::{Client, ConfigBuilder, Socks5Config};
fn main() {
let proxy = Socks5Config::new("127.0.0.1:9050");
let config = ConfigBuilder::new()
.socks5(Some(proxy))
.timeout(Some(10))
.build();
let start = std::time::SystemTime::now();
let result = Client::from_config(
"tcp://bejqtnc64qttdempkczylydg7l3ordwugbdar7yqbndck53ukx7wnwad.onion:50001",
config,
);
match result {
Ok(_) => {
println!("Successfully connected")
}
Err(e) => {
println!(
"failed to connect after {:.2}s: {e}",
start.elapsed().unwrap().as_secs_f64()
);
}
};
}
```
You'd expect the connection attempt to always fail at around 10 seconds, but in fact most attempts take considerably longer.
```
$ for i in {1..10} ; do cargo run -q ; done
failed to connect after 7.65s: host unreachable
failed to connect after 47.78s: host unreachable
failed to connect after 18.17s: host unreachable
failed to connect after 29.24s: host unreachable
failed to connect after 16.15s: host unreachable
failed to connect after 14.40s: host unreachable
failed to connect after 16.89s: host unreachable
failed to connect after 9.93s: host unreachable
failed to connect after 8.81s: host unreachable
failed to connect after 17.80s: host unreachable
```
## Cause and Fix
This was happening because the private method `Socks5Stream::connect_raw` [only respected the `timeout` parameter for _the initial connection to the proxy address_](5ecb26fd7d/src/socks/v5.rs (L200-L205)).
Once that TCP socket is established, the SOCKS5 client code must exchange a couple of messages with the proxy itself: One request/response cycle to authenticate, and then another request/response cycle to configure the forward proxy to the ultimate destination address. The latter of these two request/response cycles could block for long periods of time, in the case where the proxy was responsive but the ultimate destination was unresponsive.
Since no timeout was set on the socket at this stage, the `Socks5Stream` code would wait for an indefinite amount of time for a reply from the proxy, usually only once the proxy itself times out and finally sends a reply.
My suggested fix in this PR is to set the read/write timeouts immediately on the socket connecting to the proxy, so that if the proxy doesn't reply in time, we return an error to the caller promptly.
ACKs for top commit:
RCasatta:
utACK d8554fb550
Tree-SHA512: 4bc0ca203465c0d9722680de7251ee49dbea6d5a2b2a833a1ed42e792342a7ac977ad72649603caacd3e58d233b1a516af1ad40c6935addb8165b720d823cf42
dd3c171a7a Upgrade bitcoin (Tobin C. Harding)
Pull request description:
Upgrade bitcoin dependency to `rust-bitcoin v0.31.0-rc1`:
Allows us to remove the dependency on `bitcoin-private` because the `hex` stuff is exposed by `bitcoin` now.
ACKs for top commit:
notmandatory:
ACK dd3c171a7a
Tree-SHA512: 9082d3c2136445230bb23669f83fed58c90d1baf28d35527fc3dea1d40c9d3bdebeddbc44b3bdbc8e79c9b701e09cb453c018af0bc4ac8bcd1c4a14d11c90e39
Upgrade bitcoin dependency to `rust-bitcoin v0.31.0`:
Allows us to remove the dependency on `bitcoin-private` because the
`hex` stuff is exposed by `bitcoin` now.
54fd52d898 Add test coverage for `validate_merkle_proof` (Elias Rohrer)
fe33e19bda Add utility for validating a Merkle inclusion proof (Elias Rohrer)
dd872d6714 Make response types `Clone` (Elias Rohrer)
Pull request description:
I recently needed to validate a Merkle inclusion proof as retrieved via `transaction_get_merkle`.
As I figured it might be useful to other people, too, we add it here as a simple utility method.
ACKs for top commit:
notmandatory:
ACK 54fd52d898
Tree-SHA512: aac12160d5b91a011988f45013eb92924c2dfb244c1720e73dc5bcb731e69065c38e022502c756100d8ee6c9af06efa0de9bbfbb2b9e3c2e34d3223539206e1c
a331ae8059 Remove `webpki` and bump `webpki-roots` to `v0.25` (Yuki Kishimoto)
Pull request description:
I noticed that `webpki` dependency is no longer maintained and that has a high severity vulnerability.
This PR remove the `webpki` dependency and bump `webpki-roots` to v0.25
ACKs for top commit:
notmandatory:
ACK a331ae8059
Tree-SHA512: 63e9498dc0d56a07e7dd09dd43ca9a924d7e9ebb09934f2c762e64c9ce163cd58edb4d1563db4eba18a0fdf22642cb7d801940baeb97b6ce5473970b739278d4
adb0c7444e Add `Batch::raw` and improve docs (志宇)
Pull request description:
Being able to add raw requests to `Batch` makes sense from an API standpoint because we already allow raw non-batched requests. This is also useful when the electrum server API gets an updated version and our client is unable to keep up.
Additional to this, I have improved the documentation and made `Call` private (since `Call` is never used externally).
ACKs for top commit:
notmandatory:
ACK adb0c7444e
Tree-SHA512: 808dccf1152b750881e45a9709fb4127835ecff3da5ecccffcb9f03e62171192c58154860195db7d3d3467ae8e3e450bba845ff4e8d4dffb302c3d8d6eb837fd
3f6dd0c796 Bump to version 0.18 (Daniela Brozzoni)
Pull request description:
ACKs for top commit:
RCasatta:
ACK 3f6dd0c796
Tree-SHA512: f42f2008d79b9a0334c375652842fb2745d2481422a24f27dd4e350bcc17e47f0da2888545a344e1438ede288ed2ad0de8b359b87004fbe059f310aad886c60a
2d44350b44 Revert "errors if expecing headers notification but not subscribed" (Riccardo Casatta)
Pull request description:
This reverts commit b86f2bb22c.
Some errors started to happen in downstream tests after this commit
#114
ACKs for top commit:
danielabrozzoni:
utACK 2d44350b44
Tree-SHA512: d31174055f4245cc9d99f336b166a44271067b8daecaf2bb55d507ecfa4eb557b9802d576742fba59fd4dfda3f45fc76f02d7896af31353f19fc3a38698ac5a2
the mtgox address used got too many txs, resulting in a server error:
"too many txs"
The address is changed with the peter todd sha256 breaking challenge, which
should be very difficult to spend :) and as of now has just a few txs
f5a438cd0a Bump version to 0.16.0 (Tobin C. Harding)
9a7cc14c94 Update rustls dependency to version 0.21 (Tobin C. Harding)
Pull request description:
Update the `rustls` dependency to version 0.21 and bump the version of this crate to 0.16 so the change can be released.
I don't know what stage in your release cycle you guys are up to so I put the version bump as a separate patch - can drop it if not needed.
Done to help with https://github.com/bitcoindevkit/rust-esplora-client/pull/51
ACKs for top commit:
danielabrozzoni:
utACK f5a438cd0a
Tree-SHA512: 106cbe57651a6e1365f87836598cbec9b1e837f0376ddc1c56cb75e5615543659f5c05cea7d5eb8da8b6cf278fcbe6ef866a019b9829db40cc8055798eb0f541
