[Wallet] Prevent spending of immature coins #1760

zeptin · 2018-07-28T17:03:47Z

Fix for #1255

Most tests are passing, I had to fix several dozen that were implicitly relying on the incorrect wallet logic.

There is still an issue with the SharedSteps.MineBlocks helper method not correctly taking fees into account.

Aprogiena · 2018-07-29T08:44:43Z

src/Stratis.Bitcoin.Features.Wallet/Wallet.cs

@@ -728,12 +728,29 @@ public IEnumerable<UnspentOutputReference> GetSpendableTransactions(int currentC

                    if (confirmationCount >= confirmations)
                    {
-                        yield return new UnspentOutputReference
+                        if (includeImmature || (!(transactionData.IsCoinBase ?? false) && !(transactionData.IsCoinStake ?? false)))


What does it mean if transaction data have null for Coinbase or coinstake flags?

Aprogiena · 2018-07-29T08:46:02Z

src/Stratis.Bitcoin.Features.Wallet/Wallet.cs

+                            };
+                        }
+
+                        if (!includeImmature && ((transactionData.IsCoinBase ?? false) || (transactionData.IsCoinStake ?? false)) && (confirmationCount >= consensus.CoinbaseMaturity))


In any case it would be nice to separate the parts of the condition to variables and you could reuse that here and about

Aprogiena · 2018-07-29T08:47:02Z

src/Stratis.Bitcoin.Features.Wallet/Wallet.cs

+        /// A value indicating whether this is a coinbase transaction or not.
+        /// </summary>
+        [JsonProperty(PropertyName = "isCoinBase", NullValueHandling = NullValueHandling.Ignore)]
+        public bool? IsCoinBase { get; set; }


Aprogiena · 2018-07-29T08:47:56Z

src/Stratis.Bitcoin.Features.Wallet/WalletManager.cs

@@ -784,7 +784,7 @@ public IEnumerable<UnspentOutputReference> GetSpendableTransactionsInAccount(Wal
                        $"Account '{walletAccountReference.AccountName}' in wallet '{walletAccountReference.WalletName}' not found.");
                }

-                res = account.GetSpendableTransactions(this.chain.Tip.Height, confirmations).ToArray();
+                res = account.GetSpendableTransactions(this.chain.Tip.Height, consensus, confirmations, includeImmature: includeImmature).ToArray();


Not sure if explicitly naming the argument here has any value

Elsewhere I have explicitly named it because in 99% of cases you do not want to use anything other than false for it, so using it at all is unexpected and it should be visually apparent what parameter is being passed. However, since an argument with the same name is being passed in this case, it does not really give additional information. I will remove it.

Aprogiena · 2018-07-29T08:49:09Z

src/Stratis.Bitcoin.Features.Wallet/WalletManager.cs

@@ -985,6 +985,7 @@ private void AddTransactionToWallet(Transaction transaction, TxOut utxo, int? bl
                var newTransaction = new TransactionData
                {
                    Amount = amount,
+                    IsCoinBase = transaction.IsCoinBase == false ? (bool?)null : true,


Wouldn't it be better to use false instead of null?

This construct is used for the existing IsCoinStake, I am merely duplicating it.

By using null we prevent the IsCoinbase field from being written to the wallet file.
We could decide to skip it if it's false in the future.

Aprogiena · 2018-07-29T08:50:26Z

src/Stratis.Bitcoin.IntegrationTests.Common/Extensions/CoreNodeExtensions.cs


+            if (considerMaturity)
+            {
+                if (numberOfBlocks == 1 && maturity > 1)


Aprogiena · 2018-07-29T08:53:07Z

src/Stratis.Bitcoin.IntegrationTests.Common/Extensions/CoreNodeExtensions.cs


+                return Enumerable.Range(startBlock,
+                        ((numberOfBlocks - maturity + 1) >= startBlock ? (numberOfBlocks - maturity + 1) : startBlock))


Any explanation about the logic here?

Yes please write this a bit better so that its more readable 👍

fassadlr · 2018-07-30T08:52:06Z

src/Stratis.Bitcoin.IntegrationTests/ProofOfStakeSteps.cs

            TestHelper.WaitLoop(() =>
            {
                long staked = this.nodes[this.PremineNode].FullNode.WalletManager().GetSpendableTransactionsInWallet(this.PremineWallet).Sum(s => s.Transaction.Amount);
-                return staked > balanceShouldBe;
+                return staked > 0;


Woah dude you are essentially voiding the POS minting integration test that ensures that we can stake coins!? This isn't right at all.

I think this needs to be analysed further. I think the choice of 110 PoW rewards was an incorrect constant now that the behaviour of GetSpendableTransactionsInWallet is fixed. I will have to examine the staking integration test in more detail to see what the expected spendable balance should be.

Ok, this was very much my bad; I misread what this method was actually doing. I have added includeImmature flags to the other GetSpendable* methods so that the test logic can be used essentially unchanged here.

fassadlr · 2018-07-30T08:53:34Z

src/Stratis.Bitcoin.IntegrationTests.Common/SharedSteps.cs

-            Money amountShouldBe = node.FullNode.Network.Consensus.PremineReward + node.FullNode.Network.Consensus.ProofOfWorkReward;
+
+            // The premine and other block rewards are not mature yet.
+            Money amountShouldBe = 0;


This isn't right as we need to ensure that the wallet has enough coins to stake with.

The premine is not immediately spendable. Clearly this method only mines sufficient blocks to generate the coins, nowhere near enough for them to be spendable, the comment indicates. I am correcting a constant that was chosen under false assumptions.

Perhaps a better way would be to optionally include the immature coins and leave the calculation of the expected balance unchanged. It feels incorrect using a method that gets 'spendable' transactions to do that, though, so maybe a different or new method should be used instead.

fassadlr · 2018-07-30T08:55:33Z

src/Stratis.Bitcoin.Features.Wallet/Wallet.cs

+        /// A value indicating whether this is a coinbase transaction or not.
+        /// </summary>
+        [JsonProperty(PropertyName = "isCoinBase", NullValueHandling = NullValueHandling.Ignore)]
+        public bool? IsCoinBase { get; set; }


I also want to know how this can null? A transaction is either a coinbase tx or its not?

The IsCoinStake JSON property is nullable, so I am merely matching that with the introduction of IsCoinBase. This was presumably originally done so that the wallet JSON does not get numerous unnecessary boolean fields (as coinbase/stake transactions are the exception rather than the rule).

It would be probably better to replace it for the Coinbase as well instead.

fassadlr

@zeptin please see my comments regarding the staking tests? I'm not sure how the staking test still works with a wallet that doesn't have enough coins?

fassadlr · 2018-07-30T08:58:29Z

src/Stratis.Bitcoin.Features.Wallet/Wallet.cs

@@ -728,12 +728,29 @@ public IEnumerable<UnspentOutputReference> GetSpendableTransactions(int currentC

                    if (confirmationCount >= confirmations)
                    {


I would flip the condition here to just continue so that the nested if can be avoided.

fassadlr · 2018-07-30T08:59:31Z

src/Stratis.Bitcoin.Features.Wallet/Wallet.cs

-                        };
+                            // This output can unconditionally be included in the results.
+
+                            yield return new UnspentOutputReference


I feel this whole new if can be written better so that we avoid code duplication here.

fassadlr · 2018-07-30T09:00:22Z

src/Stratis.Bitcoin.IntegrationTests.Common/Extensions/CoreNodeExtensions.cs


+            if (considerMaturity)


You can combine this if and the one below?

fassadlr · 2018-07-30T09:00:52Z

src/Stratis.Bitcoin.IntegrationTests.Common/Extensions/CoreNodeExtensions.cs


+                return Enumerable.Range(startBlock,
+                        ((numberOfBlocks - maturity + 1) >= startBlock ? (numberOfBlocks - maturity + 1) : startBlock))


Yes please write this a bit better so that its more readable 👍

bokobza · 2018-07-30T11:26:07Z

would we ever want to include immature coins?

Aprogiena · 2018-07-31T08:08:38Z

src/Stratis.Bitcoin.IntegrationTests/ProofOfStakeSteps.cs

+            var mineToMaturity = network.Consensus.ProofOfWorkReward * 110;
+            var balanceShouldBe = premine + mineToMaturity;
+
+            var spendable = this.nodes[this.PremineNode].FullNode.WalletManager().GetSpendableTransactionsInWallet(this.PremineWallet, includeImmature: true);


I know this is just as for which we don't have many requirements in terms of code style, so just saying that about five lines should not use var

bokobza · 2018-07-31T11:00:50Z

To reiterate my question, why do we have bool includeImmature = false?
Is there a scenario where we actually want to be able to send immature coins? As opposed to it being forbidden. @zeptin

zeptin · 2018-07-31T12:35:25Z

@bokobza Removing it is an option, but some tests would have to be rearchitected to take the changed behaviour into account. Fixing the tests wasn't really in the initial time estimate for the task, I suspect, so this may push it out past the end of the sprint. I agree that it is messy having the parameter, and there is no obvious use case for it in reality.

bokobza · 2018-07-31T12:56:12Z

Fair enough. But having tests passing because something not allowed is now allowed is a bit of a problem.
How many tests are failing because of that? In any case we'll have to fix them.

…coinFullNode into wallet_coin_maturity

bokobza · 2018-08-15T10:58:30Z

src/Stratis.Bitcoin.IntegrationTests/API/ApiSteps.cs

@@ -257,7 +260,8 @@ private void RecoverViaExtPubKey(string walletName, string extPubKey, int accoun
            {
                ExtPubKey = extPubKey,
                AccountIndex = accountIndex,
-                Name = walletName
+                Name = walletName,
+                CreationDate = DateTime.Now


why the change?

It fixes the failing CreateExtPubKeyOnlyWallet_creates_wallet_with_extra_flag test that no one else seemed to have bothered about yet.

You mean this test was failing before that change?

As far as I am aware, yes. AddNewAccount_for_xpub_only_wallet_informs_user_to_create_new_wallet_ as well.

These 2 don't fail for me or for the others. The integration tests are not running smoothy and sometimes a rerun of the failed ones fixes the issue.

Not sure how that can be:

https://github.com/stratisproject/StratisBitcoinFullNode/blob/master/src/Stratis.Bitcoin.Features.Wallet/Controllers/WalletController.cs#L285-L288

Because the creationDate used is the default one: 01/01/0001 and doesn't fail.

It does default, but it still throws:

{System.ArgumentOutOfRangeException: The UTC time represented when the offset is applied must be between year 0 and 10,000. Parameter name: offset at System.DateTimeOffset.ValidateDate(DateTime dateTime, TimeSpan offset) at System.DateTimeOffset..ctor(DateTime dateTime) at System.DateTimeOffset.op_Implicit(DateTime dateTime) at Stratis.Bitcoin.Features.Wallet.WalletManager.RecoverWallet(String name, ExtPubKey extPubKey, Int32 accountIndex, DateTime creationTime) in C:\StratisBitcoinFullNode\src\Stratis.Bitcoin.Features.Wallet\WalletManager.cs:line 369 at Stratis.Bitcoin.Features.Wallet.Controllers.WalletController.RecoverViaExtPubKey(WalletExtPubRecoveryRequest request) in C:\StratisBitcoinFullNode\src\Stratis.Bitcoin.Features.Wallet\Controllers\WalletController.cs:line 285}

bokobza · 2018-08-15T10:58:43Z

src/Stratis.Bitcoin.IntegrationTests/BlockStore/ProofOfWorkSpendingSteps.cs

        private Exception caughtException;
        private Transaction lastTransaction;
        private SharedSteps sharedSteps;
        private NodeGroupBuilder nodeGroupBuilder;
+        private int coinbaseMaturity;


why has this moved down?

bokobza · 2018-08-15T11:04:09Z

src/Stratis.Bitcoin.IntegrationTests/BlockStore/RetrieveFromBlockStoreSteps.cs

@@ -62,6 +62,9 @@ private void a_pow_node_running()
            this.node = this.builder.CreateStratisPowNode(this.network);
            this.node.Start();
            this.node.NotInIBD();
+
+            this.maturity = 1;


Can we set the maturity in BeforeTest or even in private int maturity = 1; so that it's more visible, or would that not work?

bokobza · 2018-08-15T11:06:01Z

src/Stratis.Bitcoin.IntegrationTests/Mempool/MemoryPoolTests.cs

                stratisNodeSync.SetDummyMinerSecret(new BitcoinSecret(new Key(), stratisNodeSync.FullNode.Network));
-                stratisNodeSync.GenerateStratisWithMiner(105); // coinbase maturity = 100
+                stratisNodeSync.GenerateStratisWithMiner(5);


maybe create int maturity = 5 and then do stratisNodeSync.GenerateStratisWithMiner(maturity + 5); so that the calculation is clear?

could you not change the network maturity in the ctor? then it'll be available for the whole class.

bokobza · 2018-08-15T11:17:09Z

@zeptin I've made a few comments. These are valid for all the tests so please apply where possible.
Also please revert the changes aimed at improving the speed of the tests. If the test works, it's fine to wait 100 more blocks. Otherwise it makes it hard to figure out what needed to be changed vs what is an improvement. We can do the improvements in a further PR.

fassadlr · 2018-08-15T11:18:13Z

src/Stratis.Bitcoin.IntegrationTests/ProofOfStakeSteps.cs

+                    .GetAllTransactionsByCoinType((CoinType)this.nodes[this.PremineNode].FullNode.Network.Consensus
+                        .CoinType))
+                {
+                    if (!this.transactionsBeforeStaking.Contains(transactionData.Id))


@zeptin OK I understand now why this change is required. As a secondary measure, can we perhaps check the coinstake flag on the transactionData to verify that it IS in fact a coinstake tx? We can also check the value so that it matches the network's stake reward?

The whole point of this test is to ensure that we can stake and mint new coins, so just checking that there is a new tx might not be entirely sufficient?

Yeah, that seems sensible, I've added a check for that.

Checking the value might be overkill, but it isn't hard to do. It is however dependent on how many outputs the coinstake consists of (I think it is 8 by default, but this can be overridden and could change in future).

…coinFullNode into wallet_coin_maturity

bokobza · 2018-08-16T13:38:09Z

src/Stratis.Bitcoin.IntegrationTests/Mempool/MemoryPoolTests.cs

@@ -350,8 +350,6 @@ public void MempoolSyncTransactions()
                // generate blocks and wait for the downloader to pickup
                stratisNodeSync.SetDummyMinerSecret(new BitcoinSecret(new Key(), stratisNodeSync.FullNode.Network));
                stratisNodeSync.GenerateStratisWithMiner(105); // coinbase maturity = 100
-                // wait for block repo for block sync to work


don't we need to check if the node is synced?

The test failed for me unless I removed that. I tried it repeatedly.

A test should never fail if we wait a bit longer for a node to be synced.
There is only one test that we know always fail and it's "A_cut_off_miner_advanced_ahead_of_network_causes_reorg_on_reconnect".

The test does fail if it takes the node longer than the helper method's timeout to sync. Why it might be taking longer than that, I do not know.

bokobza · 2018-08-16T13:41:09Z

src/Stratis.Bitcoin.IntegrationTests/Wallet/SendingToAndFromManyAddressesSpecification_Steps.cs

@@ -56,12 +56,15 @@ private void two_connected_nodes()
                    .Connect(NodeOne, NodeTwo)
                    .AndNoMoreConnections()
                .Build();
+
+            this.CoinBaseMaturity = 1;


Here you can do KnownNetworks.RegTest.Consensus.CoinbaseMaturity = 1; in the beforeTests method.

bokobza · 2018-08-16T13:52:27Z

src/Stratis.Bitcoin.IntegrationTests/Wallet/SendingTxWithDoubleSpendSpecification_Steps.cs

@@ -100,8 +100,7 @@ private void receiving_node_attempts_to_double_spend_mempool_doesnotaccept()

        private void trx_is_mined_into_a_block_and_removed_from_mempools()
        {
-            new SharedSteps().MineBlocks(1, this.stratisSender, "account 0", "mywallet", "123456", 16360L);
-
+            new SharedSteps().MineBlocks(1, this.stratisSender, "account 0", "mywallet", "123456");


there is no more fees for the mine block?

The test fails if I do not change this.

bokobza · 2018-08-16T14:19:04Z

src/Stratis.Bitcoin.IntegrationTests/ProofOfStakeSteps.cs

            this.sharedSteps.WaitForNodeToSync(this.nodes[this.PremineNode]);
        }

        public void PremineNodeMinesTenBlocksMoreEnsuringTheyCanBeStaked()
        {
-            this.nodes[this.PremineNode].GenerateStratisWithMiner(Convert.ToInt32(this.nodes[this.PremineNode].FullNode.Network.Consensus.CoinbaseMaturity));
+            this.nodes[this.PremineNode].GenerateStratisWithMiner(10);


should that not be "10 + maturity" instead of 10?

It is used like this in GenerateCoins, implying that it is being done after maturity is already reached:

MineCoinsToMaturity(); PremineNodeMinesTenBlocksMoreEnsuringTheyCanBeStaked();

@bokobza this.nodes[this.PremineNode].GenerateStratisWithMiner(10); is correct as we want to advance the node 10 blocks so that they coins become "stake-able"

ok so I guess before we mined coins of quantity "maturity" and that was too much?

bokobza · 2018-08-16T14:38:12Z

src/Stratis.Bitcoin.IntegrationTests.Common/Extensions/CoreNodeExtensions.cs

@@ -21,11 +21,17 @@ public static Money GetProofOfWorkRewardForMinedBlocks(this CoreNode node, int n

            int startBlock = node.FullNode.Chain.Height - numberOfBlocks + 1;

-            return Enumerable.Range(startBlock, numberOfBlocks)
+            int maturity = (int)node.FullNode.Network.Consensus.CoinbaseMaturity;


The goal of this method is simply to return the reward, whether or not the rewards are spendable or not.
Are you sure this change is necessary? It shouldn't be. If it is then it's the tests using it that need looking into.

I spent the last two weeks looking at the tests. This change is the least required to get everything to work.

This change is the least required to get everything to work.

I don't understand how this is related.

I am not sure how I am supposed to simultaneously get the tests to pass, and not change anything about them. This method gets used implicitly every time SharedSteps.MineBlocks is called, to ensure internal consistency. Its result gets compared to the increase in wallet balance, and any discrepancy causes the test to immediately fail. As the wallet balance is dependent on the underlying bug this PR fixes, it necessitates that this function be changed too. The alternative is to change every test that uses MineBlocks, which results in a lot more changed code & possible structural changes to the tests.

This method is used in GetProofOfWorkRewardForMinedBlocksTest to check what's the miner reward after mining, but here you've changed the test there to match your change.
And you've added a test called GetProofOfWorkRewardForMinedBlocksTestLowMaturity that includes maturity in your calculation but it's not related to the miner reward.
@zeptin @fassadlr

bokobza · 2018-08-16T14:40:42Z

src/Stratis.Bitcoin.IntegrationTests/Wallet/WalletTests.cs

-                TestHelper.WaitLoop(() => TestHelper.IsNodeSynced(stratisSender));
-
-                //// sync all nodes
+                // Sync all nodes


Is it not better to wait for one node to be fully synced, before making sure all the nodes are in sync with each other? Same question for the other instances where TestHelper.WaitLoop(() => TestHelper.IsNodeSynced(some node)); has been removed.

bokobza · 2018-08-16T14:52:55Z

src/Stratis.Bitcoin.Features.Wallet/Wallet.cs

+                    bool isCoinStake = transactionData.IsCoinStake ?? false;
+
+                    // This output can unconditionally be included in the results.
+                    if (!isCoinBase && !isCoinStake)


Maybe we can combine the 2 ifs if we format it like this:

if ((!isCoinBase && !isCoinStake) || ((isCoinBase || isCoinStake) && (confirmationCount >= network.Consensus.CoinbaseMaturity)) { yield return new UnspentOutputReference { Account = this, Address = address, Transaction = transactionData }; }

bokobza

Almost there!

fassadlr

@bokobza Im happy with the changes that I requested, should you wish to merge today 👍

…coinFullNode into wallet_coin_maturity

…t_coin_maturity

zeptin added 4 commits July 25, 2018 22:29

Initial version without coinbase support

fb98f61

Introduce isCoinBase to wallet txns

1ff15f8

Consider both coinbase and coinstake maturity

ed9f808

Fix several erroneous tests

02aa659

Aprogiena reviewed Jul 29, 2018

View reviewed changes

Aprogiena added the pr-review-medium label Jul 29, 2018

fassadlr reviewed Jul 30, 2018

View reviewed changes

fassadlr suggested changes Jul 30, 2018

View reviewed changes

zeptin added 3 commits July 30, 2018 22:00

Unbreak tests using ProofOfStakeSteps

28c642a

Minor fixes

f2b2841

Minor fixes per review

5c63f67

Aprogiena reviewed Jul 31, 2018

View reviewed changes

zeptin added 6 commits July 31, 2018 18:40

Fix stupid logic error

ea5a278

Partially resolve MineBlocks logic

95ebb22

Merge branch 'master' of https://github.com/stratisproject/StratisBit…

d97ccc6

…coinFullNode into wallet_coin_maturity

Merge branch 'master' of https://github.com/stratisproject/StratisBit…

41907a2

…coinFullNode into wallet_coin_maturity

Merge branch 'master' of https://github.com/stratisproject/StratisBit…

0215e03

…coinFullNode into wallet_coin_maturity

Moq doesn't like optional args

84fc6fd

zeptin added 2 commits August 14, 2018 22:20

Clean up SC int tests

76f41f7

Misc cleanups

d2fe96f

bokobza reviewed Aug 15, 2018

View reviewed changes

fassadlr reviewed Aug 15, 2018

View reviewed changes

zeptin added 3 commits August 15, 2018 17:56

Merge branch 'master' of https://github.com/stratisproject/StratisBit…

dc6684c

…coinFullNode into wallet_coin_maturity

Revert

1464cf6

Tweaks

a9680ea

bokobza reviewed Aug 16, 2018

View reviewed changes

bokobza suggested changes Aug 16, 2018

View reviewed changes

Fixes per review

22a5ca6

fassadlr previously approved these changes Aug 17, 2018

View reviewed changes

zeptin added 2 commits August 19, 2018 19:49

Merge branch 'master' of https://github.com/stratisproject/StratisBit…

f5c9296

…coinFullNode into wallet_coin_maturity

Add back node syncs after helper method fix

627ac7e

zeptin dismissed fassadlr’s stale review via 627ac7e August 19, 2018 19:10

bokobza added 2 commits August 19, 2018 21:10

moodified some tests

cf143d7

Merge remote-tracking branch 'origin/wallet_coin_maturity' into walle…

6814a25

…t_coin_maturity

bokobza approved these changes Aug 19, 2018

View reviewed changes

bokobza merged commit 81b6d42 into stratisproject:master Aug 19, 2018

zeptin deleted the wallet_coin_maturity branch September 21, 2018 08:56

bokobza mentioned this pull request Oct 15, 2018

Wallet allows to spend not matured/recently staked coins stratisproject/StratisCore#44

Closed


		return Enumerable.Range(startBlock,
		((numberOfBlocks - maturity + 1) >= startBlock ? (numberOfBlocks - maturity + 1) : startBlock))

		@@ -728,12 +728,29 @@ public IEnumerable<UnspentOutputReference> GetSpendableTransactions(int currentC

		if (confirmationCount >= confirmations)
		{

[Wallet] Prevent spending of immature coins #1760

[Wallet] Prevent spending of immature coins #1760

Conversation

zeptin commented Jul 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fassadlr Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fassadlr Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fassadlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bokobza commented Jul 30, 2018

Choose a reason for hiding this comment

bokobza commented Jul 31, 2018

zeptin commented Jul 31, 2018

bokobza commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bokobza commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bokobza Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bokobza left a comment

Choose a reason for hiding this comment

fassadlr left a comment

Choose a reason for hiding this comment

fassadlr Jul 30, 2018 •

edited

Loading

fassadlr Jul 30, 2018 •

edited

Loading

bokobza Aug 16, 2018 •

edited

Loading