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

Allow warnings to be either a string or a string sequence. #353

Merged
merged 1 commit into from
May 8, 2024

Conversation

arik-so
Copy link
Contributor

@arik-so arik-so commented May 7, 2024

In recent versions of bitcoind, getblockchaininfo returns a result whose warnings key is no longer a string, but an array of strings. This PR allows the parsing of either. Fixes #352.

@tcharding
Copy link
Member

Thanks for the fix! I'm going to ack this on the assumption that you tested it because you ran into the problem.

FTR the PR can't be tested because we are so far behind :(

ACK 87c95f6

@arik-so
Copy link
Contributor Author

arik-so commented May 8, 2024

Thanks so much!

As a matter of fact, I did test it, but to do so, I also added a test module to main.rs and separately ran a local server that returned different values for the warning key based on a switch I could flip. Would it perhaps be helpful to add some unit tests besides the integration tests that could rely on a mock server?

@tcharding
Copy link
Member

Legend! You could add some tests if you like but I wouldn't bother putting too much effort in because the whole crate is in need of love.

@0xB10C
Copy link
Contributor

0xB10C commented May 8, 2024

fwiw: this was merged into Bitcoin Core master 2 days ago bitcoin/bitcoin#29845

But I don't think it hurts having this in here early.

@arik-so
Copy link
Contributor Author

arik-so commented May 8, 2024

This is wild! My colleague ran into an issue with electrs because of that just yesterday, so must have been particularly unlucky with the timing of cloning bitcoind.

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 87c95f6 also assuming that you tested this against an actual recent node

@apoelstra apoelstra merged commit d076d16 into rust-bitcoin:master May 8, 2024
10 checks passed
romanz added a commit to romanz/rust-bitcoincore-rpc that referenced this pull request May 11, 2024
apoelstra added a commit that referenced this pull request May 11, 2024
e03cd64 Support multiple warnings in RPC responses (Roman Zeyde)

Pull request description:

  Following #353.

  Tested on latest bitcoind (bitcoin/bitcoin@2cedb42) with latest electrs (romanz/electrs@7773c26).

ACKs for top commit:
  apoelstra:
    ACK e03cd64 neat! that was easy

Tree-SHA512: df9c038f8e1ccac54a07ad13c7ed87f3a92909c20128fcb598b9891d59127a289bbaf79f9a505d9c87347ffd79dc2aaa063ad750126dbd95a916541d032f4106
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.

getblockchaininfo now returns an array instead of a string for the warning key
5 participants