This adds a new rest endpoint: /rest/blockfilter/filtertype/requesttype/blockhash (eg /rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex) which exposes either the filter “header” or the filter data itself. Most of the code is cribbed from the equivalent RPC.
You can test it at http://bitcoin-rest.bitcoin.ninja/rest//blockfilter/basic/header/000000005b7a58a939b2636f61fa4ddd62258c5fed57667a35d23f2334c4f86d.hex
After reading doc/REST-interface.md I’m not entirely clear about the assumed trust boundaries.
What recommendations do we give to our users regarding exposing the REST endpoints publicly? Do the recommendations differ from our recommendations with regards to exposing the JSON-RPC endpoints publicly?
As I’ve understood it we regard the JSON-RPC interface as as an internal control plane only to be accessible by trusted clients. The assumption we’re making from a trust boundary perspective seems to be that we assume that an untrusted clients will never be able to connect to the port serving the JSON-RPC interface (which is the same port as the REST interface).
Concept ACK.
After reading doc/REST-interface.md I’m not entirely clear about the assumed trust boundaries.
That’s a fair question (FWIW the limit has always been: only public data, no complex queries, do not parse JSON as input), but I’d suggest opening a new issue for it. Please keep this one for review of the code changes.
The REST interface is a lightweight interface for querying public data. Consumers are trusted but less so than on RPC (as they don’t authenticate). I still wouldn’t recommend exposing it directly to the internet. But maybe it’s OK to open it “publicly” inside some LAN or VPN that your applications run in.
This is my last general comment on this, please open a new issue if you want to continue this discussion.
doc/REST-interface.md
Speaking of which, please update the documentation to mention this new call.
280+ if (uriParts.size() != 3) {
281+ return RESTERR(req, HTTP_BAD_REQUEST, "Invalid URI format. Expected /rest/blockfilter/filtertype/requesttype/blockhash");
282+ }
283+
284+ uint256 block_hash;
285+ if (!ParseHashStr(uriParts[2], block_hash)) {
295+ return RESTERR(req, HTTP_BAD_REQUEST, "Index is not enabled for filtertype " + uriParts[0]);
296+ }
297+
298+ long count = strtol(uriParts[1].c_str(), nullptr, 10);
299+ if (count < 1 || count > 2000) {
300+ return RESTERR(req, HTTP_BAD_REQUEST, "Header count out of range: " + uriParts[1]);
294+ if (!index) {
295+ return RESTERR(req, HTTP_BAD_REQUEST, "Index is not enabled for filtertype " + uriParts[0]);
296+ }
297+
298+ long count = strtol(uriParts[1].c_str(), nullptr, 10);
299+ if (count < 1 || count > 2000) {
2000 be turned into a constant?
367+ req->WriteHeader("Content-Type", "application/json");
368+ req->WriteReply(HTTP_OK, strJSON);
369+ return true;
370+ }
371+ default: {
372+ return RESTERR(req, HTTP_NOT_FOUND, "output format not found (available: .bin, .hex)");
397+ if (!BlockFilterTypeByName(uriParts[0], filtertype)) {
398+ return RESTERR(req, HTTP_BAD_REQUEST, "Unknown filtertype " + uriParts[0]);
399+ }
400+
401+ BlockFilterIndex* index = GetBlockFilterIndex(filtertype);
402+ if (!index) {
filtertype, index and blockhash is shared between these two functions. Should this extraction code be pulled out into their own functions?
407+ bool block_was_connected;
408+ {
409+ LOCK(cs_main);
410+ block_index = LookupBlockIndex(block_hash);
411+ if (!block_index) {
412+ return RESTERR(req, HTTP_NOT_FOUND, uriParts[1] + " not found");
"Block " + uriParts[1] + " not found"?
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
129@@ -128,8 +130,8 @@ static bool rest_headers(HTTPRequest* req,
130 return RESTERR(req, HTTP_BAD_REQUEST, "No header count specified. Use /rest/headers/<count>/<hash>.<ext>.");
131
132 long count = strtol(path[0].c_str(), nullptr, 10);
133- if (count < 1 || count > 2000)
134- return RESTERR(req, HTTP_BAD_REQUEST, "Header count out of range: " + path[0]);
135+ if (count < 1 || count > MAX_HEADERS_RESULTS)
136+ return RESTERR(req, HTTP_BAD_REQUEST, "Header count out of acceptable range (1-2000): " + path[0]);
%u here instead of 2000?
28@@ -27,6 +29,7 @@
29 #include <univalue.h>
30
31 static const size_t MAX_GETUTXOS_OUTPOINTS = 15; //allow a max of 15 outpoints to be queried at once
32+static const unsigned int MAX_HEADERS_RESULTS = 2000;
constrexpr
287@@ -285,6 +288,206 @@ static bool rest_block_notxdetails(HTTPRequest* req, const std::string& strURIPa
288 return rest_block(req, strURIPart, false);
289 }
290
291+static bool rest_filter_header(HTTPRequest* req, const std::string& strURIPart)
Couple of nits. Otherwise looks good.
Commit order is slightly confused. It adds code in one commit then changes it in later commits. I’d suggest putting the fixes first, and then adding the new block filter code in the final commit.
295+ const RetFormat rf = ParseDataFormat(param, strURIPart);
296+
297+ std::vector<std::string> uriParts;
298+ boost::split(uriParts, param, boost::is_any_of("/"));
299+ if (uriParts.size() != 3) {
300+ return RESTERR(req, HTTP_BAD_REQUEST, "Invalid URI format. Expected /rest/blockfilterheaders/filtertype/count/blockhash");
<> around parameters would be clearer
313+ BlockFilterIndex* index = GetBlockFilterIndex(filtertype);
314+ if (!index) {
315+ return RESTERR(req, HTTP_BAD_REQUEST, "Index is not enabled for filtertype " + uriParts[0]);
316+ }
317+
318+ long count = strtol(uriParts[1].c_str(), nullptr, 10);
strtol -> ParseInt32 (#20457)
rest_headers either, which this code was copied from. Should both change or is it supposed to be strtol?
MAX_HEADERS_RESULTS in rest_headers anyway, you could do this there too. Up to you.
strtol or something else is used, but whether parsing errors are ignored or not. strtol makes it easy to ignore them, so it seems reasonable to use our wrappers. (Or alternatively, add the error handling)
const auto parsed_count{ToIntegral<size_t>(uri_parts[1])};, so good, I presume (though admittedly I don’t really know what that does….mumbles about C++).
327+ const CBlockIndex* pindex = LookupBlockIndex(block_hash);
328+ while (pindex != nullptr && ::ChainActive().Contains(pindex)) {
329+ headers.push_back(pindex);
330+ if (headers.size() == (unsigned long)count)
331+ break;
332+ pindex = ::ChainActive().Next(pindex);
re: #21866
0const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(block_hash);
1CChain& active_chain = chainman.ActiveChain();
2...
3pindex = active_chain.Next(pindex);
doc/REST-interface.md that Wladimir suggested above.
279@@ -280,6 +280,9 @@ def run_test(self):
280 self.sync_all()
281 json_obj = self.test_rest_request("/headers/5/{}".format(bb_hash))
282 assert_equal(len(json_obj), 5) # now we should have 5 header objects
283+ json_obj = self.test_rest_request("/blockfilterheaders/basic/5/{}".format(bb_hash))
284+ assert_equal(len(json_obj), 5) # now we should have 5 filter header objects
285+ self.test_rest_request("/blockfilter/basic/{}".format(bb_hash), req_type=ReqType.BIN, ret_type=RetType.OBJ)
0 self.test_rest_request(f"/blockfilter/basic/{bb_hash}", req_type=ReqType.BIN, ret_type=RetType.OBJ)
concept NACK: i’m mostly of the opinion we should deprecate the rest API and make it the responsibility of a proxy server.
short of deprecating it, i don’t think we should add new data items.
concept NACK: i’m mostly of the opinion we should deprecate the rest API and make it the responsibility of a proxy server.
That’s a huge pain. The REST interface is nice precisely because you can build a proxy server for it easy - its like 10 lines of nginx.conf. JSON-RPC isn’t a nice protocol that is broadly supported, its basically a Bitcoin Core-only thing at this point.
31@@ -30,6 +32,7 @@
32 #include <univalue.h>
33
34 static const size_t MAX_GETUTXOS_OUTPOINTS = 15; //allow a max of 15 outpoints to be queried at once
35+static constexpr unsigned int MAX_HEADERS_RESULTS = 2000;
net_processing’s to a header?
349+ boost::split(uriParts, param, boost::is_any_of("/"));
350+ if (uriParts.size() != 3) {
351+ return RESTERR(req, HTTP_BAD_REQUEST, "Invalid URI format. Expected /rest/blockfilterheaders/<filtertype>/<count>/<blockhash>");
352+ }
353+
354+ uint256 blockHash;
370+ if (count < 1 || (unsigned int)count > MAX_HEADERS_RESULTS) {
371+ return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Header count out of acceptable range (1-%u): %s", MAX_HEADERS_RESULTS, uriParts[1]));
372+ }
373+
374+
375+ const CBlockIndex* tip = nullptr;
This pull request conflicts with the target branch and needs rebase.
Lol come on @MarcoFalke…merging 5 day old lint fixes against conflicting feature PRs…..
405+ errmsg += " This error is unexpected and indicates index corruption.";
406+ }
407+
408+ return RESTERR(req, HTTP_NOT_FOUND, errmsg);
409+ }
410+ filter_headers.push_back(filter_header);
std::move this
420+ std::string binaryHeader = ssHeader.str();
421+ req->WriteHeader("Content-Type", "application/octet-stream");
422+ req->WriteReply(HTTP_OK, binaryHeader);
423+ return true;
424+ }
425+
427+ CDataStream ssHeader(SER_NETWORK, PROTOCOL_VERSION);
428+ for (const uint256& header : filter_headers) {
429+ ssHeader << header;
430+ }
431+
432+ std::string strHex = HexStr(ssHeader) + "\n";
There are only two differences between BINARY and HEX:
ssHeader.str() and HexStr(ssHeader)"application/octet-stream" vs "text/plain"It could be written a lot more concisely as something like this:
0case RetFormat::BINARY:
1case RetFormat::HEX: {
2 CDataStream ssHeader(SER_NETWORK, PROTOCOL_VERSION);
3 for (const uint256& header : filter_headers) {
4 ssHeader << header;
5 }
6
7 std::string header_string = (rf == RetFormat::HEX) ? HexStr(ssHeader) : ssHeader.str();
8 std::string content_type = (rf == RetFormat::HEX) ? "text/plain" : "application/octet-stream";
9 req->WriteHeader("Content-Type", content_type);
10 req->WriteReply(HTTP_OK, header_string);
11 return true;
12}
Merge conflicts are certainly frustrating and I want to apologize for that. Though, the conflicting pull was not a “lint fix”, but a bug fix. Sure, there was no bug report, but I think that bugs should still be fixed once they are known. When the pull was created, I didn’t expect a conflict and it seems there was no activity here for more than a year (https://github.com/bitcoin/bitcoin/pull/17631#ref-pullrequest-1019792843). When the pull was merged, it had several ACKs and this one had none (IIRC). Also, it seems there was a review comment about the integer parsing still unsolved: #17631 (review). (A sister pull request #23227 was also merged after 4 days with 3 ACKs, even though there was a conflict, so I think this is generally fine).
521+ req->WriteReply(HTTP_OK, binaryResp);
522+ return true;
523+ }
524+
525+ case RetFormat::HEX: {
526+ CDataStream ssResp(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags());
0 CDataStream ssResp(SER_NETWORK, PROTOCOL_VERSION);
nit if you need to retouch. This shouldn’t have any effect, since no txs are serialized?
RPCSerializationFlags only affect txs (and thus blocks), but nothing else.
339@@ -337,6 +340,213 @@ static bool rest_block_notxdetails(const std::any& context, HTTPRequest* req, co
340 return rest_block(context, req, strURIPart, false);
341 }
342
343+
344+static bool rest_filter_header(const std::any& context, HTTPRequest* req, const std::string& strURIPart) {
345+ if (!CheckWarmup(req))
nit, if you need to re-touch, according to clang-format:
0static bool rest_filter_header(const std::any& context, HTTPRequest* req, const std::string& strURIPart)
1{
2 if (!CheckWarmup(req)) {
This adds a new rest endpoint:
/rest/blockfilter/filtertype/requesttype/blockhash (eg
/rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
which exposes either the filter "header" or the filter data itself.
Most of the code is cribbed from the equivalent RPC.
Addressed feedback, diff since the previous round of acks:
0$ git diff-tree -U1 7cf4e22080 2b64fa3251
1diff --git a/src/rest.cpp b/src/rest.cpp
2index dd1be52f86..18ae350630 100644
3--- a/src/rest.cpp
4+++ b/src/rest.cpp
5@@ -343,3 +343,4 @@ static bool rest_block_notxdetails(const std::any& context, HTTPRequest* req, co
6
7-static bool rest_filter_header(const std::any& context, HTTPRequest* req, const std::string& strURIPart) {
8+static bool rest_filter_header(const std::any& context, HTTPRequest* req, const std::string& strURIPart)
9+{
10 if (!CheckWarmup(req))
11@@ -414,3 +415,3 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const
12 case RetFormat::BINARY: {
13- CDataStream ssHeader(SER_NETWORK, PROTOCOL_VERSION);
14+ CDataStream ssHeader(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags());
15 for (const uint256& header : filter_headers) {
16@@ -424,5 +425,4 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const
17 }
18-
19 case RetFormat::HEX: {
20- CDataStream ssHeader(SER_NETWORK, PROTOCOL_VERSION);
21+ CDataStream ssHeader(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags());
22 for (const uint256& header : filter_headers) {
23@@ -451,6 +451,6 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const
24 }
25-
26 }
27
28-static bool rest_block_filter(const std::any& context, HTTPRequest* req, const std::string& strURIPart) {
29+static bool rest_block_filter(const std::any& context, HTTPRequest* req, const std::string& strURIPart)
30+{
31 if (!CheckWarmup(req))
32@@ -523,3 +523,2 @@ static bool rest_block_filter(const std::any& context, HTTPRequest* req, const s
33 }
34-
35 case RetFormat::HEX: {
36@@ -533,3 +532,2 @@ static bool rest_block_filter(const std::any& context, HTTPRequest* req, const s
37 }
38-
39 case RetFormat::JSON: {
40@@ -542,3 +540,2 @@ static bool rest_block_filter(const std::any& context, HTTPRequest* req, const s
41 }
42-
43 default: {
44$
ACK 2b64fa3251ac5ff4b4d174f1f0be7226490dce87 - Adding blockfilters to the REST interface is analogous to serving other public data such as transactions or blocks.
Would be nice to have the code DRY’d up at some point but i would consider it fine for now as that is true for many parts of rest.cpp .
Consider updating the PR description with the updated endpoint paths. ie: /rest/blockfilterheaders/basic/<COUNT>/... instead of /rest/blockfilter/basic/header/....
355+ }
356+
357+ uint256 block_hash;
358+ if (!ParseHashStr(uri_parts[2], block_hash)) {
359+ return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + uri_parts[2]);
360+ }
filtertype checks? I would expect filtertype to be checked first, unless it is done on purpose for performance reasons.
192@@ -190,8 +193,8 @@ static bool rest_headers(const std::any& context,
193 return RESTERR(req, HTTP_BAD_REQUEST, "No header count specified. Use /rest/headers/<count>/<hash>.<ext>.");
194
195 const auto parsed_count{ToIntegral<size_t>(path[0])};
196- if (!parsed_count.has_value() || *parsed_count < 1 || *parsed_count > 2000) {
197- return RESTERR(req, HTTP_BAD_REQUEST, "Header count out of range: " + path[0]);
198+ if (!parsed_count.has_value() || *parsed_count < 1 || *parsed_count > MAX_REST_HEADERS_RESULTS) {
199+ return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Header count out of acceptable range (1-%u): %s", MAX_REST_HEADERS_RESULTS, path[0]));
393+
394+ bool index_ready = index->BlockUntilSyncedToCurrentChain();
395+
396+ std::vector<uint256> filter_headers;
397+ filter_headers.reserve(*parsed_count);
398+ for (const CBlockIndex *pindex : headers) {
0 for (const CBlockIndex* pindex : headers) {
372+ const auto parsed_count{ToIntegral<size_t>(uri_parts[1])};
373+ if (!parsed_count.has_value() || *parsed_count < 1 || *parsed_count > MAX_REST_HEADERS_RESULTS) {
374+ return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Header count out of acceptable range (1-%u): %s", MAX_REST_HEADERS_RESULTS, uri_parts[1]));
375+ }
376+
377+ std::vector<const CBlockIndex *> headers;
0 std::vector<const CBlockIndex*> headers;
clang-format -i src/rest.cpp? :)
rest.cpp are from previous PRs. But in general, it would be a good thing.
369+ return RESTERR(req, HTTP_BAD_REQUEST, "Index is not enabled for filtertype " + uri_parts[0]);
370+ }
371+
372+ const auto parsed_count{ToIntegral<size_t>(uri_parts[1])};
373+ if (!parsed_count.has_value() || *parsed_count < 1 || *parsed_count > MAX_REST_HEADERS_RESULTS) {
374+ return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Header count out of acceptable range (1-%u): %s", MAX_REST_HEADERS_RESULTS, uri_parts[1]));
0 return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Header count out of acceptable range (1-%u): %s", MAX_REST_HEADERS_RESULTS, uri_parts[1]));
339@@ -337,6 +340,210 @@ static bool rest_block_notxdetails(const std::any& context, HTTPRequest* req, co
340 return rest_block(context, req, strURIPart, false);
341 }
342
343+
344+static bool rest_filter_header(const std::any& context, HTTPRequest* req, const std::string& strURIPart)
0static bool rest_filter_header(const std::any& context, HTTPRequest* req, const std::string& strURIPart)
277@@ -278,11 +278,14 @@ def run_test(self):
278 self.sync_all()
279 json_obj = self.test_rest_request(f"/headers/5/{bb_hash}")
280 assert_equal(len(json_obj), 5) # now we should have 5 header objects
281+ json_obj = self.test_rest_request(f"/blockfilterheaders/basic/5/{bb_hash}")
282+ assert_equal(len(json_obj), 5) # now we should have 5 filter header objects
283+ self.test_rest_request(f"/blockfilter/basic/{bb_hash}", req_type=ReqType.BIN, ret_type=RetType.OBJ)
Would it make sense to do some sanity testing on the returned object? For example:
0 obj = self.test_rest_request(f"/blockfilter/basic/{bb_hash}", req_type=ReqType.BIN, ret_type=RetType.OBJ)
1 assert_greater_than(len(obj.read()), 0)
or perhaps even verify that the filter is the same as that returned by the getblockfilter RPC.
339@@ -337,6 +340,210 @@ static bool rest_block_notxdetails(const std::any& context, HTTPRequest* req, co
340 return rest_block(context, req, strURIPart, false);
341 }
342
343+
344+static bool rest_filter_header(const std::any& context, HTTPRequest* req, const std::string& strURIPart)
345+{
346+ if (!CheckWarmup(req))
347+ return false;
Current style is to have the clause on the same line:
0 if (!CheckWarmup(req)) return false;
or enclosed in braces:
0 if (!CheckWarmup(req)) {
1 return false;
2 }
Same for if (!CheckWarmup(req)) in rest_block_filter() below.
Nearly ACK. A few minor comments inline. The commit log for the first commit is outdated:
0 Expose block filters over REST.
1
2 This adds a new rest endpoint:
3 /rest/blockfilter/filtertype/requesttype/blockhash (eg
4 /rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
5 which exposes either the filter "header" or the filter data itself.
6 Most of the code is cribbed from the equivalent RPC.
The endpoints are now /rest/blockfilterheaders/<FILTERTYPE>/<COUNT>/<BLOCK-HASH> and /rest/blockfilter/<FILTERTYPE>/<BLOCK-HASH>
51@@ -52,6 +52,20 @@ With the /notxdetails/ option JSON response will only contain the transaction ha
52 Given a block hash: returns <COUNT> amount of blockheaders in upward direction.
53 Returns empty if the block doesn't exist or it isn't in the active chain.
54
55+#### Blockfilter Headers
56+`GET /rest/blockfilterheaders/<FILTERTYPE>/<COUNT>/<BLOCK-HASH>.<bin|hex|json>`
From my experience (and e.g. this stackoverflow reponse), in a RESTful API path parameters are used to represent resources, and query parameters are used to control how this resource is being filtered/sorted/… To my understanding, <COUNT> does not represent a resource but rather a control on how many blockfilterheader resources to retrieve.
What do you think about changing the request into
GET /rest/blockfilterheaders/<FILTERTYPE>/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT>?
GET /rest/blockfilterheaders/<FILTERTYPE>/<COUNT>/<BLOCK-HASH><bin|hex|json> was that the blockheader endpoint already uses the same format. (GET /rest/headers/<COUNT>/<BLOCK-HASH>.<bin|hex|json>)
@stickies-v makes a good point but in my opinion staying consistent with the current API was the better move.
/headers/ and /blockfilterheaders/ endpoints.
re-ack
This feature was roughly ready when the pull was opened more than two years ago. It’s been held back on mostly non-behaviour changing “nits”, which still haven’t been addressed yet. I don’t think they’ll be addressed any time soon, so it might be better to fix them in a follow-up at this point. Going to merge to move this forward.
It’s been held back on mostly non-behaviour changing “nits”, which still haven’t been addressed yet. I don’t think they’ll be addressed any time soon, so it might be better to fix them in a follow-up at this point. @MarcoFalke I’m sure this wasn’t your intention, but it seems very inconsistent that for most contributors PRs will be closed if review comments haven’t been addressed, but in this case the PR was merged despite review comments not being addressed for many months. It shouldn’t be the case that certain contributors can get their PRs merged even if they don’t follow the project style guide or respond to review comments.
I also disagree that #17631 (review) is a “nit”. It’s feedback on the interface design and changing it later would be an API break.
It shouldn’t be the case that certain contributors can get their PRs merged even if they don’t follow the project style guide or respond to review comments.
Style-only comments are not mandatory to address. This has always been the case, unless the style issue is so “severe” that a linter enforces it. If there are any style issues here that I missed and should be blocking, someone should add a linter to enforce them. Though, I couldn’t find any.
I also disagree that #17631 (comment) is a “nit”. It’s feedback on the interface design and changing it later would be an API break.
Apologies, my bad. I wasn’t aware there is a suggestion that requests to change the behaviour.
Style-only comments are not mandatory to address. This has always been the case, unless the style issue is so “severe” that a linter enforces it. If there are any style issues here that I missed and should be blocking, someone should add a linter to enforce them. Though, I couldn’t find any.
I disagree - all review comments should be addressed, even if that only means responding with a comment “I don’t intend to change this because …”. If we don’t have a culture where all review comments are responded to, then it falls to the maintainers to make the decision about whether the ignored review comments are important or not, and sometimes important review comments fall through the gaps.
I didn’t see any NACK.
I have no idea how to deal with this, but maybe it could make sense to re-NACK a pull request whenever there is a change (push)? Or maybe use the “GitHub review” and click the red cross, which might not be hidden?
I agree with Marco that this has been basically ready for years, even if it might be imperfect. It’s not too late for further improvements in followups, but blocking this on it was letting the perfect be the enemy of the good IMO.
That being said, it does seem to be a change in merge policy (IMO for the better), and should probably be discussed with the project and applied consistently.
@MarcoFalke well it’s a Concept Nack, so it doesn’t get stale with changes (as opposed to an Approach Nack + Concept Ack).
W.r.t mutually exclusive, the contention that I was holding (and @practicalswift too I think?) is that a REST API exposed for unauthenticated users without a strong statement of the security model is probably something we shouldn’t be maintaining or increasing dependency on for core, and a preference for doing it as a layer on top of a more strongly audited and maintained interface (JSON RPC) being superior. So extending the REST API and increasing dependency on it for consumers of Core is counter to the goal of removing it and replacing with ‘userland’ proxies.
@JeremyRubin I think the discussion whether to provide a wrapper, and then later whether or not to deprecate/remove the existing REST interface is mostly independent from the question of what features should be exposed by the REST interface (whether that’s provided by bitcoind directly or by a wrapper). Whatever risks exists by having a REST interface in the first place aren’t worsened by adding another method.
I do agree the merging procedure followed here was dubious, though.
I do agree the merging procedure followed here was dubious, though.
Ok, maybe I am being blind, but can someone explain to me what was wrong about the merging procedure?
Did this have too little review?
I’d say no. There were:
If it is not possible to merge a pull request with 8 ACKs, then I honestly don’t know how much review is needed to get anything merged. I expect that 99.8% of all pull request have less than 8 ACKs.
Was this merged before a comment should have been replied to?
Yes, I missed the comment/question about the interface, and it should have been replied to before merge. Luckily it has been replied to after merge (and in my view resolved). The only unaddressed stuff now is refactoring nit comments.
I don’t think the merging procedure changed here. I still think that comments should be replied to before merge. However, I also think that style comments that haven’t been replied to should not block a pull request indefinitely. If the overall direction of a pull request has received overall support (which this pull request did), then I think style-nits can be dropped or fixed in a follow-up. If the overall direction of a pull request is not agreed upon, or if the style-nits are rendering a follow-up to be a re-implementation, then I think it should not be merged (not applicable to this pull request).
Something else?
Again, I am not seeing anything else wrong, so please elaborate.
can someone explain to me what was wrong about the merging procedure?
Was this merged before a comment should have been replied to?
Yes @MarcoFalke - this is all I’m claiming - that review comments should be acknowledged and responded to before merge. The author doesn’t have to agree with them or implement them, but if someone has taken the time to review the PR, the author should respond to that review before the PR is merged. That happens in 99% of PRs, and I don’t see why this one should have been different.
For what it’s worth, I agree with @stickies-v that we should make the REST API RESTful. I don’t feel very strongly about it because I don’t use the REST API myself, but I think the discussion should be had before merge.
And to be clear, I think you do a fantastic job as a maintainer. In this instance you missed something, but it should never have been your responsibility to assess whether that comment should have been ignored. It’s the author’s responsibility to address all feedback on their PR.
FWIW - I’ve marked my RESTful comment as resolved, since @dergoegge pointed out this is consistent with another endpoint. As such, a follow-up PR is probably the more elegant way to go. I’m also happy to incorporate the other unaddressed style change comments on #17631 into this new PR.
I appreciate everyone’s efforts in both doing the merging and keeping the process honest. I agree that requiring an author’s explicit acknowledgement (or dismissal) of all (non-spam) comments would, in my view, be preferable.