rest: Expose block filters follow-ups #23836

1. 
   dergoegge commented at 10:43 pm on December 21, 2021: member

   This PR addresses unresolved review comments from #17631. This includes:

   • various style fix-ups
   • returning a more verbose error message for invalid header counts
   • removing superfluous rpc serializations flags for block filters
   • improving the test to include comparing the block filters returned from the rest with the ones returned from the getblockfilter RPC.
2. fanquake added the label RPC/REST/ZMQ on Dec 21, 2021
3. in src/rest.cpp:198 in c67f1d74c4 outdated

   194@@ -195,7 +195,7 @@ static bool rest_headers(const std::any& context,
   195 
   196     const auto parsed_count{ToIntegral<size_t>(path[0])};
   197     if (!parsed_count.has_value() || *parsed_count < 1 || *parsed_count > MAX_REST_HEADERS_RESULTS) {
   198-        return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Header count out of acceptable range (1-%u): %s",  MAX_REST_HEADERS_RESULTS, path[0]));
   199+        return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Header count is invalid or out of acceptable range (1-%u): %s", MAX_REST_HEADERS_RESULTS, path[0]));
   

   ---

   ---

   

   MarcoFalke commented at 9:39 am on December 22, 2021:

   first commit:

   Changing the error message is a user-visible behavior change, not a refactor.

   ---

   

   dergoegge commented at 8:00 pm on December 22, 2021:

   I split this into a separate commit
4. in src/rest.cpp:416 in c67f1d74c4 outdated

   412@@ -414,7 +413,7 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const
   413 
   414     switch (rf) {
   415     case RetFormat::BINARY: {
   416-        CDataStream ssHeader(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags());
   417+        CDataStream ssHeader(SER_NETWORK, PROTOCOL_VERSION);
   

   ---

   ---

   

   MarcoFalke commented at 9:40 am on December 22, 2021:

   0        CDataStream ssHeader{SER_NETWORK, PROTOCOL_VERSION};
   

   Second commit: nit:

   While touching this line, could use C++11 init?

5. 
   MarcoFalke commented at 9:41 am on December 22, 2021: member

   Concept ACK. For reference, I’ve also opened #23599 to clean up the RPCTxSerializationFlags function.

   I think the pull title and description can be modified to be self-contained. That is, following links is not needed to understand the changes.

6. [refactor] various style fix-ups 83b8f3a896
7. [rest] add a more verbose error message for invalid header counts 064abd14a5
8. [rest] drop superfluous rpc serializations flags for block filters 3a2464f216
9. [test] compare filter and header with the result of the getblockfilter RPC 4523d28b6b
10. dergoegge force-pushed on Dec 22, 2021
11. dergoegge renamed this:
    #17631 followups
    rest: Expose block filters follow-ups
    on Dec 22, 2021
12. brunoerg approved
13. 
    brunoerg commented at 2:43 pm on December 28, 2021: member

    tACK 4523d28b6bd550bf9f9c724f9aa90c3d2d4ebe3b

    Style fix-ups and the new error message seem ok for me, I ran the test and it passed successfully.

14. 
    fanquake commented at 6:34 am on December 30, 2021: member

    cc @stickies-v @jnewbery
15. 
    jnewbery commented at 10:47 am on January 1, 2022: member

    ACK 4523d28b6bd550bf9f9c724f9aa90c3d2d4ebe3b

    Thanks for picking this up!

16. fanquake merged this on Jan 2, 2022
17. fanquake closed this on Jan 2, 2022

    ---

18. sidhujag referenced this in commit d7d5eb3753 on Jan 2, 2022
19. DrahtBot locked this on Jan 2, 2023

Contributors
dergoegge MarcoFalke brunoerg fanquake jnewbery

Labels
RPC/REST/ZMQ