Add minreq usage #94

tcharding · 2023-05-03T05:48:19Z

Add a new module minreq_http that implements client::Transport using minreq as the underlying HTTP transport.

fuzzing infrastructure copied from rust-bitcoin
fuzz test copied from simple_http
basic integration test copied from rust-bitcoincore-rpc
A public function to parse a bitcoind cookie file

https://github.com/neonmoe/minreq/

apoelstra · 2023-05-06T19:28:18Z

Since #92 is merged maybe we can rebase and un-draft this?

apoelstra · 2023-05-08T15:16:53Z

CI failure is because we need to add a doccomment now to the serde_json re-export. Unrelated to this PR.

Cargo.toml

src/simple_minreq.rs

apoelstra · 2023-05-08T15:23:19Z

src/simple_minreq.rs

+    pub fn simple_http_minreq(
+        url: &str,
+        user: Option<String>,
+        pass: Option<String>,


In d20ba2e:

I think we should take &str rather than String here for both params. (Ideally it could be generic but that sucks because if the user passes None then the compiler will complain.)

What value do these helper constructors even add? I rekon we should get rid of them from all the modules.

Can you confirm that you are ok with my removal of these "helper" constrctors mate?

I had a quick check to make sure the new code was not non-uniform and only simlple_http provides these construtors on Client so its the odd one out not minreq_http.

src/simple_minreq.rs

src/lib.rs

src/simple_minreq.rs

apoelstra · 2023-05-08T15:32:43Z

src/simple_minreq.rs

+impl Default for SimpleHttpTransport {
+    fn default() -> Self {
+        SimpleHttpTransport {
+            url: format!("localhost:{}", DEFAULT_PORT),


In d20ba2e:

minreq seems to require a http:// prefix on this or else you get the error "was redirected to an absolute url with an invalid protocol".

I"ll add integration testing against a bitcoind instance like we do in rust-bitcoincorerpc eh, to catch this sort of mistake.

That would be amazing. TBH I usually test nontrivial PRs to this crate by syncing my hot wallet with it, which is probably not the safest testing strategy..

Added http:// to the string, FTR I have not tested this code path.

apoelstra · 2023-05-08T15:42:43Z

Done testing d20ba2e. It looks like it "just works", though it's a little bit (1-5% maybe?) slower than simple http.

OTOH if it works for Mac users without bullshit, that's a huge win.

tcharding · 2023-05-09T03:19:00Z

Now includes fuzzing and integration tests pulled out of rust-bitcoincore-rpc work by @stevenroose

tcharding · 2023-05-09T03:25:26Z

Will need changes in rust-bitcoin/rust-bitcoin#1842 if that merges.

apoelstra · 2023-05-09T19:47:41Z

@tcharding I actually don't think it'll need the #1842 changes, because we still haven't changed this crate to rename cfg(fuzzing) to cfg(jsonrpc_fuzzing) and haven't updated our CI fuzzing to the new rust-bitcoin model.

tcharding · 2023-05-09T22:36:49Z

Oh my bad, I misread the PR. The broken-crypto thing has nothing to do with fuzzing here but do we want to change this one to use jsonrcp_fuzzing as well just for uniformity and also in case some other reason ever turns up where we want total control of the cfg option instead of using hongfuzz's?

apoelstra · 2023-05-10T13:30:07Z

@tcharding So, I did actually find myself wanting to use actual JSONRPC with network access within a fuzzer recently ... I was doing unholy things, but it did come up. So it's nice to be able to turn it off.

Maybe rather than renaming it we should add another feature gate that you have to turn on to re-enable the real stuff. So our cfg(fuzzing) guards would be cfg(all(fuzzing, not(jsonrpc_real_fuzzing)) or something. Exact name TBD.

apoelstra · 2023-05-10T13:31:02Z

Or maybe we should just copy the rust-bitcoin/hashes/secp strategy, for consistency's sake.

stevenroose · 2023-05-10T14:57:55Z

If minreq does async, maybe you could see if it also applies to #56?

apoelstra · 2023-05-10T15:06:41Z

@stevenroose it does not, but we would like it to. The maintainers seem pretty friendly so we're hopeful that they'll accept something.

tcharding · 2023-05-11T02:00:34Z

cfg(all(fuzzing, not(jsonrpc_real_fuzzing))

I've opened a separate PR to tackle this idea. For now, this PR just uses --cfg=jsonrpc_fuzz.

tcharding · 2023-05-15T06:17:11Z

Please don't merge without an ack from @stevenroose because I stole the integration testing from his code and also the Auth struct - not sure if the Auth struct should be included in this since it is only used in integration testing but i thought then rust-bitcoincore-rpc could use it from here?

apoelstra · 2023-05-15T17:35:13Z

Can you move all the fuzz stuff to a second commit?

apoelstra · 2023-05-19T13:15:34Z

Fuzzing stuff looks good now. Now that #98 is merged we could maybe rebase again. I think we still need to tweak the auth stuff.

tcharding · 2023-05-19T20:54:29Z

Sure thing, thanks man.

tcharding · 2023-05-20T22:13:37Z

Fixed up auth stuff, I ended up doing so by removing all the stuff in the http module and just adding an example to the Builder::cookie_auth method showing how to use it. Sound ok?

Also , I pull all the fuzzing stuff out of the initial patch and into the final "add fuzzing" patch.

apoelstra · 2023-05-22T15:41:09Z

The new integration_test crate does not build because of workspace confusion, but even if that were resolved, it would not build because it refers to the http::read_auth_from_file function which does not exist.

Can you split out the integration stuff into its own commit (which updates the workspace list and CI)?

tcharding · 2023-05-24T12:19:57Z

Sorry bro, guess I did not run that before re-pushing. Will fix.

tcharding · 2023-05-25T08:49:41Z

Added file reading code in the integration test and added the integration test to the workspace.

Add a module that implements `client::Transport` by way of the `minreq` HTTP client crate (https://github.com/neonmoe/minreq/). Includes fuzzing types in the `simple_minreq` module (with the old "fuzzing" cfg), but fuzz tests will be added next commit (including change to use jsonrpc_fuzzing).

Add a integration infrastructure for testing against bitcoind, copied from `rust-bitocincore-rpc`. Add a single simple test.

Update the fuzzing infrastructure by copying scripts etc. from `rust-bitcoin`. Enable usage of `RUSTFLAGS="--cfg=jsonrpc_fuzz` to run fuzz tests instead of "--cfg=fuzzing".

tcharding · 2023-05-25T08:53:38Z

... and split out the integration tests to a separate patch as requested.

apoelstra · 2023-05-25T22:33:44Z

@tcharding ok 297963b is almost there :)

Basically, the fuzz stuff needs to be updated -- in particular the version of honggfuzz needs to be 0.5.55 rather than 0.5, and the honggfuzz feature should be removed. Probably the simplest thing would be to copy the latest fuzztest infra from rust-bitcoin or rust-miniscript.

We can do this in a followup PR if you'd like, but since this one introduces fuzztesting we might as well go all the way with it.

tcharding · 2023-05-26T01:17:34Z

Shit, super sloppy. I'll sort it in this PR. Thanks

tcharding · 2023-05-26T02:38:58Z

Now I'm confused, what code are you looking at? In commit: ccef3a3 Add fuzzing I see:

-[features]
-honggfuzz_fuzz = ["honggfuzz"]
-
 [dependencies]
-honggfuzz = { version = "0.5", optional = true, default-features = false }
-jsonrpc = { path = ".." }
+honggfuzz = { version = "0.5.55", default-features = false }
+jsonrpc = { path = "..", features = ["minreq_http"] }

I rekon you are on a stale branch.

apoelstra · 2023-05-26T13:34:31Z

Yep, I'm sorry, I think I was looking at the second commit of this branch rather than the last. I think this is good to go :)

apoelstra

ACK ccef3a3

apoelstra · 2023-05-26T15:13:19Z

@tcharding could you open a PR to cut a release? Then I can ACK/merge it.

apoelstra · 2023-05-26T15:14:08Z

Oof actually we have to fix CI first ... https://github.com/apoelstra/rust-jsonrpc/actions/runs/5092343466 <.<

I don't know why Github is so shitty at signalling problems with these yaml files.

tcharding · 2023-05-27T02:37:49Z

I don't think we should blame github for that one, its not even using the correct cfg flags, my bad. Fixed in #99

apoelstra · 2023-05-27T12:48:48Z

I still blame github. I think if they're gonna use an opaque ad-hoc language to configure their CI, which can only be debugged via reading green checkmarks, then they shouldn't show green checkmarks on PRs that do it wrong.

Or they should use a real language so people can review the code.

darosior · 2023-06-28T11:57:48Z

src/http/minreq_http.rs

+        let resp = req.send()?;
+        let json = resp.json()?;


Shouldn't we check the status_code of the resp here?
Otherwise calling json() on a response that isn't 200 would try to deserialize an empty body and therefore confusingly return a SerdeJsonError (https://github.com/neonmoe/minreq/blob/75f28d4eba632e7652b6c15adc746a251c657ad2/src/response.rs#L164-L177) whereas it's not a JSON error but an invalid request.

Bitcoin Core does not return an empty body alongside HTTP error codes. It returns a JSON blob with error information.

It depends on the error. For instance when the work queue is exceeded bitcoind would return a 503 with an error message that is not JSON (see here). It's necessary for application to be able to match on these errors to determine on which ones to wait and retry vs on which one to crash the software.

A trick to make sure we don't try to parse JSON when bitcoind doesn't send some is to look under which conditions would bitcoin-cli expect JSON or not: https://github.com/bitcoin/bitcoin/blob/d6ee03507f39223889a5f039c4db7204ddfb91d5/src/bitcoin-cli.cpp#L821-L850.

I think we could just try to parse the JSON, and in the case of error, check whether the response was 200. For 200 we should return the JSON error, for non-200 we should return a status error.

Trying to match Core's exact behavior here, which I doubt is a stable part of their API, seems too fragile.

Sounds good!

…n body 66fb441 minreq_http: return an HTTP error on error with no JSON in body (Antoine Poinsot) Pull request description: It is useful for downstream users to be matching on errors that do not contain a valid JSONRPC error in the HTTP response body. One instance is if the HTTP server work queue depth is exceeded, as they probably want to retry the request later. On such error we would return a JSON deserialization error, without exposing neither the HTTP status code nor the body of the response. This made it impossible to detect such transient errors. Instead, introduce a new HttpError variant that gets returned when the requested is responded to by an error that does not contain valid JSON in its body. See also: #94 (comment). ACKs for top commit: apoelstra: ACK 66fb441 Tree-SHA512: 76c831331c3b84e2f4319d62828b18574fd6af39a49c98d06c0c5a9c7833c828c9e02522b635a52f289e149cfc0a1abc160ac18d5edaf36114126186d39f4e0b

tcharding force-pushed the 05-03-simple-minreq branch from 8780d81 to d20ba2e Compare May 8, 2023 01:51