Change to work with updated libraries.

[jeandudey]

No description provided.

[apoelstra]

Thanks for all this work!
Is there any way we can get rid of any TLS dependencies anywhere?

[jeandudey]

@apoelstra I think it isn't possible right now, reqwest includes TLS support by default.

[apoelstra]

Hmmm, ok. That is a very serious regression vs the existing old libraries.

[apoelstra]

Are there any HTTP libraries that only deal with ASCII and don't support TLS?

I wonder how much work it would be to implement the bare minimum HTTP client required to work with Bitcoin Core.

[dpc]

@apoelstra Are there any specific constrains you're worried about? Stripping TLS is kind of understanble (though TLS is becoming inseparable part of HTTP), doing only ASCII in a language that is UTF-8 native, seems odd.

While it would probably be possible to make TLS support compile time option in reqwest, unless someone really needs that separation, and is willing to work on it, I would just include TLS and not worry about it, no?

[dpc]

Also, is this library supposed to be general purpose jsonrpc, or just Bitcoin Core one? :)

[apoelstra]

@dpc the goal is to minimize dependencies, hence the aversion to complex unicode (and other charsets) parsing libraries. Avoiding anything TLS related is a hard requirement because it provides zero value (anyone can use a reverse proxy to layer TLS onto whatever they're doing, and even get some isolation from their real application) and has historically been a source of serious security vulnerabilities.

I was under the impression that JSON was ASCII-only (hence it having the ridiculous \u escape), but according to www.json.org a char is a "Unicode" character, whatever that means.

[dpc]

@apoelstra Well, it is true, maybe much less severe in Rust, but still.

However, now I'm looking for a standard way to do jsonrpc in Rust, right now, and there is this library, which seems incompatible with reset of the ecosystem, but in the best shape and most popular one. While ability to disable TLS would be nice to have, but doesn't matter for my usecase.

Should I look for other options / fork?

[dpc]

~~I've created~~~ There is already: seanmonstar/reqwest#225 about TLS as compile time feature.

Other than that, personally, I would rather use a standard, well supported, well reviewed code, written in robust, memory safe language, than a stripped down version, that doesn't get a the same amount of maintenance and mind share.

[apoelstra]

The current version of this library works and is used in production systems. I'm not sure in what way it is "incompatible with the rest of the ecosystem", though of course it's good practice to keep up with dependency updates and replace deprecated libraries.

If you want to fix this in a way that would bring in a TLS dependency, yes, you will need to fork the project. Although I hope we can fix this upstream in reqwest -- thank you for opening an issue about it.

[dpc]

@apoelstra

I'm not sure in what way it is "incompatible with the rest of the ecosystem",

Mostly it's about serde being stabilized with 1.0, and it is hard to mix serde-0.6-comptible datastructures with serde-1.x-compatible ones.

[apoelstra]

:/. In strason we can't update to serde 1.0 because they broke the ability to detect decoders, so I can't pass decimals through it unscathed. They added support for arbitrary precision numerics but require a newer compiler than Debian ships in order to use it.

[jeandudey]

@apoelstra

What do you think about releasing a 0.9.0-beta version of the crate that uses the current reqwest version (including TLS support by default)? And when reqwest adds support for a no-tls feature, we bump the crate to 0.9.0.

I did this work to upgrade the dependencies only in order to be compatible with serde-1.0 and if you think it isn't right to use TLS by default (I think it should have a no-tls feature too) I can submit anohter PR only upgrading the serde dependency and delay the upgrade to reqwest until it gets the feature.

[apoelstra]

I think just updating serde is fine. I'd like things to still compile with Rust 1.14, but since this isn't an official rust-bitcoin project it's probably fine if it doesn't.

[jeandudey]

Done in #8

[apoelstra]

After further reflection, my plan is:

1. Increase the minimum compiler version to 1.21. This works with the latest hyper which has no SSL support, and is the minimum version for rust-url which suggests that I'm not going to get very far trying to decrease it further. (I'm going with hyper over reqwest for two reasons: the TLS thing, but also because even with all features disabled it has 82 dependencies which is unmanageable and anxiety-causing.)
2. Replace strason with serde_json since it now supports everything I need for Bitcoin, has better review, and does not use unsafe code.

I think this will drop our total dependency list to ~25 entries, mostly big projects with a history of stability and tiny projects which never change. Which compares very favorably to our current 30 dependencies, including some specific ones which have broken rust-jsonrpc in the past.

[apoelstra]

Are there any specific reasons you prefer reqwest to hyper?

[jeandudey]

@apoelstra I choose to use reqwest because it's more focused on synchronous I/O, and it's a little bit more type safe. You're right with reqwest having too much dependencies and a lot of them are unneeded for our use cases (during a build I saw it pulls libflate, for what are we going to use that?), my bad, sorry 😢.

I think it's more healthy to use the newest hyper. Also an asynchronous JSON-RPC client could be made.

[apoelstra]

Oof, so, hyper 0.11 introduced a lot of complex asynchronous tokio stuff that we don't need; 0.12 made it optional (yay!) but didn't replace it with anything, meaning that we'd have to implement a lot of connection management ourselves. So our choices with hyper 0.12 are either to use the default options, bringing us up to 45 deps (lots of which are quickly-changing tokio), or re-implement a lot of old-hyper on our own.

With that in mind I'm simply going to upgrade from hyper 0.9 to 0.10; the main changes there are dropping SSL and supporting code, dropping cookie and some supporting code, fixing this security vuln which did not affect rust-jsonrpc hyperium/hyper@2603d78 , and various bugfixes/perf improvements.