rest: add `/deploymentinfo` endpoint #25412

brunoerg · 2022-06-18T19:51:43Z

#23508 added a new RPC named getdeploymentinfo, it moved the softfork section from getblockchaininfo into this new one. In the REST interface, we have an endpoint named/rest/chaininfo.json (which refers to getblockchaininfo), so, this PR adds a new REST endpoint named /deploymentinfo which refers to getdeploymentinfo.

You can use it by passing a block hash, e.g: '/rest/deploymentinfo/.json' or you can use it without passing a block hash to get the 'deploymentinfo' for the last block.

src/rest.cpp

brunoerg · 2022-06-19T11:53:10Z

Force-pushed addressing @luke-jr`s review

w0xlt

Tested ACK c7c4fa2

doc/REST-interface.md

src/rpc/blockchain.cpp

src/rest.cpp

brunoerg · 2022-06-19T18:30:07Z

Force-pushed addressing @w0xlt's review for documentation.

w0xlt

reACK 06fcbb9

DrahtBot · 2022-06-21T00:09:38Z

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

Conflicts

No conflicts as of last run.

jonatack

Tested concept/approach ACK. I think the first commit removing static ought to be in the second commit, "rest: add `/deploymentinfo", as it is the only reason for removing it. Edit: does this need a release note?

test/functional/interface_rest.py

src/rest.cpp

doc/REST-interface.md

src/rest.cpp

brunoerg · 2022-07-07T21:15:10Z

Force-pushed addressing @jonatack's review and adding release note.

Thanks.

jonatack · 2022-07-09T09:41:24Z

doc/REST-interface.md

+`GET /rest/deploymentinfo/<BLOCKHASH>.json`
+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.


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

doc/REST-interface.md

src/rest.cpp

jonatack

Would merge the first two commits. A few other comments.

doc/release-notes-25412.md

src/rest.cpp

brunoerg · 2022-07-11T17:21:42Z

Force-pushed addressing @jonatack's review! Thanks for it!

jonatack · 2022-07-14T12:17:58Z

ACK with the following suggestions

test improvements

diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py
index 096794e1e1..1f3cc5f7ba 100755
--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -378,22 +378,33 @@ class RESTTest (BitcoinTestFramework):
         blockchain_info = self.nodes[0].getblockchaininfo()
         assert_equal(blockchain_info, json_obj)
 
+        # Test compatibility of deprecated and newer endpoints
+        self.log.info("Test compatibility of deprecated and newer endpoints")
+        assert_equal(self.test_rest_request(f"/headers/{bb_hash}", query_params={"count": 1}), self.test_rest_request(f"/headers/1/{bb_hash}"))
+        assert_equal(self.test_rest_request(f"/blockfilterheaders/basic/{bb_hash}", query_params={"count": 1}), self.test_rest_request(f"/blockfilterheaders/basic/5/{bb_hash}"))
+
         self.log.info("Test the /deploymentinfo URI")
 
         deployment_info = self.nodes[0].getdeploymentinfo()
         assert_equal(deployment_info, self.test_rest_request('/deploymentinfo'))
 
+        non_existing_blockhash = '42759cde25462784395a337460bde75f58e73d3f08bd31fdc3507cbac856a2c4'
+        resp = self.test_rest_request(f'/deploymentinfo/{non_existing_blockhash}', ret_type=RetType.OBJ, status=400)
+        assert_equal(resp.read().decode('utf-8').rstrip(), "Block not found")
+
+        resp = self.test_rest_request(f"/deploymentinfo/{INVALID_PARAM}", ret_type=RetType.OBJ, status=400)
+        assert_equal(resp.read().decode('utf-8').rstrip(), f"Invalid hash: {INVALID_PARAM}")
+
+        newblockhash = self.generate(self.nodes[1], 1)
         json_obj = self.test_rest_request(f'/deploymentinfo/{newblockhash[0]}')
+        assert(deployment_info != json_obj)
+
+        deployment_info = self.nodes[0].getdeploymentinfo()
         assert_equal(deployment_info, json_obj)
 
         deployment_info_newblockhash = self.nodes[0].getdeploymentinfo(newblockhash[0])
         assert_equal(deployment_info_newblockhash, json_obj)
 
-        # Test compatibility of deprecated and newer endpoints
-        self.log.info("Test compatibility of deprecated and newer endpoints")
-        assert_equal(self.test_rest_request(f"/headers/{bb_hash}", query_params={"count": 1}), self.test_rest_request(f"/headers/1/{bb_hash}"))
-        assert_equal(self.test_rest_request(f"/blockfilterheaders/basic/{bb_hash}", query_params={"count": 1}), self.test_rest_request(f"/blockfilterheaders/basic/5/{bb_hash}"))
-
 
 if __name__ == '__main__':
     RESTTest().main()

naming style for new code

diff --git a/src/rest.cpp b/src/rest.cpp
index d3e878a7bf..c5ea9090e6 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -592,12 +592,12 @@ static bool rest_chaininfo(const std::any& context, HTTPRequest* req, const std:
 
 RPCHelpMan getdeploymentinfo();
 
-static bool rest_deploymentinfo(const std::any& context, HTTPRequest* req, const std::string& strURIPart)
+static bool rest_deploymentinfo(const std::any& context, HTTPRequest* req, const std::string& uri_part_str)
 {
     if (!CheckWarmup(req)) return false;
 
-    std::string hashStr;
-    const RESTResponseFormat rf = ParseDataFormat(hashStr, strURIPart);
+    std::string hash_str;
+    const RESTResponseFormat rf = ParseDataFormat(hash_str, uri_part_str);
 
     switch (rf) {
     case RESTResponseFormat::JSON: {
@@ -605,19 +605,19 @@ static bool rest_deploymentinfo(const std::any& context, HTTPRequest* req, const
         jsonRequest.context = context;
         jsonRequest.params = UniValue(UniValue::VARR);
 
-        if (!hashStr.empty()) {
+        if (!hash_str.empty()) {
             uint256 hash;
-            if (!ParseHashStr(hashStr, hash)) {
-                return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
+            if (!ParseHashStr(hash_str, hash)) {
+                return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hash_str);
             }
 
             const ChainstateManager* chainman = GetChainman(context, req);
             if (!chainman) return false;
-            if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(ParseHashV(hashStr, "blockhash")))) {
+            if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(ParseHashV(hash_str, "blockhash")))) {
                 return RESTERR(req, HTTP_BAD_REQUEST, "Block not found");
             }
 
-            jsonRequest.params.pushKV("blockhash", hashStr);
+            jsonRequest.params.pushKV("blockhash", hash_str);
         }

stickies-v

ACK c65f82b

I think this PR would benefit from moving the getdeploymentinfo() declaration to blockchain.h ~~and from removing the ret_type=RetType.OBJ as commented here~~, but neither are blocking imo.

Edit: happy to ignore my previous comment re double endpoints too, even though it's not my first choice it is in line with the current API organization so it's reasonable to do it this way.

test/functional/interface_rest.py

w0xlt

reACK 394f177

src/rest.cpp

stickies-v

re-ACK 394f177

src/rpc/blockchain.h

brunoerg · 2022-08-16T17:30:42Z

Force pushed addressing last @stickies-v's comment. Removed the comment in rpc/blockchain.h

I think now it is ready for a final review.
cc: @jonatack @w0xlt

w0xlt

reACK 9ef5823

src/rpc/blockchain.h

jonatack · 2022-08-16T22:48:39Z

re-ACK a8250e3 rebase-only since my last review at c65f82b

stickies-v

re-ACK a8250e3

brunoerg · 2022-08-23T16:32:01Z

@w0xlt have in mind a final review here?

achow101 · 2022-10-13T17:23:06Z

ACK a8250e3

a8250e3 doc: add release note about `/rest/deploymentinfo` (brunoerg) 5c96020 doc: add `/deploymentinfo` in REST-interface (brunoerg) 3e44bee test: add coverage for `/rest/deploymentinfo` (brunoerg) 9149703 rest: add `/deploymentinfo` (brunoerg) Pull request description: bitcoin#23508 added a new RPC named `getdeploymentinfo`, it moved the softfork section from `getblockchaininfo` into this new one. In the REST interface, we have an endpoint named`/rest/chaininfo.json` (which refers to `getblockchaininfo`), so, this PR adds a new REST endpoint named `/deploymentinfo` which refers to `getdeploymentinfo`. You can use it by passing a block hash, e.g: '/rest/deploymentinfo/<BLOCKHASH>.json' or you can use it without passing a block hash to get the 'deploymentinfo' for the last block. ACKs for top commit: jonatack: re-ACK a8250e3 rebase-only since my last review at c65f82b achow101: ACK a8250e3 stickies-v: re-ACK bitcoin@a8250e3 Tree-SHA512: 0735183b6828d51a72ed0e2be5a09b314ac4693f548982c6e9adaa0ef07a55aa428d3b2d1b1de70b83169811a663a8624b686166e5797f624dcc00178b9796e6

brunoerg force-pushed the 2022-06-rest-deploymentinfo branch 2 times, most recently from 6879cc5 to 5d71170 Compare June 18, 2022 20:59

DrahtBot added the RPC/REST/ZMQ label Jun 18, 2022

luke-jr suggested changes Jun 18, 2022

View reviewed changes

brunoerg force-pushed the 2022-06-rest-deploymentinfo branch from 5d71170 to c7c4fa2 Compare June 19, 2022 11:52

w0xlt approved these changes Jun 19, 2022

View reviewed changes

doc/REST-interface.md Show resolved Hide resolved

Riahiamirreza reviewed Jun 19, 2022

View reviewed changes

src/rpc/blockchain.cpp Show resolved Hide resolved

Riahiamirreza reviewed Jun 19, 2022

View reviewed changes

src/rest.cpp Show resolved Hide resolved

brunoerg force-pushed the 2022-06-rest-deploymentinfo branch from c7c4fa2 to 06fcbb9 Compare June 19, 2022 18:29

Riahiamirreza approved these changes Jun 19, 2022

View reviewed changes

w0xlt approved these changes Jun 20, 2022

View reviewed changes

jonatack reviewed Jul 6, 2022

View reviewed changes

test/functional/interface_rest.py Outdated Show resolved Hide resolved

src/rest.cpp Outdated Show resolved Hide resolved

doc/REST-interface.md Outdated Show resolved Hide resolved

src/rest.cpp Show resolved Hide resolved

src/rest.cpp Outdated Show resolved Hide resolved

brunoerg force-pushed the 2022-06-rest-deploymentinfo branch from 06fcbb9 to 0be3019 Compare July 7, 2022 21:14

brunoerg requested a review from jonatack July 8, 2022 14:19

jonatack reviewed Jul 9, 2022

View reviewed changes

doc/release-notes-25412.md Outdated Show resolved Hide resolved

jonatack reviewed Jul 9, 2022

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

jonatack reviewed Jul 9, 2022

View reviewed changes

src/rest.cpp Show resolved Hide resolved

brunoerg force-pushed the 2022-06-rest-deploymentinfo branch from 0be3019 to 4399c61 Compare July 11, 2022 16:15

brunoerg force-pushed the 2022-06-rest-deploymentinfo branch 5 times, most recently from 02f9f23 to 07237ec Compare July 20, 2022 01:40

stickies-v mentioned this pull request Jul 30, 2022

rest: Further API and code cleanup with query parameters #25752

Closed

stickies-v approved these changes Aug 1, 2022

View reviewed changes

test/functional/interface_rest.py Show resolved Hide resolved

DrahtBot added the Needs rebase label Aug 5, 2022

brunoerg force-pushed the 2022-06-rest-deploymentinfo branch 2 times, most recently from aa33c3b to c975e8b Compare August 15, 2022 18:01

DrahtBot removed the Needs rebase label Aug 15, 2022

brunoerg force-pushed the 2022-06-rest-deploymentinfo branch from c975e8b to 394f177 Compare August 16, 2022 14:35

w0xlt approved these changes Aug 16, 2022

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

stickies-v approved these changes Aug 16, 2022

View reviewed changes

src/rpc/blockchain.h Outdated Show resolved Hide resolved

brunoerg force-pushed the 2022-06-rest-deploymentinfo branch from 394f177 to 9ef5823 Compare August 16, 2022 17:21

w0xlt approved these changes Aug 16, 2022

View reviewed changes

jonatack reviewed Aug 16, 2022

View reviewed changes

src/rpc/blockchain.h Outdated Show resolved Hide resolved

brunoerg force-pushed the 2022-06-rest-deploymentinfo branch from 9ef5823 to 28d8e11 Compare August 16, 2022 21:23

brunoerg added 4 commits August 16, 2022 19:21

rest: add /deploymentinfo

9149703

test: add coverage for /rest/deploymentinfo

3e44bee

doc: add /deploymentinfo in REST-interface

5c96020

doc: add release note about /rest/deploymentinfo

a8250e3

brunoerg force-pushed the 2022-06-rest-deploymentinfo branch from 28d8e11 to a8250e3 Compare August 16, 2022 22:22

stickies-v approved these changes Aug 17, 2022

View reviewed changes

achow101 merged commit 92be831 into bitcoin:master Oct 13, 2022

bitcoin locked and limited conversation to collaborators Oct 13, 2023

rest: add /deploymentinfo endpoint #25412

rest: add /deploymentinfo endpoint #25412

Uh oh!

Conversation

brunoerg commented Jun 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Jun 19, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Jun 19, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Jul 7, 2022

Uh oh!

jonatack Jul 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Jul 11, 2022

Uh oh!

jonatack commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brunoerg commented Aug 16, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Aug 16, 2022

Uh oh!

stickies-v left a comment

rest: add `/deploymentinfo` endpoint #25412

rest: add `/deploymentinfo` endpoint #25412

DrahtBot commented Jun 21, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack commented Jul 14, 2022 •

edited

Loading

stickies-v left a comment •

edited

Loading

achow101 commented Oct 13, 2022 •

edited

Loading