Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

dev7ba
Copy link
Contributor

@dev7ba dev7ba commented Mar 22, 2023

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
Copy link
Member

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
Copy link
Contributor Author

dev7ba commented Mar 24, 2023

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
Copy link
Member

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.

added test for get_index_info

Added version check to test_get_index_info
@dev7ba
Copy link
Contributor Author

dev7ba commented Mar 27, 2023

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

Done.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 300b3cc

@apoelstra apoelstra merged commit 64f2dca into rust-bitcoin:master Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants