Revert "Wallet has correct spendables when only one unconfirmed is in…

…cluded in a block (#4056)" (#4057)

This reverts commit e7818cd.

src/Stratis.Bitcoin.Features.MemoryPool/Interfaces/ITxMempool.cs

@@ -215,15 +215,14 @@ public interface ITxMempool
         /// </summary>
         /// <param name="stage">Staged transactions.</param>
         /// <param name="updateDescendants">Whether to update decendants.</param>
-        /// <param name="removeForBlock">Whether the transaction is being removed because it was included in a block.</param>
         /// <remarks>
         /// If a transaction is in this set, then all in-mempool descendants must
         /// also be in the set, unless this transaction is being removed for being
         /// in a block.
         /// Set updateDescendants to true when removing a tx that was in a block, so
         /// that any in-mempool descendants have their ancestor state updated.
         /// </remarks>
-        void RemoveStaged(TxMempool.SetEntries stage, bool updateDescendants, bool removeForBlock = false);
+        void RemoveStaged(TxMempool.SetEntries stage, bool updateDescendants);
 
         /// <summary>
         /// Set how frequent the sanity check is executed.

src/Stratis.Bitcoin.Features.MemoryPool/TransactionRemovedFromMemoryPool.cs

@@ -11,26 +11,9 @@ public class TransactionRemovedFromMemoryPool : EventBase
     {
         public Transaction RemovedTransaction { get; }
 
-        /// <summary>
-        /// Whether this transaction was removed from the mempool because
-        /// it was included in a block that was added to the chain.
-        /// </summary>
-        /// <remarks>
-        /// This is useful to discern whether we expect to see a transaction again.
-        ///
-        /// In the case that this is true, we can expect the transaction hash to still
-        /// be useful, as it has been or will soon be "confirmed" and exist as part of the chain.
-        ///
-        /// In the case that this is false, we are most likely to not see the transaction hash again.
-        /// It was likely removed because of an input conflict or a similar error, so we may
-        /// want to discard the transaction hash entirely.
-        /// </remarks>
-        public bool RemovedForBlock { get; }
-
-        public TransactionRemovedFromMemoryPool(Transaction removedTransaction, bool removedForBlock)
+        public TransactionRemovedFromMemoryPool(Transaction removedTransaction)
         {
             this.RemovedTransaction = removedTransaction;
-            this.RemovedForBlock = removedForBlock;
         }
     }
 }

src/Stratis.Bitcoin.Features.MemoryPool/TxMemPool.cs

@@ -653,13 +653,13 @@ public void RemoveRecursive(Transaction origTx)
         }
 
         /// <inheritdoc />
-        public void RemoveStaged(SetEntries stage, bool updateDescendants, bool removedForBlock = false)
+        public void RemoveStaged(SetEntries stage, bool updateDescendants)
         {
             //AssertLockHeld(cs);
             this.UpdateForRemoveFromMempool(stage, updateDescendants);
             foreach (TxMempoolEntry it in stage)
             {
-                this.RemoveUnchecked(it, removedForBlock);
+                this.RemoveUnchecked(it);
             }
         }
 
@@ -688,7 +688,7 @@ public int Expire(long time)
         /// Removes entry from memory pool.
         /// </summary>
         /// <param name="entry">Entry to remove.</param>
-        private void RemoveUnchecked(TxMempoolEntry entry, bool removedForBlock)
+        private void RemoveUnchecked(TxMempoolEntry entry)
         {
             uint256 hash = entry.TransactionHash;
             foreach (TxIn txin in entry.Transaction.Inputs)
@@ -719,7 +719,7 @@ private void RemoveUnchecked(TxMempoolEntry entry, bool removedForBlock)
 
             if (this.signals != null)
             {
-                this.signals.Publish(new TransactionRemovedFromMemoryPool(entry.Transaction, removedForBlock));
+                this.signals.Publish(new TransactionRemovedFromMemoryPool(entry.Transaction));
             }
         }
 
@@ -854,7 +854,7 @@ public void RemoveForBlock(IEnumerable<Transaction> vtx, int blockHeight)
                 {
                     var stage = new SetEntries();
                     stage.Add(entry);
-                    this.RemoveStaged(stage, true, true);
+                    this.RemoveStaged(stage, true);
                 }
 
                 this.RemoveConflicts(tx);

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

@@ -111,20 +111,8 @@ private void OnTransactionAdded(TransactionAddedToMemoryPool transactionAddedToM
 
         private void OnTransactionRemoved(TransactionRemovedFromMemoryPool transactionRemovedFromMempool)
         {
-            this.logger.LogDebug("Transaction '{0}' was removed from the mempool. RemovedForBlock={1}", 
-                transactionRemovedFromMempool.RemovedTransaction.GetHash(), transactionRemovedFromMempool.RemovedForBlock);
-
-            // If the transaction was removed from the mempool because it's part of a block, we don't want to remove it.
-            // It makes more sense to keep the current entry in the database and update it with the confirmation details
-            // when the wallet processes that block. This also avoids race conditions where users might try and build transactions
-            // right after this operation, but just before the wallet processes the next queued block.
-
-            // However if it was removed for any other reason, we will be out of sync with what's in the mempool.
-            // So lets remove it from the wallet to keep the wallet up to date.
-            if (!transactionRemovedFromMempool.RemovedForBlock)
-            {
-                this.walletManager.RemoveUnconfirmedTransaction(transactionRemovedFromMempool.RemovedTransaction);
-            }
+            this.logger.LogDebug("Removing transaction '{0}' as it was removed from the mempool.", transactionRemovedFromMempool.RemovedTransaction.GetHash());
+            this.walletManager.RemoveUnconfirmedTransaction(transactionRemovedFromMempool.RemovedTransaction);
         }
 
         /// <inheritdoc />

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

@@ -84,80 +84,6 @@ public void WalletCanReceiveAndSendCorrectly()
             }
         }
 
-        [Fact]
-        public void WalletBalanceCorrectWhenOnlySomeUnconfirmedAreIncludedInABlock()
-        {
-            using (NodeBuilder builder = NodeBuilder.Create(this))
-            {
-                CoreNode stratisSender = builder.CreateStratisPowNode(this.network).WithWallet().Start();
-                CoreNode stratisReceiver = builder.CreateStratisPowNode(this.network).WithWallet().Start();
-
-                int maturity = (int)stratisSender.FullNode.Network.Consensus.CoinbaseMaturity;
-                TestHelper.MineBlocks(stratisSender, maturity + 1 + 5);
-
-                // The mining should add coins to the wallet
-                long total = stratisSender.FullNode.WalletManager().GetSpendableTransactionsInWallet(WalletName).Sum(s => s.Transaction.Amount);
-                Assert.Equal(Money.COIN * 6 * 50, total);
-
-                // Sync both nodes
-                TestHelper.ConnectAndSync(stratisSender, stratisReceiver);
-
-                // Send coins to the receiver
-                HdAddress sendto = stratisReceiver.FullNode.WalletManager().GetUnusedAddress(new WalletAccountReference(WalletName, Account));
-                Transaction trx = stratisSender.FullNode.WalletTransactionHandler().BuildTransaction(CreateContext(stratisSender.FullNode.Network,
-                    new WalletAccountReference(WalletName, Account), Password, sendto.ScriptPubKey, Money.COIN * 100, FeeType.Medium, 101));
-
-                // Broadcast to the other node
-                stratisSender.FullNode.NodeController<WalletController>().SendTransaction(new SendTransactionRequest(trx.ToHex()));
-
-                // Wait for the transaction to arrive
-                TestBase.WaitLoop(() => stratisReceiver.CreateRPCClient().GetRawMempool().Length > 0);
-                TestBase.WaitLoop(() => stratisReceiver.FullNode.WalletManager().GetSpendableTransactionsInWallet(WalletName).Any());
-
-                long receivetotal = stratisReceiver.FullNode.WalletManager().GetSpendableTransactionsInWallet(WalletName).Sum(s => s.Transaction.Amount);
-                Assert.Equal(Money.COIN * 100, receivetotal);
-
-                // Generate two new blocks so the transaction is confirmed
-                TestHelper.MineBlocks(stratisSender, 2);
-                TestBase.WaitLoop(() => TestHelper.AreNodesSynced(stratisReceiver, stratisSender));
-
-
-                // Send 1 transaction from the second node and let it get to the first.
-                sendto = stratisSender.FullNode.WalletManager().GetUnusedAddress(new WalletAccountReference(WalletName, Account));
-                Transaction testTx1 = stratisReceiver.FullNode.WalletTransactionHandler().BuildTransaction(CreateContext(stratisSender.FullNode.Network,
-                    new WalletAccountReference(WalletName, Account), Password, sendto.ScriptPubKey, Money.COIN * 10, FeeType.Medium, 0));
-                stratisReceiver.FullNode.NodeController<WalletController>().SendTransaction(new SendTransactionRequest(testTx1.ToHex()));
-                TestBase.WaitLoop(() => stratisSender.CreateRPCClient().GetRawMempool().Length > 0);
-
-                // Disconnect so the first node doesn't get any more transactions.
-                TestHelper.Disconnect(stratisReceiver, stratisSender);
-
-                // Send a second unconfirmed transaction on the second node which consumes the first.
-                Transaction testTx2 = stratisReceiver.FullNode.WalletTransactionHandler().BuildTransaction(CreateContext(stratisSender.FullNode.Network,
-                    new WalletAccountReference(WalletName, Account), Password, sendto.ScriptPubKey, Money.COIN * 10, FeeType.Medium, 0));
-                stratisReceiver.FullNode.NodeService<IBroadcasterManager>().BroadcastTransactionAsync(testTx2);
-
-                // Now we can mine a block on the first node with only 1 of the transactions in it.
-                TestHelper.MineBlocks(stratisSender, 1);
-
-                // Connect the nodes again. 
-                TestHelper.Connect(stratisSender, stratisReceiver);
-
-                // Second node receives a block with only one transaction in it.
-                TestBase.WaitLoop(() => TestHelper.AreNodesSynced(stratisReceiver, stratisSender, true));
-
-                // Now lets see what is in the second node's wallet!
-                IEnumerable<UnspentOutputReference> spendableTxs = stratisReceiver.FullNode.WalletManager().GetSpendableTransactionsInWallet(WalletName);
-
-                // There should be one spendable transaction. And it should be testTx2.
-                Assert.Single(spendableTxs);
-                Assert.Equal(testTx2.GetHash(), spendableTxs.First().Transaction.Id);
-
-                // It follows that if the above assert was violated we would have conflicts when we build a transaction. 
-                // Specifically what we don't want is to have testTx1 in our spendable transactions, which was causing the known issue.
-            }
-        }
-
         [Fact]
         public void WalletValidatesIncorrectPasswordAfterCorrectIsUsed()
         {

src/Stratis.Features.SQLiteWalletRepository/Commands/ProcessBlockCommands.cs

@@ -63,11 +63,8 @@ public static void RegisterProcessBlockCommands(this DBConnection conn)
 
         public static DBCommand CmdUploadPrevOut(this DBConnection conn)
         {
-            // UPSERTs TransactionDatas. If they already exist (i.e. as mempool transactions), they will be
-            // "confirmed" into a block, otherwise they will be created and inserted.
-
             return conn.CreateCommand($@"
-                INSERT INTO HDTransactionData
+                REPLACE INTO HDTransactionData
                 SELECT A.WalletID
                 ,      A.AccountIndex
                 ,      A.AddressType
@@ -103,10 +100,7 @@ JOIN   HDTransactionData TD
                        ON     TD.OutputTxId = T.OutputTxId
                        AND    TD.OutputIndex = T.OutputIndex
                        AND    TD.ScriptPubKey = T.ScriptPubKey
-                       AND    (TD.OutputBlockHash IS NOT NULL OR TD.OutputBlockHeight IS NOT NULL))
-                ON CONFLICT(WalletId, AccountIndex, AddressType, AddressIndex, OutputTxId, OutputIndex) DO UPDATE SET 
-                       OutputBlockHeight = excluded.OutputBlockHeight,
-                       OutputBlockHash = excluded.OutputBlockHash");
+                       AND    (TD.OutputBlockHash IS NOT NULL OR TD.OutputBlockHeight IS NOT NULL))");
         }
 
         public static DBCommand CmdReplacePayments(this DBConnection conn)
@@ -180,8 +174,6 @@ FROM     HDWallet
 
         public static DBCommand CmdUpdateOverlaps(this DBConnection conn)
         {
-            // Gets conflicting transactions, while leaving the transactions themselves.
-
             return conn.CreateCommand($@"
                 SELECT TD.*
                 FROM   temp.TempPrevOut T
@@ -190,7 +182,6 @@ JOIN   HDTransactionData TD
                 AND    TD.OutputIndex = T.OutputIndex
                 AND    TD.ScriptPubKey = T.ScriptPubKey
                 AND    TD.SpendTxId IS NOT NULL
-                AND    TD.SpendTxId != T.SpendTxId
                 AND    TD.WalletId IN (
                        SELECT   WalletId
                        FROM     HDWallet

src/Stratis.Features.SQLiteWalletRepository/DBConnection.cs

@@ -618,8 +618,6 @@ internal void ProcessTransactions(IEnumerable<IEnumerable<string>> tableScripts,
 
             // Check for spending overlaps.
             // Performs checks that we do not affect a confirmed transaction's spends.
-            // This will detect any existing unconfirmed transactions that spend the same inputs as incoming confirmed transactions, 
-            // unless it's the same transaction - those will be updated rather than inserted by the CmdUploadPrevOut query.  
             var cmdUpdateOverlaps = this.Commands["CmdUpdateOverlaps"];
             cmdUpdateOverlaps.Bind("walletName", walletName);
             cmdUpdateOverlaps.Bind("prevHash", prevHash);