rust-bitcoin / rust-bitcoincore-rpc

Rust RPC client library for the Bitcoin Core JSON-RPC API.
343 stars 257 forks source link

Take all get_index_info rpc call return values as Option<> #285

Closed dev7ba closed 1 year ago

dev7ba commented 1 year ago

The values returned by get_index_info are optional depending on whether the values txindex=1, blockfilterindex=1, and coinstatsindex=1 are configured in bitcoin.conf.

This causes the library to fail in case any of these indices are not configured.

I closed last pull request due to problems with rebase.

apoelstra commented 1 year ago

Do you also need to add serde(default) on these? I'm not sure.

Can you add a unit test for decoding these, so we can be sure?

dev7ba commented 1 year ago

Do you also need to add serde(default) on these? I'm not sure.

No, I think what we want is an Option::None in case an index is not enabled in bitcoind. Returning a default value is much less elegant/useful. There are other examples using Option<> with serde in the library.

Can you add a unit test for decoding these, so we can be sure?

Sure, be aware that getindexinfo is for 0.21.0 versions and above. Also, it seems that bitcoind options txindex and basicblockfilterindex are enabled in your test suit but not coinstatsindex.

apoelstra commented 1 year ago

Looks good now, thanks! Can you squash the top two commits though so that all commits pass the test suite?

No, I think what we want is an Option::None in case an index is not enabled in bitcoind.

Right, this is what I thought serde(default) would do. Without it, you'd just get an error. Maybe I don't understand it.

dev7ba commented 1 year ago

Looks good now, thanks! Can you squash the top two commits though so that all commits pass the test suite?

Done.