Merge bitcoin/bitcoin#24098: rest: Use query parameters to control resource loading

54b39cf Add release notes (stickies-v)
f959fc0 Update /<count>/ endpoints to use a '?count=' query parameter instead (stickies-v)
a094976 Add GetQueryParameter helper function (stickies-v)
fff771e Handle query string when parsing data format (stickies-v)
c1aad1b scripted-diff: rename RetFormat to RESTResponseFormat (stickies-v)
9f1c547 Refactoring: move declarations to rest.h (stickies-v)

Pull request description:

  In RESTful APIs, [typically](https://rapidapi.com/blog/api-glossary/parameters/query/) path parameters  (e.g. `/some/unique/resource/`) are used to represent resources, and query parameters (e.g. `?sort=asc`) are used to control how these resources are being loaded through e.g. sorting, pagination, filtering, ...

  As first [discussed in #17631](bitcoin/bitcoin#17631 (comment)), the [current REST api](https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md) contains two endpoints `/headers/` and `/blockfilterheaders/` that rather unexpectedly use path parameters to control how many (filter) headers are returned in the response. While this is no critical issue, it is unintuitive and we are still early enough to easily phase this behaviour out and ensure new endpoints (if any) do not have to stick to non-standard behaviour just for internal consistency.

  In this PR, a new `HTTPRequest::GetQueryParameter` method is introduced to easily parse query parameters, as well as two new `/headers/` and `/blockfilterheaders/` endpoints that use a count query parameter are introduced. The old path parameter-based endpoints are kept without too much overhead, but the documentation now points to the new query parameter-based endpoints as the default interface to encourage standardness.

  ## Behaviour change
  ### New endpoints and default values
  `/headers/` and `/blockfilterheaders/` now have 2 new endpoints that contain query parameters (`?count=<count>`) instead of path parameters (`/<count>/`), as described in REST-interface.md. Since query parameters can easily have default values, I have set this at 5 for both endpoints.

  **headers**
  `GET /rest/headers/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>`
  should now be used instead of
  `GET /rest/headers/<COUNT>/<BLOCK-HASH>.<bin|hex|json>`

  **blockfilterheaders**
  `GET /rest/blockfilterheaders/<FILTERTYPE>/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>`
  should now be used instead of
  `GET /rest/blockfilterheaders/<FILTERTYPE>/<COUNT>/<BLOCK-HASH>.<bin|hex|json>`

  ### Some previously invalid API calls are now valid
  API calls that contained query strings in the URI could not be parsed prior to this PR. This PR changes behaviour in that previously invalid calls (e.g. `GET /rest/headers/5/somehash.json?someunusedparam=foo`) would now become valid, as the query parameters are properly parsed, and discarded if unused.
  For example, prior to this PR, adding an irrelevant `someparam` parameter would be illegal:
  ```
  GET /rest/headers/5/0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?someparam=true
  ->
  Invalid hash: 0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?someparam=true
  ```
  **This behaviour change affects all rest endpoints, not just the 2 new ones introduced here.**

  *(Note: I'd be open to implementing additional logic to refuse requests containing unrecognized query parameters to minimize behaviour change, but for the endpoints that we currently have I don't really see the point for that added complexity. E.g. I don't see any scenarios where misspelling a parameter could lead to harmful outcomes)*

  ## Using the REST API

  To run the API HTTP server, start a bitcoind instance with the `-rest` flag enabled. To use the
  `blockfilterheaders` endpoint, you'll also need to set `-blockfilterindex=1`:
  ```
  ./bitcoind -signet -rest -blockfilterindex=1
  ```

  As soon as bitcoind is fully up and running, you should be able to query the API, for example by
  using curl on the command line: ```curl "127.0.0.1:38332/rest/chaininfo.json"```.
  To more easily parse the JSON output, you can also use tools like 'jq' or `json_pp`, e.g.:
  ```
  curl -s "localhost:38332/rest/blockfilterheaders/basic/0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?count=2" | json_pp .
  ```

  ## To do
  - [x] update `doc/release-notes`

  ## Feedback
  This is my first PR (hooray!). Please don't hold back on any feedback/comments/nits/... you may have, big or small, whether they are code, process, language, ... related. I welcome private messages too if there's anything you don't want to clutter the PR with. I'm here to learn and am grateful for everyone's input.

ACKs for top commit:
  stickies-v:
    I've had to push a tiny doc update to `REST-interface.md` (`git range-diff 219d728 9aac438 54b39cf`) since this was not merged for v23, but since there are no significant changes beyond theStack and jnewbery's ACKs I think this PR is now ready to be considered for merging? @MarcoFalke
  jnewbery:
    ACK 54b39cf
  theStack:
    re-ACK 54b39cf

Tree-SHA512: 3b393ffde34f25605ca12c0b1300799a19684b816a1d03aed38b0f5439df47bfe6a589ffbcd7b83fd2def6c9d00a1bae5e45b1d18df4ae998c617c709990f83f

Author: MarcoFalke

doc/REST-interface.md

@@ -47,18 +47,24 @@ The HTTP request and response are both handled entirely in-memory.
 With the /notxdetails/ option JSON response will only contain the transaction hash instead of the complete transaction details. The option only affects the JSON response.
 
 #### Blockheaders
-`GET /rest/headers/<COUNT>/<BLOCK-HASH>.<bin|hex|json>`
+`GET /rest/headers/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>`
 
 Given a block hash: returns <COUNT> amount of blockheaders in upward direction.
 Returns empty if the block doesn't exist or it isn't in the active chain.
 
+*Deprecated (but not removed) since 24.0:*
+`GET /rest/headers/<COUNT>/<BLOCK-HASH>.<bin|hex|json>`
+
 #### Blockfilter Headers
-`GET /rest/blockfilterheaders/<FILTERTYPE>/<COUNT>/<BLOCK-HASH>.<bin|hex|json>`
+`GET /rest/blockfilterheaders/<FILTERTYPE>/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>`
 
 Given a block hash: returns <COUNT> amount of blockfilter headers in upward
 direction for the filter type <FILTERTYPE>.
 Returns empty if the block doesn't exist or it isn't in the active chain.
 
+*Deprecated (but not removed) since 24.0:*
+`GET /rest/blockfilterheaders/<FILTERTYPE>/<COUNT>/<BLOCK-HASH>.<bin|hex|json>`
+
 #### Blockfilters
 `GET /rest/blockfilter/<FILTERTYPE>/<BLOCK-HASH>.<bin|hex|json>`
 

doc/release-notes-24098.md

@@ -0,0 +1,22 @@
+Notable changes
+===============
+
+Updated REST APIs
+-----------------
+
+- The `/headers/` and `/blockfilterheaders/` endpoints have been updated to use
+  a query parameter instead of path parameter to specify the result count. The
+  count parameter is now optional, and defaults to 5 for both endpoints. The old
+  endpoints are still functional, and have no documented behaviour change.
+
+  For `/headers`, use
+  `GET /rest/headers/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>`
+  instead of
+  `GET /rest/headers/<COUNT>/<BLOCK-HASH>.<bin|hex|json>` (deprecated)
+
+  For `/blockfilterheaders/`, use
+  `GET /rest/blockfilterheaders/<FILTERTYPE>/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>`
+  instead of
+  `GET /rest/blockfilterheaders/<FILTERTYPE>/<COUNT>/<BLOCK-HASH>.<bin|hex|json>` (deprecated)
+
+  (#24098)

src/Makefile.am

@@ -206,6 +206,7 @@ BITCOIN_CORE_H = \
   psbt.h \
   random.h \
   randomenv.h \
+  rest.h \
   reverse_iterator.h \
   rpc/blockchain.h \
   rpc/client.h \

src/Makefile.test.include

@@ -94,6 +94,7 @@ BITCOIN_TESTS =\
   test/fs_tests.cpp \
   test/getarg_tests.cpp \
   test/hash_tests.cpp \
+  test/httpserver_tests.cpp \
   test/i2p_tests.cpp \
   test/interfaces_tests.cpp \
   test/key_io_tests.cpp \
@@ -116,6 +117,7 @@ BITCOIN_TESTS =\
   test/prevector_tests.cpp \
   test/raii_event_tests.cpp \
   test/random_tests.cpp \
+  test/rest_tests.cpp \
   test/reverselock_tests.cpp \
   test/rpc_tests.cpp \
   test/sanity_tests.cpp \

src/httpserver.cpp

@@ -23,18 +23,20 @@
 
 #include <deque>
 #include <memory>
+#include <optional>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string>
 
 #include <sys/types.h>
 #include <sys/stat.h>
 
-#include <event2/thread.h>
 #include <event2/buffer.h>
 #include <event2/bufferevent.h>
-#include <event2/util.h>
+#include <event2/http.h>
 #include <event2/keyvalq_struct.h>
+#include <event2/thread.h>
+#include <event2/util.h>
 
 #include <support/events.h>
 
@@ -639,6 +641,37 @@ HTTPRequest::RequestMethod HTTPRequest::GetRequestMethod() const
     }
 }
 
+std::optional<std::string> HTTPRequest::GetQueryParameter(const std::string& key) const
+{
+    const char* uri{evhttp_request_get_uri(req)};
+
+    return GetQueryParameterFromUri(uri, key);
+}
+
+std::optional<std::string> GetQueryParameterFromUri(const char* uri, const std::string& key)
+{
+    evhttp_uri* uri_parsed{evhttp_uri_parse(uri)};
+    const char* query{evhttp_uri_get_query(uri_parsed)};
+    std::optional<std::string> result;
+
+    if (query) {
+        // Parse the query string into a key-value queue and iterate over it
+        struct evkeyvalq params_q;
+        evhttp_parse_query_str(query, &params_q);
+
+        for (struct evkeyval* param{params_q.tqh_first}; param != nullptr; param = param->next.tqe_next) {
+            if (param->key == key) {
+                result = param->value;
+                break;
+            }
+        }
+        evhttp_clear_headers(&params_q);
+    }
+    evhttp_uri_free(uri_parsed);
+
+    return result;
+}
+
 void RegisterHTTPHandler(const std::string &prefix, bool exactMatch, const HTTPRequestHandler &handler)
 {
     LogPrint(BCLog::HTTP, "Registering HTTP handler for %s (exactmatch %d)\n", prefix, exactMatch);

src/httpserver.h

@@ -5,8 +5,9 @@
 #ifndef BITCOIN_HTTPSERVER_H
 #define BITCOIN_HTTPSERVER_H
 
-#include <string>
 #include <functional>
+#include <optional>
+#include <string>
 
 static const int DEFAULT_HTTP_THREADS=4;
 static const int DEFAULT_HTTP_WORKQUEUE=16;
@@ -83,6 +84,17 @@ class HTTPRequest
      */
     RequestMethod GetRequestMethod() const;
 
+    /** Get the query parameter value from request uri for a specified key, or std::nullopt if the
+     * key is not found.
+     *
+     * If the query string contains duplicate keys, the first value is returned. Many web frameworks
+     * would instead parse this as an array of values, but this is not (yet) implemented as it is
+     * currently not needed in any of the endpoints.
+     *
+     * @param[in] key represents the query parameter of which the value is returned
+     */
+    std::optional<std::string> GetQueryParameter(const std::string& key) const;
+
     /**
      * Get the request header specified by hdr, or an empty string.
      * Return a pair (isPresent,string).
@@ -115,6 +127,20 @@ class HTTPRequest
     void WriteReply(int nStatus, const std::string& strReply = "");
 };
 
+/** Get the query parameter value from request uri for a specified key, or std::nullopt if the key
+ * is not found.
+ *
+ * If the query string contains duplicate keys, the first value is returned. Many web frameworks
+ * would instead parse this as an array of values, but this is not (yet) implemented as it is
+ * currently not needed in any of the endpoints.
+ *
+ * Helper function for HTTPRequest::GetQueryParameter.
+ *
+ * @param[in] uri is the entire request uri
+ * @param[in] key represents the query parameter of which the value is returned
+ */
+std::optional<std::string> GetQueryParameterFromUri(const char* uri, const std::string& key);
+
 /** Event handler closure.
  */
 class HTTPClosure