kernel: De-globalize fReindex #29817

TheCharlatan · 2024-04-05T20:40:26Z

fReindex is one of the last remaining globals exposed by the kernel library, so move it into the blockstorage class to reduce the amount of global mutable state and make the kernel library a bit less awkward to use.

This pull request is part of the libbitcoinkernel project.

DrahtBot · 2024-04-05T20:40:29Z

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	ryanofsky, mzumsande, stickies-v, achow101

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:

#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#29678 (Bugfix: Correct first-run free space checks by luke-jr)
#28052 (blockstorage: XOR blocksdir *.dat files by maflcko)
#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.

DrahtBot · 2024-04-06T02:02:20Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/23504545411}

stickies-v

Approach ACK d9bcbbf and code LGTM.

Some thoughts I have from reviewing this PR, but that are orthogonal to it:

It seems like we have 4 places where we store information about reindexing being required or being in progress:

BlockTreeDB (through WriteReindexing() and ReadReindexing())
BlockManager::m_reindex (previously fReindex)
BlockManagerOpts::reindex
ChainstateLoadOptions::reindex

1. and 2. are updated based on reindex progress, and should stay in sync with each other most of the time. 3. and 4. are set up to one time after initialization, and then never updated again, and thus will stay in sync with each other but may fall out of sync with 1. and 2.. Perhaps this is intentional?

2. (BlockManager::m_reindex) seems to be indicate both whether a reindex is requested or in progress, whereas 1. (BlockTreeDB) is exclusively used to indicate if the reindex is in progress, and 3. and 4. (BlockManagerOpts::reindex and ChainstateLoadOptions::reindex) is exclusively used to indicate if the reindex is requested.

I wonder if this could and should be cleaned up? For example, replacing all uses of ChainstateLoadOptions::reindex with BlockManager::m_reindex is a trivial code change and passes unit and functional tests:

git diff on d9bcbbf

diff --git a/src/init.cpp b/src/init.cpp
index 0550eec970..c9183b5c7b 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1560,7 +1560,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
         node::ChainstateLoadOptions options;
         options.mempool = Assert(node.mempool.get());
-        options.reindex = chainman.m_blockman.m_reindex;
         options.reindex_chainstate = fReindexChainState;
         options.prune = chainman.m_blockman.IsPruneMode();
         options.check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
@@ -1602,7 +1601,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
         if (!fLoaded && !ShutdownRequested(node)) {
             // first suggest a reindex
-            if (!options.reindex) {
+            if (!chainman.m_blockman.m_reindex) {
                 bool fRet = uiInterface.ThreadSafeQuestion(
                     error + Untranslated(".\n\n") + _("Do you want to rebuild the block database now?"),
                     error.original + ".\nPlease restart with -reindex or -reindex-chainstate to recover.",
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 3ee712ab7e..70d4c18c1d 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -45,10 +45,10 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         .path = chainman.m_options.datadir / "blocks" / "index",
         .cache_bytes = static_cast<size_t>(cache_sizes.block_tree_db),
         .memory_only = options.block_tree_db_in_memory,
-        .wipe_data = options.reindex,
+        .wipe_data = chainman.m_blockman.m_reindex,
         .options = chainman.m_options.block_tree_db});
 
-    if (options.reindex) {
+    if (chainman.m_blockman.m_reindex) {
         pblocktree->WriteReindexing(true);
         //If we're reindexing in prune mode, wipe away unusable block files and all undo data files
         if (options.prune) {
@@ -89,7 +89,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
     }
 
     auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
-        return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
+        return chainman.m_blockman.m_reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
     };
 
     assert(chainman.m_total_coinstip_cache > 0);
@@ -110,7 +110,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         chainstate->InitCoinsDB(
             /*cache_size_bytes=*/chainman.m_total_coinsdb_cache * init_cache_fraction,
             /*in_memory=*/options.coins_db_in_memory,
-            /*should_wipe=*/options.reindex || options.reindex_chainstate);
+            /*should_wipe=*/chainman.m_blockman.m_reindex || options.reindex_chainstate);
 
         if (options.coins_error_cb) {
             chainstate->CoinsErrorCatcher().AddReadErrCallback(options.coins_error_cb);
@@ -142,7 +142,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
         }
     }
 
-    if (!options.reindex) {
+    if (!chainman.m_blockman.m_reindex) {
         auto chainstates{chainman.GetAll()};
         if (std::any_of(chainstates.begin(), chainstates.end(),
                         [](const Chainstate* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) {
@@ -188,7 +188,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
     // Load a chain created from a UTXO snapshot, if any exist.
     bool has_snapshot = chainman.DetectSnapshotChainstate();
 
-    if (has_snapshot && (options.reindex || options.reindex_chainstate)) {
+    if (has_snapshot && (chainman.m_blockman.m_reindex || options.reindex_chainstate)) {
         LogPrintf("[snapshot] deleting snapshot chainstate due to reindexing\n");
         if (!chainman.DeleteSnapshotChainstate()) {
             return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")};
@@ -247,7 +247,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
 ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options)
 {
     auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
-        return options.reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
+        return chainman.m_blockman.m_reindex || options.reindex_chainstate || chainstate->CoinsTip().GetBestBlock().IsNull();
     };
 
     LOCK(cs_main);
diff --git a/src/node/chainstate.h b/src/node/chainstate.h
index a6e9a0331b..ebcff861ce 100644
--- a/src/node/chainstate.h
+++ b/src/node/chainstate.h
@@ -22,7 +22,6 @@ struct ChainstateLoadOptions {
     CTxMemPool* mempool{nullptr};
     bool block_tree_db_in_memory{false};
     bool coins_db_in_memory{false};
-    bool reindex{false};
     bool reindex_chainstate{false};
     bool prune{false};
     //! Setting require_full_verification to true will require all checks at
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 3140f43dcf..1b960ad266 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -278,7 +278,6 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
     options.mempool = Assert(m_node.mempool.get());
     options.block_tree_db_in_memory = m_block_tree_db_in_memory;
     options.coins_db_in_memory = m_coins_db_in_memory;
-    options.reindex = chainman.m_blockman.m_reindex;
     options.reindex_chainstate = m_args.GetBoolArg("-reindex-chainstate", false);
     options.prune = chainman.m_blockman.IsPruneMode();
     options.check_blocks = m_args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);

Most of these concerns are quite orthogonal to this PR, since it's the same situation on master with fReindex, so we don't need to discuss this further here but I thought I'd raise it.

stickies-v · 2024-04-15T13:18:24Z

src/node/blockmanager_args.cpp

@@ -33,6 +33,8 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Op

    if (auto value{args.GetBoolArg("-fastprune")}) opts.fast_prune = *value;

+    opts.reindex = args.GetBoolArg("-reindex", false);


To avoid defining the default value twice (default member init in BlockManagerOpts), perhaps better to use:

Suggested change

opts.reindex = args.GetBoolArg("-reindex", false);

if (auto value{args.GetBoolArg("-reindex")}) opts.reindex = *value;

ryanofsky

Code review ACK d9bcbbf, but I think reindex /m_reindex naming is a little confusing so I left some suggestions below to clean it up.

I also really like stickies suggestion from #29817 (review) to delete the ChainstateLoadOptions::reindex variable and think it would be great to add it as a second commit. If you do add it, I also think it would also be good idea to add a third commit renaming ChainstateLoadOptions::reindex_chainstate ChainstateLoadOptions::should_wipe as a small scripted diff so that name is more descriptive and internally consistent.

Stickies other suggestion #29817 (comment) to avoid duplicating the default value also seems good.

ryanofsky · 2024-04-17T17:11:35Z

src/init.cpp

@@ -1485,7 +1484,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)

    node.notifications = std::make_unique<KernelNotifications>(*Assert(node.shutdown), node.exit_status);
    ReadNotificationArgs(args, *node.notifications);
-    fReindex = args.GetBoolArg("-reindex", false);


In commit "kernel: De-globalize fReindex" (d9bcbbf)

Note: removing this line does not change behavior, because GetBoolArg("-reindex") is now called a few lines below on line 1501 in ApplyArgsManOptions(args, blockman_opts)

src/node/blockstorage.h

src/validation.cpp

TheCharlatan · 2024-04-20T21:05:49Z

Re #29817 (review) and #29817 (review)

For example, replacing all uses of ChainstateLoadOptions::reindex with BlockManager::m_reindex is a trivial code change and passes unit and functional tests
...
I also really like stickies suggestion from #29817 (review) to delete the ChainstateLoadOptions::reindex variable and think it would be great to add it as a second commi

I thought abour this initially as well, but the way I read the code, this is actually a change in behaviour. The persisted reindexing value is read from the database with a call to LoadBlockIndex. Further down, this would lead to the coins being wiped again after a restart. Meaning if the user restarts without setting reindex, but with reindexing set in the database, it would not wipe the block tree db, but would wipe the coins, while with the current behaviour it would not wipe anything and continue from where it left off.

TheCharlatan · 2024-04-20T21:35:28Z

Updated d9bcbbf -> 9b2c9c2 (noGlobalReindex_0 -> noGlobalReindex_1, compare)

Addressed @stickies-v's comment, removed double definition of a default option value.
Addressed @ryanofsky's comment, changed new reindexing argument in GetSynchronizationState to a bool.
Addressed @ryanofsky's comment, renamed m_reindex to m_reindexing and added a comment describing its purpose.

ryanofsky

Code review ACK 9b2c9c2. This looks good in its current form. I just had one suggestion below (not important) and some questions about possible minor pre-existing bugs.

re: #29817 (comment)

I thought abour this initially as well, but the way I read the code, this is actually a change in behaviour [...]

Thanks for the explanation, that makes sense.

ryanofsky · 2024-04-24T18:00:50Z

src/init.cpp

@@ -1562,7 +1560,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)

        node::ChainstateLoadOptions options;
        options.mempool = Assert(node.mempool.get());
-        options.reindex = node::fReindex;
+        options.reindex = chainman.m_blockman.m_reindexing;


In commit "kernel: De-globalize fReindex" (9b2c9c2)

I think it would be less fragile to assign blockman_opts.reindex instead of chainman.m_blockman.m_reindex.

Assigning blockman_opts.reindex should ensure and make it more obvious that this is only affected by the value of the -reindex setting, not the value of a previous setting saved with WriteReindexing.

In commit "kernel: De-globalize fReindex" (b47bd95)

I think it would be less fragile to assign blockman_opts.reindex instead of chainman.m_blockman.m_reindex.

This suggestion was not implemented in this PR, but is implemented in followup PR #30132.

ryanofsky · 2024-04-24T18:10:13Z

src/init.cpp

@@ -1610,7 +1608,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
                    error.original + ".\nPlease restart with -reindex or -reindex-chainstate to recover.",
                    "", CClientUIInterface::MSG_ERROR | CClientUIInterface::BTN_ABORT);
                if (fRet) {
-                    fReindex = true;
+                    chainman.m_blockman.m_reindexing = true;


In commit "kernel: De-globalize fReindex" (9b2c9c2)

Note: I initially thought there was a minor bug in existing code (independent of this PR) on line 1605 above which checks if (!options.reindex) instead of if (!chainman.m_blockman.m_reindexing). It seemed wrong because it could trigger an error suggesting using reindex flag even if reindexing was already happening from a previous request. This behavior is surprising, but on second thought, could actually make sense if something has gone wrong during reindexing and there is a reason to restart it.

In commit "kernel: De-globalize fReindex" (b47bd95)

It turns out there was a bug introduced here. Setting chainman.m_blockman.m_reindexing = true has no effect because chainman is destroyed and recreated on the next loop iteration on line 1537 above. So after this change, reindexing no longer occurs when the user answers "Yes" above. This is fixed in #30132

ryanofsky · 2024-04-24T18:16:28Z

src/init.cpp

@@ -1643,17 +1641,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
    // ********************************************************* Step 8: start indexers

    if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
-        g_txindex = std::make_unique<TxIndex>(interfaces::MakeChain(node), cache_sizes.tx_index, false, fReindex);
+        g_txindex = std::make_unique<TxIndex>(interfaces::MakeChain(node), cache_sizes.tx_index, false, chainman.m_blockman.m_reindexing);


In commit "kernel: De-globalize fReindex" (9b2c9c2)

It seems like there is a prexisting bug here, and the f_wipe argument passed here should be blockman_opts.reindex instead of chainman.m_blockman.m_reindexing or fReindex. Otherwise if the node is restarted before reindexing completes the TxIndex will be wiped a second time?

Same for BlockFilterIndex and CoinStatsIndex below

Good catch! To be clear from my comments before, it is not that big of an issue if the indexes are wiped again. AFAICT the operations executed while the reindexing atomic is true do not issue any validation signals, so they don't have an effect on the indexes. However recreating them is redundant and I don't think it is particularly useful. I think it would also be good to make the behavior between the coins and the other indexes consistent, so I'll take your suggestion here.

AFAICT the operations executed while the reindexing atomic is true do not issue any validation signals, so they don't have an effect on the indexes.

I need to test this, but this would be surprising to me. I think the idea behind having a separate StartIndexBackgroundSync function that runs after index initialization is that indexes should be able to receive signals during reindexing when -reindex is used so they do not need to completely resync when indexing is finished.

Will check on this, but I think I might recommend just keeping behavior unchanged in this PR so it is a more straightforward refactoring. Or this behavior is worth changing, it seems like it could be moved to a separate commit.

I need to test this, but this would be surprising to me.

Indeed, my impression was wrong. When we restart after a previous reindex without passing in -reindex, the first step is activating the chain again.

Ok, I think I get the different mechanics involved here now and think if initially wiping an index is going to change, it should be done in a separate pull request. Will revert this change.

TheCharlatan · 2024-04-25T12:08:25Z

Updated 9b2c9c2 -> b0594f6 (noGlobalReindex_1 -> noGlobalReindex_2, compare)

Addressed @ryanofsky's comment and comment, using blockman_opts.reindex instead of chainman.m_blockman.m_reindexing.

TheCharlatan · 2024-05-15T09:39:50Z

Rebased b0594f6 -> 46b9ec1 (noGlobalReindex_2 -> noGlobalReindex_3, compare)

Fixed conflict with kernel: Remove batchpriority from kernel library #30083

fReindex is one of the last remaining globals exposed by the kernel library, so move it into the blockstorage class to reduce the amount of global mutable state and make the kernel library a bit less awkward to use.

TheCharlatan · 2024-05-16T11:44:41Z

Updated 46b9ec1 -> b47bd95 (noGlobalReindex_3 -> noGlobalReindex_4, compare)

Reverted previous change in init code that would no longer wipe the indexes when restarting in the middle of a reindex without the -reindex flag.

ryanofsky

Code review ACK b47bd95. I rereviewed the whole PR, but the only change since last review was reverting the bugfix #29817 (comment) and make the change a pure refactoring.

I think this change makes the code a lot less confusing by using separate names for the -reindex option and reindexing state, and could avoid bugs like the one mentioned in the future.

mzumsande

Code Review ACK b47bd95

mzumsande · 2024-05-16T20:23:16Z

src/validation.cpp

@@ -4823,8 +4822,8 @@ bool ChainstateManager::LoadBlockIndex()

    if (needs_init) {
        // Everything here is for *new* reindex/DBs. Thus, though


Unrelated, but this comment and the needs_init variable are confusing. Originally (eda888e) it may have made sense, but now there is just a log message left which doesn't even seem to belong here (no db initialization is happening here). So I think this could be cleaned up, but no need to do it here.

I ended up deleting all of this in #30132.

stickies-v

ACK b47bd95

stickies-v · 2024-05-17T16:14:29Z

src/validation.cpp

@@ -3255,10 +3254,10 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
    return true;
 }

-static SynchronizationState GetSynchronizationState(bool init)
+static SynchronizationState GetSynchronizationState(bool init, bool reindexing)


nit: perhaps static SynchronizationState GetSynchronizationState(const ChainstateManager& chainman) makes for a more logical function signature now, but it doesn't really seem to make for much of a clean-up either, so... not sure.

Mmh, I think I prefer this as is.

achow101 · 2024-05-17T19:38:32Z

ACK b47bd95

DrahtBot added the Validation label Apr 5, 2024

TheCharlatan marked this pull request as ready for review April 5, 2024 20:58

DrahtBot added the CI failed label Apr 6, 2024

This was referenced Apr 6, 2024

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

Draft

Bugfix: Correct first-run free space checks #29678

Open

TheCharlatan force-pushed the noGlobalReindex branch from 3afa0d1 to d9bcbbf Compare April 6, 2024 05:53

DrahtBot mentioned this pull request Apr 6, 2024

scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint #29641

Draft

DrahtBot removed the CI failed label Apr 6, 2024

This was referenced Apr 6, 2024

blockstorage: XOR blocksdir *.dat files #28052

Open

Add util::ResultPtr class #26022

Draft

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

Draft

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

Open

stickies-v reviewed Apr 15, 2024

View reviewed changes

ryanofsky approved these changes Apr 17, 2024

View reviewed changes

DrahtBot requested a review from stickies-v April 17, 2024 17:49

TheCharlatan force-pushed the noGlobalReindex branch from d9bcbbf to 9b2c9c2 Compare April 20, 2024 21:34

ryanofsky approved these changes Apr 24, 2024

View reviewed changes

TheCharlatan force-pushed the noGlobalReindex branch from 9b2c9c2 to b0594f6 Compare April 25, 2024 12:08

DrahtBot added the Needs rebase label May 14, 2024

TheCharlatan force-pushed the noGlobalReindex branch from b0594f6 to 46b9ec1 Compare May 15, 2024 09:39

DrahtBot removed the Needs rebase label May 15, 2024

kernel: De-globalize fReindex

b47bd95

fReindex is one of the last remaining globals exposed by the kernel library, so move it into the blockstorage class to reduce the amount of global mutable state and make the kernel library a bit less awkward to use.

TheCharlatan force-pushed the noGlobalReindex branch from 46b9ec1 to b47bd95 Compare May 16, 2024 11:36

ryanofsky approved these changes May 16, 2024

View reviewed changes

mzumsande reviewed May 17, 2024

View reviewed changes

stickies-v approved these changes May 17, 2024

View reviewed changes

achow101 merged commit 058af75 into bitcoin:master May 17, 2024
16 checks passed

This was referenced May 20, 2024

locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip #30111

Open

refactor: TxDownloadManager #30110

Draft

ryanofsky mentioned this pull request May 20, 2024

indexes: Don't wipe indexes again when continuing a prior reindex #30132

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kernel: De-globalize fReindex #29817

kernel: De-globalize fReindex #29817

TheCharlatan commented Apr 5, 2024

DrahtBot commented Apr 5, 2024 •

edited

DrahtBot commented Apr 6, 2024

stickies-v left a comment •

edited

stickies-v Apr 15, 2024

ryanofsky left a comment

ryanofsky Apr 17, 2024

TheCharlatan commented Apr 20, 2024

TheCharlatan commented Apr 20, 2024

ryanofsky left a comment

ryanofsky Apr 24, 2024

ryanofsky Jun 3, 2024

ryanofsky Apr 24, 2024

ryanofsky Jun 3, 2024

ryanofsky Apr 24, 2024

TheCharlatan Apr 24, 2024

ryanofsky May 15, 2024 •

edited

TheCharlatan May 15, 2024

TheCharlatan May 16, 2024

TheCharlatan commented Apr 25, 2024

TheCharlatan commented May 15, 2024

TheCharlatan commented May 16, 2024 •

edited

ryanofsky left a comment

mzumsande left a comment

mzumsande May 16, 2024

TheCharlatan May 18, 2024

stickies-v left a comment

stickies-v May 17, 2024

TheCharlatan May 17, 2024

achow101 commented May 17, 2024

		@@ -33,6 +33,8 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Op

		if (auto value{args.GetBoolArg("-fastprune")}) opts.fast_prune = *value;

		opts.reindex = args.GetBoolArg("-reindex", false);

	opts.reindex = args.GetBoolArg("-reindex", false);
	if (auto value{args.GetBoolArg("-reindex")}) opts.reindex = *value;

		@@ -4823,8 +4822,8 @@ bool ChainstateManager::LoadBlockIndex()

		if (needs_init) {
		// Everything here is for new reindex/DBs. Thus, though

kernel: De-globalize fReindex #29817

kernel: De-globalize fReindex #29817

Conversation

TheCharlatan commented Apr 5, 2024

DrahtBot commented Apr 5, 2024 • edited

Code Coverage

Reviews

Conflicts

DrahtBot commented Apr 6, 2024

stickies-v left a comment • edited

Choose a reason for hiding this comment

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

TheCharlatan commented Apr 20, 2024

TheCharlatan commented Apr 20, 2024

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

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

ryanofsky May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheCharlatan commented Apr 25, 2024

TheCharlatan commented May 15, 2024

TheCharlatan commented May 16, 2024 • edited

ryanofsky left a comment

Choose a reason for hiding this comment

mzumsande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented May 17, 2024

DrahtBot commented Apr 5, 2024 •

edited

stickies-v left a comment •

edited

ryanofsky May 15, 2024 •

edited

TheCharlatan commented May 16, 2024 •

edited