Remove wallet -> node global function calls #15288

ryanofsky · 2019-01-30T02:48:27Z

This change removes wallet calls to node functions that access global chain and mempool state.

This is the next step in the larger #10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in #10102.

DrahtBot · 2019-01-30T03:38:08Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15491 (wallet: Improve log output for errors during load by gwillen)
#15487 ([WIP] descriptor based wallet by Sjors)
#15329 (Fix InitError() and InitWarning() content by hebasto)
#15253 (Net: Consistently log outgoing INV messages by Empact)
#15157 (rpc: Bumpfee units change, satoshis to BTC by benthecarman)
#15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)
#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
#14641 (rpc: Add min_conf option to fund transaction calls by promag)
#13756 (wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)
#10823 (Allow all mempool txs to be replaced after a configurable timeout (default 6h) by greenaddress)
#9381 (Remove CWalletTx merging logic from AddToWallet 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.

src/interfaces/node.cpp

src/wallet/wallet.cpp

src/wallet/rpcwallet.cpp

src/wallet/wallet.cpp

jnewbery · 2019-01-30T15:41:11Z

Agree with @hebasto that the indentations should be fixed since those are misaligned by this PR. Not sure about fixing if braces - I would expect that it's contributors choice whether they fix existing style in lines they touch, but they shouldn't be obligated.

git diff -U0 5ff7dc424~.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v throws up a couple more style suggestions if you're interested.

I'll wait for Russ's response to the style comments before doing a full review.

ryanofsky · 2019-01-30T16:11:37Z

I'll wait for Russ's response to the style comments before doing a full review.

Will look more closely but probably I'll fix whitespace in the existing commits and add braces in a new commit.

jnewbery · 2019-01-30T16:49:05Z

Obvious Concept ACK

Empact · 2019-01-30T19:38:53Z

src/interfaces/chain.cpp

@@ -25,6 +26,8 @@
 #include <memory>
 #include <utility>

+class CReserveScript;


unnecessary given this is declared in the header

unnecessary given this is declared in the header

This was added by IWYU because CReserveScript is only forward declared in the headers included here, and defined in a different header. In theory this allows the other headers to change without breaking this file. I don't think IWYU rationale is completely airtight, but I like the IWYU tool, and don't personally think it's worth spending effort to override it if it isn't creating a problem.

Aside: Perhaps IWYU can be included in doc/developer-notes.md

Agree, and it should come with steps to install for Bitcoin Core.

Agree, and it should come with steps to install for Bitcoin Core.

It's not very convenient, but here are steps I've followed previously: #11878 (comment)

Empact · 2019-01-30T19:50:36Z

src/interfaces/chain.h

@@ -5,7 +5,8 @@
 #ifndef BITCOIN_INTERFACES_CHAIN_H
 #define BITCOIN_INTERFACES_CHAIN_H

-#include <optional.h>
+#include <optional.h>               // For Optional and nullopt


nit: IMO we could do without this comment

nit: IMO we could do without this comment

Not clear what change is being asked for. It would seem inconsistent to delete this comment and keep other comments around it.

Empact · 2019-01-30T19:54:14Z

src/interfaces/chain.h

@@ -138,6 +139,9 @@ class Chain

    //! Get virtual transaction size.
    virtual int64_t getVirtualTransactionSize(const CTransaction& tx) = 0;
+
+    //! Check if transaction is RBF opt in.
+    virtual RBFTransactionState isRBFOptIn(const CTransaction& tx) = 0;


Could return the status string and thus avoid the include / simplify the interface.

Could return the status string and thus avoid the include / simplify the interface.

Include could be removed with a forward declaration in the future. String would make the interface more cumbersome and generate JSONRPC error text in non-RPC code.

Call me enum fanboy, but I like to avoid using strings when enums suffice, and let the RPC & GUI deal with rendering them as strings. So agree with keeping it as is.

Empact · 2019-01-30T20:04:33Z

src/Makefile.bench.include

@@ -43,6 +43,10 @@ bench_bench_bitcoin_LDADD = \
  $(LIBBITCOIN_UTIL) \
  $(LIBBITCOIN_CONSENSUS) \
  $(LIBBITCOIN_CRYPTO) \
+  $(LIBBITCOIN_WALLET) \
+  $(LIBBITCOIN_SERVER) \
+  $(EVENT_PTHREADS_LIBS) \


Could you explain these? Compiles successfully with just the EVENT_LIBS on my machine.

Could you explain these? Compiles successfully with just the EVENT_LIBS on my machine.

I didn't take the time go back and see why these were needed, but I dropped the wallet and server lines. We have circular dependencies so the need to repeat libraries on the linker command line comes and goes as dependencies between individual object files change: #14437 (comment)

Empact · 2019-01-30T21:44:51Z

src/wallet/wallet.h

@@ -99,8 +99,6 @@ class CCoinControl;
 class COutput;
 class CReserveKey;
 class CScript;
-class CTxMemPool;
-class CBlockPolicyEstimator;


note: last use removed in d97fe20

src/interfaces/chain.cpp

Empact · 2019-01-30T21:58:45Z

src/interfaces/chain.cpp

@@ -258,6 +259,7 @@ class ChainImpl : public Chain
    void initMessage(const std::string& message) override { ::uiInterface.InitMessage(message); }
    void initWarning(const std::string& message) override { InitWarning(message); }
    void initError(const std::string& message) override { InitError(message); }
+    void loadWallet(std::unique_ptr<Wallet> wallet) override { ::uiInterface.LoadWallet(wallet); }


Should this be in interfaces/node? That's where other ::uiInterface access is.

Should this be in interfaces/node? That's where other ::uiInterface access is.

Previous comment #15288 (comment) about uiInterface also applies here, but in the future, the initMessage, initWarning, initError, and loadWallet methods will likely move to a new interface (maybe interfaces::WalletClient or interfaces::Ui) to allow the bitcoin-gui process to connect directly to the bitcoin-wallet process without going through bitcoin-node.

I didn't start down that road yet because ostensibly there are a lot of other ways the Chain object could be broken up, and in general I think it's less confusing to have fewer class types with more methods than more class types with fewer methods. Especially if instances would have the same cardinality and just be sitting side by side anyway.

Thanks for the explanation.

the initMessage, initWarning, initError, and loadWallet methods will likely move to a new interface

I though it could be interfaces::Wallet, the CreateWalletFromFile caller would get the wallet interface asap, then request the loading etc and then signal the loading for everyone else.

Empact · 2019-01-30T22:01:12Z

src/interfaces/chain.cpp

@@ -251,6 +252,9 @@ class ChainImpl : public Chain
    bool getPruneMode() override { return ::fPruneMode; }
    bool p2pEnabled() override { return g_connman != nullptr; }
    int64_t getAdjustedTime() override { return GetAdjustedTime(); }
+    void initMessage(const std::string& message) override { ::uiInterface.InitMessage(message); }
+    void initWarning(const std::string& message) override { InitWarning(message); }
+    void initError(const std::string& message) override { InitError(message); }


Should this be in interfaces/node? That's where other ::uiInterface access is.

Should this be in interfaces/node? That's where other ::uiInterface access is.

In #10102 there are three processes: bitcoin-node, bitcoin-wallet, and bitcoin-gui. interfaces::Node is used by the bitcoin-gui process to control the bitcoin-node process, and interfaces::Chain is used by the bitcoin-wallet process to interact with the bitcoin-node process (mainly by receiving blocks and transactions).

The uiInterface variable is used in both NodeImpl and ChainImpl classes because they both execute in the same bitcoin-node process where uiInterface is valid.

Empact · 2019-01-30T22:01:45Z

Concept ACK

hebasto · 2019-01-30T22:57:55Z

Concept ACK.

Suggested bitcoin#15288 (review)

jnewbery

utACK ba426f5. This is a great tidyup.

There are lots of commits This could be split into smaller PRs, but they're all very straightforward changes so I think it's fine to keep as one PR.

I have a few questions and suggested changes inline but none should hold up merge.

Very tidy work Russ. Thanks for your persistence with this!

src/interfaces/chain.cpp

jnewbery · 2019-02-06T15:57:37Z

src/interfaces/chain.h

+        virtual bool checkFinalTx(const CTransaction& tx) = 0;
+
+        //! Add transaction to memory pool.
+        virtual bool acceptToMemoryPool(CTransactionRef tx, CValidationState& state) = 0;


Suggestion: consider renaming this interface function submitToMemoryPool(). The wallet cannot itself accept a transaction to the memory pool, it can only submit it to the node, which will decide whether to accept the tx. I think changing the naming reinforces the division between wallet and node.

Interested to hear your thoughts on this. If you agree, we can push the naming change to a future PR.

re: #15288 (comment)

Suggestion: consider renaming this interface function submitToMemoryPool()

Makes sense, changed.

jnewbery · 2019-02-06T16:14:45Z

src/interfaces/chain.cpp

+    {
+        LockAnnotation lock(::cs_main);
+        return AcceptToMemoryPool(::mempool, state, tx, nullptr /* missing inputs */, nullptr /* txn replaced */,
+            false /* bypass limits */, ::maxTxFee /* absurd fee */);


I have a slight concern about the maxTxFee being set by the node instead of the wallet here, but that's more of a long-term design question (and is inherited from having a single maxTxFee for both the raw transaction RPCs and wallet RPCs). This PR simply moves the existing function calls to the interface classes. The API can be improved later.

Longer term, the single -maxtxfee option should not be shared between the node and wallet. See #15355 for more details.

re: #15288 (comment)

I replaced ::maxTxFee with an absurd_fee parameter here to help with goal of having separate settings. I think this also makes the current behavior more clear.

jnewbery · 2019-02-06T19:30:59Z

src/interfaces/chain.cpp

+    }
+    bool relayTransaction(const uint256& txid) override
+    {
+        if (g_connman) {


Are we ever expecting g_conman to be false? (except perhaps in the unit tests?)

In any case, you could make this a bit tidier with:

if (g_conman == nullptr) return false ...

re: https://github.com/bitcoin/bitcoin/pull/15288/files#r254417716

In commit "Remove use of g_connman / PushInventory in wallet code" (9cd8d6a)

Are we ever expecting g_conman to be false? (except perhaps in the unit tests?)

I don't think so, but it does seem reasonable for a test to leave g_connman set to null.

This is just a cut and paste of the previous code, so that's where the check comes from.

In any case, you could make this a bit tidier

Applied suggestion

jnewbery · 2019-02-06T19:33:19Z

src/wallet/wallet.cpp

-                {
-                    pnode->PushInventory(inv);
-                });
+            if (pwallet->chain().relayTransaction(GetHash())) {


could just:

return pwallet->chain().relayTransaction(GetHash())

here

EDIT: in fact, after ff0c6da, you could just change the conditional above to:

if (InMempool() || AcceptToMemoryPool(locked_chain, state) && pwallet->chain().p2pEnabled()) { pwallet->chain().relayTransaction(GetHash()); }

and drop the return value from relayTransaction() (which doesn't get called anywhere else)

re: #15288 (comment)

could just

Nice suggestions, they do make things clearer and I think I applied them all.

jnewbery · 2019-02-06T20:54:08Z

src/interfaces/chain.cpp

+        auto nLimitAncestorSize = gArgs.GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT) * 1000;
+        auto nLimitDescendants = gArgs.GetArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT);
+        auto nLimitDescendantSize = gArgs.GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT) * 1000;
+        std::string errString;


The return value of errString is unused. A future PR could rename to dummy_string or add a comment. Not required for this move-only commit.

re: #15288 (comment)

The return value of errString is unused. A future PR could rename to dummy_string or add a comment. Not required for this move-only commit.

Applied suggestion here. This wasn't 100% move only anyway, because size_t/auto change (I think made in response to practicalswift signed/unsigned warnings).

jnewbery · 2019-02-06T21:01:42Z

src/rpc/mining.h

@@ -13,6 +13,6 @@
 UniValue generateBlocks(std::shared_ptr<CReserveScript> coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript);

 /** Check bounds on a command line confirm target */
-unsigned int ParseConfirmTarget(const UniValue& value);
+unsigned int ParseConfirmTarget(const UniValue& value, unsigned int max_target);


Wow. This should be moved into rpc/util (in a future PR). (also comment is outdated!)

#15288 (comment)

Wow. This should be moved into rpc/util (in a future PR). (also comment is outdated!)

Done in #15373.

jnewbery · 2019-02-06T21:13:46Z

src/interfaces/chain.cpp

+    {
+        return ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
+    }
+    CFeeRate poolMinFee() override


nit: can you name this function mempoolMinFee()?

re: #15288 (comment)

nit: can you name this function mempoolMinFee()?

Renamed

jnewbery · 2019-02-06T21:38:31Z

src/interfaces/chain.h

+    //! Send init error.
+    virtual void initError(const std::string& message) = 0;
+
+    //! Send wallet load notification.


nit: change to "Send wallet load notification to the GUI."

re: #15288 (comment)

nit: change to "Send wallet load notification to the GUI."

Changed

jnewbery · 2019-02-06T21:46:31Z

src/interfaces/node.cpp

@@ -270,7 +270,7 @@ class NodeImpl : public Node
    }
    std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override
    {
-        return MakeHandler(::uiInterface.LoadWallet_connect([fn](std::shared_ptr<CWallet> wallet) { fn(MakeWallet(wallet)); }));
+        return MakeHandler(::uiInterface.LoadWallet_connect([fn](std::unique_ptr<Wallet>& wallet) { fn(std::move(wallet)); }));


It's not obvious to me why this (and associated changes) is necessary. Can you perhaps add commentary to the commit message why you're changing the function signature here?

re: #15288 (comment)

It's not obvious to me why this (and associated changes) is necessary

I'll add information to the commit message, but just to give some feedback now, the idea is that in #10102 the CWallet class is internal to the bitcoin-wallet process. The other bitcoin-node and bitcoin-gui processes don't use CWallet at all, and only access the wallet through interface classes (interfaces::ChainClient and interfaces::Wallet respectively).

The change from shared_ptr to unique_ptr here just follows because CWallet is used with shared_ptr, while the interface classes have more defined lifetimes and don't need shared_ptr. Any place an interface argument or return value is passed it's always by reference or with unique_ptr.

re: #15288 (comment)

Can you perhaps add commentary to the commit message why you're changing the function signature here?

Added description in f40de05

jonasschnelli · 2019-02-07T06:23:31Z

Concept ACK

Util is a better home since it's called both by wallet and mining code. Suggested bitcoin#15288 (comment)

Suggested bitcoin#15288 (review)

50e6472 Move ParseConfirmTarget from rpc/mining to rpc/util (Russell Yanofsky) Pull request description: Util is a better home since it's called both by wallet and mining code. Suggested bitcoin#15288 (comment) Tree-SHA512: 4320caf2a3f70d2885c421de04f2ec68ff3f6519258c5155fc46e245dc1765fd15c81f260af5096318f24ff9deb88fc3c5ef40eec8b7393f467f5b963d17215b

7a9046e [wallet] Refactor CWalletTx::RelayWalletTransaction() (John Newbery) Pull request description: Refactor `CWalletTx::RelayWalletTransaction()` function. This was a suggestion from the wallet-node separation PR: bitcoin#15288 (comment), which we deferred until after the main PR was merged. There are also makes two minor behavior changes: - no longer assert if fBroadcastTransactions is false. Just return false from the function. - no longer print the relay message if p2pEnabled is set to false (since the transaction is not actually relayed). ACKs for commit 7a9046: promag: utACK 7a9046e. MeshCollider: utACK bitcoin@7a9046e ryanofsky: utACK 7a9046e. No changes at all, just rebase after base PR bitcoin#15632 was merged Tree-SHA512: 2ae6214cfadd917a1b3a892c4277e5e57c3eb791e17f67511470e6fbc634d19356554b9f9c55af6b779fdef821914aad59b7cc9e6c13ece145df003bf507d486

merge bitcoin#10973, bitcoin#15039, bitcoin#15288: separate wallet from node

…n#15288 4d4e4c6 Suggested interfaces::Chain cleanups from bitcoin#15288 (Russell Yanofsky) Pull request description: Mostly documentation improvements requested in the last review of bitcoin#15288 before it was merged (bitcoin#15288 (review)) Tree-SHA512: 64e912520bbec20a44032f265a8cf3f11ad7f5126c8626b5ad5e888227b1f92ecb321522fab4bbbd613230b55450abd6ace023631d0a4f357a780d65c5638bfe

7a9046e [wallet] Refactor CWalletTx::RelayWalletTransaction() (John Newbery) Pull request description: Refactor `CWalletTx::RelayWalletTransaction()` function. This was a suggestion from the wallet-node separation PR: bitcoin#15288 (comment), which we deferred until after the main PR was merged. There are also makes two minor behavior changes: - no longer assert if fBroadcastTransactions is false. Just return false from the function. - no longer print the relay message if p2pEnabled is set to false (since the transaction is not actually relayed). ACKs for commit 7a9046: promag: utACK 7a9046e. MeshCollider: utACK bitcoin@7a9046e ryanofsky: utACK 7a9046e. No changes at all, just rebase after base PR bitcoin#15632 was merged Tree-SHA512: 2ae6214cfadd917a1b3a892c4277e5e57c3eb791e17f67511470e6fbc634d19356554b9f9c55af6b779fdef821914aad59b7cc9e6c13ece145df003bf507d486

50e6472 Move ParseConfirmTarget from rpc/mining to rpc/util (Russell Yanofsky) Pull request description: Util is a better home since it's called both by wallet and mining code. Suggested bitcoin#15288 (comment) Tree-SHA512: 4320caf2a3f70d2885c421de04f2ec68ff3f6519258c5155fc46e245dc1765fd15c81f260af5096318f24ff9deb88fc3c5ef40eec8b7393f467f5b963d17215b

ryanofsky mentioned this pull request Jan 30, 2019

Refactor: separate wallet from node #10973

Merged

fanquake added Refactoring Wallet labels Jan 30, 2019

hebasto requested changes Jan 30, 2019

View reviewed changes

ryanofsky mentioned this pull request Jan 30, 2019

Remove uses of chainActive and mapBlockIndex in wallet code #14711

Merged

Empact reviewed Jan 30, 2019

View reviewed changes

src/interfaces/chain.cpp Outdated Show resolved Hide resolved

Empact reviewed Jan 30, 2019

View reviewed changes

ryanofsky mentioned this pull request Jan 31, 2019

Factor out PSBT utilities from RPCs for use in GUI code; related refactoring. #14978

Merged

ryanofsky force-pushed the pr/wchain3 branch from 4f851c9 to ba426f5 Compare February 4, 2019 17:12

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 4, 2019

Change brace formatting

ba426f5

Suggested bitcoin#15288 (review)

jnewbery reviewed Feb 6, 2019

View reviewed changes

ryanofsky mentioned this pull request Feb 6, 2019

[RFC] Long term plan for wallet command-line args #13044

Closed

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 8, 2019

Move ParseConfirmTarget from rpc/mining to rpc/util

c706207

Util is a better home since it's called both by wallet and mining code. Suggested bitcoin#15288 (comment)

ryanofsky mentioned this pull request Feb 8, 2019

Move ParseConfirmTarget from rpc/mining to rpc/util #15373

Merged

ryanofsky force-pushed the pr/wchain3 branch from ba426f5 to 515e70d Compare February 8, 2019 22:32

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 8, 2019

Change brace formatting

515e70d

Suggested bitcoin#15288 (review)

ryanofsky force-pushed the pr/wchain3 branch from 515e70d to f5d3f6b Compare February 8, 2019 22:38

kwvg added a commit to kwvg/dash that referenced this pull request Oct 16, 2021

partial bitcoin#15288: Remove wallet -> node global function calls

98e1bd3

kwvg mentioned this pull request Oct 16, 2021

merge bitcoin#10973: separate wallet from node dashpay/dash#4513

Closed

kwvg added a commit to kwvg/dash that referenced this pull request Oct 16, 2021

partial bitcoin#15288: Remove wallet -> node global function calls

81af149

kwvg pushed a commit to kwvg/dash that referenced this pull request Oct 31, 2021

partial bitcoin#15288: Remove wallet -> node global function calls

af2eb8c

kwvg added a commit to kwvg/dash that referenced this pull request Oct 31, 2021

partial bitcoin#15288: Remove wallet -> node global function calls

d9f19ac

kwvg added a commit to kwvg/dash that referenced this pull request Oct 31, 2021

merge bitcoin#15288: Remove wallet -> node global function calls

d4a513b

kwvg mentioned this pull request Nov 1, 2021

merge bitcoin#10973, #15039, #15288: separate wallet from node dashpay/dash#4558

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Nov 1, 2021

merge bitcoin#15288: Remove wallet -> node global function calls

94b013c

kwvg added a commit to kwvg/dash that referenced this pull request Nov 4, 2021

merge bitcoin#15288: Remove wallet -> node global function calls

b0e70a9

kwvg added a commit to kwvg/dash that referenced this pull request Nov 4, 2021

merge bitcoin#15288: Remove wallet -> node global function calls

fbb268d

kwvg added a commit to kwvg/dash that referenced this pull request Nov 6, 2021

merge bitcoin#15288: Remove wallet -> node global function calls

1f28839

kwvg added a commit to kwvg/dash that referenced this pull request Nov 14, 2021

merge bitcoin#15288: Remove wallet -> node global function calls

cdc980c

kwvg added a commit to kwvg/dash that referenced this pull request Nov 14, 2021

merge bitcoin#15288: Remove wallet -> node global function calls

f7a69ed

kwvg added a commit to kwvg/dash that referenced this pull request Nov 14, 2021

merge bitcoin#15288: Remove wallet -> node global function calls

cdac124

kwvg added a commit to kwvg/dash that referenced this pull request Nov 14, 2021

merge bitcoin#15288: Remove wallet -> node global function calls

2c98320

UdjinM6 added a commit to dashpay/dash that referenced this pull request Nov 15, 2021

Merge pull request #4558 from kittywhiskers/wnsep

a3a8bfe

merge bitcoin#10973, bitcoin#15039, bitcoin#15288: separate wallet from node

pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021

merge bitcoin#15288: Remove wallet -> node global function calls

d6f9339

bitcoin locked and limited conversation to collaborators Nov 8, 2022

Remove wallet -> node global function calls #15288

Remove wallet -> node global function calls #15288

Uh oh!

Conversation

ryanofsky commented Jan 30, 2019

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Jan 30, 2019

Uh oh!

ryanofsky commented Jan 30, 2019

Uh oh!

jnewbery commented Jan 30, 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Empact Jan 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!

ryanofsky Feb 3, 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!

Empact Jan 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!

Empact commented Jan 30, 2019

Uh oh!

hebasto commented Jan 30, 2019

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 30, 2019 •

edited

Loading

Empact Jan 30, 2019 •

edited

Loading

ryanofsky Feb 3, 2019 •

edited

Loading

Empact Jan 30, 2019 •

edited

Loading

jnewbery left a comment •

edited

Loading