/deploymentinfo
#27853
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:
0jsonRequest.params = UniValue(UniValue::VARR);
0jsonRequest.params.pushKV("blockhash", hash_str);
This PR fixes it by changing pushKV to push_back and adds more test coverage.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Reviewers, this pull request conflicts with the following ones:
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.
612@@ -613,7 +613,7 @@ static bool rest_deploymentinfo(const std::any& context, HTTPRequest* req, const
613 case RESTResponseFormat::JSON: {
614 JSONRPCRequest jsonRequest;
615 jsonRequest.context = context;
616- jsonRequest.params = UniValue(UniValue::VARR);
617+ jsonRequest.params = UniValue(UniValue::VOBJ);
We don’t do any named argument transformation when calling getdeploymentinfo().HandleRequest(jsonRequest) directly, so the jsonRequest is passed as is. If you look at the implementation of getdeploymentinfo(), you’ll see it fetches the argument by position: request.params[0].
Even though this implementation works, it’s pretty fragile: it would also work if you’d used jsonRequest.params.pushKV("not-a-blockhash", hash_str);, for example.
I think sticking to a VARR is best:
0diff --git a/src/rest.cpp b/src/rest.cpp
1index 580511b9c..ba149c1a9 100644
2--- a/src/rest.cpp
3+++ b/src/rest.cpp
4@@ -613,7 +613,7 @@ static bool rest_deploymentinfo(const std::any& context, HTTPRequest* req, const
5 case RESTResponseFormat::JSON: {
6 JSONRPCRequest jsonRequest;
7 jsonRequest.context = context;
8- jsonRequest.params = UniValue(UniValue::VOBJ);
9+ jsonRequest.params = UniValue(UniValue::VARR);
10
11 if (!hash_str.empty()) {
12 uint256 hash;
13@@ -627,7 +627,7 @@ static bool rest_deploymentinfo(const std::any& context, HTTPRequest* req, const
14 return RESTERR(req, HTTP_BAD_REQUEST, "Block not found");
15 }
16
17- jsonRequest.params.pushKV("blockhash", hash_str);
18+ jsonRequest.params.push_back(hash_str);
19 }
20
21 req->WriteHeader("Content-Type", "application/json");
419@@ -420,6 +420,11 @@ def run_test(self):
420
421 deployment_info = self.nodes[0].getdeploymentinfo()
422 assert_equal(deployment_info, self.test_rest_request('/deploymentinfo'))
423+ assert_equal(deployment_info, self.test_rest_request(f"/deploymentinfo/{bb_hash}"))
424+
425+ self.generate(self.wallet, 10, sync_fun=self.no_op)
426+ deployment_info = self.nodes[0].getdeploymentinfo(bb_hash)
427+ assert_equal(deployment_info, self.test_rest_request(f"/deploymentinfo/{bb_hash}"))
Yeah I get that, but I don’t see why we wouldn’t just run the hash-specified test immediately on the older hash to avoid doing this twice? Also just querying the previous hash seems more efficient than running generate?
Upon further thought, I think it does indeed make sense to include it in this PR.
0diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py
1index 559249101..1ba8f60d9 100755
2--- a/test/functional/interface_rest.py
3+++ b/test/functional/interface_rest.py
4@@ -420,11 +420,10 @@ class RESTTest (BitcoinTestFramework):
5
6 deployment_info = self.nodes[0].getdeploymentinfo()
7 assert_equal(deployment_info, self.test_rest_request('/deploymentinfo'))
8- assert_equal(deployment_info, self.test_rest_request(f"/deploymentinfo/{bb_hash}"))
9
10- self.generate(self.wallet, 10, sync_fun=self.no_op)
11- deployment_info = self.nodes[0].getdeploymentinfo(bb_hash)
12- assert_equal(deployment_info, self.test_rest_request(f"/deploymentinfo/{bb_hash}"))
13+ previous_bb_hash = self.nodes[0].getblockhash(self.nodes[0].getblockcount() - 1)
14+ deployment_info = self.nodes[0].getdeploymentinfo(previous_bb_hash)
15+ assert_equal(deployment_info, self.test_rest_request(f"/deploymentinfo/{previous_bb_hash}"))
16
17 non_existing_blockhash = '42759cde25462784395a337460bde75f58e73d3f08bd31fdc3507cbac856a2c4'
18 resp = self.test_rest_request(f'/deploymentinfo/{non_existing_blockhash}', ret_type=RetType.OBJ, status=400)
Also just querying the previous hash seems more efficient than running generate?
For sure, and for what we’d like to test may be enough, I’m gonna change it.
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/27853/files#r1226690678
Thanks @stickies-v for the review.
