rest: clean-up for `mempool` endpoints #25760

brunoerg · 2022-07-31T12:00:52Z

The functions rest_mempool_info and rest_mempool_contents are similar, the only difference between them is:
rest_mempool_info uses MempoolInfoToJSON to get the mempool informations and rest_mempool_contents uses MempoolToJSON, for this reason this PR creates a new function to handle it and reduce duplicated code.

Also,

Rename strURIPart to str_uri_part.
Rename strJSON to str_json.

stickies-v

Concept ACK 029fad6

Alternatively, you could also only register /rest/mempool/ and inspect the str_uri_part? I think this would allow for slightly more code reduction.

DrahtBot · 2022-07-31T19:45:22Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25756 (rest: Remove support for a number of -deprecatedrest options by stickies-v)
#25755 (rest: Use from_blockhash and txdetails query parameters by stickies-v)
#25754 (rest: Extend HTTPRequest interface to support querying path (parameters) by stickies-v)
#25753 (rest: Move format string from path-like parameter to query parameter by stickies-v)
#25412 (rest: add /deploymentinfo endpoint by brunoerg)
#21422 (Add feerate histogram to getmempoolinfo by kiminuo)

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 · 2022-08-02T21:01:18Z

Alternatively, you could also only register /rest/mempool/ and inspect the str_uri_part? I think this would allow for slightly more code reduction.

Nice, I had thought about it, but I think str_uri_part wouldn't get me the whole URI but only (in this case) the format, like: '.json'. Am I missing something?

stickies-v · 2022-08-02T21:22:40Z

http_request_cb passes everything after the endpoint prefix to str_uri_part. If you register the endpoints with prefix /rest/mempool/<info|content>, then indeed only .json would get passed to str_uri_part. If however you just register /rest/mempool/, then everything after that i.e. <info|content>.json would get passed. (plug: #25754 is trying to clean up this interface)

I don't think the current approach of registering both separately is unreasonable, so this is just a suggestion if you think that implementation makes sense - intuitively it would be my first approach.

brunoerg · 2022-08-03T00:36:14Z

Nice, @stickies-v. Force-pushed addressing your suggestion.

It is clearer having only /rest/mempool/, and then check the str_uri_part.

stickies-v

Nice! I've left a few more suggestions in comments, but I think that's pretty much it from me.

Couldn't make suggested code, so here's the diff instead:

diff --git a/src/rest.cpp b/src/rest.cpp
index dd6fe8d8e..3f2ab98a8 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -594,20 +594,23 @@ static bool rest_mempool(const std::any& context, HTTPRequest* req, const std::s
 {
     if (!CheckWarmup(req))
         return false;
-    const CTxMemPool* mempool = GetMemPool(context, req);
-    if (!mempool) return false;
+
     std::string param;
     const RESTResponseFormat rf = ParseDataFormat(param, str_uri_part);
+    if (param != "contents" && param != "info") {
+        return RESTERR(req, HTTP_BAD_REQUEST, "Error: invalid URI format. Expected /rest/mempool/<info|contents>.json");
+    }
+
+    const CTxMemPool* mempool = GetMemPool(context, req);
+    if (!mempool) return false;
 
     switch (rf) {
     case RESTResponseFormat::JSON: {
         std::string str_json;
-        if (str_uri_part == "contents.json") {
+        if (param == "contents") {
             str_json = MempoolToJSON(*mempool, true).write() + "\n";
-        } else if (str_uri_part == "info.json") {
-            str_json = MempoolInfoToJSON(*mempool).write() + "\n";
         } else {
-            return RESTERR(req, HTTP_BAD_REQUEST, "Error: invalid URI format");
+            str_json = MempoolInfoToJSON(*mempool).write() + "\n";
         }
 
         req->WriteHeader("Content-Type", "application/json");

src/rest.cpp

brunoerg · 2022-08-03T18:22:50Z

Thanks, @stickies-v for your review! Force-pushed addressing the latest suggestions.

stickies-v

ACK da0c612c3d69164da19332167c38bfcd1f9777a8 - nice work on removing all the duplicate code!

Just 1 nit that I didn't see earlier, sorry.

src/rest.cpp

theStack

Nice deduplication! 👌

Code-review ACK da0c612c3d69164da19332167c38bfcd1f9777a8

Happy to also re-ACK if you decide to take stickies' nit about removing the 'Error' prefix.

brunoerg · 2022-08-05T13:29:26Z

Force-pushed removing the Error: prefix. Thanks, @stickies-v.

stickies-v

re-ACK acbea66 - verified that just the error message was updated since da0c612

theStack

re-ACK acbea66

jonatack

Concept ACK

jonatack · 2022-08-05T13:47:16Z

src/rest.cpp

+            str_json = MempoolToJSON(*mempool, true).write() + "\n";
+        } else {
+            str_json = MempoolInfoToJSON(*mempool).write() + "\n";
+        }


Maybe reduce temporaries and reassignments here.

case RESTResponseFormat::JSON: { - std::string str_json; - if (param == "contents") { - str_json = MempoolToJSON(*mempool, true).write() + "\n"; - } else { - str_json = MempoolInfoToJSON(*mempool).write() + "\n"; - } - req->WriteHeader("Content-Type", "application/json"); - req->WriteReply(HTTP_OK, str_json); + req->WriteReply(HTTP_OK, (param == "contents" ? MempoolToJSON(*mempool, /*verbose=*/true) : MempoolInfoToJSON(*mempool)).write() + "\n"); return true;

It could make sense to avoid std::string copies in WriteReply, but maybe for a follow-up?

edit: I guess that's not possible, as libevent only accepts a (read-only) span, not the memory directly

acbea66 rest: clean-up for `mempool` endpoints (brunoerg) Pull request description: The functions `rest_mempool_info` and `rest_mempool_contents` are similar, the only difference between them is: `rest_mempool_info` uses `MempoolInfoToJSON` to get the mempool informations and `rest_mempool_contents` uses `MempoolToJSON`, for this reason this PR creates a new function to handle it and reduce duplicated code. Also, 1. Rename `strURIPart` to `str_uri_part`. 2. Rename `strJSON` to `str_json`. ACKs for top commit: stickies-v: re-ACK acbea66 - verified that just the error message was updated since bitcoin@da0c612 theStack: re-ACK acbea66 Tree-SHA512: 35f6f0732a573fe8a6cdcc782f89ae3427a1de19f069a68c9c51bb525118c2b07e20303cbe19b9d4b7d1ad055d69c32def2d0fb8f886c851da562dd9ce33ad6a

fanquake added the RPC/REST/ZMQ label Jul 31, 2022

stickies-v reviewed Jul 31, 2022

View reviewed changes

brunoerg marked this pull request as ready for review August 1, 2022 13:55

brunoerg force-pushed the 2022-07-rest-improvements branch from 029fad6 to 001b45f Compare August 3, 2022 00:32

stickies-v reviewed Aug 3, 2022

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

src/rest.cpp Outdated Show resolved Hide resolved

brunoerg force-pushed the 2022-07-rest-improvements branch from 001b45f to da0c612 Compare August 3, 2022 18:22

stickies-v approved these changes Aug 3, 2022

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

theStack approved these changes Aug 5, 2022

View reviewed changes

rest: clean-up for mempool endpoints

acbea66

brunoerg force-pushed the 2022-07-rest-improvements branch from da0c612 to acbea66 Compare August 5, 2022 13:28

stickies-v approved these changes Aug 5, 2022

View reviewed changes

theStack approved these changes Aug 5, 2022

View reviewed changes

jonatack reviewed Aug 5, 2022

View reviewed changes

maflcko merged commit 7d3817b into bitcoin:master Aug 5, 2022

bitcoin locked and limited conversation to collaborators Aug 23, 2023

rest: clean-up for mempool endpoints #25760

rest: clean-up for mempool endpoints #25760

Conversation

brunoerg commented Jul 31, 2022

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 31, 2022

Conflicts

Uh oh!

brunoerg commented Aug 2, 2022

Uh oh!

stickies-v commented Aug 2, 2022

Uh oh!

brunoerg commented Aug 3, 2022

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!

Uh oh!

brunoerg commented Aug 3, 2022

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Aug 5, 2022

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 5, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 5, 2022 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rest: clean-up for `mempool` endpoints #25760

rest: clean-up for `mempool` endpoints #25760

stickies-v left a comment •

edited

Loading

maflcko Aug 5, 2022 •

edited

Loading