Update GetBlockStats fields to be optional

[Abeeujah]

This PR aims to update the GetBlockStats model/response to match the core's spec for Docs fix the rpc spec for getblockstats specifies that it takes in two arguments

1. hash_or_height (string or numeric, required) The block hash or height of the target block
2. stats (json array, optional, default=all values)

If the stats arg is provided, only the value for the stats is returned, if not provided it defaults to all, and all the fields values are returned; indicating that all the fields for GetBlockStats is optional.

To better model and represent this, all the fields in v17, v25, and model::GetBlockStats are now wrapped in Option.

• The docs has been updated to include the optional stats argument
• Tests has also been updated to accept the stats argument, and confirm it behaves as specified for all versions of core.

[jamillambert]

The v25 GetBlockStats struct needs to be changed so that the fields are Option<T> like you did for v23.

[jamillambert]

Most of it is looking good. A few comments:

Could you expand the test to call getblockstats a second time with only one stat in the stats argument. The client macro will need to be updated to accept this option arg. Then check that the returned type for v23 onwards has that stat and the other fields are None. The Core docs are not clear what happens in v17, it still takes the optional stats argument but does not list any of the fields as optional. If the fields are not returned in v17-22 there will be an error running the test and essentially the v17 GetBlockStats will need to be replaced by the v23 one you have done. And then clearly documented that if the stats argument is used only those stats are returned. i.e. the change in Core v23 might just be a docs fix.

[jamillambert]

Looking good. Just needs a comment change and the commit message updated to reflect the new changes that affect all versions.

[jamillambert]

Almost there. I had a more thorough look this time and // 2. Other fields must be `None` (validating our v23 was docs fix). is not checked in the test. Also remove the word "our". I can't think of a nice way to check all the fields without doing them all one by one, which is too verbose.

Maybe on a couple of stats check is_some() for the first case where the stats argument is empty and then is_none when you specify a single stat.

You could also use the stat height in the argument and then instead of just unwrapping you can assert_eq!(model.height, Some(101));.

[Abeeujah]

@jamillambert Sorry the review is taking this long/much roundtrips, I never wanted it to take this long/tasking on you, especially as there's a lot more things you're currently doing; that said I appreciate every moment spent reviewing this, and looking forward to this being as perfect as you'd want it to be.

[jamillambert]

Sorry the review is taking this long/much roundtrips

It's fine, it's not easy to know exactly how things are done in a specific repo as a new contributor, and corepc has a few peculiarities.

And one more roundtrip...

The test is now looking a bit unwieldy. There is a large section of repeated code. In fact the two tests are identical, or at least could be. The only required difference is how node is created. Can you move the let node = Node::with_wallet(...); into blockchain__get_block_stats__modelled() and then for both cases call a single helper function, which is exactly what is in getblockstats_txindex right now but pass in the &node.

[jamillambert]

ACK fcd37d3

Thanks for sticking with it and addressing all my comments.

[Abeeujah]

Thank you for reviewing 🙏

[tcharding]

Good test coverage, I had a play with it. How about this?

#[test]
fn blockchain__get_block_stats__modelled() {
    let node = if cfg!(feature = "v18_and_below") {
        Node::with_wallet(Wallet::Default, &["-txindex"])
    } else {
        Node::with_wallet(Wallet::Default, &[])
    };
    node.fund_wallet();

    get_block_stats_by_height(&node);
    get_block_stats_by_block_hash(&node);
    get_block_stats_with_stats(&node);
}

fn get_block_stats_by_height(node: &Node) {
    let json: GetBlockStats =
        node.client.get_block_stats_by_height(101, None).expect("getblockstats");

    let model: Result<mtype::GetBlockStats, GetBlockStatsError> = json.into_model();
    let model = model.unwrap();

    assert_eq!(model.height, Some(101));
}

fn get_block_stats_by_block_hash(node: &Node) {
    let block_hash = node.client.best_block_hash().expect("best_block_hash failed");
    let json =
        node.client.get_block_stats_by_block_hash(&block_hash, None).expect("getblockstats");

    let model = json.into_model().unwrap(); // Explicit error type already used above. 

    assert_eq!(model.block_hash, Some(block_hash));
    assert!(model.max_fee.is_some());
}

fn get_block_stats_with_stats(node: &Node) {
    let json =
        node.client.get_block_stats_by_height(101, Some(&["minfeerate", "avgfeerate"])).expect("getblockstats");

    let model = json.into_model().unwrap(); // Explicit error type already used above. 

    assert!(model.minimum_fee_rate.is_some());
    assert!(model.average_fee_rate.is_some());
    assert!(model.block_hash.is_none());
    assert!(model.height.is_none());
}

[tcharding]

ACK fcd37d3