wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. #25005

furszy · 2022-04-27T15:09:06Z

This started in #24845 but grew out of scope of it.

So, points tackled:

Avoid extra GetWalletTx lookups inside AvailableCoins -> IsSpentKey.
IsSpentKey was receiving the tx hash and index to internally lookup the tx inside the wallet's map. As all the IsSpentKey function callers already have the wtx available, them can provide the scriptPubKey directly.
Most of the time, we call Wallet::AvailableCoins, and later on the process, skip the non-spendable coins from the result in subsequent for-loops. So to speedup the process: introduced the ability to filter by "only_spendable" coins inside Wallet::AvailableCoins directly.
(the non-spendable coins skip examples are inside AttemptSelection->GroupOutputs and GetAvailableBalance).
Refactored AvailableCoins in several ways:

a) Now it will return a new struct CoinsResult instead of receiving the vCoins vector reference (which was being cleared at the beginning of the method anyway). --> this is coming from wallet: return error msg for "too-long-mempool-chain" #24845 but cherry-picked it here too to make the following commits look nicer.

b) Unified all the 'wtx.tx->vout[I]' calls into a single call (coming from this comment wallet: Improve AvailableCoins performance by reducing duplicated operations #24699 (comment)).
The wallet IsLockedCoin and IsSpent methods now accept an OutPoint instead of a hash:index. Which let me cleanup a bunch of extra code.
Speeded up the wallet 'GetAvailableBalance': filtering AvailableCoins by spendable outputs only and using the 'AvailableCoins' retrieved total_amount instead of looping over all the retrieved coins once more.

Side topic, all this process will look even nicer with #25218

DrahtBot · 2022-04-28T02:00:32Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25269 (wallet: re-activate the not triggered "AmountWithFeeExceedsBalance" error by furszy)
#25183 (rpc: Segwit-only inputs option for fundrawtransaction by aureleoules)
#24699 (wallet: Improve AvailableCoins performance by reducing duplicated operations by achow101)
#24584 (wallet: avoid mixing different OutputTypes during coin selection by josibake)

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.

jonatack · 2022-04-28T10:45:43Z

@furszy It looks like the qt wallet test is failing in the CI. You can run ./src/qt/test/test_bitcoin-qt in your dev environment to reproduce and debug the issue.

furszy · 2022-04-28T14:01:29Z

Ah, thanks @jonatack 👌. Just noticed that the GUI WalletTests isn't run by default in my OS under make check.

It's an easy fix; as the test doesn't start the balance polling timer, the model does not have the balance cached so.. as 5babfb7 make uses of it, the creation process fails with a not-enough balance error.

Will push the fix and expand + improve the test coverage a bit more.
And.. most likely will decouple that last commit into a separate PR in few days. Have some further improvements locally for the wallet model area that can package out all together that aren't really part of the scope of this PR.
(like a mutex to be able to access the model cached balance from different threads, connect it to the widgets etc..)

furszy · 2022-05-09T20:58:06Z

The GUI WalletModel::prepareTransaction will now use the cached balance to make the funds availability check if the user didn't manually selected outputs to spend. This will avoid an entire on-demand recalculation of the available balance which is quite expensive as the wallet has to walk through the entire txs map.

Dropped the last two commits that were implementing point (6) described above. They are now part of bitcoin-core/gui#598

achow101

I don't quite understand the purpose of the first commit. It doesn't seem like it materially improves anything?

achow101 · 2022-05-16T20:23:45Z

src/wallet/spend.cpp

-    }
-    return balance;
+    // Copy to filter by spendable coins only
+    CCoinControl coinControl = _coinControl ? *_coinControl : CCoinControl();


In 2cdd2a1 "wallet: speedup 'GetAvailableBalance' filtering by spendable outputs only and using the 'AvailableCoins' retrieved total_amount."

What is the purpose of the rename and making an empty CCoinControl here? AvailableCoins can take nullptr for coinControl.

The previous flow was:

std::vector<COutput> vCoins; AvailableCoins(wallet, vCoins, coinControl); for (const COutput& out : vCoins) { if (out.spendable) { --> // this is the reason balance += out.txout.nValue; } }

To remove the second unnecessary for-loop, as the function arg is const CCoinControl*, in order to continue having the same calculation (skipping the non-spendable coins), needed to create a local copy of the coinControl arg and set the new flag m_include_only_spendable_outputs to true (which by default is false).

So the coins vector and the total balance returned by AvailableCoins has all, and only, the spendable coins directly.

In the new flow:

CCoinControl coinControl = _coinControl ? *_coinControl : CCoinControl(); coinControl.m_include_only_spendable_outputs = true; // --> this enforces that non-spendable coins are filtered from `AvailableCoins` result. return AvailableCoins(wallet, &coinControl).total_amount;

furszy · 2022-05-21T17:54:51Z

Hey achow101, thanks for the review!

I don't quite understand the purpose of the first commit. It doesn't seem like it materially improves anything?

We are skipping the non-spendable coins that appear in vCoins (AvailableCoins result) later, in several parts of the CreateTransaction and GetBalance flows:

GetAvailableBalance was (1) getting all the available coins calling AvailableCoins and, right away, walking through the entire vector, skipping the non-spendable coins, to calculate the total balance. 
Inside CreateTransactionInternal —> SelectCoins(vCoins,...), we have several calls to AttemptSelection which, on each of them internally, we call twice to GroupOutputs which internally has two for-loops over the entire vCoins vector that skip the non-spendable coins.

So, the purpose is not add the non-spendable coins into the AvailableCoins result (vCoins) in the first place for the processes that aren’t using them at all, so we don’t waste resources skipping them later so many times.

Note: this speedup is for all the processes that calls to CreateTransaction and GetBalance* internally.

furszy · 2022-05-23T18:18:26Z

Rebased after #25083 merge. Conflicts solved.

achow101 · 2022-06-02T15:53:03Z

src/wallet/rpc/spend.cpp

@@ -395,6 +396,8 @@ RPCHelpMan sendmany()
        coin_control.m_signal_bip125_rbf = request.params[5].get_bool();
    }

+    coin_control.m_include_only_spendable_outputs = true;


In 3082f22 "wallet: add 'm_include_only_spendable_outputs' filter to CoinControl"

Instead of setting this parameter all the way out here in the RPC, I think it would make more sense to set it closer to the places where it actually matters, e.g. in CreateTransactionInternal right before AvailableCoins is called. This way we won't have to remember to always set this option for every caller to CreateTransaction.

I was doing it there first but the CCoinControl arg of CreateTransactionInternal is const. Options were (1) modify it by copying the coin control object to a temp variable or (2) change the function signature.

And at least in this case, I wasn't totally good with those two options (not against neither, it was a "maybe"). But open for new thoughts, would you prefer option 1 more?

Actually, since this option is really just for AvailableCoins itself, it might make more sense to make it a parameter to AvailableCoins rather than having it in CCoinControl.

👍🏼 , added it in 8daa58b.

src/wallet/spend.cpp

src/wallet/receive.cpp

furszy · 2022-06-02T18:06:07Z

Ready, feedback applied:

Moved only_spendable from be a CoinControl field to be a AvailableCoins param.
Nits tackled.

theStack · 2022-06-05T20:18:45Z

Concept ACK

theStack

LGTM, changes all make sense to me, also thanks to chat we had in IRC :)

Left a few nits below. Also, can you change the commit texts to have a shorter line length? This makes the git history more readable. Breaking at 72-80 characters seem to be good values.

src/wallet/spend.h

src/wallet/receive.cpp

theStack · 2022-06-07T17:05:25Z

src/wallet/spend.cpp

+    return AvailableCoins(wallet,
+                          coinControl,
+                          std::nullopt, /*feerate=*/
+                          1,            /*nMinimumAmount*/
+                          MAX_MONEY,    /*nMaximumAmount*/
+                          MAX_MONEY,    /*nMinimumSumAmount*/
+                      
9E88
    0,            /*nMaximumCount*/
+                          true          /*spendable_only*/
+                          ).total_amount;


Not tested, but would it make sense to set the default parameter of spendable_only of AvailableCoins to true rather than false? Then this call would simply be

Suggested change

return AvailableCoins(wallet,

coinControl,

std::nullopt, /*feerate=*/

1, /*nMinimumAmount*/

MAX_MONEY, /*nMaximumAmount*/

MAX_MONEY, /*nMinimumSumAmount*/

0, /*nMaximumCount*/

true /*spendable_only*/

).total_amount;

return AvailableCoins(wallet, coinControl).total_amount;

As far as I can see, there are only 4 instances in total that call AvailableCoins, so it should be quite easy to find out where false has to be passed explicitly for the spendable_only parameter.

You found an existing issue in the RPC sendall command :).

When send_max is set, sendall uses all the coins returned from AvailableCoins as inputs for a new transaction, without checking if the coin are spendable/solvable first.

So.. this command should be always failing in master if the user sets the send_max param and the wallet has at least one non-spendable or non-solvable coin.

Note:
This is easily solved passing the new spendable_only=false flag to AvailableCoins here.

Instead of accepting a `vCoins` reference that is cleared at the beginning of the method. Note: This new struct, down the commits line, will contain other `AvailableCoins` useful results.

This will be used in a follow-up commit to prevent extra 'GetWalletTx' lookups if the function caller already have the wtx and can just provide the scriptPubKey directly.

We are skipping the non-spendable coins that appear in vCoins ('AvailableCoins' result) later, in several parts of the CreateTransaction and GetBalance flows: GetAvailableBalance (1) gets all the available coins calling AvailableCoins and, right away, walk through the entire vector, skipping the non-spendable coins, to calculate the total balance. Inside CreateTransactionInternal —> SelectCoins(vCoins,...), we have several calls to AttemptSelection which, on each of them internally, we call twice to GroupOutputs which internally has two for-loops over the entire vCoins vector that skip the non-spendable coins. So, Purpose is not add the non-spendable coins into the AvailableCoins result (vCoins) in the first place for the processes that aren’t using them at all, so we don’t waste resources skipping them later so many times. Note: this speedup is for all the processes that call to CreateTransaction and GetBalance* internally.

…le coin Filtering `AvailableCoins` by spendable outputs only and using the retrieved total_amount.

furszy · 2022-06-08T14:36:56Z

thanks for the feedback theStack!

Applied the following changes:

Re-wrote commits titles to be 72-80 characters long.
Moved the only_spendable arg of AvailableCoins to be true by default.
By having spendable_only=true by default, fixed an already existent hidden issue in sendall RPC command -> description

brunoerg · 2022-06-15T20:42:18Z

Concept ACK

achow101 · 2022-06-16T20:52:23Z

ACK fd5c996

brunoerg

crACK fd5c996

w0xlt

Code Review ACK fd5c996

Clear improvement in code readability and Available Coins() logic.

stickies-v

Post-merge approach ACK fd5c996, nice refactor!

I did have a couple of nits/style suggestions during review, none of them crucial. I'll post them anyway since you may want to consider them in future PRs.

One thing I wonder (just out of curiosity) is why for IsSpent() and IsLockedCoin() you refactored the function signature in a single commit, whereas for IsSpentKey() you did it in three commits? I think both are done elegantly, just curious if there's a reason behind the (apparent) inconsistency.

stickies-v · 2022-06-17T20:41:00Z

src/wallet/spend.h

 /**
- * populate vCoins with vector of available COutputs.
+ * Return vector of available COutputs.


nit: CoinsResult is not technically a vector of COutputs, think this could be updated a little bit?

stickies-v · 2022-06-17T21:13:31Z

src/wallet/spend.cpp

@@ -791,11 +799,16 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal(
    CAmount selection_target = recipients_sum + not_input_fees;

    // Get available coins
-    std::vector<COutput> vAvailableCoins;
-    AvailableCoins(wallet, vAvailableCoins, &coin_control, coin_selection_params.m_effective_feerate, 1, MAX_MONEY, MAX_MONEY, 0);
+    auto res_available_coins = AvailableCoins(wallet,


stickies-v · 2022-06-17T21:16:30Z

src/wallet/spend.cpp

 {
    AssertLockHeld(wallet.cs_wallet);

-    vCoins.clear();
-    CAmount nTotal = 0;
+    CoinsResult result;


Ideally, result is declared as closely as possible to where it's used.

stickies-v · 2022-06-17T21:36:57Z

src/wallet/spend.cpp


    // Choose coins to use
-    std::optional<SelectionResult> result = SelectCoins(wallet, vAvailableCoins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params);
+    std::optional<SelectionResult> result = SelectCoins(wallet, res_available_coins.coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params);


nit: since you're already touching this, maybe you could update result to something more helpful like selected_coins? It does affect a few other lines so would understand if out of scope.

stickies-v · 2022-06-17T23:47:45Z

src/wallet/interfaces.cpp

@@ -110,7 +110,7 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet,
    result.txout = wtx.tx->vout[n];
    result.time = wtx.GetTxTime();
    result.depth_in_main_chain = depth;
-    result.is_spent = wallet.IsSpent(wtx.GetHash(), n);
+    result.is_spent = wallet.IsSpent(COutPoint(wtx.GetHash(), n));


I think it's preferred to use {} list initialization - any reason not to? (Here and in quite a few other places)

Suggested change

result.is_spent = wallet.IsSpent(COutPoint(wtx.GetHash(), n));

result.is_spent = wallet.IsSpent(COutPoint{wtx.GetHash(), n});

stickies-v · 2022-06-18T01:15:11Z

src/wallet/spend.cpp

+                          MAX_MONEY,    /*nMaximumAmount*/
+                          MAX_MONEY,    /*nMinimumSumAmount*/
+                          0             /*nMaximumCount*/
+                          ).total_amount;


Since we're explicitly relying on only_spendable to be true, I think it would be nice to make the argument explicit?

maflcko · 2022-06-18T09:11:35Z

https://cirrus-ci.com/task/4885942589718528?logs=ci#L6616

wallet/spend.cpp:237:41: error: argument name 'feerate' in comment does not match parameter name 'nMinimumAmount' [bugprone-argument-comment,-warnings-as-errors]
                          std::nullopt, /*feerate=*/
                                        ^
./wallet/spend.h:49:43: note: 'nMinimumAmount' declared here
                           const CAmount& nMinimumAmount = 1,
                                          ^
wallet/spend.cpp:87:13: note: actual callee ('AvailableCoins') is declared here
CoinsResult AvailableCoins(const CWallet& wallet,
            ^

…rate' in comment does not match parameter name" 7ca8726 wallet: fix warning: "argument name 'feerate' in comment does not match parameter name" (furszy) Pull request description: Should solve the tiny bitcoin/bitcoin#25005 (comment). Which merely happens for the extra "=" character after the comma. ACKs for top commit: Empact: Code Review ACK bitcoin/bitcoin@7ca8726 Tree-SHA512: e5368c1114f715bd93cb653c607fd0942ab0b79f709ed7aa627b3fc7e7efd096c92c5c86908c7f26c363b21e391a8faa812727eb32c285e54da3ce0429290361

…ns' + s…

4584d30 GUI: remove now unneeded 'm_balances' field from overviewpage (furszy) 050e8b1 GUI: 'getAvailableBalance', use cached balance if the user did not select UTXO manually (furszy) 96e3264 GUI: use cached balance in overviewpage and sendcoinsdialog (furszy) 321335b GUI: add getter for WalletModel::m_cached_balances field (furszy) e62958d GUI: sendCoinsDialog, remove duplicate wallet().getBalances() call (furszy) Pull request description: As per the title says, we are recalculating the entire wallet balance on different situations calling to `wallet().getBalances()`, when should instead make use of the wallet model cached balance. This has the benefits of (1) not spending resources calculating a balance that we already have cached, and (2) avoid blocking the main thread for a long time, in case of big wallets, walking through the entire wallet's tx map more than what it's really needed. Changes: 1) Fix: `SendCoinsDialog` was calling `wallet().getBalances()` twice during `setModel`. 2) Use the cached balance if the user did not select any UTXO manually inside the wallet model `getAvailableBalance` call. ----------------------- As an extra note, this work born in [#25005](bitcoin/bitcoin#25005) but grew out of scope of it. ACKs for top commit: jarolrod: ACK 4584d30 hebasto: re-ACK 4584d30, only suggested changes and commit message formatting since my [recent](#598 (review)) review. Tree-SHA512: 6633ce7f9a82a3e46e75aa7295df46c80a4cd4a9f3305427af203c9bc8670573fa8a1927f14a279260c488cc975a08d238faba2e9751588086fea1dcf8ea2b28

3a4f8bc bench: add benchmark for wallet 'AvailableCoins' function. (furszy) Pull request description: #### Rationale `AvailableCoins` is part of several important flows for the wallet; from RPC commands that create transactions like `fundrawtransaction`, `send`, `walletcreatefundedpsbt`, get the available balance, list the available coins with `listunspent` etc. to GUI connected processes that perform the same or similar actions: tx creation, available balance calculation, present the spendable coins in the coin control dialog. As we are improving this process in #24699, #25005 and there are more structural changes coming on the way. This benchmark aims to ensure us that, at least, there are no regressions (obviously performance improvements are great but, at least for me, this heads into the direction of having a base metric to compare future structural changes). #### Implementation Notes There are 5 new benchmarks, one per wallet supported output type (LEGACY, P2SH_SEGWIT, BECH32, BECH32M), plus a multi-output-type wallet benchmark which contains outputs from all the descriptor types. The test, by default, fills-up the wallet with 1k transactions, 2k outputs. Mainly to not consume much time if the user just want to verify that no substantial regressions were introduced. But, my expectation for those who are focused on this process is to use a much higher number locally to really note the differences across commits. ACKs for top commit: achow101: ACK 3a4f8bc hernanmarino: ACK 3a4f8bc aureleoules: ACK 3a4f8bc Tree-SHA512: d0bb4c165f1efa181b47cb31561e6217eff9135bcd1b6761a7292f9018e456d13d18a1b886c2e2268d35c52f9e1fd8e0f252972424e5c5f00c280620b79c5a1b

… function. 3a4f8bc bench: add benchmark for wallet 'AvailableCoins' function. (furszy) Pull request description: #### Rationale `AvailableCoins` is part of several important flows for the wallet; from RPC commands that create transactions like `fundrawtransaction`, `send`, `walletcreatefundedpsbt`, get the available balance, list the available coins with `listunspent` etc. to GUI connected processes that perform the same or similar actions: tx creation, available balance calculation, present the spendable coins in the coin control dialog. As we are improving this process in bitcoin#24699, bitcoin#25005 and there are more structural changes coming on the way. This benchmark aims to ensure us that, at least, there are no regressions (obviously performance improvements are great but, at least for me, this heads into the direction of having a base metric to compare future structural changes). #### Implementation Notes There are 5 new benchmarks, one per wallet supported output type (LEGACY, P2SH_SEGWIT, BECH32, BECH32M), plus a multi-output-type wallet benchmark which contains outputs from all the descriptor types. The test, by default, fills-up the wallet with 1k transactions, 2k outputs. Mainly to not consume much time if the user just want to verify that no substantial regressions were introduced. But, my expectation for those who are focused on this process is to use a much higher number locally to really note the differences across commits. ACKs for top commit: achow101: ACK 3a4f8bc hernanmarino: ACK 3a4f8bc aureleoules: ACK 3a4f8bc Tree-SHA512: d0bb4c165f1efa181b47cb31561e6217eff9135bcd1b6761a7292f9018e456d13d18a1b886c2e2268d35c52f9e1fd8e0f252972424e5c5f00c280620b79c5a1b

fanquake added the Wallet label Apr 27, 2022

fanquake requested a review from achow101 April 27, 2022 15:10

This was referenced Apr 28, 2022

wallet: return error msg for "too-long-mempool-chain" #24845

Merged

wallet: Improve AvailableCoins performance by reducing duplicated operations #24699

Merged

wallet: avoid mixing different OutputTypes during coin selection #24584

Merged

DrahtBot mentioned this pull request Apr 28, 2022

wallet: increase BnB upper limit #24752

Closed

furszy changed the title ~~wallet: remove extra wtx lookup in 'AvailableCoins' + several code improvements.~~ wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. Apr 29, 2022

DrahtBot mentioned this pull request May 8, 2022

Set effective_value when initializing a COutput #25083

Merged

furszy mentioned this pull request May 9, 2022

Avoid recalculating the wallet balance - use model cache bitcoin-core/gui#598

Merged

furszy force-pushed the 2022_availableBalance_improvements branch from f2c1694 to 2cdd2a1 Compare May 9, 2022 20:53

DrahtBot mentioned this pull request May 13, 2022

wallet: unify “allow/block other inputs“ concept #25118

Merged

achow101 reviewed May 16, 2022

View reviewed changes

DrahtBot mentioned this pull request May 21, 2022

rpc: Filter inputs by type during CoinSelection #25183

Closed

furszy force-pushed the 2022_availableBalance_improvements branch from 2cdd2a1 to 5b6124d Compare May 23, 2022 17:51

This was referenced May 28, 2022

bench: add benchmark for wallet 'AvailableCoins' function. #25234

Merged

RPC/Wallet: Add "use_txids" to output of getaddressinfo #22693

Closed

wallet: re-activate "AmountWithFeeExceedsBalance" error #25269

Open

achow101 reviewed Jun 2, 2022

View reviewed changes

furszy force-pushed the 2022_availableBalance_improvements branch from 5b6124d to 46ac311 Compare June 2, 2022 17:57

furszy force-pushed the 2022_availableBalance_improvements branch from 46ac311 to 5646ccf Compare June 5, 2022 14:20

theStack reviewed Jun 7, 2022

View reviewed changes

wallet: return 'CoinsResult' struct in AvailableCoins

4ce235e

Instead of accepting a `vCoins` reference that is cleared at the beginning of the method. Note: This new struct, down the commits line, will contain other `AvailableCoins` useful results.

furszy added 8 commits June 8, 2022 10:25

wallet: AvailableCoins, don't call 'wtx.tx->vout[i]' multiple times

9472ca0

wallet: IsLockedCoin, 'COutPoint' arg instead of (hash, index)

91902b7

wallet: IsSpent, 'COutPoint' arg instead of (hash, index)

a06fa94

wallet: decouple IsSpentKey(scriptPubKey) from IsSpentKey(hash, n)

3d8a282

This will be used in a follow-up commit to prevent extra 'GetWalletTx' lookups if the function caller already have the wtx and can just provide the scriptPubKey directly.

wallet: avoid extra IsSpentKey -> GetWalletTx lookups

4b83bf8

wallet: remove unused IsSpentKey(hash, index) method

cdf185c

wallet: GetAvailableBalance, remove double walk-through every availab…

fd5c996

…le coin Filtering `AvailableCoins` by spendable outputs only and using the retrieved total_amount.

furszy force-pushed the 2022_availableBalance_improvements branch from 5646ccf to fd5c996 Compare June 8, 2022 14:31

brunoerg approved these changes Jun 16, 2022

View reviewed changes

fanquake requested review from theStack and murchandamus June 16, 2022 22:15

w0xlt approved these changes Jun 17, 2022

View reviewed changes

achow101 merged commit 8be652e into bitcoin:master Jun 17, 2022

stickies-v reviewed Jun 18, 2022

View reviewed changes

furszy mentioned this pull request Jun 18, 2022

wallet: fix warning: "argument name 'feerate' in comment does not match parameter name" #25410

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 19, 2022

Merge bitcoin#25005: wallet: remove extra wtx lookup in 'AvailableCoi…

581c6a7

…ns' + s…

furszy deleted the 2022_availableBalance_improvements branch May 27, 2023 01:51

bitcoin locked and limited conversation to collaborators May 26, 2024

	auto res_available_coins = AvailableCoins(wallet,
	auto available_coins = AvailableCoins(wallet,

	result.is_spent = wallet.IsSpent(COutPoint(wtx.GetHash(), n));
	result.is_spent = wallet.IsSpent(COutPoint{wtx.GetHash(), n});

wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. #25005

wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. #25005

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!