validation: UpdateTip/CheckBlockIndex assumeutxo support #21526

jamesob · 2021-03-24T19:39:48Z

This is part of the assumeutxo project (parent PR: #15606)

Modify UpdateTip and CheckBlockIndex for use with multiple chainstates. Includes a new unittest verifying g_best_block behavior (previously untested at the unit level) and various changes necessary for running and testing ProcessNewBlock()-like behavior on the background validation chainstate.

This changeset introduces a new block index nStatus flag called BLOCK_ASSUMED_VALID, and it is applied to block index entries that are beneath the UTXO snapshot base block upon snapshot load. Once each block is validated (during async background validation), the flag is removed. This allows us to avoid (ab)using BLOCK_VALID_* flags for snapshot chain block entries, and preserves the original meaning of those flags.

Note: this PR previously incorporated changes to LoadBlockIndex() and RewindBlockIndex() as noted in Russ' comments below, but once I generated the changes necessary to test the UpdateTip change, I decided to split this changes out into another PR due to the size of this one.

DrahtBot · 2021-03-24T21:36:03Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22937 (refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method by ryanofsky)
#22932 (consensus: require CBlockIndex::GetBlockPos() to hold mutex cs_main by jonatack)
#22564 ([WIP] refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl)
#21603 (log: Mitigate disk filling attacks by rate limiting LogPrintf by dergoegge)
#19790 (Flag when blocks have had their scripts checked instead of skipped by luke-jr)

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.

ryanofsky · 2021-03-29T01:41:55Z

re: #21526 (comment)

Don't run UpdateTip for background validation chainstates.

Does this prevent bugs in the future when there can be a background validation chainstate? It would help to say what would happen without this change. It could also be helpful to say more about the motivation for this set of changes, and maybe summarize the current state of things for forgetful people like me who somewhat lost the thread on assumeutxo 🐠

Modify LoadBlockIndex() to populate the "faked" nChainTx value when using a yet-unvalidated snapshot chainstate. This allows assumeutxo sync-to-tip to offer normal progress estimates on the basis of having nChainTx without the associated block data.

IIUC, the faking isn't new, and the value was faked before this change, but startup code is now fixed so things work if the node is restarted before the assumeutxo snapshot is validated? Again could help to say what bad things would happen without this.

In LoadBlockIndex(), ensure that assumed-valid CBlockIndex entries aren't added to the background validation chainstate's setBlockIndexCandidates (since we want that chainstate to actually validate those entries at some point).

Wondering also what bad things happen without this change, and why it's in the same commit as the last change. Is it just that both changes affect the LoadBlockIndex() function?

Prevent RewindBlockIndex() from rewinding past the assumeutxo base on the snapshot chainstate.

I'm a little confused by this code (also the existing pruning code here). It says the purpose of the break is to prevent the DisconnectTip call immediately below from failing. But don't we want to fail in these cases? It seems like now if rewind isn't possible, this just flushes some things and returns true. I'm also wondering if there's a reason this change is in the same commit as the other changes, which seem tangentially related.

Also, there's no test coverage here. Maybe is the best we can do when adding dead code to older functions that weren't really designed to be unit-tested. But are there at least future tests in #15606 that would fail without these different fixes?

Thanks for any clarifications, and great to see assumeutxo work moving along again!

jamesob · 2021-03-30T15:22:47Z

@ryanofsky thanks for the thorough look! I agree that I should take a look at writing unittests for these changes and do so if practical. Also appreciate your questions on the logic - I'll get together some responses for those and either adjust the code (especially since RewindBlockIndex() is probably going away in #21009) or add some motivating documentation.

jamesob · 2021-04-08T14:58:07Z

Okay, after taking @ryanofsky's advice, I sat down to write some unittests for the chainstate-dependent UpdateTip() behavior. It turns out that in the process, trying to get those tests to work uncovered a number of changes that needed to be made to other test-related stuff, including slightly modifying CheckBlockIndex() to account for the specific nature of the background validation chainstate. Additionally I've added some unittesting utils that help with testing multiple chainstates.

Because that's dragged in so much new code, I've narrowed the scope of this PR to the above and I'll deal with the other BlockIndex-related changes in a follow-up PR.

So all the logic changes in this are now tested (and this actually adds unit-level coverage of the UpdateTip() behvaior that we didn't have previously). So hopefully that entices people to review, because testing future assumeutxo-related changes will probably depend on some of the unittest utils added here.

Also worth mentioning is that I had to remove some of the active chainstate assertions that @dongcarl added for the sake of his work in #20158. This should all be okay because those assertions are actually made obsolete by assumeutxo functionality.

ryanofsky

Started review (will update list below with progress). Thanks for splitting this up!

b9f805b validation: change UpdateTip for multiple chainstates (1/6)
d3cec2a validation: fix CheckBlockIndex for multiple chainstates (2/6)
0fd884d move-only: unittest: add test/util/chainstate.h (3/6)
6e5871c test: refactor: separate CreateBlock in TestChain100Setup (4/6)
d7fc5c3 refactor: remove assertions preventing multiple chainstates (5/6)
3f1346a test: validation: add unittest for UpdateTip behavior (6/6)

ryanofsky

Code review ACK 3f1346a. This is great! Changes are small and easy to understand and new test is surprisingly straightforward, not requiring too much setup.

src/validation.cpp

src/test/util/setup_common.h

ryanofsky · 2021-04-16T19:21:47Z

src/test/validation_chainstate_tests.cpp

+            pblock, state, chainparams, &pindex, true, nullptr, &newblock);
+        BOOST_CHECK(accepted);
+    }
+    bool block_added = background_cs.ActivateBestChain(state, chainparams, pblock);


In commit "test: validation: add unittest for UpdateTip behavior" (3f1346a)

Since this test is designed to check UpdateTip background behavior, but the test doesn't call UpdateTip directly, it may be good to add a comment saying this line is what calls UpdateTip.

Also could maybe a simple check could be added here to verify the new background tip hash?

Also curious if there's an existing test (or a planned one) to cover the background chain catching up to the activated snapshot.

Done.

Also curious if there's an existing test (or a planned one) to cover the background chain catching up to the activated snapshot.

Yes; I'll look at writing a test for this sometime today in addition to some chainstate-concurrency torture tests.

jamesob · 2021-04-23T18:38:14Z

Thanks for the great feedback, @ryanofsky!

DrahtBot · 2021-05-03T09:32:53Z

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

ryanofsky

Code review ACK eb9623d. Since last review just rebase and various suggestions applied, biggest one updating CheckBlockIndex in a different way where I think it does some more checks than the previous change and some fewer, but is now a simpler change. Overall this looks good and please ignore my new suggestion unless something needs to be updated for another reason.

Recommend this PR for any reviewer looking for a simple review of some critical code. The first commit's the only code commit that's even a little complicated and the other commits are either test updates or trivial code changes.

ryanofsky · 2021-05-04T00:16:15Z

src/test/util/setup_common.cpp

+CBlock TestChain100Setup::CreateBlock(
+    const std::vector<CMutableTransaction>& txns,
+    const CScript& scriptPubKey,
+    CChainState* chainstate)


In commit "test: refactor: separate CreateBlock in TestChain100Setup" (6d5d096)

Would be nice to make the last argument a CChainState& reference instead of a pointer since it isn't allowed to be null (and it's a new method so there's no backward compatility concern)

In commit "test: refactor: separate CreateBlock in TestChain100Setup" (e061837)

Would be nice to make the last argument a CChainState& reference instead of a pointer since it isn't allowed to be null (and it's a new method so there's no backward compatility concern)

(This is still applicable, in case the PR gets another update)

Sjors

Mostly happy with eb9623d, but I'm a bit confused about some code paths that I thought shouldn't be possible on the background chainstate.

I'd like to do some testing with the parent PR, if you don't mind rebasing that.

Sjors · 2021-05-05T17:36:33Z

src/validation.cpp

@@ -2321,8 +2355,7 @@ static void UpdateTip(CTxMemPool& mempool, const CBlockIndex* pindexNew, const C
    }

    bilingual_str warning_messages;
-    assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));


23c2d26: given the above return, you don't have to drop this assert

Sjors · 2021-05-05T17:46:46Z

src/validation.cpp

    EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
 {
+    auto& func_name = __func__;
+    auto& coins_view = chainstate.CoinsTip();
+    auto log_progress = [pindexNew, &func_name, &coins_view, &chainParams](


validation.cpp:2321:38: warning: lambda capture 'func_name' is not required to be captured for this use [-Wunused-lambda-capture] auto log_progress = [pindexNew, &func_name, &coins_view, &chainParams]( ~~~^~~~~~~~~ 1 warning generated.

Fails CI when addressing this warning.

Sjors · 2021-05-05T18:11:11Z

src/miner.cpp

@@ -116,7 +116,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
    pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end

    LOCK2(cs_main, m_mempool.cs);
-    assert(std::addressof(*::ChainActive().Tip()) == std::addressof(*m_chainstate.m_chain.Tip()));


23b5ebd why would CreateNewBlock ever be called on the background chainstate?

Gets called during unittests.

Because (unlike in actual snapshot usage) we don't have pre-prepared snapshots to use from within unittests, we have to use the same chainstate objects that we ultimately test on to generate snapshots for use during the test. As a result, the background validation chainstate is already at the snapshot base block, so in order to test appending to the bg chainstate, we have to call CreateNewBlock with it.

While I agree this usage isn't realistic, we don't have great alternatives within tests. I guess another option would involve using a temporary chainstate to generate the snapshot and then loading it into a new chainstatemanager object, but that sounds even hairier than what we're doing here, and much more code.

Sjors · 2021-05-05T18:14:08Z

src/txmempool.cpp

@@ -635,7 +635,6 @@ void CTxMemPool::check(CChainState& active_chainstate) const
    uint64_t innerUsage = 0;

    CCoinsViewCache& active_coins_tip = active_chainstate.CoinsTip();
-    assert(std::addressof(::ChainstateActive().CoinsTip()) == std::addressof(active_coins_tip)); // TODO: REVIEW-ONLY, REMOVE IN FUTURE COMMIT


23b5ebd: I thought the background chain didn't have a mempool? Or it had a dummy mempool. I guess it's fine to remove this assert though.

Same as above; called during unittests.

achow101 · 2021-09-15T22:44:03Z

ACK 673a5bd

jamesob · 2021-09-16T16:46:57Z

Okay, thanks everyone for bearing with the build issues. I think I've addressed all feedback here; in the latest change I've

made the auto -> const auto change (thanks again @ryanofsky), and
moved the g_best_block documentation change into its own commit (thanks @JeremyRubin).

I think this is in good shape pending some reACKs.

jonatack · 2021-09-16T17:02:46Z

@jamesob in case it's accidental, just noticed one commit (b217020) has a different email for you than the others.

Code-review re-ACK 673a5bd reviewed diff, rebased to master/debug build/ran unit+functional tests

ryanofsky

Code review ACK 673a5bd. Just linker fix and split commit changes mentioned #21526 (comment) since last review

naumenkogs · 2021-09-17T08:48:36Z

ACK 673a5bd

benthecarman

ACK 673a5bd

fjahr · 2021-09-19T20:10:00Z

Code review ACK 673a5bd

ariard · 2021-09-19T23:56:52Z

utACK 673a5bd

ryanofsky · 2021-09-22T14:11:11Z

Anything holding this up for merge at this point?

… support

To avoid linker error on some platforms: bitcoin#21526 (comment) Co-authored-by: Russ Yanofsky <russ@yanofsky.org>

maflcko · 2021-09-24T12:11:45Z

src/chain.h

+     * If set, this indicates that the block index entry is assumed-valid.
+     * Certain diagnostics will be skipped in e.g. CheckBlockIndex().
+     * It almost certainly means that the block's full validation is pending
+     * on a background chainstate. See `doc/assumeutxo.md`.


There is no such document. Also, it would be good to explain how the unlikely case where this is not pending on background validation might happen. Even more so when this comment contradicts the very next comment (IsAssumedValid).

To avoid linker error on some platforms: bitcoin#21526 (comment) Co-authored-by: Russ Yanofsky <russ@yanofsky.org>

9ab4401 doc: add assumeutxo notes (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606) --- Adds some notes on assumeutxo design. Related: bitcoin/bitcoin#21526 (comment) ACKs for top commit: ariard: ACK 9ab4401 naumenkogs: ACK 9ab4401 michaelfolkson: ACK 9ab4401 fjahr: ACK 9ab4401 Tree-SHA512: 2fca8373b78701754957d12bc43ce18aa6928507965448741cb4e8c56589ad61d261f8542e348094fc9631d46ee6a7afee75c965c0db993fc816758569137b74

To avoid linker error on some platforms: bitcoin/bitcoin#21526 (comment) Co-authored-by: Russ Yanofsky <russ@yanofsky.org>

Summary: After feedback from Russ, I realized that there are some extraneous assumeutxo methods that are not necessary and probably just overly confusing. These include - `Validated*()` - `IsBackgroundIBD()` and they can be removed. Also add comment for g_best_block This is a backport of [[bitcoin/bitcoin#21526 | core#21526]] [2 & 3/12] bitcoin/bitcoin@ac4051d bitcoin/bitcoin@665072a Backported out of order (unrelated refactorings first) Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12267

Summary: > move-only: unittest: add test/util/chainstate.h > > and move `CreateAndActivateUTXOSnapshot()` into it for reuse > in future test modules. > test: refactor: declare NoMalleation const auto > > To avoid linker error on some platforms: > bitcoin/bitcoin#21526 (comment) > > Co-authored-by: Russ Yanofsky <russ@yanofsky.org> This is a backport of [[bitcoin/bitcoin#21526 | core#21526]] [9 & 10/12] bitcoin/bitcoin@0712009 bitcoin/bitcoin@298bf5d Backported out of order (refactoring commits first) Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12268

Summary: This is so we can create blocks within unittests and have them be processed by specific chainstates (instead of the just the active one). This is a backport of [[bitcoin/bitcoin#21526 | core#21526]] [11/12] bitcoin/bitcoin@2705570 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12269

Summary: Add an upwards reference to chainstate instances to the owning ChainstateManager. This is necessary because there are a number of `this_chainstate == chainman.ActiveChainstate()` checks that will happen (as a result of assumeutxo) in functions that otherwise don't have an easily-accessible reference to the chainstate's ChainManager. This is a backport of [[bitcoin/bitcoin#21526 | core#21526]] [1/12] bitcoin/bitcoin@9f6bb53 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12277

Summary: Only perform certain behavior (namely that related to servicing the getblocktemplate RPC call) for the active chainstate when calling UpdateTip. Co-authored-by: Jon Atack <jon@atack.com> This is a backport of [[bitcoin/bitcoin#21526 | core#21526]] [3/12] bitcoin/bitcoin@b217020 Depends on D12277 Notes: - the main difference from the source material is that we do not have the code handling BIP9 deployments in `UpdateTip`. As a result, there is no need for the `warning_messages` arguments in `UpdateTipLog` - the `const` qualifier for the `CblockIndex *pindexNew` argument in `UpdateTip` was missed in D1972 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12278

fanquake added the Validation label Mar 24, 2021

DrahtBot mentioned this pull request Mar 25, 2021

Remove RewindBlockIndex logic #21009

Merged

ryanofsky mentioned this pull request Apr 1, 2021

assumeutxo #15606

Closed

18 tasks

jamesob force-pushed the 2021-03-multi-chainstates branch 2 times, most recently from d7bb92c to c1b1d66 Compare April 8, 2021 14:38

jamesob changed the title ~~validation: misc. multi-chainstate preparation~~ validation: UpdateTip/CheckBlockIndex assumeutxo support Apr 8, 2021

jamesob force-pushed the 2021-03-multi-chainstates branch from c1b1d66 to e07edda Compare April 8, 2021 14:52

This was referenced Apr 8, 2021

log: Mitigate disk filling attacks by rate limiting LogPrintf #21603

Closed

[p2p] Introduce node rebroadcast module #21061

Closed

jamesob force-pushed the 2021-03-multi-chainstates branch 2 times, most recently from 14c0232 to 3f1346a Compare April 12, 2021 19:58

DrahtBot mentioned this pull request Apr 13, 2021

[Bundle 5/n] Prune g_chainman usage in RPC modules #21391

Merged

2 tasks

ryanofsky reviewed Apr 14, 2021

View reviewed changes

ryanofsky approved these changes Apr 16, 2021

View reviewed changes

DrahtBot mentioned this pull request Apr 16, 2021

log: Mitigate disk filling attacks by globally rate limiting LogPrintf(…) #21706

Closed

DrahtBot added the Needs rebase label Apr 17, 2021

jamesob force-pushed the 2021-03-multi-chainstates branch 2 times, most recently from 2eb6a58 to eb9623d Compare April 23, 2021 18:36

DrahtBot removed the Needs rebase label Apr 23, 2021

DrahtBot mentioned this pull request Apr 24, 2021

[Bundle 6/n] Prune g_chainman usage in auxiliary modules #21767

Merged

ryanofsky approved these changes May 4, 2021

View reviewed changes

Sjors reviewed May 5, 2021

View reviewed changes

This was referenced May 6, 2021

[Bundle 7/7] validation: Farewell, global Chainstate! #21866

Merged

rpc: allow dumptxoutset to dump human-readable data #18689

Closed

jamesob force-pushed the 2021-03-multi-chainstates branch from a07e850 to 673a5bd Compare September 15, 2021 19:47

ryanofsky approved these changes Sep 16, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 18, 2021

Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main #22932

Merged

benthecarman approved these changes Sep 18, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 20, 2021

refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method #22937

Merged

laanwj merged commit b7e3600 into bitcoin:master Sep 23, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 24, 2021

Merge bitcoin#21526: validation: UpdateTip/CheckBlockIndex assumeutxo…

41eedca

… support

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 24, 2021

test: refactor: declare NoMalleation const auto

f3f027f

To avoid linker error on some platforms: bitcoin#21526 (comment) Co-authored-by: Russ Yanofsky <russ@yanofsky.org>

maflcko reviewed Sep 24, 2021

View reviewed changes

jamesob mentioned this pull request Oct 1, 2021

doc: add assumeutxo notes #23154

Merged

kwvg mentioned this pull request Oct 9, 2021

merge bitcoin#15948, #15976, #16194...: assumeutxo project backports (part 1) dashpay/dash#4498

Merged

rebroad pushed a commit to rebroad/bitcoin that referenced this pull request Oct 13, 2021

test: refactor: declare NoMalleation const auto

2f9c0f2

To avoid linker error on some platforms: bitcoin#21526 (comment) Co-authored-by: Russ Yanofsky <russ@yanofsky.org>

ryanofsky mentioned this pull request Oct 27, 2021

CI failure: ThreadSanitizer: data race /usr/lib/llvm-12/bin/../include/c++/v1/ios:523:12 in std::__1::ios_base::width() const #23366

Open

jamesob mentioned this pull request Oct 28, 2021

validation: have LoadBlockIndex account for snapshot use #23174

Merged

janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Nov 11, 2021

test: refactor: declare NoMalleation const auto

4c0f1d6

To avoid linker error on some platforms: bitcoin/bitcoin#21526 (comment) Co-authored-by: Russ Yanofsky <russ@yanofsky.org>

kwvg mentioned this pull request Feb 25, 2022

merge bitcoin#16240...21526: assumeutxo project backports (part 2) dashpay/dash#4703

Merged

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validation: UpdateTip/CheckBlockIndex assumeutxo support #21526

validation: UpdateTip/CheckBlockIndex assumeutxo support #21526

jamesob commented Mar 24, 2021 •

edited

Loading

DrahtBot commented Mar 24, 2021 •

edited

Loading

ryanofsky commented Mar 29, 2021 •

edited

Loading

jamesob commented Mar 30, 2021 •

edited

Loading

jamesob commented Apr 8, 2021

ryanofsky left a comment •

edited

Loading

ryanofsky left a comment

ryanofsky Apr 16, 2021

jamesob Apr 23, 2021

jamesob commented Apr 23, 2021

DrahtBot commented May 3, 2021

ryanofsky left a comment

ryanofsky May 4, 2021

ryanofsky Jun 21, 2021

Sjors left a comment

Sjors May 5, 2021

Sjors May 5, 2021

jamesob May 6, 2021

Sjors May 5, 2021

jamesob May 6, 2021

Sjors May 6, 2021

jamesob May 12, 2021

Sjors May 5, 2021

jamesob May 6, 2021

achow101 commented Sep 15, 2021

jamesob commented Sep 16, 2021

jonatack commented Sep 16, 2021 •

edited

Loading

ryanofsky left a comment

naumenkogs commented Sep 17, 2021

benthecarman left a comment

fjahr commented Sep 19, 2021

ariard commented Sep 19, 2021

ryanofsky commented Sep 22, 2021

maflcko Sep 24, 2021

validation: UpdateTip/CheckBlockIndex assumeutxo support #21526

validation: UpdateTip/CheckBlockIndex assumeutxo support #21526

Conversation

jamesob commented Mar 24, 2021 • edited Loading

DrahtBot commented Mar 24, 2021 • edited Loading

Conflicts

ryanofsky commented Mar 29, 2021 • edited Loading

jamesob commented Mar 30, 2021 • edited Loading

jamesob commented Apr 8, 2021

ryanofsky left a comment • edited Loading

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesob commented Apr 23, 2021

DrahtBot commented May 3, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Sep 15, 2021

jamesob commented Sep 16, 2021

jonatack commented Sep 16, 2021 • edited Loading

ryanofsky left a comment

Choose a reason for hiding this comment

naumenkogs commented Sep 17, 2021

benthecarman left a comment

Choose a reason for hiding this comment

fjahr commented Sep 19, 2021

ariard commented Sep 19, 2021

ryanofsky commented Sep 22, 2021

Choose a reason for hiding this comment

jamesob commented Mar 24, 2021 •

edited

Loading

DrahtBot commented Mar 24, 2021 •

edited

Loading

ryanofsky commented Mar 29, 2021 •

edited

Loading

jamesob commented Mar 30, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading

jonatack commented Sep 16, 2021 •

edited

Loading