refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager` #22564

dongcarl · 2021-07-27T19:58:39Z

This is a small part of the work to accomplish what I described in 972c516:

Over time, we should probably move these mutable global state variables
into ChainstateManager or CChainState so it's easier to reason about
their lifecycles.

::UnloadBlockIndex manually resets a subset of our mutable globals in addition to unloading the ChainstateManager and clearing the mempool. The need for this manual reset (AFAICT) arises out of the fact that many of these globals are closely related to the block index (hence ::UnloadBlockIndex), and need to be reset with it.

I've shot this "manual reset" gun at my foot several times while doing the de-globalize chainman work.

Thankfully, now that we have a BlockManager class that owns the block index, these globals should be moved under that class so that they can live and die with the block index. These moves, along with making the block index non-heap-based, eliminates:

3585b52 The need to reason about when we need to manually call ::UnloadBlockIndex (this decision can at times seem almost arbitrary)
f741623 The need to have an ::UnloadBlockIndex or explicit ~ChainstateManager at all

dongcarl · 2021-07-27T20:02:06Z

Ping @jnewbery, since #21866 (comment) ☺️

jnewbery · 2021-07-27T21:48:03Z

Concept ACK. Globals must fall!

maflcko

Concept ACK

DrahtBot · 2021-07-28T10:06:28Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24595 (deploymentstatus: move g_versionbitscache global to ChainstateManager by ajtowns)
#24232 (assumeutxo: add init and completion logic by jamesob)
#24008 (assumeutxo: net_processing changes by jamesob)
#23443 (p2p: Erlay support signaling by naumenkogs)
#20030 (validation: Remove useless call to mempool->clear() by MarcoFalke)
#19888 (rpc, test: Improve getblockstats for unspendables by fjahr)

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.

ajtowns · 2021-08-01T12:23:58Z

src/deploymentstatus.h

 /** Determine if a deployment is active for the next block */
 inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::BuriedDeployment dep)
 {
    assert(Consensus::ValidDeployment(dep));
    return (pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1) >= params.DeploymentHeight(dep);
 }

-inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep)
+inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep, VersionBitsCache& versionbitscache)


Hard NACK -- the signatures for the two versions of DeploymentActiveAfter (and ..At) should only differ by whether they accept a DeploymentPos or a BuriedDeployment, otherwise we're back to having to edit bunches of call sites when burying deployments.

BUT... I think you can get the result you're trying for by making DeploymentActiveAfter (and ..At) inline member functions of BlockManager. Proof of concept at https://github.com/ajtowns/bitcoin/commits/202108-deplotmentstatus-blockmanager

Would it be feasible to move BlockManager to its own module to reduce the amount of stuff validation.cpp does? Giving BlockManager a CChainParams member reference and having CChainState and ChainstateManager reference it via their BlockManager references, and also removing the Consensus::Params and CChainParams params from bunches of functions (including the DeploymentActive.. ones) seems like it would be a win.

Wouldn't another class in validation be better suited to host the deployment status? My goal was to move BlockManger to node/storage and have it deal with storage only, not validation.

@MarcoFalke Maybe? I look at deployment status as an expansion of Consensus::Params -- so more as input to validation than a result of it. If we switch to using the coinbase witness nonce for deployment signalling we'd need to change the way we store blocks so that it's easy to access that info without having to load every signalling block from disk, so it's kind-of storage related in that sense?

the signatures for the two versions of DeploymentActiveAfter (and ..At) should only differ by whether they accept a DeploymentPos or a BuriedDeployment, otherwise we're back to having to edit bunches of call sites when burying deployments.

Oh I did not realize that this was the case! Note to self: add a code comment documenting this.

BUT... I think you can get the result you're trying for by making DeploymentActiveAfter (and ..At) inline member functions of BlockManager. Proof of concept at https://github.com/ajtowns/bitcoin/commits/202108-deplotmentstatus-blockmanager

This looks good to me, and it seems like they would eventually end up having to interact with BlockManager anyway according to the following?

If we switch to using the coinbase witness nonce for deployment signalling we'd need to change the way we store blocks so that it's easy to access that info without having to load every signalling block from disk, so it's kind-of storage related in that sense?

Would it be feasible to move BlockManager to its own module to reduce the amount of stuff validation.cpp does? Giving BlockManager a CChainParams member reference and having CChainState and ChainstateManager reference it via their BlockManager references, and also removing the Consensus::Params and CChainParams params from bunches of functions (including the DeploymentActive.. ones) seems like it would be a win.

Yes I think as part of some other work I'll have to pull validation apart anyway, I will look into this!

This looks good to me, and it seems like they would eventually end up having to interact with BlockManager anyway according to the following?

Well, using the cb wit nonce for activation signalling might never happen, and even if it does, I think it's a fair way off (since it would need getblocktemplate changes or stratumv2 or similar to make it even possible), so I wouldn't put too much weight on it, but it seems plausible.

I think this is dealt with by #24595

ajtowns · 2021-08-01T13:06:26Z

src/validation.cpp

@@ -4106,7 +4107,6 @@ void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman)
    LOCK(cs_main);
    chainman.Unload();
    if (mempool) mempool->clear();
-    g_versionbitscache.Clear();


Note that there shouldn't be any logical need for this to be cleared here -- the cached information doesn't become invalid when we load up different blocks, only if the consensus parameters are changed.

Is this the same case for warningcache as well? If so, perhaps we'll just move the clearing logic to SelectParams and not move these two structures into BlockManager?

Yes, warningcache only relies on the chain history, consensus params, and versionbitscache state. Moving the clearing logic to SelectParams would introduce a circular dependency though, I think?

Here's a rebase of this that replaces moving g_versionbits with just removing g_versionbitscache.Clear() from UnloadBlockIndex -- https://github.com/ajtowns/bitcoin/commits/202201-vb-unloadblockindex ... What do you think about doing that instead? I've got some updates for the versionbits code I'd like to PR, and would prefer to minimise conflicts with this one...

If warningcache and g_versionbitscache are both only dependent on consensus parameters, then perhaps (perhaps not in this PR) we should move them to ChainstateManager or NodeContext? At the very least we would need to clear them in ~ChainTestingSetup so that tests with different consensus params won't get confused.

Moving them somewhere rather than having them be globals would be good eventually; I had a look at moving them into ChainstateManager but it felt a bit too invasive when what I was hoping to do was avoid/defer introducing conflicts...

The patch above clears g_versionbitscache in the BasicTestingSetup constructor instead (where SelectParams is invoked), which I think should be good enough? It's also already cleared explicitly in versionbits_tests.cpp which manually changes the consensus params being used.

#24595 moves g_versionbitscache into ChainstateManager and builds on this PR, so hopefully resolves this thread

src/validation.cpp

ryanofsky

This PR generally looks pretty good and it would be nice to see an update.

re: #22564 (comment)

In particular, I'd love to get some feedback on 6a897c0, since it changes the key type of BlockMap from CBlockIndex* to CBlockIndex

This seems like an improvement in every respect: removing a level of indirection, removing need for manual deletions, avoiding pointer bugs. No downsides are apparent to me at least.

src/validation.h

dongcarl · 2021-08-24T17:57:12Z

Pushed f741623 -> 6e6472e

Address refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564 (comment)
Address refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564 (comment)

dongcarl · 2021-08-24T18:45:58Z

Pushed 6e6472e -> 6bb9ed3:

Rebased on master

maflcko

ACK b1ab563 🏐

Show signature

Signature:

maflcko · 2022-04-25T15:03:41Z

src/validation.cpp

@@ -2533,7 +2533,7 @@ void CChainState::UpdateTip(const CBlockIndex* pindexNew)
        const CBlockIndex* pindex = pindexNew;
        for (int bit = 0; bit < VERSIONBITS_NUM_BITS; bit++) {
            WarningBitsConditionChecker checker(bit);
-            ThresholdState state = checker.GetStateFor(pindex, m_params.GetConsensus(), warningcache[bit]);
+            ThresholdState state = checker.GetStateFor(pindex, m_params.GetConsensus(), warningcache.at(bit));


Unrelated.

Sometimes I wonder how safe it is to throw in random places in the consensus code and then continue the program. Too bad there is no asserting at().

There is a compile-time erroring at() -- std::get<N>(my_array) won't compile if N is outside the range. I've got a patchset that switches versionbits to working that way, fwiw; needs a lot of tidying up yet though, and builds on these patches.

Nice. TIL https://en.cppreference.com/w/cpp/container/array/get

src/validation.h

maflcko · 2022-04-25T15:15:10Z

src/init.cpp

@@ -1269,18 +1269,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
    // as they would never get updated.
    if (!ignores_incoming_txs) node.fee_estimator = std::make_unique<CBlockPolicyEstimator>();

-    assert(!node.mempool);
    int check_ratio = std::min<int>(std::max<int>(args.GetIntArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000);


In the third commit:

I think this should be moved as well, otherwise it is easy to confuse it with some other check_ratio

Edit: I see it is renamed later, but I still think it could make sense to move as well.

Also, would it be better to split the peerman move into a separate commit?

Also, it would be best to do a rebase first to pull in the intermediate changes to init.cpp?

I think this should be moved as well, otherwise it is easy to confuse it with some other check_ratio

Done in 976679b

Also, would it be better to split the peerman move into a separate commit?

Don't think so, the diff itself isn't that large, and the peerman move only makes sense in the context of moving the mempool and chainman

Also, it would be best to do a rebase first to pull in the intermediate changes to init.cpp?

Done as of the 133d98f push

dongcarl · 2022-04-25T16:40:51Z

Pushed b1ab563 -> 133d98f

Rebased over master
Addressed Marco's review comments (thanks Marco!)

ajtowns

The m_total_coins*_cache changes seem wrong to me, otherwise looks good.

Commit 678f5c5

src/init.cpp

src/node/chainstate.cpp

src/init.cpp

src/bitcoin-chainstate.cpp

Fixes bitcoin#22964 Previously, we used UnloadBlockIndex() in order to reset node.mempool and node.chainman. However, that has proven to be fragile (see bitcoin#22964), and requires UnloadBlockIndex and its callees to be updated manually for each member that's introduced to the mempool and chainman classes. In this commit, we stop using the UnloadBlockIndex function and we simply reconstruct node.mempool and node.chainman. Since PeerManager needs a valid reference to both node.mempool and node.chainman, we also move PeerManager's construction via `::make` to after the chainstate activation sequence is complete. There are no more callers to UnloadBlockIndex after this commit, so it and its sole callees can be pruned.

It's a bit clearer and restricts the scope of fLoaded

Also add TODO item to deglobalize the {versionbits,warning}cache, which should really only need to be cleared if we change the chainparams.

The only caller that uses this is ~ChainTestingSetup() where we immediately destroy the mempool afterwards.

In previous commits in this patchset, we've made sure that every Unload/UnloadBlockIndex member function resets its own members, and does not reach out to globals. This means that their corresponding classes' default destructors can now replace them, and do an even more thorough job without the need to be updated for every new member variable. Therefore, we can remove them, and also remove UnloadBlockIndex since that's not used anymore. Unfortunately, chainstatemanager_loadblockindex relies on CChainState::UnloadBlockIndex, so that needs to stay for now.

dongcarl · 2022-04-27T15:17:04Z

Pushed 678f5c5 -> 7ab07e0

Addressed outstanding reviews (Thanks AJ!)

ryanofsky

Code review ACK 7ab07e0. This all looks good and simplifies things nicely. I left some minor suggestions below but feel free to ignore.

ryanofsky · 2022-04-27T14:59:06Z

src/validation.cpp

@@ -1921,7 +1921,7 @@ class WarningBitsConditionChecker : public AbstractThresholdConditionChecker
    }
 };

-static ThresholdConditionCache warningcache[VERSIONBITS_NUM_BITS] GUARDED_BY(cs_main);
+static std::array<ThresholdConditionCache, VERSIONBITS_NUM_BITS> warningcache GUARDED_BY(cs_main);


In commit "refactor: Convert warningcache to std::array" (98f4bda)

This commit seems ok but it's not clear why it's good or what the connection is to the PR. Would be good to mention in commit message.

I think it is just "updating the code", since it is already touched for other reasons in this PR

ryanofsky · 2022-04-27T15:25:28Z

src/init.cpp

+    assert(!node.peerman);
+    node.peerman = PeerManager::make(chainparams, *node.connman, *node.addrman, node.banman.get(),
+                                     chainman, *node.mempool, ignores_incoming_txs);
+    RegisterValidationInterface(node.peerman.get());


In commit "init: Reset mempool and chainman via reconstruction" (976679b)

Is it ok for peerman to register for validation notification events later than before? Maybe because none will be sent above, or because peerman doesn't care about ones that would be sent? (would be helpful info for commit message)

My understanding was that no events are sent, since blocks and the mempool are only imported later on

And it looks like (from testing) with existing blocks and without import/reindex/empty datadir no events are sent at all during startup. I can only see events when the block import thread is running.

ryanofsky · 2022-04-27T15:26:19Z

src/node/chainstate.cpp

@@ -32,8 +32,6 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
    chainman.m_total_coinstip_cache = nCoinCacheUsage;
    chainman.m_total_coinsdb_cache = nCoinDBCache;

-    UnloadBlockIndex(mempool, chainman);


In commit "init: Reset mempool and chainman via reconstruction" (5921b86)

UnloadBlockIndex is still clearing g_versionbitscache and warningcache at this point. Should this commit come after the later commit "Clear {versionbits,warning}cache in ~Chainstatemanager" (572d831) to avoid changing behavior?

Have you seen #22564 (comment) ?

ryanofsky · 2022-04-27T16:03:46Z

src/validation.cpp

+    // TODO: The version bits cache and warning cache should probably become
+    // non-globals
+    g_versionbitscache.Clear();
+    for (auto& i : warningcache) {
+        i.clear();
+    }


In commit "Clear {versionbits,warning}cache in ~Chainstatemanager" (572d831)

It would seem more appropriate to clear these variables in the constructor not the destructor. Using the constructor to initialize things a class needs is more straightforward than doing things in the destructor to prepare for a future instance being created.

Maybe this can be discussed in the versionbits follow-up?

maflcko · 2022-04-28T07:39:59Z

ACK 7ab07e0 👘

Show signature

Signature:

ajtowns · 2022-04-28T10:06:46Z

ACK 7ab07e0

…oadBloc…

Summary: This is a partial backport of [[bitcoin/bitcoin#22564 | core#22564]] bitcoin/bitcoin@6e747e8 Test Plan: With clang and DEBUG `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D13086

Summary: Fixes bitcoin/bitcoin#22964 Previously, we used UnloadBlockIndex() in order to reset node.mempool and node.chainman. However, that has proven to be fragile (see bitcoin/bitcoin#22964), and requires UnloadBlockIndex and its callees to be updated manually for each member that's introduced to the mempool and chainman classes. In this commit, we stop using the UnloadBlockIndex function and we simply reconstruct node.mempool and node.chainman. Since PeerManager needs a valid reference to both node.mempool and node.chainman, we also move PeerManager's construction via `::make` to after the chainstate activation sequence is complete. There are no more callers to UnloadBlockIndex after this commit, so it and its sole callees can be pruned. This is a partial backport of [[bitcoin/bitcoin#22564 | core#22564]] bitcoin/bitcoin@5921b86 Depends on D13086 and D13081 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D13087

Summary: It's a bit clearer and restricts the scope of fLoaded This is a partial backport of [[bitcoin/bitcoin#22564 | core#22564]] bitcoin/bitcoin@fe96a2e Depends on D13087 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D13088

Summary: This is a partial backport of [[bitcoin/bitcoin#22564 | core#22564]] bitcoin/bitcoin@eca4ca4 Depends on D13088 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D13089

Summary: The only caller that uses this is ~ChainTestingSetup() where we immediately destroy the mempool afterwards. This is a partial backport of [[bitcoin/bitcoin#22564 | core#22564]] bitcoin/bitcoin@572d831 bitcoin/bitcoin@7d99d72 Note: the first commit is mostly not applicable to Bitcoin ABC, I mention it only because of the moving of `~ChainstateManager()` from `validation.h` to `validation.cpp`. `g_versionbitscache` and `warningcache` are related to Core's BIP 9 and "buried deployments" business. Depends on D13089 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Subscribers: sdulfari Differential Revision: https://reviews.bitcoinabc.org/D13090

Summary: In previous commits in this patchset, we've made sure that every Unload/UnloadBlockIndex member function resets its own members, and does not reach out to globals. This means that their corresponding classes' default destructors can now replace them, and do an even more thorough job without the need to be updated for every new member variable. Therefore, we can remove them, and also remove UnloadBlockIndex since that's not used anymore. Unfortunately, chainstatemanager_loadblockindex relies on CChainState::UnloadBlockIndex, so that needs to stay for now. This concludes backport of [[bitcoin/bitcoin#22564 | core#22564]] bitcoin/bitcoin@7ab07e0 Depends on D13091 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D13092

DrahtBot added Block storage Consensus Mining P2P RPC/REST/ZMQ Validation Wallet labels Jul 27, 2021

maflcko reviewed Jul 28, 2021

View reviewed changes

ajtowns reviewed Aug 1, 2021

View reviewed changes

ryanofsky reviewed Aug 23, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

dongcarl force-pushed the 2021-07-kirby-inhale-global-muts branch from f741623 to 6e6472e Compare August 24, 2021 17:55

dongcarl force-pushed the 2021-07-kirby-inhale-global-muts branch from 6e6472e to 6bb9ed3 Compare August 24, 2021 18:45

This was referenced Aug 25, 2021

refactor: Clarify and disable unused ArgsManager flags #22766

Merged

validation: mempool validation and submission for packages of 1 child + parents #22674

Merged

rpc: Add level 3 verbosity to getblock RPC call. #21245

Closed

maflcko approved these changes Apr 25, 2022

View reviewed changes

dongcarl force-pushed the 2021-07-kirby-inhale-global-muts branch from b1ab563 to 133d98f Compare April 25, 2022 16:38

DrahtBot added the Needs rebase label Apr 26, 2022

dongcarl and others added 2 commits April 26, 2022 18:41

refactor: Convert warningcache to std::array

98f4bda

validation: default initialize and guard chainman members

6e747e8

dongcarl force-pushed the 2021-07-kirby-inhale-global-muts branch from 133d98f to 678f5c5 Compare April 26, 2022 22:45

DrahtBot removed the Needs rebase label Apr 26, 2022

ajtowns reviewed Apr 27, 2022

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/node/chainstate.cpp Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

src/bitcoin-chainstate.cpp Show resolved Hide resolved

dongcarl added 6 commits April 27, 2022 11:09

style-only: Use for instead of when loading Chainstate

fe96a2e

It's a bit clearer and restricts the scope of fLoaded

style-only: Use std::clamp for check_ratio, rename

eca4ca4

Clear {versionbits,warning}cache in ~Chainstatemanager

572d831

Also add TODO item to deglobalize the {versionbits,warning}cache, which should really only need to be cleared if we change the chainparams.

validation: No mempool clearing in UnloadBlockIndex

7d99d72

The only caller that uses this is ~ChainTestingSetup() where we immediately destroy the mempool afterwards.

dongcarl force-pushed the 2021-07-kirby-inhale-global-muts branch from 678f5c5 to 7ab07e0 Compare April 27, 2022 15:15

ryanofsky approved these changes Apr 27, 2022

View reviewed changes

maflcko merged commit b51e60f into bitcoin:master Apr 28, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 29, 2022

Merge bitcoin#22564: refactor: Move mutable globals cleared in `::Unl…

76ed692

…oadBloc…

hebasto mentioned this pull request Sep 8, 2022

Problems with running my Bitcoin Node, I only synced it up yesterday and it was working fine #26042

Closed

bitcoin locked and limited conversation to collaborators Apr 28, 2023

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Conversation

dongcarl commented Jul 27, 2021 • edited Loading

dongcarl commented Jul 27, 2021

jnewbery commented Jul 27, 2021

maflcko left a comment

Choose a reason for hiding this comment

DrahtBot commented Jul 28, 2021 • edited Loading

Conflicts

ajtowns Aug 1, 2021 • edited Loading

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

Choose a reason for hiding this comment

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

dongcarl commented Aug 24, 2021

dongcarl commented Aug 24, 2021

maflcko 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

dongcarl Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

dongcarl commented Apr 25, 2022

ajtowns left a comment • edited Loading

Choose a reason for hiding this comment

dongcarl commented Apr 27, 2022

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

ryanofsky Apr 27, 2022 • edited Loading

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

maflcko commented Apr 28, 2022

ajtowns commented Apr 28, 2022

refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager` #22564

refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager` #22564

dongcarl commented Jul 27, 2021 •

edited

Loading

DrahtBot commented Jul 28, 2021 •

edited

Loading

ajtowns Aug 1, 2021 •

edited

Loading

dongcarl Apr 25, 2022 •

edited

Loading

ajtowns left a comment •

edited

Loading

ryanofsky Apr 27, 2022 •

edited

Loading