Bitreq client builder (rustls + native-tls)

[ton-anywhere]

Summary

This PR addresses issue #473 by creating a Client Builder able to receive custom root certificates at runtime.
The ClientBuilder requires async-https-rustls or async-https-native-tls features.

Changes

• Add Client Builder pattern.
• Creates TlsConfig module.
• Append certificate using with_root_certificate builder method.
• Disable default certificates using disable_default_certificates builder method.
• Load certificates once per client instead of per connection.
• Supports multiple custom certificates.

This PR substitutes the 502

[tcharding]

Hi @APonce911, little comment on dev process. Its better if you rebase on master instead of merging if you want review. With 65 patches reviewers are not going to know where to look. Many of us review by pulling down the PR and reading each patch in our terminals.

Different folk treat things differently but to me if a PR is 'open' it is a request to merge so it should be in a mergable state. We use the process where each commit should be a single logical change. There shouldn't be WIP commits and 'fix something I did three commits ago' commits. The whole set of commits (aka the 'patch set') should be clean and reviewable as stand alone changes. Each described in the commit log.

[ton-anywhere]

@tcharding thanks for the feedback. That's something I'll keep in mind! As I've told you, I'm not used to open source so every bit of insight of how you do things is helpful. The process is a bit different than what I'm used to but I'm more than happy to adapt.

About those patches and then small subsequent fixes, I've had a hard time trying to replicate the CI suite locally, it didn't catch some corner cases. I could have reset some commits, but didn't think about it by that time.

[tcharding]

In a perfect world every commit builds and passes CI cleanly. We do not however check each commit in CI. But locally during dev you should really, in my opinion, be trying to make sure that at least cargo test --all-features and cargo test --no-default-features run cleanly on each commit [0]. That is the commands I default to and I tend to rely on CI to catch my other mistakes. Your mileage may vary. I don't even run just sane in bitcoin because I'm too impatient, but I more-or-less have a gut feel for what sorts of changes will break what test commands so I amend them if I need to (adding features ect.) Its not a perfect system by any means.

[0] I have shell alias' for these

[tcharding]

If you want to get past the ci-doesn't-run-for-first-time-contributors just throw up a quick docs fix PR and I'll merge it.

[ton-anywhere]

@tcharding will definitely do that! However I will be off for a few days and won't be able to proceed with the rebase adjustments and doc pr until next week. Thanks for you time again!

[TheBlueMatt]

Okay this is starting to look pretty good. Aside from the above and below, we now need to clean up the git history...You can try rebasing, but it might at this point be simpler to just git reset upstream/master and then manually re-commit what you have.

[TheBlueMatt]

Awesome! Can you clean up the cfg flags so that they're more condensed? It would make the code a ton more readable.

[ton-anywhere]

working on the rebase

[ton-anywhere]

rebased. @TheBlueMatt whenever you can, it's ready for review again.

[tcharding]

Needs rebase again pls mate.

[ton-anywhere]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

[jamillambert]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

I just checked and cargo test fails one test with features="std rustls". native-tls is the same as https-native-tls so makes no difference. tokio-native-tls passes with std but there may be another combination that causes an issue with it.

[ton-anywhere]

Guys I've created a PR for the TLS gates that can help us here, standardizing it for both TLS backends.

After that i'll proceed with the git rebase.

[tcharding]

Please note development has migrated to https://git.rust-bitcoin.org/rust-bitcoin/corepc. Any further comments or pushes here on github may be ignored or lost.

Please consider containing this PR on the forgejo instance.

[ton-anywhere]

Summary

This PR addresses issue #473 by creating a Client Builder able to receive custom root certificates at runtime.
The ClientBuilder requires async-https-rustls or async-https-native-tls features.

Changes

• Add Client Builder pattern.
• Creates TlsConfig module.
• Append certificate using with_root_certificate builder method.
• Disable default certificates using disable_default_certificates builder method.
• Load certificates once per client instead of per connection.
• Supports multiple custom certificates.

This PR substitutes the 502

[tcharding]

Hi @APonce911, little comment on dev process. Its better if you rebase on master instead of merging if you want review. With 65 patches reviewers are not going to know where to look. Many of us review by pulling down the PR and reading each patch in our terminals.

Different folk treat things differently but to me if a PR is 'open' it is a request to merge so it should be in a mergable state. We use the process where each commit should be a single logical change. There shouldn't be WIP commits and 'fix something I did three commits ago' commits. The whole set of commits (aka the 'patch set') should be clean and reviewable as stand alone changes. Each described in the commit log.

[ton-anywhere]

@tcharding thanks for the feedback. That's something I'll keep in mind! As I've told you, I'm not used to open source so every bit of insight of how you do things is helpful. The process is a bit different than what I'm used to but I'm more than happy to adapt.

About those patches and then small subsequent fixes, I've had a hard time trying to replicate the CI suite locally, it didn't catch some corner cases. I could have reset some commits, but didn't think about it by that time.

[tcharding]

In a perfect world every commit builds and passes CI cleanly. We do not however check each commit in CI. But locally during dev you should really, in my opinion, be trying to make sure that at least cargo test --all-features and cargo test --no-default-features run cleanly on each commit [0]. That is the commands I default to and I tend to rely on CI to catch my other mistakes. Your mileage may vary. I don't even run just sane in bitcoin because I'm too impatient, but I more-or-less have a gut feel for what sorts of changes will break what test commands so I amend them if I need to (adding features ect.) Its not a perfect system by any means.

[0] I have shell alias' for these

[tcharding]

If you want to get past the ci-doesn't-run-for-first-time-contributors just throw up a quick docs fix PR and I'll merge it.

[ton-anywhere]

@tcharding will definitely do that! However I will be off for a few days and won't be able to proceed with the rebase adjustments and doc pr until next week. Thanks for you time again!

[TheBlueMatt]

Okay this is starting to look pretty good. Aside from the above and below, we now need to clean up the git history...You can try rebasing, but it might at this point be simpler to just git reset upstream/master and then manually re-commit what you have.

[TheBlueMatt]

Awesome! Can you clean up the cfg flags so that they're more condensed? It would make the code a ton more readable.

[ton-anywhere]

working on the rebase

[ton-anywhere]

rebased. @TheBlueMatt whenever you can, it's ready for review again.

[tcharding]

Needs rebase again pls mate.

[ton-anywhere]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

[jamillambert]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

I just checked and cargo test fails one test with features="std rustls". native-tls is the same as https-native-tls so makes no difference. tokio-native-tls passes with std but there may be another combination that causes an issue with it.

[ton-anywhere]

Guys I've created a PR for the TLS gates that can help us here, standardizing it for both TLS backends.

After that i'll proceed with the git rebase.

[tcharding]

Please note development has migrated to https://git.rust-bitcoin.org/rust-bitcoin/corepc. Any further comments or pushes here on github may be ignored or lost.

Please consider containing this PR on the forgejo instance.

[ton-anywhere]

Summary

This PR addresses issue #473 by creating a Client Builder able to receive custom root certificates at runtime.
The ClientBuilder requires async-https-rustls or async-https-native-tls features.

Changes

• Add Client Builder pattern.
• Creates TlsConfig module.
• Append certificate using with_root_certificate builder method.
• Disable default certificates using disable_default_certificates builder method.
• Load certificates once per client instead of per connection.
• Supports multiple custom certificates.

This PR substitutes the 502

[tcharding]

Hi @APonce911, little comment on dev process. Its better if you rebase on master instead of merging if you want review. With 65 patches reviewers are not going to know where to look. Many of us review by pulling down the PR and reading each patch in our terminals.

Different folk treat things differently but to me if a PR is 'open' it is a request to merge so it should be in a mergable state. We use the process where each commit should be a single logical change. There shouldn't be WIP commits and 'fix something I did three commits ago' commits. The whole set of commits (aka the 'patch set') should be clean and reviewable as stand alone changes. Each described in the commit log.

[ton-anywhere]

@tcharding thanks for the feedback. That's something I'll keep in mind! As I've told you, I'm not used to open source so every bit of insight of how you do things is helpful. The process is a bit different than what I'm used to but I'm more than happy to adapt.

About those patches and then small subsequent fixes, I've had a hard time trying to replicate the CI suite locally, it didn't catch some corner cases. I could have reset some commits, but didn't think about it by that time.

[tcharding]

In a perfect world every commit builds and passes CI cleanly. We do not however check each commit in CI. But locally during dev you should really, in my opinion, be trying to make sure that at least cargo test --all-features and cargo test --no-default-features run cleanly on each commit [0]. That is the commands I default to and I tend to rely on CI to catch my other mistakes. Your mileage may vary. I don't even run just sane in bitcoin because I'm too impatient, but I more-or-less have a gut feel for what sorts of changes will break what test commands so I amend them if I need to (adding features ect.) Its not a perfect system by any means.

[0] I have shell alias' for these

[tcharding]

If you want to get past the ci-doesn't-run-for-first-time-contributors just throw up a quick docs fix PR and I'll merge it.

[ton-anywhere]

@tcharding will definitely do that! However I will be off for a few days and won't be able to proceed with the rebase adjustments and doc pr until next week. Thanks for you time again!

[TheBlueMatt]

Okay this is starting to look pretty good. Aside from the above and below, we now need to clean up the git history...You can try rebasing, but it might at this point be simpler to just git reset upstream/master and then manually re-commit what you have.

[TheBlueMatt]

Awesome! Can you clean up the cfg flags so that they're more condensed? It would make the code a ton more readable.

[ton-anywhere]

working on the rebase

[ton-anywhere]

rebased. @TheBlueMatt whenever you can, it's ready for review again.

[tcharding]

Needs rebase again pls mate.

[ton-anywhere]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

[jamillambert]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

I just checked and cargo test fails one test with features="std rustls". native-tls is the same as https-native-tls so makes no difference. tokio-native-tls passes with std but there may be another combination that causes an issue with it.

[ton-anywhere]

Guys I've created a PR for the TLS gates that can help us here, standardizing it for both TLS backends.

After that i'll proceed with the git rebase.

[tcharding]

Please note development has migrated to https://git.rust-bitcoin.org/rust-bitcoin/corepc. Any further comments or pushes here on github may be ignored or lost.

Please consider containing this PR on the forgejo instance.

[ton-anywhere]

Summary

This PR addresses issue #473 by creating a Client Builder able to receive custom root certificates at runtime.
The ClientBuilder requires async-https-rustls or async-https-native-tls features.

Changes

• Add Client Builder pattern.
• Creates TlsConfig module.
• Append certificate using with_root_certificate builder method.
• Disable default certificates using disable_default_certificates builder method.
• Load certificates once per client instead of per connection.
• Supports multiple custom certificates.

This PR substitutes the 502

[tcharding]

Hi @APonce911, little comment on dev process. Its better if you rebase on master instead of merging if you want review. With 65 patches reviewers are not going to know where to look. Many of us review by pulling down the PR and reading each patch in our terminals.

Different folk treat things differently but to me if a PR is 'open' it is a request to merge so it should be in a mergable state. We use the process where each commit should be a single logical change. There shouldn't be WIP commits and 'fix something I did three commits ago' commits. The whole set of commits (aka the 'patch set') should be clean and reviewable as stand alone changes. Each described in the commit log.

[ton-anywhere]

@tcharding thanks for the feedback. That's something I'll keep in mind! As I've told you, I'm not used to open source so every bit of insight of how you do things is helpful. The process is a bit different than what I'm used to but I'm more than happy to adapt.

About those patches and then small subsequent fixes, I've had a hard time trying to replicate the CI suite locally, it didn't catch some corner cases. I could have reset some commits, but didn't think about it by that time.

[tcharding]

In a perfect world every commit builds and passes CI cleanly. We do not however check each commit in CI. But locally during dev you should really, in my opinion, be trying to make sure that at least cargo test --all-features and cargo test --no-default-features run cleanly on each commit [0]. That is the commands I default to and I tend to rely on CI to catch my other mistakes. Your mileage may vary. I don't even run just sane in bitcoin because I'm too impatient, but I more-or-less have a gut feel for what sorts of changes will break what test commands so I amend them if I need to (adding features ect.) Its not a perfect system by any means.

[0] I have shell alias' for these

[tcharding]

If you want to get past the ci-doesn't-run-for-first-time-contributors just throw up a quick docs fix PR and I'll merge it.

[ton-anywhere]

@tcharding will definitely do that! However I will be off for a few days and won't be able to proceed with the rebase adjustments and doc pr until next week. Thanks for you time again!

[TheBlueMatt]

Okay this is starting to look pretty good. Aside from the above and below, we now need to clean up the git history...You can try rebasing, but it might at this point be simpler to just git reset upstream/master and then manually re-commit what you have.

[TheBlueMatt]

Awesome! Can you clean up the cfg flags so that they're more condensed? It would make the code a ton more readable.

[ton-anywhere]

working on the rebase

[ton-anywhere]

rebased. @TheBlueMatt whenever you can, it's ready for review again.

[tcharding]

Needs rebase again pls mate.

[ton-anywhere]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

[jamillambert]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

I just checked and cargo test fails one test with features="std rustls". native-tls is the same as https-native-tls so makes no difference. tokio-native-tls passes with std but there may be another combination that causes an issue with it.

[ton-anywhere]

Guys I've created a PR for the TLS gates that can help us here, standardizing it for both TLS backends.

After that i'll proceed with the git rebase.

[tcharding]

Please note development has migrated to https://git.rust-bitcoin.org/rust-bitcoin/corepc. Any further comments or pushes here on github may be ignored or lost.

Please consider containing this PR on the forgejo instance.

[ton-anywhere]

Summary

This PR addresses issue #473 by creating a Client Builder able to receive custom root certificates at runtime.
The ClientBuilder requires async-https-rustls or async-https-native-tls features.

Changes

• Add Client Builder pattern.
• Creates TlsConfig module.
• Append certificate using with_root_certificate builder method.
• Disable default certificates using disable_default_certificates builder method.
• Load certificates once per client instead of per connection.
• Supports multiple custom certificates.

This PR substitutes the 502

[tcharding]

Hi @APonce911, little comment on dev process. Its better if you rebase on master instead of merging if you want review. With 65 patches reviewers are not going to know where to look. Many of us review by pulling down the PR and reading each patch in our terminals.

Different folk treat things differently but to me if a PR is 'open' it is a request to merge so it should be in a mergable state. We use the process where each commit should be a single logical change. There shouldn't be WIP commits and 'fix something I did three commits ago' commits. The whole set of commits (aka the 'patch set') should be clean and reviewable as stand alone changes. Each described in the commit log.

[ton-anywhere]

@tcharding thanks for the feedback. That's something I'll keep in mind! As I've told you, I'm not used to open source so every bit of insight of how you do things is helpful. The process is a bit different than what I'm used to but I'm more than happy to adapt.

About those patches and then small subsequent fixes, I've had a hard time trying to replicate the CI suite locally, it didn't catch some corner cases. I could have reset some commits, but didn't think about it by that time.

[tcharding]

In a perfect world every commit builds and passes CI cleanly. We do not however check each commit in CI. But locally during dev you should really, in my opinion, be trying to make sure that at least cargo test --all-features and cargo test --no-default-features run cleanly on each commit [0]. That is the commands I default to and I tend to rely on CI to catch my other mistakes. Your mileage may vary. I don't even run just sane in bitcoin because I'm too impatient, but I more-or-less have a gut feel for what sorts of changes will break what test commands so I amend them if I need to (adding features ect.) Its not a perfect system by any means.

[0] I have shell alias' for these

[tcharding]

If you want to get past the ci-doesn't-run-for-first-time-contributors just throw up a quick docs fix PR and I'll merge it.

[ton-anywhere]

@tcharding will definitely do that! However I will be off for a few days and won't be able to proceed with the rebase adjustments and doc pr until next week. Thanks for you time again!

[TheBlueMatt]

Okay this is starting to look pretty good. Aside from the above and below, we now need to clean up the git history...You can try rebasing, but it might at this point be simpler to just git reset upstream/master and then manually re-commit what you have.

[TheBlueMatt]

Awesome! Can you clean up the cfg flags so that they're more condensed? It would make the code a ton more readable.

[ton-anywhere]

working on the rebase

[ton-anywhere]

rebased. @TheBlueMatt whenever you can, it's ready for review again.

[tcharding]

Needs rebase again pls mate.

[ton-anywhere]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

[jamillambert]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

I just checked and cargo test fails one test with features="std rustls". native-tls is the same as https-native-tls so makes no difference. tokio-native-tls passes with std but there may be another combination that causes an issue with it.

[ton-anywhere]

Guys I've created a PR for the TLS gates that can help us here, standardizing it for both TLS backends.

After that i'll proceed with the git rebase.

[tcharding]

Please note development has migrated to https://git.rust-bitcoin.org/rust-bitcoin/corepc. Any further comments or pushes here on github may be ignored or lost.

Please consider containing this PR on the forgejo instance.

[ton-anywhere]

Summary

This PR addresses issue #473 by creating a Client Builder able to receive custom root certificates at runtime.
The ClientBuilder requires async-https-rustls or async-https-native-tls features.

Changes

• Add Client Builder pattern.
• Creates TlsConfig module.
• Append certificate using with_root_certificate builder method.
• Disable default certificates using disable_default_certificates builder method.
• Load certificates once per client instead of per connection.
• Supports multiple custom certificates.

This PR substitutes the 502

[tcharding]

Hi @APonce911, little comment on dev process. Its better if you rebase on master instead of merging if you want review. With 65 patches reviewers are not going to know where to look. Many of us review by pulling down the PR and reading each patch in our terminals.

Different folk treat things differently but to me if a PR is 'open' it is a request to merge so it should be in a mergable state. We use the process where each commit should be a single logical change. There shouldn't be WIP commits and 'fix something I did three commits ago' commits. The whole set of commits (aka the 'patch set') should be clean and reviewable as stand alone changes. Each described in the commit log.

[ton-anywhere]

@tcharding thanks for the feedback. That's something I'll keep in mind! As I've told you, I'm not used to open source so every bit of insight of how you do things is helpful. The process is a bit different than what I'm used to but I'm more than happy to adapt.

About those patches and then small subsequent fixes, I've had a hard time trying to replicate the CI suite locally, it didn't catch some corner cases. I could have reset some commits, but didn't think about it by that time.

[tcharding]

In a perfect world every commit builds and passes CI cleanly. We do not however check each commit in CI. But locally during dev you should really, in my opinion, be trying to make sure that at least cargo test --all-features and cargo test --no-default-features run cleanly on each commit [0]. That is the commands I default to and I tend to rely on CI to catch my other mistakes. Your mileage may vary. I don't even run just sane in bitcoin because I'm too impatient, but I more-or-less have a gut feel for what sorts of changes will break what test commands so I amend them if I need to (adding features ect.) Its not a perfect system by any means.

[0] I have shell alias' for these

[tcharding]

If you want to get past the ci-doesn't-run-for-first-time-contributors just throw up a quick docs fix PR and I'll merge it.

[ton-anywhere]

@tcharding will definitely do that! However I will be off for a few days and won't be able to proceed with the rebase adjustments and doc pr until next week. Thanks for you time again!

[TheBlueMatt]

Okay this is starting to look pretty good. Aside from the above and below, we now need to clean up the git history...You can try rebasing, but it might at this point be simpler to just git reset upstream/master and then manually re-commit what you have.

[TheBlueMatt]

Awesome! Can you clean up the cfg flags so that they're more condensed? It would make the code a ton more readable.

[ton-anywhere]

working on the rebase

[ton-anywhere]

rebased. @TheBlueMatt whenever you can, it's ready for review again.

[tcharding]

Needs rebase again pls mate.

[ton-anywhere]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

[jamillambert]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

I just checked and cargo test fails one test with features="std rustls". native-tls is the same as https-native-tls so makes no difference. tokio-native-tls passes with std but there may be another combination that causes an issue with it.

[ton-anywhere]

Guys I've created a PR for the TLS gates that can help us here, standardizing it for both TLS backends.

After that i'll proceed with the git rebase.

[tcharding]

Please note development has migrated to https://git.rust-bitcoin.org/rust-bitcoin/corepc. Any further comments or pushes here on github may be ignored or lost.

Please consider containing this PR on the forgejo instance.

[ton-anywhere]

Summary

This PR addresses issue #473 by creating a Client Builder able to receive custom root certificates at runtime.
The ClientBuilder requires async-https-rustls or async-https-native-tls features.

Changes

• Add Client Builder pattern.
• Creates TlsConfig module.
• Append certificate using with_root_certificate builder method.
• Disable default certificates using disable_default_certificates builder method.
• Load certificates once per client instead of per connection.
• Supports multiple custom certificates.

This PR substitutes the 502

[tcharding]

Hi @APonce911, little comment on dev process. Its better if you rebase on master instead of merging if you want review. With 65 patches reviewers are not going to know where to look. Many of us review by pulling down the PR and reading each patch in our terminals.

Different folk treat things differently but to me if a PR is 'open' it is a request to merge so it should be in a mergable state. We use the process where each commit should be a single logical change. There shouldn't be WIP commits and 'fix something I did three commits ago' commits. The whole set of commits (aka the 'patch set') should be clean and reviewable as stand alone changes. Each described in the commit log.

[ton-anywhere]

@tcharding thanks for the feedback. That's something I'll keep in mind! As I've told you, I'm not used to open source so every bit of insight of how you do things is helpful. The process is a bit different than what I'm used to but I'm more than happy to adapt.

About those patches and then small subsequent fixes, I've had a hard time trying to replicate the CI suite locally, it didn't catch some corner cases. I could have reset some commits, but didn't think about it by that time.

[tcharding]

In a perfect world every commit builds and passes CI cleanly. We do not however check each commit in CI. But locally during dev you should really, in my opinion, be trying to make sure that at least cargo test --all-features and cargo test --no-default-features run cleanly on each commit [0]. That is the commands I default to and I tend to rely on CI to catch my other mistakes. Your mileage may vary. I don't even run just sane in bitcoin because I'm too impatient, but I more-or-less have a gut feel for what sorts of changes will break what test commands so I amend them if I need to (adding features ect.) Its not a perfect system by any means.

[0] I have shell alias' for these

[tcharding]

If you want to get past the ci-doesn't-run-for-first-time-contributors just throw up a quick docs fix PR and I'll merge it.

[ton-anywhere]

@tcharding will definitely do that! However I will be off for a few days and won't be able to proceed with the rebase adjustments and doc pr until next week. Thanks for you time again!

[TheBlueMatt]

Okay this is starting to look pretty good. Aside from the above and below, we now need to clean up the git history...You can try rebasing, but it might at this point be simpler to just git reset upstream/master and then manually re-commit what you have.

[TheBlueMatt]

Awesome! Can you clean up the cfg flags so that they're more condensed? It would make the code a ton more readable.

[ton-anywhere]

working on the rebase

[ton-anywhere]

rebased. @TheBlueMatt whenever you can, it's ready for review again.

[tcharding]

Needs rebase again pls mate.

[ton-anywhere]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

[jamillambert]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

I just checked and cargo test fails one test with features="std rustls". native-tls is the same as https-native-tls so makes no difference. tokio-native-tls passes with std but there may be another combination that causes an issue with it.

[ton-anywhere]

Guys I've created a PR for the TLS gates that can help us here, standardizing it for both TLS backends.

After that i'll proceed with the git rebase.

[tcharding]

Please note development has migrated to https://git.rust-bitcoin.org/rust-bitcoin/corepc. Any further comments or pushes here on github may be ignored or lost.

Please consider containing this PR on the forgejo instance.

[ton-anywhere]

Summary

This PR addresses issue #473 by creating a Client Builder able to receive custom root certificates at runtime.
The ClientBuilder requires async-https-rustls or async-https-native-tls features.

Changes

• Add Client Builder pattern.
• Creates TlsConfig module.
• Append certificate using with_root_certificate builder method.
• Disable default certificates using disable_default_certificates builder method.
• Load certificates once per client instead of per connection.
• Supports multiple custom certificates.

This PR substitutes the 502

[tcharding]

Hi @APonce911, little comment on dev process. Its better if you rebase on master instead of merging if you want review. With 65 patches reviewers are not going to know where to look. Many of us review by pulling down the PR and reading each patch in our terminals.

Different folk treat things differently but to me if a PR is 'open' it is a request to merge so it should be in a mergable state. We use the process where each commit should be a single logical change. There shouldn't be WIP commits and 'fix something I did three commits ago' commits. The whole set of commits (aka the 'patch set') should be clean and reviewable as stand alone changes. Each described in the commit log.

[ton-anywhere]

@tcharding thanks for the feedback. That's something I'll keep in mind! As I've told you, I'm not used to open source so every bit of insight of how you do things is helpful. The process is a bit different than what I'm used to but I'm more than happy to adapt.

About those patches and then small subsequent fixes, I've had a hard time trying to replicate the CI suite locally, it didn't catch some corner cases. I could have reset some commits, but didn't think about it by that time.

[tcharding]

In a perfect world every commit builds and passes CI cleanly. We do not however check each commit in CI. But locally during dev you should really, in my opinion, be trying to make sure that at least cargo test --all-features and cargo test --no-default-features run cleanly on each commit [0]. That is the commands I default to and I tend to rely on CI to catch my other mistakes. Your mileage may vary. I don't even run just sane in bitcoin because I'm too impatient, but I more-or-less have a gut feel for what sorts of changes will break what test commands so I amend them if I need to (adding features ect.) Its not a perfect system by any means.

[0] I have shell alias' for these

[tcharding]

If you want to get past the ci-doesn't-run-for-first-time-contributors just throw up a quick docs fix PR and I'll merge it.

[ton-anywhere]

@tcharding will definitely do that! However I will be off for a few days and won't be able to proceed with the rebase adjustments and doc pr until next week. Thanks for you time again!

[TheBlueMatt]

Okay this is starting to look pretty good. Aside from the above and below, we now need to clean up the git history...You can try rebasing, but it might at this point be simpler to just git reset upstream/master and then manually re-commit what you have.

[TheBlueMatt]

Awesome! Can you clean up the cfg flags so that they're more condensed? It would make the code a ton more readable.

[ton-anywhere]

working on the rebase

[ton-anywhere]

rebased. @TheBlueMatt whenever you can, it's ready for review again.

[tcharding]

Needs rebase again pls mate.

[ton-anywhere]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

[jamillambert]

@jamillambert we still have gates on the following dependencies: rustls, native-tls and tokio-native-tls. Should we gate them with Bitreq cargo features as well? (https-rustls, https-native-tls and async-https-native-tls)

If yes, I can work on that in a new PR before rebasing.

WDYT?

I just checked and cargo test fails one test with features="std rustls". native-tls is the same as https-native-tls so makes no difference. tokio-native-tls passes with std but there may be another combination that causes an issue with it.

[ton-anywhere]

Guys I've created a PR for the TLS gates that can help us here, standardizing it for both TLS backends.

After that i'll proceed with the git rebase.

[tcharding]

Please note development has migrated to https://git.rust-bitcoin.org/rust-bitcoin/corepc. Any further comments or pushes here on github may be ignored or lost.

Please consider containing this PR on the forgejo instance.