Batch write imported stuff in importmulti #15741

achow101 · 2019-04-03T23:18:01Z

Instead of writing each item to the wallet database individually, do them in batches so that the import runs faster.

This was tested by importing a ranged descriptor for 10,000 keys.

Current master

$ time src/bitcoin-cli -regtest -rpcwallet=importbig importmulti '[{"desc": "sh(wpkh([73111820/44h/1h/0h]tpubDDoT2SgEjaU5rerQpfcRDWPAcwyZ5g7xxHgVAfPwidgPDKVjm89d6jJ8AQotp35Np3m6VaysfUY1C2g68wFqUmraGbzhSsMF9YBuTGxpBaW/1/*))#3w7php47", "range": [0, 10000], "timestamp": "now", "internal": true, "keypool": false, "watchonly": true}]'
...

real	7m45.29s

This PR:

$ time src/bitcoin-cli -regtest -rpcwallet=importbig4 importmulti '[{"desc": "pkh([73111820/44h/1h/0h]tpubDDoT2SgEjaU5rerQpfcRDWPAcwyZ5g7xxHgVAfPwidgPDKVjm89d6jJ8AQotp35Np3m6VaysfUY1C2g68wFqUmraGbzhSsMF9YBuTGxpBaW/1/*)#v65yjgmc", "range": [0, 10000], "timestamp": "now", "internal": true, "keypool": false, "watchonly": true}]'
...

real	3.93s

Fixes #15739

gwillen · 2019-04-03T23:30:47Z

utACK although it would be good to factor out the counting into a WalletWriteCache or something, that flushes itself at intervals so you don't have to do it manually.

gwillen · 2019-04-03T23:31:53Z

src/wallet/rpcdump.cpp

             const CPubKey& pubkey = entry->second;
             CPubKey temp;
-             if (!pwallet->GetPubKey(id, temp) && !pwallet->AddWatchOnly(GetScriptForRawPubKey(pubkey), timestamp)) {
+             if (!pwallet->GetPubKey(id, temp) && !pwallet->AddWatchOnlyWithDB(*batch, GetScriptForRawPubKey(pubkey), timestamp)) {


There is a weird indentation glitch here, although it looks like it was preexisting.

gwillen · 2019-04-03T23:32:40Z

src/wallet/rpcdump.cpp

                pwallet->SetAddressBook(dest, label, "receive");
            }
        }
+        batch.reset();


Is this necessary? It's about to go out of scope, right?

Probably not. I did that just as a belt-and-suspenders thing.

DrahtBot · 2019-04-04T01:10:15Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15876 ([rpc] signer bump fee by Sjors)
#15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
#15424 ([wallet] wallet-tool: command to remove key metadata by Sjors)
#15414 ([wallet] allow adding pubkeys from imported private keys to keypool by Sjors)
#14912 ([WIP] External signer support (e.g. hardware wallet) by Sjors)
#13756 (wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof)

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.

Empact · 2019-04-04T09:18:16Z

Empact@dd5b0aa CWallet::AddKeyOrigin(const CPubKey& pubkey, const KeyOriginInfo& info) and CWallet::WriteKeyMetadata(const CKeyMetadata& meta, const CPubKey& pubkey, const bool overwrite) can be removed.

Empact@c5d9eec How about moving this functionality into CWallet, so that we don't operate externally on CWallet::database any more than we already do. As the GetDBHandle comments say:

bitcoin/src/wallet/wallet.h

Lines 750 to 753 in ba54342

    
               /** Get database handle used by this wallet. Ideally this function would 
        
                * not be necessary. 
        
                */ 
        
               WalletDatabase& GetDBHandle()

Practically speaking, this also explicitly limits the scope of the batch operations on database to within the lifecycle of the CWallet instance.

Empact@b363aa7 Could also apply the batch treatment to SetAddressBook

Empact · 2019-04-04T09:52:44Z

src/wallet/wallet.h


    /** Add a KeyOriginInfo to the wallet */
    bool AddKeyOrigin(const CPubKey& pubkey, const KeyOriginInfo& info);
+    bool AddKeyOriginWithDB(WalletBatch& batch, const CPubKey& pubkey, const KeyOriginInfo& info);


nit: May be a bad example to follow, but AddKeypoolPubkeyWithDB has batch as the last argument, which makes as much sense to me.

I was following the AddKeyPubKeyWithDB way of argument ordering.

Empact · 2019-04-04T09:53:11Z

Concept ACK

promag

Concept ACK, nice improvement.

promag · 2019-04-04T12:57:53Z

src/wallet/rpcdump.cpp

                 throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");
             }
             pwallet->UpdateTimeFirstKey(timestamp);
+             if (++cnt % 1000 == 0) {


What is the problem of one batch only?

The batch can get very large in memory if lots of keys are involved, so we limit the size to avoid blowing up memory during imports.

maflcko · 2019-04-04T17:46:53Z

See also:

Use a single wallet batch for UpgradeKeyMetadata Use a single wallet batch for UpgradeKeyMetadata #15433

achow101 · 2019-04-04T18:10:45Z

I've pulled in @Empact's changes (Empact/bitcoin@dd5b0aa was squashed into 59ec412, cherry-picked the other two commits).

gwillen · 2019-04-04T22:59:32Z

Especially if there are now multiple places we're manually batching writes like this, and more where we're considering it: Let me suggest again and more strongly that the batch object should be handling autoflushing after every X calls, instead of open-coding it in the caller like this. If it's appropriate for every caller, it could go into WalletBatch, else into a new class that's an extremely thin wrapper around it. I bet the total number of lines added in this PR would actually go down.

Sjors · 2019-04-05T01:32:44Z

Concept ACK. I tried the first two commits (so without address book change) with #14145 and importing 2x 1000 keys seems about 6 times faster.

Empact · 2019-04-05T04:09:19Z

@gwillen Agree it should be done but would be appropriate as a follow-up

practicalswift · 2019-04-05T11:45:05Z

Looks like AddKeypoolPubkey is no longer used and can be removed?

achow101 · 2019-04-05T19:24:36Z

I've implemented @gwillen's suggestion. WalletBatch will flush to disk automatically after every 1000 writes. This appears to be faster too as it does not close and reopen the database on every flush. With this change, I've also updated UpgradeKeyMetadata.

Looks like AddKeypoolPubkey is no longer used and can be removed?

Done

maflcko · 2019-04-05T20:29:51Z

src/wallet/wallet.cpp

in commit ebb0ea85539b3e2f515fca89231c20ac86fb52d5:

Why is this no longer needed?

When batch goes out of scope, it will write. This was just a belt and suspenders.

The comment suggests that in this case -- unlike the batch write in importmulti -- there's a specific reason to force it to write before the end of the scope.

achow101 · 2019-04-05T21:50:04Z

I'm unsure why tests are currently failing.

promag · 2019-04-05T22:08:11Z

src/wallet/walletdb.h

            return false;
        }
        m_database.IncrementUpdateCounter();
+        if (m_database.nUpdateCounter % 1000 == 0) {


I get that this makes the code simple, but IMO this defeats the batch purpose. Like, it shouldn't be something to worry about here.

I don't understand what you mean by 'defeats the batch purpose'. Are there cases where the caller is depending on the batch not being flushed?

I think it is far better engineering practice to deal with periodic flushing once, inside the batch-management code, than to deal with it independently at each callsite. If there's some situation where periodic flushing is undesirable, I think it would be better to offer both versions.

@gwillen I mean you should be able to create a batch with 1001 changes no? I was suggesting something on top of WalletBatch to do that.

Well, you can add as many changes as you want, it just flushes after every 1000. This should be completely transparent to the user. Unless there is some reason that flushing can hurt, like the caller is using the batch as though it were an atomic transaction?

Unless there is some reason that flushing can hurt, like the caller is using the batch as though it were an atomic transaction?

There is only one place where an atomic transaction is used, and if an atomic transaction is active, then Flush() will do nothing.

Well I may be wrong but I see the batch as an atomic transaction. Making 1500 updates and then aborting the batch should not commit 1000 updates IMO.

Well I may be wrong but I see the batch as an atomic transaction. Making 1500 updates and then aborting the batch should not commit 1000 updates IMO.

Except that's not how the batch works normally. Batches are not atomic unless you explicitly make them so by starting a transaction with the underlying BerkeleyBatch. In that case, then the flushing won't do anything. In the normal case, each update in a batch itself is atomic and should be expected to be written to the wallet file. There is no aborting. As soon as a batch goes out of scope, every single update is committed to the file. If you make 1500 updates, the first 1000 will be written after you do the 1000th write operation, and then the last 500 will be written when the batch object is deleted.

So is the description of WalletBatch out of date? It says This represents a single transaction at the database. It will be committed when the object goes out of scope.

I believe that is out of date

I've updated the comment.

promag · 2019-04-05T22:14:33Z

src/wallet/db.cpp

        nMinutes = 1;

-    env->dbenv->txn_checkpoint(nMinutes ? gArgs.GetArg("-dblogsize", DEFAULT_WALLET_DBLOGSIZE) * 1024 : 0, nMinutes, 0);
+    if (env) {


Why this change?

So that tests pass. When there's a dummy database, env will be nulltpr and this causes a segfault in tests which use a dummy database.

Can't we make env is defined instead? Otherwise a comment would make sense as this doesn't really happen.

Can't we make env is defined instead?

No. env is nullptr for dummy databases which are used in tests. A check like this is used in many other places in this file in the form of BerkeleyDatabase::IsDummy().

Otherwise a comment would make sense as this doesn't really happen.

Done.

nit: could deal with it earlier (i.e. early return), and use IsDummy to make it more explicit.

IsDummy is done by BerkeleyDatabase, not BerkeleyBatch so is not available to do here (although the check is completely identical).

src/wallet/wallet.cpp

achow101 · 2019-05-18T17:21:36Z

I noticed that importing scripts would also be slow so I've added AddCScriptWIthDB in order to apply the same batching performance boost to such imports.

Also updated OP.

jb55 · 2019-05-19T06:52:53Z

tACK 87f7befca04a095a06af003f9fe20d2e3c155d2f sooo fast. tested a rescan with my all my trezor keys from HWI and it worked great. thanks @achow101, hw wallet + core is actually usable now (at least for watching :)!

promag · 2019-05-19T17:06:13Z

Restarted travis, it was failing with 😕

47/121 - rpc_psbt.py failed, Duration: 5 s
stdout:
2019-05-18T17:46:43.670000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20190518_173555/rpc_psbt_71
2019-05-18T17:46:47.538000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/test_framework.py", line 194, in main
    self.run_test()
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/rpc_psbt.py", line 156, in run_test
    assert_raises_rpc_error(-22, "", self.nodes[0].converttopsbt, signedtx['hex'])
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/util.py", line 105, in assert_raises_rpc_error
    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
AssertionError: No exception raised

promag · 2019-05-19T20:36:29Z

utACK 87f7bef. nit, ampersand should be before space.

Empact · 2019-05-27T21:54:00Z

src/wallet/wallet.h

nit: could be private

Empact · 2019-05-27T21:56:00Z

src/wallet/wallet.h

nit: could be private

Empact · 2019-05-27T21:57:10Z

utACK 87f7bef

This maintains encapsulation of CWallet::database in the face of batching, e.g. allows making the `WithDB` methods private.

…PubKeys

jb55 · 2019-05-28T21:40:15Z

utACK 0db94e5

ariard · 2019-05-29T02:16:35Z

Tested ACK 0db94e5

Drop import time for 10000 keys ranged descriptor from 495.57s to 3.626s with 2.8 GHz Intel Core i7.

Empact · 2019-05-29T03:05:54Z

re-utACK 0db94e5 only change is re the privacy of UnsetWalletFlagWithDB and AddCScriptWithDB.

0db94e5 wallet: Pass WalletBatch to CWallet::UnsetWalletFlag (João Barbosa) 6cb888b Apply the batch treatment to CWallet::SetAddressBook via ImportScriptPubKeys (Ben Woosley) 6154a09 Move some of ProcessImport into CWallet::Import* (Ben Woosley) ccb26cf Batch writes for importmulti (Andrew Chow) d6576e3 Have WalletBatch automatically flush every 1000 updates (Andrew Chow) 366fe0b Add AddWatchOnlyWithDB, AddKeyOriginWithDB, AddCScriptWithDB functions (Andrew Chow) Pull request description: Instead of writing each item to the wallet database individually, do them in batches so that the import runs faster. This was tested by importing a ranged descriptor for 10,000 keys. Current master ``` $ time src/bitcoin-cli -regtest -rpcwallet=importbig importmulti '[{"desc": "sh(wpkh([73111820/44h/1h/0h]tpubDDoT2SgEjaU5rerQpfcRDWPAcwyZ5g7xxHgVAfPwidgPDKVjm89d6jJ8AQotp35Np3m6VaysfUY1C2g68wFqUmraGbzhSsMF9YBuTGxpBaW/1/*))#3w7php47", "range": [0, 10000], "timestamp": "now", "internal": true, "keypool": false, "watchonly": true}]' ... real 7m45.29s ``` This PR: ``` $ time src/bitcoin-cli -regtest -rpcwallet=importbig4 importmulti '[{"desc": "pkh([73111820/44h/1h/0h]tpubDDoT2SgEjaU5rerQpfcRDWPAcwyZ5g7xxHgVAfPwidgPDKVjm89d6jJ8AQotp35Np3m6VaysfUY1C2g68wFqUmraGbzhSsMF9YBuTGxpBaW/1/*)#v65yjgmc", "range": [0, 10000], "timestamp": "now", "internal": true, "keypool": false, "watchonly": true}]' ... real 3.93s ``` Fixes #15739 ACKs for commit 0db94e: jb55: utACK 0db94e5 ariard: Tested ACK 0db94e5 Empact: re-utACK 0db94e5 only change is re the privacy of `UnsetWalletFlagWithDB` and `AddCScriptWithDB`. Tree-SHA512: 3481308a64c99b6129f7bd328113dc291fe58743464628931feaebdef0e6ec770ddd5c19e4f9fbc1249a200acb04aaf62a8d914d53b0a29ac1e557576659c0cc

0db94e5 wallet: Pass WalletBatch to CWallet::UnsetWalletFlag (João Barbosa) 6cb888b Apply the batch treatment to CWallet::SetAddressBook via ImportScriptPubKeys (Ben Woosley) 6154a09 Move some of ProcessImport into CWallet::Import* (Ben Woosley) ccb26cf Batch writes for importmulti (Andrew Chow) d6576e3 Have WalletBatch automatically flush every 1000 updates (Andrew Chow) 366fe0b Add AddWatchOnlyWithDB, AddKeyOriginWithDB, AddCScriptWithDB functions (Andrew Chow) Pull request description: Instead of writing each item to the wallet database individually, do them in batches so that the import runs faster. This was tested by importing a ranged descriptor for 10,000 keys. Current master ``` $ time src/bitcoin-cli -regtest -rpcwallet=importbig importmulti '[{"desc": "sh(wpkh([73111820/44h/1h/0h]tpubDDoT2SgEjaU5rerQpfcRDWPAcwyZ5g7xxHgVAfPwidgPDKVjm89d6jJ8AQotp35Np3m6VaysfUY1C2g68wFqUmraGbzhSsMF9YBuTGxpBaW/1/*))#3w7php47", "range": [0, 10000], "timestamp": "now", "internal": true, "keypool": false, "watchonly": true}]' ... real 7m45.29s ``` This PR: ``` $ time src/bitcoin-cli -regtest -rpcwallet=importbig4 importmulti '[{"desc": "pkh([73111820/44h/1h/0h]tpubDDoT2SgEjaU5rerQpfcRDWPAcwyZ5g7xxHgVAfPwidgPDKVjm89d6jJ8AQotp35Np3m6VaysfUY1C2g68wFqUmraGbzhSsMF9YBuTGxpBaW/1/*)#v65yjgmc", "range": [0, 10000], "timestamp": "now", "internal": true, "keypool": false, "watchonly": true}]' ... real 3.93s ``` Fixes bitcoin#15739 ACKs for commit 0db94e: jb55: utACK 0db94e5 ariard: Tested ACK 0db94e5 Empact: re-utACK bitcoin@0db94e5 only change is re the privacy of `UnsetWalletFlagWithDB` and `AddCScriptWithDB`. Tree-SHA512: 3481308a64c99b6129f7bd328113dc291fe58743464628931feaebdef0e6ec770ddd5c19e4f9fbc1249a200acb04aaf62a8d914d53b0a29ac1e557576659c0cc

40ad2f6 Have importwallet use ImportPrivKeys and ImportScripts (Andrew Chow) 78941da Optionally allow ImportScripts to set script creation timestamp (Andrew Chow) 94bf156 Have importaddress use ImportScripts and ImportScriptPubKeys (Andrew Chow) a00d1e5 Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions (Andrew Chow) c6a8274 Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys (Andrew Chow) fae7a5b Log when an import is being skipped because we already have it (Andrew Chow) ab28e31 Change ImportScriptPubKeys' internal to apply_label (Andrew Chow) Pull request description: #15741 introduced `ImportPrivKeys`, `ImportPubKeys`, `ImportScripts`, and `ImportScriptPubKeys` in `CWallet` which are used by `importmulti`. This PR changes the remaining `import*` RPCs (`importaddress`, `importprivkey`, `importpubkey`, and `importwallet`) to use these functions as well instead of directly adding the imported items to the wallet. ACKs for top commit: MarcoFalke: ACK 40ad2f6 (checked that behavior changes are mentioned in the commit body) ryanofsky: utACK 40ad2f6. Only change since last review is a tweaked commit message (mentioning label update in importpubkey commit) Sjors: ACK 40ad2f6. Those extra tests also pass. Tree-SHA512: 910e3bbe20b6f8809a47b7293775db234125615d886c7fd99c194f4cdf00c765eb1e24b1799260f1213b98c88f9bbe696796f36087c182925e567d44e9194c98

40ad2f6 Have importwallet use ImportPrivKeys and ImportScripts (Andrew Chow) 78941da Optionally allow ImportScripts to set script creation timestamp (Andrew Chow) 94bf156 Have importaddress use ImportScripts and ImportScriptPubKeys (Andrew Chow) a00d1e5 Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions (Andrew Chow) c6a8274 Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys (Andrew Chow) fae7a5b Log when an import is being skipped because we already have it (Andrew Chow) ab28e31 Change ImportScriptPubKeys' internal to apply_label (Andrew Chow) Pull request description: bitcoin#15741 introduced `ImportPrivKeys`, `ImportPubKeys`, `ImportScripts`, and `ImportScriptPubKeys` in `CWallet` which are used by `importmulti`. This PR changes the remaining `import*` RPCs (`importaddress`, `importprivkey`, `importpubkey`, and `importwallet`) to use these functions as well instead of directly adding the imported items to the wallet. ACKs for top commit: MarcoFalke: ACK 40ad2f6 (checked that behavior changes are mentioned in the commit body) ryanofsky: utACK 40ad2f6. Only change since last review is a tweaked commit message (mentioning label update in importpubkey commit) Sjors: ACK 40ad2f6. Those extra tests also pass. Tree-SHA512: 910e3bbe20b6f8809a47b7293775db234125615d886c7fd99c194f4cdf00c765eb1e24b1799260f1213b98c88f9bbe696796f36087c182925e567d44e9194c98

Summary: * Add AddWatchOnlyWithDB, AddKeyOriginWithDB, AddCScriptWithDB functions AddWatchOnlyWithDB, AddKeyOriginWithDB, and AddCScriptWithDB add their respective data to the wallet using the provided WalletBatch instead of creating a new WalletBatch object every time. This allows for batching writes to the database. * Have WalletBatch automatically flush every 1000 updates Since it now automatically flushes, we don't need to have UpgradeKeyMetadata count and flush separately * Batch writes for importmulti When writing all of the imported data to the wallet, use a common WalletBatch object so that batch writes are done and the writes finish more quickly. AddKeypoolPubkey is no longer needed so it is also removed * Move some of ProcessImport into CWallet::Import* This maintains encapsulation of CWallet::database in the face of batching, e.g. allows making the `WithDB` methods private. * Apply the batch treatment to CWallet::SetAddressBook via ImportScriptPubKeys * wallet: Pass WalletBatch to CWallet::UnsetWalletFlag This is a backport of Core [[bitcoin/bitcoin#15741 | PR15741]] I did it all in one block even though it's quite large because intermediate commits do not work individualy. Depends on D6385 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D6387

fanquake added RPC/REST/ZMQ Wallet labels Apr 3, 2019

gwillen reviewed Apr 3, 2019

View reviewed changes

Empact reviewed Apr 4, 2019

View reviewed changes

promag reviewed Apr 4, 2019

View reviewed changes

achow101 force-pushed the batch-write-importmulti branch from 508c486 to 7455c9d Compare April 4, 2019 18:09

achow101 force-pushed the batch-write-importmulti branch from 7455c9d to 27fdf12 Compare April 5, 2019 19:22

achow101 force-pushed the batch-write-importmulti branch from 27fdf12 to 72ec512 Compare April 5, 2019 19:57

maflcko reviewed Apr 5, 2019

View reviewed changes

achow101 force-pushed the batch-write-importmulti branch from 72ec512 to f2b9b32 Compare April 5, 2019 22:01

promag reviewed Apr 5, 2019

View reviewed changes

achow101 force-pushed the batch-write-importmulti branch from f2b9b32 to 2811f37 Compare April 5, 2019 22:42

achow101 mentioned this pull request Apr 6, 2019

Native descriptor wallets #15764

Closed

achow101 force-pushed the batch-write-importmulti branch from 2811f37 to ffafc25 Compare April 8, 2019 17:02

DrahtBot added the Needs rebase label Apr 9, 2019

achow101 force-pushed the batch-write-importmulti branch from ffafc25 to a14a5ba Compare April 9, 2019 14:53

DrahtBot removed the Needs rebase label Apr 9, 2019

Empact reviewed Apr 10, 2019

View reviewed changes

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

Empact reviewed May 27, 2019

View reviewed changes

Empact and others added 3 commits May 28, 2019 11:03

Move some of ProcessImport into CWallet::Import*

6154a09

This maintains encapsulation of CWallet::database in the face of batching, e.g. allows making the `WithDB` methods private.

Apply the batch treatment to CWallet::SetAddressBook via ImportScript…

6cb888b

…PubKeys

wallet: Pass WalletBatch to CWallet::UnsetWalletFlag

0db94e5

achow101 force-pushed the batch-write-importmulti branch from 87f7bef to 0db94e5 Compare May 28, 2019 15:04

meshcollider merged commit 0db94e5 into bitcoin:master May 29, 2019

achow101 mentioned this pull request Jun 28, 2019

Use CWallet::Import* functions in all import* RPCs #16301

Merged

chris-belcher mentioned this pull request Sep 21, 2019

Commit 4b4f8c9 slows down makers and causes regtest coinjoins to fail JoinMarket-Org/joinmarket-clientserver#401

Closed

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

Batch write imported stuff in importmulti #15741

Batch write imported stuff in importmulti #15741

Uh oh!

Conversation

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

Uh oh!

Uh oh!

gwillen commented Apr 3, 2019

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!

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

Uh oh!

Conflicts

Uh oh!

Empact commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact commented Apr 4, 2019

Uh oh!

promag 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!

maflcko commented Apr 4, 2019

Uh oh!

achow101 commented Apr 4, 2019

Uh oh!

gwillen commented Apr 4, 2019

Uh oh!

Sjors commented Apr 5, 2019

Uh oh!

Empact commented Apr 5, 2019

Uh oh!

practicalswift commented Apr 5, 2019

Uh oh!

achow101 commented Apr 5, 2019

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!

achow101 commented Apr 5, 2019

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!

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

achow101 commented Apr 3, 2019 •

edited

Loading

DrahtBot commented Apr 4, 2019 •

edited

Loading

Empact commented Apr 4, 2019 •

edited

Loading

promag Apr 8, 2019 •

edited

Loading

Empact Apr 10, 2019 •

edited

Loading

jb55 commented May 19, 2019 •

edited

Loading