Skip to content

Conversation

toxeus
Copy link
Contributor

@toxeus toxeus commented Feb 18, 2020

Before, macros defined in <event2/http.h> have been used for some HTTP status codes. <event2/http.h> is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.

Now, the HTTPStatusCode enum from <rpc/protocol.h> is consistently used for all HTTP response implementations.

@practicalswift
Copy link
Contributor

Nice find and welcome as a contributor!

Concept ACK

In addition to HTTP_INTERNAL it seems like we have the same issue also for HTTP_BADMETHOD and HTTP_NOTFOUND?

$ for S in $(git grep '>WriteReply(' | tr "()," "\n" | egrep ^HTTP_ | sort -u); do
      echo "=== $S"
      grep "$S" src/rpc/protocol.h
      echo
  done
=== HTTP_BADMETHOD

=== HTTP_BAD_METHOD
    HTTP_BAD_METHOD            = 405,

=== HTTP_FORBIDDEN
    HTTP_FORBIDDEN             = 403,

=== HTTP_INTERNAL
    HTTP_INTERNAL_SERVER_ERROR = 500,

=== HTTP_NOTFOUND

=== HTTP_OK
    HTTP_OK                    = 200,

=== HTTP_UNAUTHORIZED
    HTTP_UNAUTHORIZED          = 401,

@toxeus
Copy link
Contributor Author

toxeus commented Feb 18, 2020

Thanks! I've stumbled upon this while debugging a client that exceeds the HTTP worker queue. I'll cleanup the remaining status codes.

@toxeus toxeus force-pushed the 500 branch 2 times, most recently from bf4a595 to aff2748 Compare February 18, 2020 07:26
Before, macros defined in `<event2/http.h>` have been used
for some HTTP status codes.
`<event2/http.h>` is included implicitly and the usage
of its status code macros is inconsistent with the majority
HTTP response implementations in this file.

Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is
consistently used for all HTTP response implementations.
@practicalswift
Copy link
Contributor

@toxeus To get some structure in the handling of status code we might want to consider changing from enum HTTPStatusCode to enum class HTTPStatusCode?

Something along the lines of ...

Click for patch
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index c085095a2..b9eeff819 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -388,7 +388,7 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
             responseErrorMessage = strprintf(" (error code %d - \"%s\")", response.error, http_errorstring(response.error));
         }
         throw CConnectionFailed(strprintf("Could not connect to the server %s:%d%s\n\nMake sure the bitcoind server is running and that you are connecting to the correct RPC port.", host, port, responseErrorMessage));
-    } else if (response.status == HTTP_UNAUTHORIZED) {
+    } else if (response.status == static_cast<int>(HTTPStatusCode::HTTP_UNAUTHORIZED)) {
         if (failedToGetAuthCookie) {
             throw std::runtime_error(strprintf(
                 "Could not locate RPC credentials. No authentication cookie could be found, and RPC password is not set.  See -rpcpassword and -stdinrpcpass.  Configuration file: (%s)",
@@ -396,7 +396,7 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
         } else {
             throw std::runtime_error("Authorization failed: Incorrect rpcuser or rpcpassword");
         }
-    } else if (response.status >= 400 && response.status != HTTP_BAD_REQUEST && response.status != HTTP_NOT_FOUND && response.status != HTTP_INTERNAL_SERVER_ERROR)
+    } else if (response.status >= 400 && response.status != static_cast<int>(HTTPStatusCode::HTTP_BAD_REQUEST) && response.status != static_cast<int>(HTTPStatusCode::HTTP_NOT_FOUND) && response.status != static_cast<int>(HTTPStatusCode::HTTP_INTERNAL_SERVER_ERROR))
         throw std::runtime_error(strprintf("server returned HTTP error %d", response.status));
     else if (response.body.empty())
         throw std::runtime_error("no response from server");
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index ff7578922..ec5734997 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -76,13 +76,13 @@ static bool g_rpc_whitelist_default = false;
 static void JSONErrorReply(HTTPRequest* req, const UniValue& objError, const UniValue& id)
 {
     // Send error reply from json-rpc error object
-    int nStatus = HTTP_INTERNAL_SERVER_ERROR;
+    HTTPStatusCode nStatus = HTTPStatusCode::HTTP_INTERNAL_SERVER_ERROR;
     int code = find_value(objError, "code").get_int();
 
     if (code == RPC_INVALID_REQUEST)
-        nStatus = HTTP_BAD_REQUEST;
+        nStatus = HTTPStatusCode::HTTP_BAD_REQUEST;
     else if (code == RPC_METHOD_NOT_FOUND)
-        nStatus = HTTP_NOT_FOUND;
+        nStatus = HTTPStatusCode::HTTP_NOT_FOUND;
 
     std::string strReply = JSONRPCReply(NullUniValue, objError, id);
 
@@ -155,14 +155,14 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
 {
     // JSONRPC handles only POST
     if (req->GetRequestMethod() != HTTPRequest::POST) {
-        req->WriteReply(HTTP_BAD_METHOD, "JSONRPC server handles only POST requests");
+        req->WriteReply(HTTPStatusCode::HTTP_BAD_METHOD, "JSONRPC server handles only POST requests");
         return false;
     }
     // Check authorization
     std::pair<bool, std::string> authHeader = req->GetHeader("authorization");
     if (!authHeader.first) {
         req->WriteHeader("WWW-Authenticate", WWW_AUTH_HEADER_DATA);
-        req->WriteReply(HTTP_UNAUTHORIZED);
+        req->WriteReply(HTTPStatusCode::HTTP_UNAUTHORIZED);
         return false;
     }
 
@@ -177,7 +177,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
         MilliSleep(250);
 
         req->WriteHeader("WWW-Authenticate", WWW_AUTH_HEADER_DATA);
-        req->WriteReply(HTTP_UNAUTHORIZED);
+        req->WriteReply(HTTPStatusCode::HTTP_UNAUTHORIZED);
         return false;
     }
 
@@ -194,7 +194,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
         bool user_has_whitelist = g_rpc_whitelist.count(jreq.authUser);
         if (!user_has_whitelist && g_rpc_whitelist_default) {
             LogPrintf("RPC User %s not allowed to call any methods\n", jreq.authUser);
-            req->WriteReply(HTTP_FORBIDDEN);
+            req->WriteReply(HTTPStatusCode::HTTP_FORBIDDEN);
             return false;
 
         // singleton request
@@ -202,7 +202,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
             jreq.parse(valRequest);
             if (user_has_whitelist && !g_rpc_whitelist[jreq.authUser].count(jreq.strMethod)) {
                 LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, jreq.strMethod);
-                req->WriteReply(HTTP_FORBIDDEN);
+                req->WriteReply(HTTPStatusCode::HTTP_FORBIDDEN);
                 return false;
             }
             UniValue result = tableRPC.execute(jreq);
@@ -222,7 +222,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
                         std::string strMethod = find_value(request, "method").get_str();
                         if (!g_rpc_whitelist[jreq.authUser].count(strMethod)) {
                             LogPrintf("RPC User %s not allowed to call method %s\n", jreq.authUser, strMethod);
-                            req->WriteReply(HTTP_FORBIDDEN);
+                            req->WriteReply(HTTPStatusCode::HTTP_FORBIDDEN);
                             return false;
                         }
                     }
@@ -234,7 +234,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
             throw JSONRPCError(RPC_PARSE_ERROR, "Top-level object parse error");
 
         req->WriteHeader("Content-Type", "application/json");
-        req->WriteReply(HTTP_OK, strReply);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strReply);
     } catch (const UniValue& objError) {
         JSONErrorReply(req, objError, jreq.id);
         return false;
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 0e13b8580..54f0d077c 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -228,7 +228,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
     if (!ClientAllowed(hreq->GetPeer())) {
         LogPrint(BCLog::HTTP, "HTTP request from %s rejected: Client network is not allowed RPC access\n",
                  hreq->GetPeer().ToString());
-        hreq->WriteReply(HTTP_FORBIDDEN);
+        hreq->WriteReply(HTTPStatusCode::HTTP_FORBIDDEN);
         return;
     }
 
@@ -236,7 +236,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
     if (hreq->GetRequestMethod() == HTTPRequest::UNKNOWN) {
         LogPrint(BCLog::HTTP, "HTTP request from %s rejected: Unknown HTTP request method\n",
                  hreq->GetPeer().ToString());
-        hreq->WriteReply(HTTP_BADMETHOD);
+        hreq->WriteReply(HTTPStatusCode::HTTP_BAD_METHOD);
         return;
     }
 
@@ -268,10 +268,10 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
             item.release(); /* if true, queue took ownership */
         else {
             LogPrintf("WARNING: request rejected because http work queue depth exceeded, it can be increased with the -rpcworkqueue= setting\n");
-            item->req->WriteReply(HTTP_INTERNAL, "Work queue depth exceeded");
+            item->req->WriteReply(HTTPStatusCode::HTTP_INTERNAL_SERVER_ERROR, "Work queue depth exceeded");
         }
     } else {
-        hreq->WriteReply(HTTP_NOTFOUND);
+        hreq->WriteReply(HTTPStatusCode::HTTP_NOT_FOUND);
     }
 }
 
@@ -519,7 +519,7 @@ HTTPRequest::~HTTPRequest()
     if (!replySent) {
         // Keep track of whether reply was sent to avoid request leaks
         LogPrintf("%s: Unhandled request\n", __func__);
-        WriteReply(HTTP_INTERNAL, "Unhandled request");
+        WriteReply(HTTPStatusCode::HTTP_INTERNAL_SERVER_ERROR, "Unhandled request");
     }
     // evhttpd cleans up the request, as long as a reply was sent.
 }
@@ -567,7 +567,7 @@ void HTTPRequest::WriteHeader(const std::string& hdr, const std::string& value)
  * Replies must be sent in the main loop in the main http thread,
  * this cannot be done from worker threads.
  */
-void HTTPRequest::WriteReply(int nStatus, const std::string& strReply)
+void HTTPRequest::WriteReply(HTTPStatusCode nStatus, const std::string& strReply)
 {
     assert(!replySent && req);
     if (ShutdownRequested()) {
@@ -579,7 +579,7 @@ void HTTPRequest::WriteReply(int nStatus, const std::string& strReply)
     evbuffer_add(evb, strReply.data(), strReply.size());
     auto req_copy = req;
     HTTPEvent* ev = new HTTPEvent(eventBase, true, [req_copy, nStatus]{
-        evhttp_send_reply(req_copy, nStatus, nullptr, nullptr);
+        evhttp_send_reply(req_copy, static_cast<int>(nStatus), nullptr, nullptr);
         // Re-enable reading from the socket. This is the second part of the libevent
         // workaround above.
         if (event_get_version_number() >= 0x02010600 && event_get_version_number() < 0x02020001) {
diff --git a/src/httpserver.h b/src/httpserver.h
index 46820e6ae..f9710f844 100644
--- a/src/httpserver.h
+++ b/src/httpserver.h
@@ -5,6 +5,8 @@
 #ifndef BITCOIN_HTTPSERVER_H
 #define BITCOIN_HTTPSERVER_H
 
+#include <rpc/protocol.h>
+
 #include <string>
 #include <functional>
 
@@ -112,7 +114,7 @@ public:
      * @note Can be called only once. As this will give the request back to the
      * main thread, do not call any other HTTPRequest methods after calling this.
      */
-    void WriteReply(int nStatus, const std::string& strReply = "");
+    void WriteReply(HTTPStatusCode nStatus, const std::string& strReply = "");
 };
 
 /** Event handler closure.
diff --git a/src/rest.cpp b/src/rest.cpp
index 062955758..cab7226a1 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -83,7 +83,7 @@ static bool RESTERR(HTTPRequest* req, enum HTTPStatusCode status, std::string me
 static CTxMemPool* GetMemPool(HTTPRequest* req)
 {
     if (!g_rpc_node || !g_rpc_node->mempool) {
-        RESTERR(req, HTTP_NOT_FOUND, "Mempool disabled or instance not found");
+        RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "Mempool disabled or instance not found");
         return nullptr;
     }
     return g_rpc_node->mempool;
@@ -130,7 +130,7 @@ static bool CheckWarmup(HTTPRequest* req)
 {
     std::string statusmessage;
     if (RPCIsInWarmup(&statusmessage))
-         return RESTERR(req, HTTP_SERVICE_UNAVAILABLE, "Service temporarily unavailable: " + statusmessage);
+         return RESTERR(req, HTTPStatusCode::HTTP_SERVICE_UNAVAILABLE, "Service temporarily unavailable: " + statusmessage);
     return true;
 }
 
@@ -145,16 +145,16 @@ static bool rest_headers(HTTPRequest* req,
     boost::split(path, param, boost::is_any_of("/"));
 
     if (path.size() != 2)
-        return RESTERR(req, HTTP_BAD_REQUEST, "No header count specified. Use /rest/headers/<count>/<hash>.<ext>.");
+        return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "No header count specified. Use /rest/headers/<count>/<hash>.<ext>.");
 
     long count = strtol(path[0].c_str(), nullptr, 10);
     if (count < 1 || count > 2000)
-        return RESTERR(req, HTTP_BAD_REQUEST, "Header count out of range: " + path[0]);
+        return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Header count out of range: " + path[0]);
 
     std::string hashStr = path[1];
     uint256 hash;
     if (!ParseHashStr(hashStr, hash))
-        return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
+        return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
 
     const CBlockIndex* tip = nullptr;
     std::vector<const CBlockIndex *> headers;
@@ -180,7 +180,7 @@ static bool rest_headers(HTTPRequest* req,
 
         std::string binaryHeader = ssHeader.str();
         req->WriteHeader("Content-Type", "application/octet-stream");
-        req->WriteReply(HTTP_OK, binaryHeader);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, binaryHeader);
         return true;
     }
 
@@ -192,7 +192,7 @@ static bool rest_headers(HTTPRequest* req,
 
         std::string strHex = HexStr(ssHeader.begin(), ssHeader.end()) + "\n";
         req->WriteHeader("Content-Type", "text/plain");
-        req->WriteReply(HTTP_OK, strHex);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strHex);
         return true;
     }
     case RetFormat::JSON: {
@@ -202,11 +202,11 @@ static bool rest_headers(HTTPRequest* req,
         }
         std::string strJSON = jsonHeaders.write() + "\n";
         req->WriteHeader("Content-Type", "application/json");
-        req->WriteReply(HTTP_OK, strJSON);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
         return true;
     }
     default: {
-        return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: .bin, .hex)");
+        return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: .bin, .hex)");
     }
     }
 }
@@ -222,7 +222,7 @@ static bool rest_block(HTTPRequest* req,
 
     uint256 hash;
     if (!ParseHashStr(hashStr, hash))
-        return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
+        return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
 
     CBlock block;
     CBlockIndex* pblockindex = nullptr;
@@ -232,14 +232,14 @@ static bool rest_block(HTTPRequest* req,
         tip = ::ChainActive().Tip();
         pblockindex = LookupBlockIndex(hash);
         if (!pblockindex) {
-            return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
+            return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, hashStr + " not found");
         }
 
         if (IsBlockPruned(pblockindex))
-            return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)");
+            return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, hashStr + " not available (pruned data)");
 
         if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus()))
-            return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
+            return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, hashStr + " not found");
     }
 
     switch (rf) {
@@ -248,7 +248,7 @@ static bool rest_block(HTTPRequest* req,
         ssBlock << block;
         std::string binaryBlock = ssBlock.str();
         req->WriteHeader("Content-Type", "application/octet-stream");
-        req->WriteReply(HTTP_OK, binaryBlock);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, binaryBlock);
         return true;
     }
 
@@ -257,7 +257,7 @@ static bool rest_block(HTTPRequest* req,
         ssBlock << block;
         std::string strHex = HexStr(ssBlock.begin(), ssBlock.end()) + "\n";
         req->WriteHeader("Content-Type", "text/plain");
-        req->WriteReply(HTTP_OK, strHex);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strHex);
         return true;
     }
 
@@ -265,12 +265,12 @@ static bool rest_block(HTTPRequest* req,
         UniValue objBlock = blockToJSON(block, tip, pblockindex, showTxDetails);
         std::string strJSON = objBlock.write() + "\n";
         req->WriteHeader("Content-Type", "application/json");
-        req->WriteReply(HTTP_OK, strJSON);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
         return true;
     }
 
     default: {
-        return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
+        return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
     }
     }
 }
@@ -302,11 +302,11 @@ static bool rest_chaininfo(HTTPRequest* req, const std::string& strURIPart)
         UniValue chainInfoObject = getblockchaininfo(jsonRequest);
         std::string strJSON = chainInfoObject.write() + "\n";
         req->WriteHeader("Content-Type", "application/json");
-        req->WriteReply(HTTP_OK, strJSON);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
         return true;
     }
     default: {
-        return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: json)");
+        return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: json)");
     }
     }
 }
@@ -326,11 +326,11 @@ static bool rest_mempool_info(HTTPRequest* req, const std::string& strURIPart)
 
         std::string strJSON = mempoolInfoObject.write() + "\n";
         req->WriteHeader("Content-Type", "application/json");
-        req->WriteReply(HTTP_OK, strJSON);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
         return true;
     }
     default: {
-        return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: json)");
+        return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: json)");
     }
     }
 }
@@ -349,11 +349,11 @@ static bool rest_mempool_contents(HTTPRequest* req, const std::string& strURIPar
 
         std::string strJSON = mempoolObject.write() + "\n";
         req->WriteHeader("Content-Type", "application/json");
-        req->WriteReply(HTTP_OK, strJSON);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
         return true;
     }
     default: {
-        return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: json)");
+        return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: json)");
     }
     }
 }
@@ -367,7 +367,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
 
     uint256 hash;
     if (!ParseHashStr(hashStr, hash))
-        return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
+        return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
 
     if (g_txindex) {
         g_txindex->BlockUntilSyncedToCurrentChain();
@@ -376,7 +376,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
     CTransactionRef tx;
     uint256 hashBlock = uint256();
     if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock))
-        return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
+        return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, hashStr + " not found");
 
     switch (rf) {
     case RetFormat::BINARY: {
@@ -385,7 +385,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
 
         std::string binaryTx = ssTx.str();
         req->WriteHeader("Content-Type", "application/octet-stream");
-        req->WriteReply(HTTP_OK, binaryTx);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, binaryTx);
         return true;
     }
 
@@ -395,7 +395,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
 
         std::string strHex = HexStr(ssTx.begin(), ssTx.end()) + "\n";
         req->WriteHeader("Content-Type", "text/plain");
-        req->WriteReply(HTTP_OK, strHex);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strHex);
         return true;
     }
 
@@ -404,12 +404,12 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
         TxToUniv(*tx, hashBlock, objTx);
         std::string strJSON = objTx.write() + "\n";
         req->WriteHeader("Content-Type", "application/json");
-        req->WriteReply(HTTP_OK, strJSON);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
         return true;
     }
 
     default: {
-        return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
+        return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
     }
     }
 }
@@ -431,7 +431,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
     // throw exception in case of an empty request
     std::string strRequestMutable = req->ReadBody();
     if (strRequestMutable.length() == 0 && uriParts.size() == 0)
-        return RESTERR(req, HTTP_BAD_REQUEST, "Error: empty request");
+        return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Error: empty request");
 
     bool fInputParsed = false;
     bool fCheckMemPool = false;
@@ -453,7 +453,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
             std::string strOutput = uriParts[i].substr(uriParts[i].find('-')+1);
 
             if (!ParseInt32(strOutput, &nOutput) || !IsHex(strTxid))
-                return RESTERR(req, HTTP_BAD_REQUEST, "Parse error");
+                return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Parse error");
 
             txid.SetHex(strTxid);
             vOutPoints.push_back(COutPoint(txid, (uint32_t)nOutput));
@@ -462,7 +462,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
         if (vOutPoints.size() > 0)
             fInputParsed = true;
         else
-            return RESTERR(req, HTTP_BAD_REQUEST, "Error: empty request");
+            return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Error: empty request");
     }
 
     switch (rf) {
@@ -478,7 +478,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
             if (strRequestMutable.size() > 0)
             {
                 if (fInputParsed) //don't allow sending input over URI and HTTP RAW DATA
-                    return RESTERR(req, HTTP_BAD_REQUEST, "Combination of URI scheme inputs and raw post data is not allowed");
+                    return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Combination of URI scheme inputs and raw post data is not allowed");
 
                 CDataStream oss(SER_NETWORK, PROTOCOL_VERSION);
                 oss << strRequestMutable;
@@ -487,24 +487,24 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
             }
         } catch (const std::ios_base::failure&) {
             // abort in case of unreadable binary data
-            return RESTERR(req, HTTP_BAD_REQUEST, "Parse error");
+            return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Parse error");
         }
         break;
     }
 
     case RetFormat::JSON: {
         if (!fInputParsed)
-            return RESTERR(req, HTTP_BAD_REQUEST, "Error: empty request");
+            return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Error: empty request");
         break;
     }
     default: {
-        return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
+        return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
     }
     }
 
     // limit max outpoints
     if (vOutPoints.size() > MAX_GETUTXOS_OUTPOINTS)
-        return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Error: max outpoints exceeded (max: %d, tried: %d)", MAX_GETUTXOS_OUTPOINTS, vOutPoints.size()));
+        return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, strprintf("Error: max outpoints exceeded (max: %d, tried: %d)", MAX_GETUTXOS_OUTPOINTS, vOutPoints.size()));
 
     // check spentness and form a bitmap (as well as a JSON capable human-readable string representation)
     std::vector<unsigned char> bitmap;
@@ -551,7 +551,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
         std::string ssGetUTXOResponseString = ssGetUTXOResponse.str();
 
         req->WriteHeader("Content-Type", "application/octet-stream");
-        req->WriteReply(HTTP_OK, ssGetUTXOResponseString);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, ssGetUTXOResponseString);
         return true;
     }
 
@@ -561,7 +561,7 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
         std::string strHex = HexStr(ssGetUTXOResponse.begin(), ssGetUTXOResponse.end()) + "\n";
 
         req->WriteHeader("Content-Type", "text/plain");
-        req->WriteReply(HTTP_OK, strHex);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strHex);
         return true;
     }
 
@@ -591,11 +591,11 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
         // return json string
         std::string strJSON = objGetUTXOResponse.write() + "\n";
         req->WriteHeader("Content-Type", "application/json");
-        req->WriteReply(HTTP_OK, strJSON);
+        req->WriteReply(HTTPStatusCode::HTTP_OK, strJSON);
         return true;
     }
     default: {
-        return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
+        return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
     }
     }
 }
@@ -609,14 +609,14 @@ static bool rest_blockhash_by_height(HTTPRequest* req,
 
     int32_t blockheight;
     if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {
-        return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
+        return RESTERR(req, HTTPStatusCode::HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
     }
 
     CBlockIndex* pblockindex = nullptr;
     {
         LOCK(cs_main);
         if (blockheight > ::ChainActive().Height()) {
-            return RESTERR(req, HTTP_NOT_FOUND, "Block height out of range");
+            return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "Block height out of range");
         }
         pblockindex = ::ChainActive()[blockheight];
     }
@@ -625,23 +625,23 @@ static bool rest_blockhash_by_height(HTTPRequest* req,
         CDataStream ss_blockhash(SER_NETWORK, PROTOCOL_VERSION);
         ss_blockhash << pblockindex->GetBlockHash();
         req->WriteHeader("Content-Type", "application/octet-stream");
-        req->WriteReply(HTTP_OK, ss_blockhash.str());
+        req->WriteReply(HTTPStatusCode::HTTP_OK, ss_blockhash.str());
         return true;
     }
     case RetFormat::HEX: {
         req->WriteHeader("Content-Type", "text/plain");
-        req->WriteReply(HTTP_OK, pblockindex->GetBlockHash().GetHex() + "\n");
+        req->WriteReply(HTTPStatusCode::HTTP_OK, pblockindex->GetBlockHash().GetHex() + "\n");
         return true;
     }
     case RetFormat::JSON: {
         req->WriteHeader("Content-Type", "application/json");
         UniValue resp = UniValue(UniValue::VOBJ);
         resp.pushKV("blockhash", pblockindex->GetBlockHash().GetHex());
-        req->WriteReply(HTTP_OK, resp.write() + "\n");
+        req->WriteReply(HTTPStatusCode::HTTP_OK, resp.write() + "\n");
         return true;
     }
     default: {
-        return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
+        return RESTERR(req, HTTPStatusCode::HTTP_NOT_FOUND, "output format not found (available: " + AvailableDataFormatsString() + ")");
     }
     }
 }
diff --git a/src/rpc/protocol.h b/src/rpc/protocol.h
index d1475f452..23d6e94b6 100644
--- a/src/rpc/protocol.h
+++ b/src/rpc/protocol.h
@@ -7,7 +7,7 @@
 #define BITCOIN_RPC_PROTOCOL_H
 
 //! HTTP status codes
-enum HTTPStatusCode
+enum class HTTPStatusCode
 {
     HTTP_OK                    = 200,
     HTTP_BAD_REQUEST           = 400,

@toxeus
Copy link
Contributor Author

toxeus commented Feb 18, 2020

@practicalswift your suggestion increases type safety therefore I like it. But the suggested change is logically independent from this change therefore I'd prefer to make a follow up PR for that.

@practicalswift
Copy link
Contributor

ACK aff2748 -- patch looks correct

@luke-jr
Copy link
Member

luke-jr commented Feb 19, 2020

I'd rather we go the opposite direction, and adopt libevent's macros (eliminating our own).

Is there a reason to have our own duplicated macros? Or are they just holdovers from the pre-libevent implementation?

@toxeus
Copy link
Contributor Author

toxeus commented Feb 19, 2020

@laanwj added the enum in 2012 (285746d) which is before the introduction of evhttpd (40b556d). I'm fine with eliminating the enums and using the libevent macros. Maybe @laanwj has an opinion on this?

@jgarzik
Copy link
Contributor

jgarzik commented Feb 19, 2020

In general, enums confer more type information, passing more information further down into the compile pipeline, vs macros. All other things being equal, that lends weight to a enums>macros preference, for greater -O0 debug-ability -- symbol names in gdb, vs numbers -- as well as greater type clarity & safety.

@practicalswift
Copy link
Contributor

Agree with @jgarzik.

This is what the C++ Core Guidelines has to say:

@kristapsk
Copy link
Contributor

If we go with enum over libevent macros, why not also change type to HTTPStatusCode everywhere (for example, line int nStatus = HTTP_INTERNAL_SERVER_ERROR; in src/httprpc.cpp)?

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK adding new enum class and dropping #include <rpc/protocol.h> from src/httpserver.cpp.

@toxeus
Copy link
Contributor Author

toxeus commented Feb 21, 2020

@promag your statement is self-contradictory. The enum in discussion is in #include <rpc/protocol.h>.

@promag
Copy link
Contributor

promag commented Feb 21, 2020

@toxeus yes I know, I think it should be in src/httpserver.h, where it's needed by WriteReply(). Also sounds weird including a rpc header in httpserver code, should be the other way around.

@toxeus
Copy link
Contributor Author

toxeus commented Feb 21, 2020

Makes sense. Thanks for clarifying 👍

@luke-jr
Copy link
Member

luke-jr commented Feb 21, 2020

HTTP status codes aren't really an enum, though...

@toxeus
Copy link
Contributor Author

toxeus commented Feb 27, 2020

HTTP status codes aren't really an enum, though...

enums are used often beyond the strict semantics you're referring to. They group constants together that belong together. enum classes even add type safety. The fact that the HTTP status codes are defined in an enum since years and nobody seemed to care supports my argument I think.

Still, I'm also not happy about duplicating here something that is also defined in libevent. OTOH, the added type safety and improved code readability do justify a duplication using an enum class.

@jgarzik
Copy link
Contributor

jgarzik commented Feb 27, 2020

HTTP status codes aren't really an enum, though...

You're right. They are a protocol & specification from RFC 2616 and others, which can be implemented as a macro, enum, JSON database, or some other method which best suits the situation.

@laanwj
Copy link
Member

laanwj commented Mar 2, 2020

ACK aff2748
This slightly improves consistency. I do not think converting status codes to an enum class is necessary. As said by others they're well-standardized numbers, not arbitrary enum items.

@laanwj laanwj merged commit ac5c5d0 into bitcoin:master Mar 2, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 2, 2020
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov)

Pull request description:

  Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.

  Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.

ACKs for top commit:
  practicalswift:
    ACK aff2748 -- patch looks correct
  laanwj:
    ACK aff2748

Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Apr 9, 2020
- rpc/wallet: initialize nFeeRequired to avoid using garbage value on failure bitcoin#18274
- httpserver: use own HTTP status codes bitcoin#18168
- tests: Add fuzzing harnesses for various Base{32,58,64} and hex related functions bitcoin#17229
- util: Don't allow Base32/64-decoding or ParseMoney(…) on strings with embedded NUL characters. Add tests. bitcoin#17753
- util: Fail to parse empty string in ParseMoney bitcoin#18225
- util: Fail to parse whitespace-only strings in ParseMoney(...) (instead of parsing as zero) bitcoin#18270
- Replace the LogPrint function with a macro bitcoin#17218
- Fix wallet unload race condition bitcoin#18338
- qt: Fix Window -> Minimize menu item bitcoin#18549
- windows: Enable heap terminate-on-corruption bitcoin#17916
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov)

Pull request description:

  Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.

  Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.

ACKs for top commit:
  practicalswift:
    ACK aff2748 -- patch looks correct
  laanwj:
    ACK aff2748

Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 5, 2021
Summary:
Before, macros defined in `<event2/http.h>` have been used
for some HTTP status codes.
`<event2/http.h>` is included implicitly and the usage
of its status code macros is inconsistent with the majority
HTTP response implementations in this file.

Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is
consistently used for all HTTP response implementations.

This is a backport of Core [[bitcoin/bitcoin#18168 | PR18168]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D8779
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov)

Pull request description:

  Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.

  Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.

ACKs for top commit:
  practicalswift:
    ACK aff2748 -- patch looks correct
  laanwj:
    ACK aff2748

Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov)

Pull request description:

  Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.

  Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.

ACKs for top commit:
  practicalswift:
    ACK aff2748 -- patch looks correct
  laanwj:
    ACK aff2748

Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov)

Pull request description:

  Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.

  Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.

ACKs for top commit:
  practicalswift:
    ACK aff2748 -- patch looks correct
  laanwj:
    ACK aff2748

Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov)

Pull request description:

  Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.

  Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.

ACKs for top commit:
  practicalswift:
    ACK aff2748 -- patch looks correct
  laanwj:
    ACK aff2748

Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov)

Pull request description:

  Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.

  Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.

ACKs for top commit:
  practicalswift:
    ACK aff2748 -- patch looks correct
  laanwj:
    ACK aff2748

Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 14, 2021
aff2748 httpserver: use own HTTP status codes (Filip Gospodinov)

Pull request description:

  Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.

  Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.

ACKs for top commit:
  practicalswift:
    ACK aff2748 -- patch looks correct
  laanwj:
    ACK aff2748

Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
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.

8 participants