Refactor CWallet's inheritance chain #16227

achow101 · 2019-06-17T22:46:29Z

This PR compresses the CWallet chain of inheritance from 5 classes to 3 classes. CBasicKeyStore is renamed to FillableSigningProvider and some parts of it (the watchonly parts) are moved into CWallet. CKeyStore and CCrypoKeyStore are completely removed. CKeyStore's Have* functions are moved into SigningProvider and the Add* moved into FillableSigningProvider, thus allowing it to go away entirely. CCryptoKeyStore's functionality is moved into CWallet. The new inheritance chain is:

SigningProvider -> FillableSigningProvider -> CWallet

SigningProvider now is the class the provides keys and scripts and indicates whether keys and scripts are present. FillableSigningProvider allows keys and scripts to be added to the signing provider via Add* functions. CWallet handles all of the watchonly stuff (AddWatchOnly, HaveWatchOnly, RemoveWatchOnly which were previously in CKeyStore) and key encryption (previously in CCryptoKeyStore).

Implements the 2nd prerequisite from the wallet restructure.

DrahtBot · 2019-06-18T04:20:18Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16341 (WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
#16273 (refactor: Remove unused includes by practicalswift)
#15845 (wallet: Fast rescan with BIP157 block filters by MarcoFalke)
#14144 (Refactoring: Clarify code using encrypted_batch in CWallet by domob1812)

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.

achow101 · 2019-06-19T22:09:07Z

Rebased

achow101 · 2019-06-21T17:55:14Z

Rebased

src/wallet/wallet.cpp

src/wallet/crypter.h

maflcko · 2019-06-24T14:18:45Z

Looks like this doesn't compile with ./configure CC=clang CXX=clang++

achow101 · 2019-06-24T14:53:55Z

There were some lock order issues (I guess bad copy paste?) that I fixed.

Looks like this doesn't compile with ./configure CC=clang CXX=clang++

Seems to be working for me now although I didn't actually do anything to try to fix it. Does it work for you now?

maflcko · 2019-06-24T15:45:07Z

You can see the failure here: https://travis-ci.org/bitcoin/bitcoin/jobs/549788027#L3052 (in the build step)

wallet/wallet.cpp:4653:12: warning: reading variable 'vMasterKey' requires holding mutex 'cs_wallet' [-Wthread-safety-analysis]
    return vMasterKey.empty();
           ^
wallet/wallet.cpp:4663:9: warning: reading variable 'vMasterKey' requires holding mutex 'cs_wallet' [-Wthread-safety-analysis]
        vMasterKey.clear();
        ^
wallet/wallet.cpp:4735:9: warning: writing variable 'vMasterKey' requires holding mutex 'cs_wallet' exclusively [-Wthread-safety-analysis]
        vMasterKey = vMasterKeyIn;
        ^
wallet/wallet.cpp:4788:40: warning: reading variable 'mapCryptedKeys' requires holding mutex 'cs_KeyStore' [-Wthread-safety-analysis]
    CryptedKeyMap::const_iterator mi = mapCryptedKeys.find(address);
                                       ^
wallet/wallet.cpp:4789:15: warning: reading variable 'mapCryptedKeys' requires holding mutex 'cs_KeyStore' [-Wthread-safety-analysis]
    if (mi != mapCryptedKeys.end())
              ^
5 warnings generated.

achow101 · 2019-06-24T18:04:07Z

Fixed those warnings.

ryanofsky

utACK first seven commits (last commit seems harmless but not desirable). I left some comments, but feel free to ignore them. This PR is easy to understand and very nicely broken up. I confirmed the relevant parts are move only and that this doesn't change behavior.

faa513ea3dc376a69d382b943e3b1358d8baed6d Add HaveKey and HaveCScript to SigningProvider (1/8)
4f1b9d808efab7562099a2d07c406aeb29e8516e Remove CKeyStore and squash into CBasicKeyStore (2/8)
b8f6f3f25f9fd45d8659bfa15800c09bda951872 Remove HaveKey static function from keystore (3/8)
ec83adc1aed0503c62d9a3cb397508097b2b95a0 scripted-diff: rename CBasicKeyStore to FillableSigningProvider (4/8)
39659203e9104fbce270d7dd62790e954d55f55d Move KeyOriginInfo to its own header file (5/8)
a96967901b2b8b4a36a06b63581f28ae0ea3f145 Move various SigningProviders to signingprovider.{cpp,h} (6/8)
9a65a1e55ec5ac141c3b5a7e4d167f48712dc3cf Remove CCryptoKeyStore and move all of it's functionality into CWallet (7/8)
8ce8d6a3200921c960d41bca0ba5bf0ab176c5ee Move WatchOnly stuff from SigningProvider to CWallet (8/8)

ryanofsky · 2019-06-24T20:36:35Z

src/keystore.cpp

In commit "Remove HaveKey static function from keystore" (b8f6f3f25f9fd45d8659bfa15800c09bda951872)

Would be good to mention why removing this doesnt' change behavior in the commit description (I'd assume because hd keys are always compressed).

Actually, looking at it now, think this does actually change behavior. I think I will move this function into rpcwallet.cpp where it is used.

nit: don't forget to rename the commit

ryanofsky · 2019-06-24T21:13:03Z

src/script/signingprovider.h

In commit "Move WatchOnly stuff from SigningProvider to CWallet" (8ce8d6a3200921c960d41bca0ba5bf0ab176c5ee)

The changes in this commit seem undesirable and I don't understand the motivation for them. Why move watch only storage code away from script and key storage code that it's actually similar to, into the cwallet class which less related and more complicated? I could understand if this was actually simplifying something, for example by removing ability to store watch only keys alongside signing keys. But this doesn't seem to be simplifying anything.

I think i'd advise dropping this commit from the PR. Even if it is doing something useful (which it might be), it seems like it's going in the opposite direction as the other changes by splitting things up instead of consolidating.

The point of this was that SigningProviders provide things for signing which watchonly things are not. Those are just scripts and keys that CWallet needs to know and are not related to the things that a SigningProvider needs to provide. So that's why I moved them into CWallet. Later these will get moved in the the LegacyScryptPubKeyManager.

How does solvability play into this?

l2a5b1 · 2019-06-28T15:33:18Z

@achow101 per @MarcoFalke's request please see #16303 (comment)

achow101 · 2019-06-28T15:58:13Z

Fixed the include mentioned in #16303

l2a5b1

On a concept level I was wondering if it makes sense to:

make SigningProvider a "pure interface" (preferably defined in its own header file).
choose composition of FillableSigningProvider over inheritance in CWallet.

The names of the SigningProvider subtypes didn't really resonate with me. Perhaps there are better alternatives. I am obviously nitpicking here.

l2a5b1 · 2019-06-30T11:14:19Z

src/script/signingprovider.h

Any thoughts on:

(1) turning SigningProvider into a "pure interface" that only declares pure virtual functions?
(2) isolating SigningProvider and define it in its own header file?

These changes could make sense because we are using SigningProvider as an interface.

I think it is fine to leave it here. Making it a pure interface does make sense, but I don't think I will do that here.

l2a5b1 · 2019-06-30T11:15:32Z

src/wallet/wallet.h

In the spirit of this pull request it could make sense to choose composition of FillableSigningProvider over inheritance.

We do that later with the rest of the restructure with the introduction of the ScriptPubKeyMan. I'm not sure that it makes sense to make it a member now rather than later where a bunch of things will also change.

kallewoof

utACK f76c4819f6f37711f2b35562131ede4fe0438671

40da275a86d81a41b4a331efce33cd3e2074e592: Typo in commit message (it's should be its).
48f95eed20dae512a1ad62070d9d67771464ec74: Verified move-only.

I point out a number of include orderings; there are more; I left them there because it looked like attention was paid to this in some places.

src/keystore.h

src/bench/ccoins_caching.cpp

src/bitcoin-tx.cpp

src/rpc/rawtransaction_util.cpp

src/rpc/rawtransaction.cpp

src/rpc/util.cpp

src/script/sign.cpp

src/script/signingprovider.cpp

Sjors

ACK f76c481

Nit: you can remove "Built on" from the description.

Maybe keep EncryptSecret, DecryptSecret and DecryptKey in wallet/crypter.cpp? It makes me slightly sad to see wallet/wallet.cpp grow.

I checked the scripted diff (to my surprise, it works on macOS).

git show [hash] --color-moved=dimmed-zebra is quite handy for some of these commits.

It could make sense to turn the last commit, which moves watch-only stuff to CWallet, into a separate PR. See this discussion.

Sjors · 2019-07-09T13:39:57Z

src/keystore.cpp

nit: don't forget to rename the commit

src/keystore.h

Sjors · 2019-07-09T14:24:53Z

src/script/signingprovider.cpp

Why is this safe to remove?

This is only called directly after adding the privkey or watchonly key:

CCryptoKeyStore::AddCryptedKey CBasicKeyStore::AddKeyPubKey CBasicKeyStore::AddWatchOnly

perhaps leave a comment at the top of this function to the effect.

-BEGIN VERIFY SCRIPT- git grep -l "CBasicKeyStore" | xargs sed -i -e 's/CBasicKeyStore/FillableSigningProvider/g' -END VERIFY SCRIPT-

Moves all of the various SigningProviders out of sign.{cpp,h} and keystore.{cpp,h}. As such, keystore.{cpp,h} is also removed. Includes and the Makefile are updated to reflect this. Includes were largely changed using: git grep -l "keystore.h" | xargs sed -i -e 's;keystore.h;script/signingprovider.h;g'

Instead of having a separate CCryptoKeyStore that handles the encryption stuff, just roll it all into CWallet.

Sjors · 2019-07-10T14:30:52Z

re-ACK 93ce4a0; it keeps EncryptSecret, DecryptSecret and DecryptKey in wallet/crypter.cpp, but makes them not static. It improves alphabetical includes, reorders some function definitions, fixes commit message, brings back lost code comment.

instagibbs · 2019-07-11T20:06:06Z

src/keystore.h

    virtual bool AddKeyPubKey(const CKey &key, const CPubKey &pubkey) =0;

    //! Check whether a key corresponding to a given address is present in the store.
-    virtual bool HaveKey(const CKeyID &address) const =0;


mu-nit: code commend above should be removed as well

instagibbs · 2019-07-11T20:24:39Z

src/script/signingprovider.cpp

This is only called directly after adding the privkey or watchonly key:

CCryptoKeyStore::AddCryptedKey CBasicKeyStore::AddKeyPubKey CBasicKeyStore::AddWatchOnly

perhaps leave a comment at the top of this function to the effect.

instagibbs · 2019-07-11T20:37:49Z

utACK 93ce4a0

easy to follow

laanwj · 2019-07-11T20:42:34Z

code review ACK 93ce4a0

93ce4a0 Move WatchOnly stuff from SigningProvider to CWallet (Andrew Chow) 8f5b81e Remove CCryptoKeyStore and move all of it's functionality into CWallet (Andrew Chow) 37a79a4 Move various SigningProviders to signingprovider.{cpp,h} (Andrew Chow) 16f8096 Move KeyOriginInfo to its own header file (Andrew Chow) d9becff scripted-diff: rename CBasicKeyStore to FillableSigningProvider (Andrew Chow) a913e3f Move HaveKey static function from keystore to rpcwallet where it is used (Andrew Chow) c7797ec Remove CKeyStore and squash into CBasicKeyStore (Andrew Chow) 1b699a5 Add HaveKey and HaveCScript to SigningProvider (Andrew Chow) Pull request description: This PR compresses the `CWallet` chain of inheritance from 5 classes to 3 classes. `CBasicKeyStore` is renamed to `FillableSigningProvider` and some parts of it (the watchonly parts) are moved into `CWallet`. `CKeyStore` and `CCrypoKeyStore` are completely removed. `CKeyStore`'s `Have*` functions are moved into `SigningProvider` and the `Add*` moved into `FillableSigningProvider`, thus allowing it to go away entirely. `CCryptoKeyStore`'s functionality is moved into `CWallet`. The new inheritance chain is: ``` SigningProvider -> FillableSigningProvider -> CWallet ``` `SigningProvider` now is the class the provides keys and scripts and indicates whether keys and scripts are present. `FillableSigningProvider` allows keys and scripts to be added to the signing provider via `Add*` functions. `CWallet` handles all of the watchonly stuff (`AddWatchOnly`, `HaveWatchOnly`, `RemoveWatchOnly` which were previously in `CKeyStore`) and key encryption (previously in `CCryptoKeyStore`). Implements the 2nd [prerequisite](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#cwallet-subclass-stack) from the wallet restructure. ACKs for top commit: Sjors: re-ACK 93ce4a0; it keeps `EncryptSecret`, `DecryptSecret` and `DecryptKey` in `wallet/crypter.cpp`, but makes them not static. It improves alphabetical includes, reorders some function definitions, fixes commit message, brings back lost code comment. instagibbs: utACK 93ce4a0 Tree-SHA512: 393dfd0623ad2dac38395eb89b862424318d6072f0b7083c92a0d207fd032c48b284f5f2cb13bc492f34557de350c5fee925da02e47daf011c5c6930a721b6d3

93ce4a0 Move WatchOnly stuff from SigningProvider to CWallet (Andrew Chow) 8f5b81e Remove CCryptoKeyStore and move all of it's functionality into CWallet (Andrew Chow) 37a79a4 Move various SigningProviders to signingprovider.{cpp,h} (Andrew Chow) 16f8096 Move KeyOriginInfo to its own header file (Andrew Chow) d9becff scripted-diff: rename CBasicKeyStore to FillableSigningProvider (Andrew Chow) a913e3f Move HaveKey static function from keystore to rpcwallet where it is used (Andrew Chow) c7797ec Remove CKeyStore and squash into CBasicKeyStore (Andrew Chow) 1b699a5 Add HaveKey and HaveCScript to SigningProvider (Andrew Chow) Pull request description: This PR compresses the `CWallet` chain of inheritance from 5 classes to 3 classes. `CBasicKeyStore` is renamed to `FillableSigningProvider` and some parts of it (the watchonly parts) are moved into `CWallet`. `CKeyStore` and `CCrypoKeyStore` are completely removed. `CKeyStore`'s `Have*` functions are moved into `SigningProvider` and the `Add*` moved into `FillableSigningProvider`, thus allowing it to go away entirely. `CCryptoKeyStore`'s functionality is moved into `CWallet`. The new inheritance chain is: ``` SigningProvider -> FillableSigningProvider -> CWallet ``` `SigningProvider` now is the class the provides keys and scripts and indicates whether keys and scripts are present. `FillableSigningProvider` allows keys and scripts to be added to the signing provider via `Add*` functions. `CWallet` handles all of the watchonly stuff (`AddWatchOnly`, `HaveWatchOnly`, `RemoveWatchOnly` which were previously in `CKeyStore`) and key encryption (previously in `CCryptoKeyStore`). Implements the 2nd [prerequisite](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#cwallet-subclass-stack) from the wallet restructure. ACKs for top commit: Sjors: re-ACK 93ce4a0; it keeps `EncryptSecret`, `DecryptSecret` and `DecryptKey` in `wallet/crypter.cpp`, but makes them not static. It improves alphabetical includes, reorders some function definitions, fixes commit message, brings back lost code comment. instagibbs: utACK bitcoin@93ce4a0 Tree-SHA512: 393dfd0623ad2dac38395eb89b862424318d6072f0b7083c92a0d207fd032c48b284f5f2cb13bc492f34557de350c5fee925da02e47daf011c5c6930a721b6d3

Summary: bitcoin/bitcoin@1b699a5 --- Partial backport of Core [[bitcoin/bitcoin#16227 | PR16227]] Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix, nakihito Reviewed By: #bitcoin_abc, deadalnix, nakihito Differential Revision: https://reviews.bitcoinabc.org/D6553

Summary: bitcoin/bitcoin@c7797ec --- Depends on D6553 Partial backport of Core [[bitcoin/bitcoin#16227 | PR16227]] Test Plan: ninja check Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D6554

…cwallet where it is used Summary: bitcoin/bitcoin@a913e3f --- Depends on D6554 Partial backport of Core [[bitcoin/bitcoin#16227 | PR16227]] Test Plan: ninja check Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D6555

…SigningProvider Summary: -BEGIN VERIFY SCRIPT- git grep -l "CBasicKeyStore" | xargs sed -i -e 's/CBasicKeyStore/FillableSigningProvider/g' -END VERIFY SCRIPT- bitcoin/bitcoin@d9becff --- Depends on D6555 Partial backport of Core [[bitcoin/bitcoin#16227 | PR16227]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D6556

Summary: bitcoin/bitcoin@16f8096 --- Depends on D6556 Partial backport of Core [[bitcoin/bitcoin#16227 | PR16227]] Test Plan: ninja check Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D6557

….{cpp,h} Summary: Moves all of the various SigningProviders out of sign.{cpp,h} and keystore.{cpp,h}. As such, keystore.{cpp,h} is also removed. Includes and the Makefile are updated to reflect this. Includes were largely changed using: git grep -l "keystore.h" | xargs sed -i -e 's;keystore.h;script/signingprovider.h;g' bitcoin/bitcoin@37a79a4 --- Depends on D6557 Partial backport of Core [[bitcoin/bitcoin#16227 | PR16227]] Test Plan: In CMake build dir: ninja check-all In autotools build dir: make check-recursive ./test/functional/test_runner.py Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Subscribers: nakihito Differential Revision: https://reviews.bitcoinabc.org/D6558

…tionality into CWallet Summary: Instead of having a separate CCryptoKeyStore that handles the encryption stuff, just roll it all into CWallet. bitcoin/bitcoin@8f5b81e --- Depends on D6558 Partial backport of Core [[bitcoin/bitcoin#16227 | PR16227]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6560

…llet Summary: bitcoin/bitcoin@93ce4a0?file-filters%5B%5D=.h#diff-12635a58447c65585f51d32b7e04075bR1051 --- Depends on D6560 Concludes backport of Core [[bitcoin/bitcoin#16227 | PR16227]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Subscribers: nakihito Differential Revision: https://reviews.bitcoinabc.org/D6561

Summary: bitcoin/bitcoin@1b699a5 --- Partial backport of Core [[bitcoin/bitcoin#16227 | PR16227]] Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix, nakihito Reviewed By: #bitcoin_abc, deadalnix, nakihito Differential Revision: https://reviews.bitcoinabc.org/D6553

fanquake added Wallet Refactoring labels Jun 17, 2019

fanquake requested a review from meshcollider June 18, 2019 00:25

DrahtBot added the Needs rebase label Jun 18, 2019

achow101 force-pushed the rm-keystores branch from 83b9299 to c0f336a Compare June 19, 2019 22:09

DrahtBot added Needs rebase and removed Needs rebase labels Jun 19, 2019

achow101 force-pushed the rm-keystores branch from c0f336a to c2975f1 Compare June 21, 2019 17:54

DrahtBot removed the Needs rebase label Jun 21, 2019

maflcko reviewed Jun 21, 2019

View reviewed changes

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

achow101 force-pushed the rm-keystores branch from c2975f1 to 30c0be7 Compare June 21, 2019 19:55

maflcko reviewed Jun 24, 2019

View reviewed changes

src/wallet/crypter.h Outdated Show resolved Hide resolved

achow101 force-pushed the rm-keystores branch from 30c0be7 to aff4adf Compare June 24, 2019 14:52

achow101 force-pushed the rm-keystores branch from aff4adf to 8ce8d6a Compare June 24, 2019 18:04

ryanofsky approved these changes Jun 24, 2019

View reviewed changes

achow101 force-pushed the rm-keystores branch 2 times, most recently from 5110e8a to ad2e52c Compare June 27, 2019 14:42

maflcko mentioned this pull request Jun 28, 2019

bench: Replace <wallet/crypter.h> with <keystore.h> in src/bench/ccoins_caching.cpp #16303

Closed

achow101 force-pushed the rm-keystores branch from ad2e52c to f67758d Compare June 28, 2019 15:58

l2a5b1 reviewed Jun 30, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jul 3, 2019

achow101 mentioned this pull request Jul 5, 2019

Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) #16341

Closed

kallewoof approved these changes Jul 9, 2019

View reviewed changes

Sjors approved these changes Jul 9, 2019

View reviewed changes

Remove CKeyStore and squash into CBasicKeyStore

c7797ec

achow101 force-pushed the rm-keystores branch 2 times, most recently from 9ebe1f6 to 794ce1b Compare July 9, 2019 20:13

achow101 added 6 commits July 9, 2019 16:20

Move HaveKey static function from keystore to rpcwallet where it is used

a913e3f

scripted-diff: rename CBasicKeyStore to FillableSigningProvider

d9becff

-BEGIN VERIFY SCRIPT- git grep -l "CBasicKeyStore" | xargs sed -i -e 's/CBasicKeyStore/FillableSigningProvider/g' -END VERIFY SCRIPT-

Move KeyOriginInfo to its own header file

16f8096

Remove CCryptoKeyStore and move all of it's functionality into CWallet

8f5b81e

Instead of having a separate CCryptoKeyStore that handles the encryption stuff, just roll it all into CWallet.

Move WatchOnly stuff from SigningProvider to CWallet

93ce4a0

achow101 force-pushed the rm-keystores branch from 794ce1b to 93ce4a0 Compare July 9, 2019 20:20

instagibbs reviewed Jul 11, 2019

View reviewed changes

laanwj merged commit 93ce4a0 into bitcoin:master Jul 11, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Refactor CWallet's inheritance chain #16227

Refactor CWallet's inheritance chain #16227

Uh oh!

Conversation

achow101 commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

achow101 commented Jun 19, 2019

Uh oh!

achow101 commented Jun 21, 2019

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jun 24, 2019

Uh oh!

achow101 commented Jun 24, 2019

Uh oh!

maflcko commented Jun 24, 2019

Uh oh!

achow101 commented Jun 24, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l2a5b1 commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jun 28, 2019

Uh oh!

l2a5b1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

achow101 commented Jun 17, 2019 •

edited

Loading

DrahtBot commented Jun 18, 2019 •

edited

Loading

l2a5b1 commented Jun 28, 2019 •

edited

Loading

achow101 Jun 30, 2019 •

edited

Loading

kallewoof left a comment •

edited

Loading

Sjors left a comment •

edited

Loading

instagibbs Jul 11, 2019 •

edited

Loading