Disable util::Result copying and assignment #29906

ryanofsky · 2024-04-18T19:09:54Z

This PR just contains the first two commits of #25665.

It disables copying of util::Result objects because unnecessary copies are inefficient and not possible after #25665, which makes util::Result object move-only.

It disables the assignment operator and replaces it with an Update() method, because #25665 adds more information to util::Result objects (warning and error messages and failure values) and having an assignment operator that overwrites data instead of merging it would make it easy to accidentally erase existing information while trying to assign new information.

DrahtBot · 2024-04-18T19:09:58Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, furszy, achow101
Stale ACK	TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29936 (fuzz: wallet: add target for CreateTransaction by brunoerg)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#29442 (wallet: Target a pre-defined utxo set composition by adjusting change outputs by remyers)
#28984 (Cluster size 2 package rbf by instagibbs)
#28676 ([WIP] Cluster mempool implementation by sdaftuar)
#26022 (Add util::ResultPtr class by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

stickies-v

Concept ACK, breaking it out in a smaller PR seems like a good idea, thanks.

I'm unsure about removing the assignment operator altogether. I think it's good to be careful about footguns, but I'm worried about making the interface overly unintuitive.

Would it make sense to only merge another Result when using the Merge (currently called Update) method, and use the assignment operator to overwrite the current result? I think when people want to merge results, they'll probably want to be explicit about that, and in other cases overwriting/clearing everything seems like expected behaviour?

src/wallet/test/fuzz/coinselection.cpp

src/wallet/spend.cpp

stickies-v · 2024-04-19T16:43:58Z

src/util/result.h

@@ -75,6 +85,9 @@ class Result
    const T& operator*() const LIFETIMEBOUND { return value(); }
    T* operator->() LIFETIMEBOUND { return &value(); }
    T& operator*() LIFETIMEBOUND { return value(); }
+
+    //! Update this result by moving from another result object.
+    Result& Update(Result&& other) LIFETIMEBOUND { return *this = std::move(other); }


Update is a bit of an ambiguous name imo, it doesn't clearly indicate if it means replacing or merging into the existing result. I think for this commit, where we can only overwrite the existing result, I think Replace would be better. For the next commits in #25665 where we actually add the merging functionality, I think Merge or Combine would be a better choice.

re: #29906 (comment)

Update is a bit of an ambiguous name imo, it doesn't clearly indicate if it means replacing or merging into the existing result. I think for this commit, where we can only overwrite the existing result, I think Replace would be better. For the next commits in #25665 where we actually add the merging functionality, I think Merge or Combine would be a better choice.

We could definitely have a Replace method in the future. The only reason I didn't add one now is that so far after updating a lot of code to use the Result class in PR's #29700 and #25722, I haven't seen uses for a Replace method. If you think having a Replace() method would good to add here for a future use-case, or for the sake of completeness, I'd be happy to add it. However, I don't think any changed lines here should be calling Replace() because they are just trying to update result values, not reset and replace the entire result object.

I did originally consider calling this method Merge instead of Update but I liked Update better for 2 reasons. First, I liked consistency with python's dict.update method which takes fields from two different dict objects and combines them, just like this Update function does. Second, I think the name Merge would be misleading in the majority of cases where individual values are not merged, only different fields are combined, and new field values overwrite old values. ¹

Your suggestion Combine could also work instead of Update but I think is less clear syntactically. For example, I think a.Update(b) maps pretty well to "update object a with contents of object b", but it's less obvious what a.Combine(b) would be supposed to do.

Footnotes

It is true that with #25722, you can go out of your way to pass a custom type to util::Result and specialize ResultTraits so field values are merged when Update() is called. I haven't seen cases where this is useful for success types, only for some failure types where multiple things can fail and you want to return information about all the failures. I think name Update is generic enough to cover atypical cases where values are merged without implying that values are merged typically. ↩

I haven't seen uses for a Replace method.

I agree. The use cases I was thinking of should just be addressed with list initialization, so I think not implementing the method until we actually need it is the way to go, thanks.

Second, I think the name Merge would be misleading ... only different fields are combined

That's a good point, I agree Update > Merge

it's less obvious what a.Combine(b) would be supposed to do

I don't think I agree, but I do agree there's no clear winner, so I'm happy to leave this as is.

ryanofsky

Updated 974b16b -> c45cb13 (pr/saferesult.1 -> pr/saferesult.2, compare) with suggestions

re: #29906 (review)

I'm unsure about removing the assignment operator altogether. I think it's good to be careful about footguns, but I'm worried about making the interface overly unintuitive.

We could add back a safer, more limited form of operator= in the future if not having it causes friction. I rewrote commit message to be clearer and mention that. But I think having a named Update() method should make it easier to figure out what code is doing and avoid mistakes.

Would it make sense to only merge another Result when using the Merge (currently called Update) method, and use the assignment operator to overwrite the current result? I think when people want to merge results, they'll probably want to be explicit about that, and in other cases overwriting/clearing everything seems like expected behaviour?

Hmm, I'm actually not sure what this is suggesting. If there is a need to overwrite/clear results, IMO, the best way to do that would be to add a Reset() method. I haven't seen an actual need to do that anywhere though. The intended use for Update is to be able to write code like:

util::Result<SuccessType, FailureType> result;
if (do_something()) {
  result.Update(success_value);
} else {
  result.Update({util::Error{_("error message"), failure_value});
}

ryanofsky · 2024-04-19T19:11:30Z

src/util/result.h

@@ -75,6 +85,9 @@ class Result
    const T& operator*() const LIFETIMEBOUND { return value(); }
    T* operator->() LIFETIMEBOUND { return &value(); }
    T& operator*() LIFETIMEBOUND { return value(); }
+
+    //! Update this result by moving from another result object.
+    Result& Update(Result&& other) LIFETIMEBOUND { return *this = std::move(other); }


re: #29906 (comment)

Update is a bit of an ambiguous name imo, it doesn't clearly indicate if it means replacing or merging into the existing result. I think for this commit, where we can only overwrite the existing result, I think Replace would be better. For the next commits in #25665 where we actually add the merging functionality, I think Merge or Combine would be a better choice.

We could definitely have a Replace method in the future. The only reason I didn't add one now is that so far after updating a lot of code to use the Result class in PR's #29700 and #25722, I haven't seen uses for a Replace method. If you think having a Replace() method would good to add here for a future use-case, or for the sake of completeness, I'd be happy to add it. However, I don't think any changed lines here should be calling Replace() because they are just trying to update result values, not reset and replace the entire result object.

I did originally consider calling this method Merge instead of Update but I liked Update better for 2 reasons. First, I liked consistency with python's dict.update method which takes fields from two different dict objects and combines them, just like this Update function does. Second, I think the name Merge would be misleading in the majority of cases where individual values are not merged, only different fields are combined, and new field values overwrite old values. ¹

Your suggestion Combine could also work instead of Update but I think is less clear syntactically. For example, I think a.Update(b) maps pretty well to "update object a with contents of object b", but it's less obvious what a.Combine(b) would be supposed to do.

Footnotes

It is true that with #25722, you can go out of your way to pass a custom type to util::Result and specialize ResultTraits so field values are merged when Update() is called. I haven't seen cases where this is useful for success types, only for some failure types where multiple things can fail and you want to return information about all the failures. I think name Update is generic enough to cover atypical cases where values are merged without implying that values are merged typically. ↩

src/wallet/spend.cpp

src/wallet/test/fuzz/coinselection.cpp

ryanofsky

Updated c45cb13 -> 6602231 (pr/saferesult.2 -> pr/saferesult.3, compare) adding change to actually disable the copy constructor

src/wallet/test/fuzz/coinselection.cpp

TheCharlatan

ACK 6602231

furszy

ACK 6602231

src/util/result.h

furszy · 2024-04-25T13:44:36Z

src/qt/addresstablemodel.cpp

+            op_dest.Update(walletModel->wallet().getNewDestination(address_type, strLabel));
            if (!op_dest) {
                editStatus = KEY_GENERATION_FAILURE;
                return QString();


Not for this PR but.. we could remove this Update call pretty easily (e8a3305). We are calling getNewDestination twice only to verify if the wallet is locked or not.

re: #29906 (comment)

It seems like the goal of the code is to get a new address without unlocking. But I haven't looked into this. It definitely looks a little clumsy, though, because it isn't actually using the error message that is returned.

`util::Result` objects are aggregates that can hold multiple fields with different information. Currently Result objects can only hold a success value of an arbitrary type or a single bilingual_str error message. In followup PR bitcoin#25722, Result objects may be able to hold both success and failure values of different types, plus error and warning messages. Having a Result::operator= assignment operator that completely erases all existing Result information before assigning new information is potentially dangerous in this case. For example, code that looks like it is assigning a warning value could erase previously-assigned success or failure values. Conversely, code that looks like it is just assigning a success or failure value could erase previously assigned error and warning messages. To prevent potential bugs like this, disable Result::operator= assignment operator. It is possible in the future we may want to re-enable operator= in limited cases (such as when implicit conversions are not used) or add a Replace() or Reset() method that mimicks default operator= behavior. Followup PR bitcoin#25722 also adds a Result::Update() method providing another way to update an existing Result object. Co-authored-by: stickies-v <stickies-v@protonmail.com>

Copying util::Result values is less efficient than moving them because they allocate memory and contain strings. Also this is needed to avoid compile errors in bitcoin#25722 which adds a std::unique_ptr member to util::Result which implicity disables copying.

stickies-v

Approach ACK, code LGTM 6602231 with some non-blocking suggestions.

We could add back a safer, more limited form of operator= in the future if not having it causes friction.

I made my previous comment because I thought there are some instances of somewhat awkward Update usage in this PR. The majority of util::Result usage is/should probably not require Update() at all, because we're just returning either a success value or an error string. But, in those simple cases, we should just use list initialization, imo, and not assignment initialization. So, I've left a couple of code comments that remove all the instances I find awkward.

The intended use for Update is to be able to write code like: ...

Hmmm. In the code example you give, I'd find assignment much more intuitive, since the goal doesn't seem to be combining results. If Update() is aimed to be used exclusively (?) when we intend to combine results, doesn't that make for a more clear interface?

src/init.cpp

src/test/mempool_tests.cpp

src/wallet/test/fuzz/notifications.cpp

src/wallet/test/spend_tests.cpp

stickies-v · 2024-04-25T19:55:03Z

src/util/result.h

@@ -75,6 +85,9 @@ class Result
    const T& operator*() const LIFETIMEBOUND { return value(); }
    T* operator->() LIFETIMEBOUND { return &value(); }
    T& operator*() LIFETIMEBOUND { return value(); }
+
+    //! Update this result by moving from another result object.
+    Result& Update(Result&& other) LIFETIMEBOUND { return *this = std::move(other); }


I haven't seen uses for a Replace method.

I agree. The use cases I was thinking of should just be addressed with list initialization, so I think not implementing the method until we actually need it is the way to go, thanks.

Second, I think the name Merge would be misleading ... only different fields are combined

That's a good point, I agree Update > Merge

it's less obvious what a.Combine(b) would be supposed to do

I don't think I agree, but I do agree there's no clear winner, so I'm happy to leave this as is.

ryanofsky · 2024-04-25T20:49:57Z

Thanks @stickies-v, I agree with your suggestions to avoid updating existing Result objects where possible and will apply them.

But I wonder if there are is anything else I can do to help make the interface more intuitive though. I'm not exactly clear on why it is not intuitive currently. A few places you mentioned "Update here is confusing" but I don't understand why it is confusing. Saying result.Update(success_value) is just meant to update the result object with a success value while keeping other result data intact. Saying result.Update({util::Error{_("error message"), failure_value}); is meant to update the result object with an error message and failure value, again keeping other result data intact.

I think it's very similar to python dict.update function where if you start with an existing dictionary:

d = {'a': 1, 'b': 2, 'c': 3}

and call the update method with new values:

d.update({'a': 4, 'd': 5})

And the dictionary will be updated with the new values overwriting the old ones:

{'a': 4, 'b': 2, 'c': 3, 'd': 5}

In the code example you give, I'd find assignment much more intuitive, since the goal doesn't seem to be combining results. If Update() is aimed to be used exclusively (?) when we intend to combine results, doesn't that make for a more clear interface?

Update is just intended to update some data in a result object without necessarily erasing other data in the result object. Assigning is conceptually different and would imply that result being assigned to would be cleared and completely replaced by value being assigned. In this PR, the result object only has two mutually exclusive fields (success value and error string), so assigning and updating are the same thing. In #25665, result objects contain more fields so assigning and updating are different things.

ryanofsky

Updated 6602231 -> 4fb08c2 (pr/saferesult.3 -> pr/saferesult.4, compare) with suggested changes using new result objects to avoid updating existing ones where possible
Update 4fb08c2 -> 88aa093 (pr/saferesult.4 -> pr/saferesult.5, compare) with no changes overall, just moving changes out of first commit that belonged in second commit

src/init.cpp

src/test/mempool_tests.cpp

src/wallet/test/fuzz/notifications.cpp

src/wallet/test/spend_tests.cpp

stickies-v · 2024-04-25T21:34:51Z

Sorry, I've been doing a poor job explaining myself. I am fully onboard with how Update works. When I used the word "confusion", I was talking about usage, not interface*. Prior to this push, Update() was used in a couple of places where we know or expect the Result to be empty, and in those case I think we should just be explicit about that and use list initialization. When I see result.Update() being called, I have to look up which values are in result already and ensure that's correct, whereas with auto result{...}; I know right away that we're starting from an empty result.

To stick with the python analogy: using update when we know or expect a to be an empty dictionary, is imo an anti-pattern and should be avoided.

a = {}
// do some logic to calculate `value`
a.update({1: value})

Instead, it is much clearer to use assignment, so the reader doesn't have to figure out which values a held already.

// do some logic to calculate `value`
a = {1: value}

In short: I think it would be helpful if we only use result.Update() when result (potentially) has had fields set already, and use list initialization (or if necessary assignment initialization) for when we know (and thus enforce that) result is empty. This is also the concern I had with the code example at the end of this comment.

I hope I did a better job explaining myself, but otherwise I'm happy to talk about it more offline to not pollute this PR more.

well, except for making sure the method names align with how we intend people to use them, hence my earlier renaming suggestions.

ryanofsky · 2024-04-26T01:11:42Z

Rebased 88aa093 -> 0cbcb66 (pr/saferesult.5 -> pr/saferesult.6, compare) dropping Result::Update() method in this PR, and last 2 uses of it.
Update 0cbcb66 -> e1be443 (pr/saferesult.6 -> pr/saferesult.7, compare) just changing a commit message to no longer mention Result::Update().

re: #29906 (comment)

Thanks for clarifying. I think we might disagree whether certain uses of an update function would be antipatterns, but I completely agree with all the code changes you suggested, and that in general if you have a choice between initializing values once or letting them change over time, you should prefer initializing them once so there's less changing state to reason about. I also looked at remaining two update calls in this PR, and thought they both were not good uses, so I removed them as well.

My update example was very simple because I wanted to show minimal code that demonstrated how to use the update function, but the code was probably too simple to demonstrate why you would want to use it. The use-case for update is when you have a function that is performing multiple operations and you want to accumulate warnings and errors from all the operations and return them. Examples would be the BlockManager::SaveBlockToDisk function from #29700 or the CompleteChainstateInitialization function from #25665. I think these are reasonable uses of the update function, but it's possible there could be better ways to write this code, and you might have another idea what it should look like.

stickies-v

ACK e1be443

The use-case for update is when you have a function that is performing multiple operations and you want to accumulate warnings and errors from all the operations and return them.

I agree, and from what I've seen so far, I would prefer that to be the only use-case.

I also looked at remaining two update calls in this PR, and thought they both were not good uses, so I removed them as well.

Ah, nice one. Given that chaining results is not actually implemented yet, it makes sense that there should be no existing usage of Update in these first commits. I looked at the initial implementation of these 2 new updates to make sure we're not missing anything and indeed, chaining is not relevant.

I've posted my response to the second part of your comment about Update() usage on #25665, since Update() is no longer in this PR.

src/test/mempool_tests.cpp

src/wallet/test/fuzz/notifications.cpp

src/validation.cpp

achow101 · 2024-04-26T18:37:40Z

ACK e1be443

src/util/result.h

ryanofsky

Updated e1be443 -> 9552619 (pr/saferesult.7 -> pr/saferesult.8, compare) with small suggested cleanups

src/test/mempool_tests.cpp

src/util/result.h

src/validation.cpp

src/wallet/test/fuzz/notifications.cpp

stickies-v · 2024-04-27T14:12:24Z

Re-ACK 9552619 , just addressing nits, no other changes

furszy

ACK 9552619

TheCharlatan

ACK 9552619

ryanofsky

Updated 9552619 -> 6a8b2be (pr/saferesult.8 -> pr/saferesult.9, compare) tweaking variables to avoid shadowing

src/validation.cpp

stickies-v · 2024-04-30T14:54:53Z

re-ACK 6a8b2be

furszy

re-ACK 6a8b2be

achow101 · 2024-04-30T16:04:55Z

ACK 6a8b2be

ryanofsky mentioned this pull request Apr 18, 2024

refactor: Add util::Result failure values, multiple error and warning messages #25665

Open

DrahtBot mentioned this pull request Apr 19, 2024

wallet: Target a pre-defined utxo set composition by adjusting change outputs #29442

Draft

stickies-v reviewed Apr 19, 2024

View reviewed changes

This was referenced Apr 19, 2024

Add util::ResultPtr class #26022

Draft

refactor: Use util::Result class for wallet loading #25722

Draft

laanwj added the Refactoring label Apr 21, 2024

DrahtBot mentioned this pull request Apr 24, 2024

fuzz: wallet: add target for CreateTransaction #29936

Open

ryanofsky force-pushed the pr/saferesult branch from 974b16b to c45cb13 Compare April 24, 2024 15:51

ryanofsky commented Apr 24, 2024

View reviewed changes

ryanofsky force-pushed the pr/saferesult branch from c45cb13 to 6602231 Compare April 24, 2024 19:16

ryanofsky commented Apr 24, 2024

View reviewed changes

src/wallet/test/fuzz/coinselection.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 24, 2024

kernel, refactor: return error status on all fatal errors #29700

Draft

TheCharlatan approved these changes Apr 25, 2024

View reviewed changes

DrahtBot requested a review from stickies-v April 25, 2024 12:41

furszy reviewed Apr 25, 2024

View reviewed changes

ryanofsky and others added 2 commits April 25, 2024 16:08

stickies-v reviewed Apr 25, 2024

View reviewed changes

ryanofsky force-pushed the pr/saferesult branch from 6602231 to 4fb08c2 Compare April 25, 2024 21:09

ryanofsky commented Apr 25, 2024

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/test/mempool_tests.cpp Outdated Show resolved Hide resolved

src/wallet/test/fuzz/notifications.cpp Outdated Show resolved Hide resolved

src/wallet/test/spend_tests.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/saferesult branch from 4fb08c2 to 88aa093 Compare April 25, 2024 21:26

ryanofsky force-pushed the pr/saferesult branch from 88aa093 to 0cbcb66 Compare April 26, 2024 00:52

ryanofsky force-pushed the pr/saferesult branch from 0cbcb66 to e1be443 Compare April 26, 2024 01:21

stickies-v approved these changes Apr 26, 2024

View reviewed changes

src/test/mempool_tests.cpp Show resolved Hide resolved

DrahtBot requested review from TheCharlatan and furszy April 26, 2024 13:23

maflcko reviewed Apr 26, 2024

View reviewed changes

src/wallet/test/fuzz/notifications.cpp Outdated Show resolved Hide resolved

furszy reviewed Apr 26, 2024

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from furszy April 26, 2024 13:54

This was referenced Apr 26, 2024

Cluster size 2 package rbf #28984

Open

[WIP] Cluster mempool implementation #28676

Draft

achow101 reviewed Apr 26, 2024

View reviewed changes

src/util/result.h Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/saferesult branch from e1be443 to 9552619 Compare April 26, 2024 22:38

ryanofsky commented Apr 26, 2024

View reviewed changes

src/test/mempool_tests.cpp Show resolved Hide resolved

src/util/result.h Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/wallet/test/fuzz/notifications.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from achow101 April 27, 2024 14:12

furszy reviewed Apr 29, 2024

View reviewed changes

TheCharlatan approved these changes Apr 30, 2024

View reviewed changes

ryanofsky force-pushed the pr/saferesult branch from 9552619 to 6a8b2be Compare April 30, 2024 14:23

ryanofsky commented Apr 30, 2024

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from furszy and TheCharlatan April 30, 2024 14:54

furszy reviewed Apr 30, 2024

View reviewed changes

achow101 merged commit 2d30567 into bitcoin:master Apr 30, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable util::Result copying and assignment #29906

Disable util::Result copying and assignment #29906

ryanofsky commented Apr 18, 2024

DrahtBot commented Apr 18, 2024 •

edited

stickies-v left a comment

stickies-v Apr 19, 2024

ryanofsky Apr 19, 2024

stickies-v Apr 25, 2024

ryanofsky left a comment

ryanofsky Apr 19, 2024

ryanofsky left a comment

TheCharlatan left a comment

furszy left a comment

furszy Apr 25, 2024

ryanofsky Apr 25, 2024

stickies-v left a comment

stickies-v Apr 25, 2024

ryanofsky commented Apr 25, 2024

ryanofsky left a comment •

edited

stickies-v commented Apr 25, 2024

ryanofsky commented Apr 26, 2024 •

edited

stickies-v left a comment

achow101 commented Apr 26, 2024

ryanofsky left a comment

stickies-v commented Apr 27, 2024

furszy left a comment

TheCharlatan left a comment

ryanofsky left a comment

stickies-v commented Apr 30, 2024

furszy left a comment

achow101 commented Apr 30, 2024

Disable util::Result copying and assignment #29906

Disable util::Result copying and assignment #29906

Conversation

ryanofsky commented Apr 18, 2024

DrahtBot commented Apr 18, 2024 • edited

Code Coverage

Reviews

Conflicts

stickies-v left a comment

Choose a reason for hiding this comment

stickies-v Apr 19, 2024

Choose a reason for hiding this comment

ryanofsky Apr 19, 2024

Choose a reason for hiding this comment

Footnotes

stickies-v Apr 25, 2024

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Apr 19, 2024

Choose a reason for hiding this comment

Footnotes

ryanofsky left a comment

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

furszy Apr 25, 2024

Choose a reason for hiding this comment

ryanofsky Apr 25, 2024

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

stickies-v Apr 25, 2024

Choose a reason for hiding this comment

ryanofsky commented Apr 25, 2024

ryanofsky left a comment • edited

Choose a reason for hiding this comment

stickies-v commented Apr 25, 2024

ryanofsky commented Apr 26, 2024 • edited

stickies-v left a comment

Choose a reason for hiding this comment

achow101 commented Apr 26, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

stickies-v commented Apr 27, 2024

furszy left a comment

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

stickies-v commented Apr 30, 2024

furszy left a comment

Choose a reason for hiding this comment

achow101 commented Apr 30, 2024

DrahtBot commented Apr 18, 2024 •

edited

ryanofsky left a comment •

edited

ryanofsky commented Apr 26, 2024 •

edited