Follow ups from adding Core tests

[tcharding]

Follow up to #606, some things I did not notice during review.

Context

The integration tests have some peculiarities which are not immediately obvious for devs new to the project. When we added the _core.rs set of tests we added an additional peculiarity and I believe the water is even more muddy now.

I think that we should document somewhere what the _core.rs tests are and when/why someone would want to update them. In particular we want to isolate the peculiarities from the other integration tests so they don't leak into the core ones, some of which have already.

Usage of explicit types

We use explicit types to test that we got re-exports correct. This is unnecessary in _core files.

#[test]
fn blockchain__get_best_block_hash__modelled() {
    let node = BitcoinD::with_wallet(Wallet::None, &[]);

    let json: GetBestBlockHash = node.client.get_best_block_hash().expect("getbestblockhash");
    let model: Result<mtype::GetBestBlockHash, hex::HexToArrayError> = json.into_model();
    model.unwrap();
}

Action: We should remove all explicit types from _core files

What exactly are the _core files testing?

In review I failed to notice that its not exactly clear what the files are testing. I assumed the aim was to test the version specific types not the model types. I.e. we use extensive Core tests to verify that we got the shape and types correct for everything Core spits out at us. The conversion to rust-bitcoin types (i.e. the model types) is a different concern. For example

#[test]
fn get_chain_tx_stats_default_window_has_optional_fields() {
    let node = BitcoinD::with_wallet(Wallet::Default, &[]);
    let addr = node.client.new_address().unwrap();
    node.client.generate_to_address(200, &addr).unwrap();

    let json: GetChainTxStats = node.client.get_chain_tx_stats().unwrap();
    let stats: mtype::GetChainTxStats = json.into_model().unwrap();

    #[cfg(not(feature = "v18_and_below"))]
    assert!(stats.window_final_block_height.is_some());
    assert!(stats.window_tx_count.is_some());
    assert!(stats.window_interval.is_some());
    assert!(stats.tx_rate.is_some());
}

This test reads to me like it is testing that we correctly convert to model and don't loose fields that were present in v19 onwards. This is a good thing to test but its not what was in my mind the reason for these tests. If we are going to do this sort of testing we should do it thoroughly and separate it from other testing strategies if we are to have any hope of being confident that we did do it thoroughly - FTR I don't think its high priority to test this because its so easy to get it right when adding/reviewing the new fields.

Action: in _core do not call into_model ever, only test on the json returned by Core (version specific types).

JSON non-'empty' values

Caveat: This point is not fully thought out and my be incorrect.

Related to the last point; for strings I think we should be checking that they are not empty and for numbers that they are not 0. I don't know if either JSON or Core treats 'non-existent' and empty/0 as the same. (Consider our usage of serde(deny_unknown_fields) when reasoning about this point.)

eg

    let json: GetTxOutSetInfo =
        node.client.call("gettxoutsetinfo", &[bitcoind::serde_json::json!("muhash")]).unwrap();

    assert!(json.muhash.is_some());

I think it would be better to test on json that the string is non-empty.