Skip to content

Conversation

brunoerg
Copy link
Contributor

@brunoerg brunoerg commented Jun 11, 2023

Calling /deploymentinfo passing a valid blockhash makes bitcoind to crash. It happens because we're pushing a JSON value of type array when it expects type object. See:

jsonRequest.params = UniValue(UniValue::VARR);
jsonRequest.params.pushKV("blockhash", hash_str);

This PR fixes it by changing pushKV to push_back and adds more test coverage.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 11, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v, achow101
Stale ACK andrewtoth

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27788 (rpc: Be able to access RPC parameters by name by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@brunoerg
Copy link
Contributor Author

Credits to @andrewtoth for noticing it!

@bitcoin bitcoin deleted a comment Jun 11, 2023
@bitcoin bitcoin deleted a comment Jun 11, 2023
@bitcoin bitcoin deleted a comment Jun 11, 2023
Copy link
Contributor

@andrewtoth andrewtoth left a comment

Choose a reason for hiding this comment

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

ACK b550e9a

@brunoerg brunoerg force-pushed the 2023-06-bugfix-rest-deploymentinfo branch from b550e9a to e19b922 Compare June 12, 2023 13:27
@brunoerg
Copy link
Contributor Author

Force-pushed addressing #27853 (comment)

@brunoerg brunoerg force-pushed the 2023-06-bugfix-rest-deploymentinfo branch from e19b922 to 7d452d8 Compare June 12, 2023 16:31
@brunoerg
Copy link
Contributor Author

Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/27853/files#r1226690678

Thanks @stickies-v for the review.

Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK 7d452d8

@DrahtBot DrahtBot requested a review from andrewtoth June 12, 2023 17:43
@achow101
Copy link
Member

ACK 7d452d8

@DrahtBot DrahtBot requested review from andrewtoth and removed request for andrewtoth June 12, 2023 22:28
@achow101 achow101 merged commit d80348c into bitcoin:master Jun 12, 2023
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 14, 2023
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 14, 2023
@fanquake fanquake mentioned this pull request Jun 14, 2023
@fanquake
Copy link
Member

Backported in #27887.

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 15, 2023
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 15, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 15, 2023
…loymentinfo`

7d452d8 test: add coverage for `/deploymentinfo` passing a blockhash (brunoerg)
ce887ea rest: bugfix, fix crash error when calling `/deploymentinfo` (brunoerg)

Pull request description:

  Calling `/deploymentinfo` passing a valid blockhash makes bitcoind to crash. It happens because we're pushing a JSON value of type array when it expects type object. See:
  ```cpp
  jsonRequest.params = UniValue(UniValue::VARR);
  ```
  ```cpp
  jsonRequest.params.pushKV("blockhash", hash_str);
  ```

  This PR fixes it by changing `pushKV` to `push_back` and adds more test coverage.

ACKs for top commit:
  achow101:
    ACK 7d452d8
  stickies-v:
    ACK 7d452d8

Tree-SHA512: f01551e556aba2380c3eaed0bc59057304302c202d317d7c1eec5f7ef839851f672aed80819a8719cb1cbbad2aad735d6d44314ac7d6d98bff8217f5a16c312b
fanquake added a commit that referenced this pull request Jun 16, 2023
6233049 ci: Switch to `amd64` container in "ARM" task (Hennadii Stepanov)
d845a3e test: add coverage for `/deploymentinfo` passing a blockhash (brunoerg)
72ead86 rest: bugfix, fix crash error when calling `/deploymentinfo` (brunoerg)
6f7a0ae ci: Use podman stop over podman kill (MarcoFalke)
de56daa ci: Use podman for persistent workers (MarcoFalke)
71f626e ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN (MarcoFalke)

Pull request description:

  Currently backports:
  * #27777
  * #27844
  * #27853
  * #27886

  Effectively also backports: #27562.

ACKs for top commit:
  stickies-v:
    ACK 6233049

Tree-SHA512: d0f3d5c4cd0cf9792f3270078b49ad6661e28e7a883f91e5981e8cfe95d01c6e415762ee2e3a5fea17d198299989a8b5412961c2cc788cef975b2ee45d84550c
@bitcoin bitcoin locked and limited conversation to collaborators Jun 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants