bug(corepc-types): type of prune_target_size field in model::GetBlockchainInfo should be i64 instead of u32

[RazorBest]

You can configure a bitcoin-core node with prune=5000 or greater (which is in MB). As a consequence, the RPC call getblockchaininfo will return "prune_target_size": 5242880000 , or greater, which is the value in bytes of the prune limit that was set for the node. This value is outside of u32.

Demo:

use std::error::Error;

use corepc_client::client_sync::Auth;
use corepc_client::client_sync::v29::Client;

fn main() -> Result<(), Box<dyn Error>> {
    let rpc_host = "http://127.0.0.1:8332";
    let cookie_path = "/home/btc/.bitcoin/.cookie";
    let auth = Auth::CookieFile(cookie_path.into());
    let client = Client::new_with_auth(rpc_host, auth)?;

    let rpc_result = client.get_blockchain_info()?;
    println!("RPC response: {rpc_result:?}");

    let generic_result = rpc_result.into_model()?;
    println!("Generic response: {generic_result:?}");

    Ok(())
}

Cargo.toml:

[dependencies]
corepc-client = { version = "0.10.0", features = ["client-sync"] }

Configure a node with prune=5000 or higher. Change rpc_host and cookie_path accordingly.

Output:

RPC response: GetBlockchainInfo ...
Error: Numeric(Overflow { field: "prune_target_size", value: 5242880000 })

Expected behaviour: no error.

---

v17::GetBlockchainInfo and v29::GetBlockchainInfo both define the field prune_target_size as Option<i64>. So, only model::GetBlockchainInfo::prune_target_size seems to be the outlier.

I don't know if it was a deliberate decision to represent all the version non-specific type fields integers as 32-bit ints. But unless proven wrongly, I'll just categorize this as a bug.

[RazorBest]

I later discovered these comments:

corepc/types/src/lib.rs

Lines 48 to 77 in 207e17d

	/// Converts an `i64` numeric type to a `u32`.
	///
	/// The Bitcoin Core JSONRPC API has fields marked as 'numeric'. It is not obvious what Rust
	/// type these fields should be.
	///
	/// We want the version specific JSON types to just work (TM).
	///
	/// 1. We use an `i64` because its the biggest signed integer on "common" machines.
	/// 2. We use a signed integer because Core sometimes returns -1.
	///
	/// (2) was discovered in the wild but is hard to test for.
	pub fn to_u32(value: i64, field: &str) -> Result<u32, NumericError> {
	if value.is_negative() {
	return Err(NumericError::Negative { value, field: field.to_owned() });
	}
	u32::try_from(value).map_err(|_| NumericError::Overflow { value, field: field.to_owned() })
	}
	/// Error converting an `i64` to a `u32`.
	///
	/// If we expect a numeric value to sanely fit inside a `u32` we use that type in the `model`
	/// module, this requires converting the `i64` returned by the JSONRPC API into a `u32`, if our
	/// expectations are not met this error will be encountered.
	#[derive(Debug)]
	pub enum NumericError {
	/// Expected an unsigned numeric value however the value was negative.
	Negative { field: String, value: i64 },
	/// A value larger than `u32::MAX` was unexpectedly encountered.
	Overflow { field: String, value: i64 },
	}

Which show that the choice to convert all the i64 to u32 was deliberate. And any integer that exceeds u32 is against the expectations. However, it's not clear whether "the expectations" are an invariant of this library.

[tcharding]

I'd take a patch that changes the model type from a u32 to a u64. Conversion from i64 to u64 is fallible though so there'd need to be some error checking as well.

Good find, thanks. I imagine there are a ton of this class of bug in types, no clue how to solve this?

[RazorBest]

I imagine there are a ton of this class of bug in types, no clue how to solve this?

I accidentally found this while using corepc-types on my node. So, probably, for all the other cases, a concrete example is needed.

I'd take a patch that changes the model type from a u32 to a u64

Just to be clear, does that apply to prune_target_size, or all the types in the model?

I can try to make a patch. It's not complicated, just a lot of the same modification in multiple files.

[tcharding]

Just the field you found to be wrong.

So, probably, for all the other cases, a concrete example is needed.

Yes more test coverage would definitely help.

[RazorBest]

I believe size_on_disk has the same issue. But this can only be verified against a node that claims to use more than 4 GB storage, which I don't know how it can be tested.

[tcharding]

Lets change it.

[RazorBest]

Nevermind. size_on_disk is already u64.