Depend on the new bitcoind-json-rpc group of crates

[tcharding]

There is an effort to improve the state of affairs in regards to integration testing extensively against multiple versions of Bitcoin Core. As part of this do:

• Depend on the new rust-bitcoind-json-rpc crates
• Run the integration tests against most versions of Core since 0.17.1

(Note the latest supported version is currently 26.0)

ref: https://crates.io/search?q=bitcoind-json-rpc

[tcharding]

CI fails look unrelated to this PR, will come back to them.

[apoelstra]

CI should be fixed in #697

[apoelstra]

concept ACK. This is really shaping up.

A few comments:

• Something really weird is going on with the docs https://docs.rs/bitcoind-json-rpc-regtest/0.2.0/bitcoind_json_rpc_regtest/struct.Client.html see "non-crate feature" which suggestst that somehow your feature-gating is broken.
• Would be good to add a len method directly to https://docs.rs/bitcoind-json-rpc-regtest/0.2.0/bitcoind_json_rpc_regtest/json/struct.GenerateToAddress.html so users don't have to do .0.len(). (Probably true for a long tail of Vec methods, fine to do it piecemeal since it's non-breaking to add them; but we should add them as we encounter them.)
• Alternately GenerateToAddress should just be a struct with a named field. It's really the .0 that feels weird; if it were .blocks that'd be fine.
• We should have some sort of ToModel trait so you don't need to write let concrete: model::GetTransaction = json.try_into().unwrap(); and can instead just tack .into_model().unwrap() onto the line that defines json.
• I noticed you dropped all the args on get_new_address and send_to_address. What is the plan to restore these?

[tcharding]

concept ACK. This is really shaping up.

Thanks man, appreciate you taking the time to look at it.

A few comments:

• Something really weird is going on with the docs https://docs.rs/bitcoind-json-rpc-regtest/0.2.0/bitcoind_json_rpc_regtest/struct.Client.html see "non-crate feature" which suggestst that somehow your feature-gating is broken.

I'll have a dig.

• Would be good to add a len method directly to https://docs.rs/bitcoind-json-rpc-regtest/0.2.0/bitcoind_json_rpc_regtest/json/struct.GenerateToAddress.html so users don't have to do .0.len(). (Probably true for a long tail of Vec methods, fine to do it piecemeal since it's non-breaking to add them; but we should add them as we encounter them.)
• Alternately GenerateToAddress should just be a struct with a named field. It's really the .0 that feels weird; if it were .blocks that'd be fine.

I'll sort it, cheers.

• We should have some sort of ToModel trait so you don't need to write let concrete: model::GetTransaction = json.try_into().unwrap(); and can instead just tack .into_model().unwrap() onto the line that defines json.

Can do, I was toying up that idea vs using an inherent into_model function.

• I noticed you dropped all the args on get_new_address and send_to_address. What is the plan to restore these?

I was thinking of just adding more functions to the clients as we needed them, the initial client was intended to only be really useful to prove the shape of the returned json, so I was only going to add support for optional args if they effected the returned data (like the verbosity flag to getblock). But supporting miniscript already broke this idea. Not sure of a clean way to do it? Open to suggestions

[tcharding]

• Something really weird is going on with the docs https://docs.rs/bitcoind-json-rpc-regtest/0.2.0/bitcoind_json_rpc_regtest/struct.Client.html see "non-crate feature" which suggestst that somehow your feature-gating is broken.

The reason is that to get --no-default-features to work I used a massive if-not clause of all the features but rustdoc does not like it one bit.

hmmm, I tried removing the --no-default-features kludge and that just results in the docs saying Client is only available if feature "26_0" is enabled. I'm not sure the non-standard feature stuff we are doing is going play nicely with docs.rs

[tcharding]

I played with adding an into_model function, looks much better. I don't see the benefit of a trait - am I missing something?

Example usage:

fn get_vout(cl: &Client, txid: Txid, value: Amount, spk: ScriptBuf) -> (OutPoint, TxOut) {
    let model = cl.get_transaction(txid)
        .expect("rpc call failed")
        .into_model()
        .expect("conversion to model type failed");
    let tx = model.tx;

And over in bitcoind-json-rpc I just replaced the TryFrom implentation with an impl block and the into_model function. I'm going to remove the TryFrom impls altogether.

[apoelstra]

I was thinking of just adding more functions to the clients as we needed them, the initial client was intended to only be really useful to prove the shape of the returned json, so I was only going to add support for optional args if they effected the returned data (like the verbosity flag to getblock). But supporting miniscript already broke this idea. Not sure of a clean way to do it? Open to suggestions

I think we could have a giant SendToAddressParams struct, have Into<SendToAddressParams> for various smaller/simpler structs (including a bare Address) and then our RPC could accept a generic Into<SendToAddressParams>?

I played with adding an into_model function, looks much better. I don't see the benefit of a trait - am I missing something?

No, I think you're right. No need for a trait.

[tcharding]

Needs #697

[tcharding]

If we merge #682 first I don't have to fix pinning on this PR.

[apoelstra]

Yeah, I'd like to do #682 first because it makes other testing-related changes easier to follow.

[tcharding]

As I suspected, breaks MSRV build. Needs the lockfiles manually fixing after pinning ... definitely one of the not-so-awesome aspects of Rust development, whinge whinge whinge.

[tcharding]

Alright, that wasn't so hard. The fix was to exclude the bitcoind-tests crate from normal builds (ie in the manifest). And then pin everything in the minimal file (only needed cc pinning to v1.0.28, win). This means there is a whole load of red in the diff of the lock files.

[tcharding]

Note, the minimal/recent lockfiles are the same :(

[apoelstra]

If we remove bitcoind-tests from the workspace like this then I can't run them locally, since I only have access to individual workspaces (and their dependencies) from my Nix derivations.

Furthermore, even if I did could jury-rig access to the crate, which crate2nix does not like to do, I wouldn't have access to its dependencies because they've all been removed from the lockfiles.

[tcharding]

Ah no worries, I'll fix it up.

[tcharding]

I've rebased on top of #719 to see if that allows me to avoid messing around with dependency versions. Now the minimal/recent lock files are the same.

[tcharding]

Now includes a patch to add fuzz to CRATES as you noticed over in rust-jsonrpc.

[apoelstra]

In a0e9028:

Actually can you just remove every one of the lint things (except maybe deny(unsafe))? These do not belong in the source tree and sometimes they cause old branches to fail with new compilers.

[apoelstra]

Successfully ran local tests on d6571b6.

[tcharding]

Reduced the coding convention attributes in an additional patch. No changes to the final patch.

[apoelstra]

ACK 9cf60c8 successfully ran local tests

[tcharding]

BOOM!