Expose block filters over REST #17631

TheBlueMatt · 2019-11-29T04:45:23Z

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

promag · 2019-11-29T09:00:19Z

A test would be nice.

practicalswift · 2019-11-29T09:24:12Z

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).

laanwj · 2019-11-29T09:32:37Z

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.

practicalswift · 2019-11-29T09:37:16Z

@laanwj Without knowing if consumers are trusted or not it is pretty hard to review it from a security perspective :)

laanwj · 2019-11-29T09:42:04Z

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.

src/rest.cpp

jnewbery · 2019-11-29T19:21:29Z

Concept ACK

TheBlueMatt · 2019-11-29T19:37:25Z

Added a basic sanity test, redid the way headers work to make it easy to get many of them just like the /headers/ request.

src/rest.cpp

paymog · 2019-12-01T07:37:17Z

src/rest.cpp

+    }
+
+    BlockFilterIndex* index = GetBlockFilterIndex(filtertype);
+    if (!index) {


the code for extracting filtertype, index and blockhash is shared between these two functions. Should this extraction code be pulled out into their own functions?

Lets leave DRY'ing up rest.cpp for a separate commit. I played with it a bit and there isn't an obvious solution here that doesn't end up adding more lines, but the whole of REST probably could get DRY'd up a lot especially in the results-providing section.

paymog · 2019-12-01T07:38:27Z

src/rest.cpp

+        LOCK(cs_main);
+        block_index = LookupBlockIndex(block_hash);
+        if (!block_index) {
+            return RESTERR(req, HTTP_NOT_FOUND, uriParts[1] + " not found");


nit: can the error message be changed to "Block " + uriParts[1] + " not found"?

Hmm, its coped from the block code, so to keep it the same everywhere I'll leave it.

jonasschnelli · 2019-12-01T21:06:04Z

Concept ACK

DrahtBot · 2019-12-05T16:05:19Z

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

Conflicts

No conflicts as of last run.

src/rest.cpp

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. Github-Pull: bitcoin#17631 Rebased-From: aeca9ab

TheBlueMatt · 2020-05-01T17:40:16Z

Rebased.

jnewbery

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.

src/rest.cpp

naumenkogs · 2020-05-18T16:14:26Z

Code review ACK 16d8d2d.
Perhaps would be good to address the style suggestion above.

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. Github-Pull: bitcoin#17631 Rebased-From: aeca9ab

…rtial) Github-Pull: bitcoin#17631 Rebased-From: 84c127d (partial)

maflcko · 2021-12-20T08:00:40Z

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.

jnewbery · 2021-12-20T08:55:25Z

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 (comment) is a "nit". It's feedback on the interface design and changing it later would be an API break.

maflcko · 2021-12-20T09:03:25Z

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.

jnewbery · 2021-12-20T09:18:33Z

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.

maflcko · 2021-12-20T09:33:37Z

Ok, again apologies for missing the comment(s). I've marked this "Up for grabs", so that someone can address the feedback before the next major release (23.0). If no one does, I'll probably revert the changes.

JeremyRubin · 2021-12-20T16:49:22Z

It also feels premature given my nack + alternative path to implementing this in #23309 not having been reviewed -- were it just my NACK and no alternate, I'd agree that perhpas it was sufficiently addressed, but it was my NACK + Alternative + Acks on that approach from > 1 contributor.

maflcko · 2021-12-20T16:55:17Z

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?

maflcko · 2021-12-20T16:56:45Z

And to comment on the content on the NACK: I don't see how this and #23309 are mutually exclusive.

luke-jr · 2021-12-20T18:00:45Z

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.

JeremyRubin · 2021-12-20T18:42:42Z

@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.

sipa · 2021-12-20T18:52:50Z

@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.

2b64fa3 Update REST docs with new accessors (Matt Corallo) ef7c822 Expose block filters over REST. (Matt Corallo) Pull request description: 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 ACKs for top commit: dergoegge: ACK 2b64fa3 - Adding blockfilters to the REST interface is analogous to serving other public data such as transactions or blocks. Tree-SHA512: d487bc694266375c94d6fcf2e9d788a8a42a3b94e8d3290e46335a64cbcde55084ce5ea6119b79a4065888d94d7c3ae25a59a901fa46e3711f0eb296add12696

maflcko · 2021-12-21T09:17:31Z

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:

Concept ACK laanwj Expose block filters over REST #17631 (comment)
Concept ACK jonasschnelli Expose block filters over REST #17631 (comment)
Concept ACK sipa Expose block filters over REST #17631 (comment)
Nearly ACK jnewbery Expose block filters over REST #17631 (review)
Review ACK naumenkogs Expose block filters over REST #17631 (comment)
Review ACK meshcollider Expose block filters over REST #17631 (review)
Review ACK myself Expose block filters over REST #17631 (comment)
Review ACK dergoegge Expose block filters over REST #17631 (comment)

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.

jnewbery · 2021-12-21T10:07:50Z

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.

stickies-v · 2021-12-21T14:08:27Z

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.

maflcko · 2021-12-22T09:44:09Z

Looks like there is a follow up in #23836, so I've removed the "up for grabs" label again.

4523d28 [test] compare filter and header with the result of the getblockfilter RPC (Niklas Gögge) 3a2464f [rest] drop superfluous rpc serializations flags for block filters (Niklas Gögge) 064abd1 [rest] add a more verbose error message for invalid header counts (Niklas Gögge) 83b8f3a [refactor] various style fix-ups (Niklas Gögge) Pull request description: This PR addresses unresolved review comments from [#17631](bitcoin/bitcoin#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. ACKs for top commit: jnewbery: ACK 4523d28 brunoerg: tACK 4523d28 Tree-SHA512: 634e6b2ae3e1d5f31675a50cfe11a5e74bf5a51b9e7b512d9e18879bf2ed424fc0ac6ec633023f192e3ad12cf0c73b0b51de79dd7ec00844dba3e1493d823b8c

4523d28 [test] compare filter and header with the result of the getblockfilter RPC (Niklas Gögge) 3a2464f [rest] drop superfluous rpc serializations flags for block filters (Niklas Gögge) 064abd1 [rest] add a more verbose error message for invalid header counts (Niklas Gögge) 83b8f3a [refactor] various style fix-ups (Niklas Gögge) Pull request description: This PR addresses unresolved review comments from [bitcoin#17631](bitcoin#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. ACKs for top commit: jnewbery: ACK 4523d28 brunoerg: tACK 4523d28 Tree-SHA512: 634e6b2ae3e1d5f31675a50cfe11a5e74bf5a51b9e7b512d9e18879bf2ed424fc0ac6ec633023f192e3ad12cf0c73b0b51de79dd7ec00844dba3e1493d823b8c

stickies-v · 2022-01-18T22:22:06Z

#24098 is now ready for review to address the last unaddressed comments (by myself) on this PR.

@MarcoFalke

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](#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

TheBlueMatt force-pushed the 2019-11-filter-rest branch from a2abac0 to 661f03f Compare November 29, 2019 05:02

DrahtBot added the RPC/REST/ZMQ label Nov 29, 2019

cvengler reviewed Nov 29, 2019

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2019-11-filter-rest branch from 661f03f to 6f2e02f Compare November 29, 2019 19:37

paymog reviewed Dec 1, 2019

View reviewed changes

TheBlueMatt force-pushed the 2019-11-filter-rest branch 2 times, most recently from 8c33533 to 3ab6abc Compare December 3, 2019 22:06

DrahtBot added the Needs rebase label Dec 9, 2019

luke-jr suggested changes Jan 3, 2020

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2019-11-filter-rest branch 2 times, most recently from b555be4 to dc3a7e8 Compare May 1, 2020 17:43

DrahtBot removed the Needs rebase label May 1, 2020

TheBlueMatt force-pushed the 2019-11-filter-rest branch 3 times, most recently from 9ff0076 to 16d8d2d Compare May 2, 2020 02:15

jnewbery reviewed May 15, 2020

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

src/rest.cpp Outdated Show resolved Hide resolved

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020

Fix a few bugs in REST-headers handling that got copied to filter (pa…

61e26c9

…rtial) Github-Pull: bitcoin#17631 Rebased-From: 84c127d (partial)

maflcko merged commit 70d6a09 into bitcoin:master Dec 20, 2021

maflcko added the Up for grabs label Dec 20, 2021

maflcko added this to the 23.0 milestone Dec 20, 2021

dergoegge mentioned this pull request Dec 21, 2021

rest: Expose block filters follow-ups #23836

Merged

maflcko removed this from the 23.0 milestone Dec 22, 2021

maflcko removed the Up for grabs label Dec 22, 2021

stickies-v mentioned this pull request Jan 8, 2022

rest: Use query parameters to control resource loading #24012

Closed

1 task

theStack mentioned this pull request Jan 17, 2022

refactor: introduce single-separator split helper (boost::split replacement) #22953

Merged

stickies-v mentioned this pull request Jan 18, 2022

rest: Use query parameters to control resource loading #24098

Merged

1 task

bitcoin locked and limited conversation to collaborators Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose block filters over REST #17631

Expose block filters over REST #17631

TheBlueMatt commented Nov 29, 2019 •

edited

Loading

promag commented Nov 29, 2019

practicalswift commented Nov 29, 2019 •

edited

Loading

laanwj commented Nov 29, 2019

practicalswift commented Nov 29, 2019

laanwj commented Nov 29, 2019 •

edited

Loading

jnewbery commented Nov 29, 2019

TheBlueMatt commented Nov 29, 2019

paymog Dec 1, 2019

TheBlueMatt Dec 2, 2019

paymog Dec 1, 2019

TheBlueMatt Dec 2, 2019

jonasschnelli commented Dec 1, 2019

DrahtBot commented Dec 5, 2019 •

edited

Loading

TheBlueMatt commented May 1, 2020

jnewbery left a comment

naumenkogs commented May 18, 2020

maflcko commented Dec 20, 2021

jnewbery commented Dec 20, 2021

maflcko commented Dec 20, 2021

jnewbery commented Dec 20, 2021

maflcko commented Dec 20, 2021

JeremyRubin commented Dec 20, 2021

maflcko commented Dec 20, 2021

maflcko commented Dec 20, 2021

luke-jr commented Dec 20, 2021

JeremyRubin commented Dec 20, 2021

sipa commented Dec 20, 2021 •

edited

Loading

maflcko commented Dec 21, 2021

jnewbery commented Dec 21, 2021

stickies-v commented Dec 21, 2021

maflcko commented Dec 22, 2021

stickies-v commented Jan 18, 2022

Expose block filters over REST #17631

Expose block filters over REST #17631

Conversation

TheBlueMatt commented Nov 29, 2019 • edited Loading

promag commented Nov 29, 2019

practicalswift commented Nov 29, 2019 • edited Loading

laanwj commented Nov 29, 2019

practicalswift commented Nov 29, 2019

laanwj commented Nov 29, 2019 • edited Loading

jnewbery commented Nov 29, 2019

TheBlueMatt commented Nov 29, 2019

paymog Dec 1, 2019

Choose a reason for hiding this comment

TheBlueMatt Dec 2, 2019

Choose a reason for hiding this comment

paymog Dec 1, 2019

Choose a reason for hiding this comment

TheBlueMatt Dec 2, 2019

Choose a reason for hiding this comment

jonasschnelli commented Dec 1, 2019

DrahtBot commented Dec 5, 2019 • edited Loading

Conflicts

TheBlueMatt commented May 1, 2020

jnewbery left a comment

Choose a reason for hiding this comment

naumenkogs commented May 18, 2020

maflcko commented Dec 20, 2021

jnewbery commented Dec 20, 2021

maflcko commented Dec 20, 2021

jnewbery commented Dec 20, 2021

maflcko commented Dec 20, 2021

JeremyRubin commented Dec 20, 2021

maflcko commented Dec 20, 2021

maflcko commented Dec 20, 2021

luke-jr commented Dec 20, 2021

JeremyRubin commented Dec 20, 2021

sipa commented Dec 20, 2021 • edited Loading

maflcko commented Dec 21, 2021

jnewbery commented Dec 21, 2021

stickies-v commented Dec 21, 2021

maflcko commented Dec 22, 2021

stickies-v commented Jan 18, 2022

TheBlueMatt commented Nov 29, 2019 •

edited

Loading

practicalswift commented Nov 29, 2019 •

edited

Loading

laanwj commented Nov 29, 2019 •

edited

Loading

DrahtBot commented Dec 5, 2019 •

edited

Loading

sipa commented Dec 20, 2021 •

edited

Loading