rest: add `/deploymentinfo` endpoint #25412

This doesn't work with /rest/deploymentinfo.json

--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -380,8 +380,10 @@ class RESTTest (BitcoinTestFramework):
 
         self.log.info("Test the /deploymentinfo URI")
         json_obj = self.test_rest_request(f'/deploymentinfo/{newblockhash[0]}')
+        assert_equal(json_obj, self.test_rest_request(f'/deploymentinfo'))
         deployment_info = self.nodes[0].getdeploymentinfo(newblockhash[0])
         assert_equal(deployment_info, json_obj)
+        assert(deployment_info != self.test_rest_request(f'/deploymentinfo/{bb_hash}'))
 
         json_obj = self.test_rest_request('/deploymentinfo/')
         deployment_info = self.nodes[0].getdeploymentinfo()

Just noticed that newblockhash[0] and bb_hash are the same, so the assertion: assert(deployment_info != self.test_rest_request(f'/deploymentinfo/{bb_hash}')) would fail.

Fixed /rest/deploymentinfo.json

bb_hash is set to the best block, before newblockhash[0] is mined...

Before?

newblockhash = self.generate(self.nodes[1], 1) -> L348 bb_hash = self.nodes[0].getbestblockhash() -> L372

    if (!CheckWarmup(req)) return false;

            if (!ParseHashStr(hashStr, hash)) {
                return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
            }

Could move this up after jsonRequest.context = context;, and just pushKV directly

Nice, thanks!

Maybe it's better to specify that the <BLOCKHASH> parameter is optional and rest/deploymentinfo.json will use the hash of current chain tip.

Nice!

Why the RPCHelpMan is not static anymore?

@Riahiamirreza To be able to use it in src/rest.cpp, see getblockchaininfo.

Why this api supports both /rest/deploymentinfo and /rest/deploymentinfo/ while others not. Is this a good idea to add the both to other apis?

This API supports both because getdeploymentinfo can be used by passing or not passing a block hash. So, you can use: /rest/deploymentinfo/BLOCKHASH.json or /rest/deploymentinfo.json if you want to get the information from the current chain tip.

For other endpoints, passing a BLOCKHASH or a TXID is a must which is not this case.

I would like to reopen this comment. Even though a few other endpoints do the same thing, I think it is bad practice and I would not add the /rest/deploymentinfo endpoint in addition to the regular /rest/deploymentinfo/

If you want to access the endpoint without providing a blockhash, you can do so already with GET /rest/deploymentinfo/.json. It looks a bit ugly maybe, but that's because the format parameter should be a query parameter in the first place to properly resolve this (which I'll be proposing in a new issue/PR very soon). In the current implementation, people would be able to call GET /rest/deploymentinfoa.json and get Invalid hash: a as response, which is highly intuitive imo and not the API spec.

+0 on this. I don't think it would be a bad practice, in a user perspective I'd prefer: /rest/deploymentinfo.json to get all and /rest/deploymentinfo/<BLOCKHASH>.json to get a specific one. I don't feel myself (as a final user) confortable with /rest/deploymentinfo/.json.

I agree 100000% with you the format parameter should be a query one, and if you're working on this, maybe we can keep it this way and change it after your PR?

If no one else thinks this is an issue I'm okay to proceed with it, but having something like GET /rest/deploymentinfoa.json resolve to a blockhash of a does not sit well with me. Either way, it doesn't impact the upgrade path with query params I have in mind so from that end I'm not too bothered.

suggestions: consistent assert args order, remove unneeded temporary, and define localvar with largest scope first

@@ -379,13 +379,12 @@ class RESTTest (BitcoinTestFramework):
         assert_equal(blockchain_info, json_obj)
 
         self.log.info("Test the /deploymentinfo URI")
-
-        json_obj = self.test_rest_request('/deploymentinfo')
         deployment_info = self.nodes[0].getdeploymentinfo()
-        assert_equal(deployment_info, json_obj)
+        assert_equal(deployment_info, self.test_rest_request('/deploymentinfo'))
 
         json_obj = self.test_rest_request(f'/deploymentinfo/{newblockhash[0]}')
-        assert_equal(json_obj, deployment_info)
+        assert_equal(deployment_info, json_obj)
+
         deployment_info_newblockhash = self.nodes[0].getdeploymentinfo(newblockhash[0])
         assert_equal(deployment_info_newblockhash, json_obj)

done!

I think you can reduce the lock scope to only this line:

-            const CBlockIndex* blockindex = chainman.m_blockman.LookupBlockIndex(hash_bi);
+            const CBlockIndex* blockindex{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(hash_bi))};

(also prefer an absolute reference ::cs_main for new code)

Cool, thanks!

ideas

 #### Deployment info
+`GET /rest/deploymentinfo.json`
 `GET /rest/deploymentinfo/<BLOCKHASH>.json`
-Returns an object containing various state info regarding deployments of consensus changes.
-BLOCKHASH parameter is optional and `/rest/deploymentinfo.json` will use the hash of current chain tip.
+Returns an object containing various state info regarding deployments of
+consensus changes at the current chain tip, or at <BLOCKHASH> if provided.
 Only supports JSON as output format.
 Refer to the `getdeploymentinfo` RPC help for details.

Great idea!

It looks like this section can be simplified/shortened by removing 3 unneeded temporaries (and reducing the lock scope as mentioned above)

@@ -610,23 +610,17 @@ static bool rest_deploymentinfo(const std::any& context, HTTPRequest* req, const
             if (!ParseHashStr(hashStr, hash)) {
                 return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
             }
-
-            LOCK(cs_main);
-            const uint256 hash_bi(ParseHashV(hashStr, "blockhash"));
-            ChainstateManager* maybe_chainman = GetChainman(context, req);
-            if (!maybe_chainman) return false;
-            ChainstateManager& chainman = *maybe_chainman;
-            const CBlockIndex* blockindex = chainman.m_blockman.LookupBlockIndex(hash_bi);
-            if (!blockindex) {
+            const ChainstateManager* chainman = GetChainman(context, req);
+            if (!chainman) return false;
+            if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(ParseHashV(hashStr, "blockhash")))) {
                 return RESTERR(req, HTTP_BAD_REQUEST, "Block not found");
             }
-
-
             jsonRequest.params.pushKV("blockhash", hashStr);
         }

Very nice! Gonna change it!

hashStr, deploymentInfoObject, strJSON -> could use current naming style for these new localvars (the latter two could also be const)

Interesting.. just had tried to keep same names from other methods, I am gonna change it, thanks!

Thanks for updating. Would suggest hashStr -> hash_str (and str_json -> json_str for consistency).

Edit: can just drop str_json, see #25412 (review).

Is there a reason not to support the hex and bin formats, which most of the other endpoints support?

It may be clearer to indicate both endpoints.

 #### Deployment info
+`GET /rest/deploymentinfo.json`
 `GET /rest/deploymentinfo/<BLOCKHASH>.json`

Probably not a "notable change" (maybe the person merging the release notes will edit that, unsure).

Done! and merged the first two commits!

hash_bi is an unnecessary temporary (if you want to keep it, would just name it hash)

-            const uint256 hash_bi{ParseHashV(hashStr, "blockhash")};
             const ChainstateManager* chainman = GetChainman(context, req);
             if (!chainman) return false;
-            if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(hash_bi))) {
+            if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(ParseHashV(hashStr, "blockhash")))) {
                 return RESTERR(req, HTTP_BAD_REQUEST, "Block not found");
             }

Addressed it because makes the code cleaner. nice one!

Looks like this section could be simplified further by dropping two temporaries that add allocations and move operations, are each used only once and don't seem to make the code clearer.

-        const UniValue deployment_info_object = getdeploymentinfo().HandleRequest(jsonRequest);
-        const std::string str_json = deployment_info_object.write() + "\n";
         req->WriteHeader("Content-Type", "application/json");
-        req->WriteReply(HTTP_OK, str_json);
+        req->WriteReply(HTTP_OK, getdeploymentinfo().HandleRequest(jsonRequest).write() + "\n");
         return true;

Nice! Done!

Any reason you're not declaring this in blockchain.h instead? That seems less messy to me.

There is no specific reason from my side, maybe because of the type RPCHelpMan, gonna check it.

I see now that this was also done for getblockchaininfo(). At the time #7766 was merged, src/rpc/blockchain.h did not yet exist - it was added only with https://github.com/bitcoin/bitcoin/commit/e6dcfeec05d6527ab148550836937ff435e6449c .

I don't see any reason not to declare it in blockchain.h now though, you'll just have to #include <rpc/util.h> and everything works fine.

Yes, you're right. I'm gonna change it and move getblockchaininfo to there as well

Any reason you're not declaring this in blockchain.h instead? That seems less messy to me.

It looks like the reason was not to needlessly add #include <rpc/util.h> and 400 lines of code to rpc/blockchain.h.

I did not consider that, thanks for the insight @jonatack . Makes sense.

I think this test makes more sense without the binary return type, since the endpoint only supports JSON?

        resp = self.test_rest_request(f'/deploymentinfo/{non_existing_blockhash}', status=400)
        assert_equal(resp.read().decode('utf-8').rstrip(), "Block not found")

        resp = self.test_rest_request(f"/deploymentinfo/{INVALID_PARAM}", status=400)

Let me check but I think if I don't use ret_type=RetType.OBJ here, it will throw an error because it won't be able to decode it.

You're right, my bad. I was confusing ret_type with req_type. Can be marked as resolved, thanks.

Is a new line required ?

I've often seen feedback that "used by" comments are not maintainable and should not be used, would remove this line? It's quite trivial to check where they're being used.

Makes sense, gonna remove it.

Adding 400 LOC into this file doesn't seem to be an improvement relative to my last ACKed version at https://github.com/bitcoin/bitcoin/commit/c65f82bb6420b1c43a6f39f5b5e31779744a847a.

I'm going to revert it so. Addressed that thinking about keeping the code more organized but you're right about adding 400 LOC.