Skip to content
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

json: add Other variant to SoftforkType #268

Merged
merged 1 commit into from
Dec 21, 2022

Conversation

benma
Copy link
Contributor

@benma benma commented Dec 20, 2022

Fixes #267. Without this variant, a call to
client.get_blockchain_info() likely fails on some versions of
bitcoind where other types of softfork deployments were active.

Fixes rust-bitcoin#267. Without this variant, a call to
`client.get_blockchain_info()` likely fails on some versions of
bitcoind where other types of softfork deployments were active.
@benma benma changed the title json: add Unknown variant to SoftforkType json: add Other variant to SoftforkType Dec 20, 2022
@apoelstra
Copy link
Member

Lol, LGTM. If this were a field we expected to be used in the future I'd complain and say that the new variant should contain more information so that we can round-trip it. But I'll accept this for the sake of avoiding a parse error in old versions of Core (or litecoin).

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 3f04a34

@apoelstra
Copy link
Member

Will let @tcharding also chime in before merging.

@benma
Copy link
Contributor Author

benma commented Dec 20, 2022

Lol, LGTM. If this were a field we expected to be used in the future I'd complain and say that the new variant should contain more information so that we can round-trip it. But I'll accept this for the sake of avoiding a parse error in old versions of Core (or litecoin).

I did it this way with the same reasoning 😄 Adding more info in this variant would have added quite a lot of boilerplate code and effort.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 3f04a34

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 3f04a34

@apoelstra apoelstra merged commit 6c4cd8a into rust-bitcoin:master Dec 21, 2022
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.

get_blockchain_info() errors because SoftforkType is missing bip8
3 participants