Merge pull request #594 from HorizenOfficial/gp/post_hard_fork_tx_evi…

…ction

[ZEND-552] Remove stale transactions when a hard fork activates

The RemoveStaleTransaction() function is currently called every time a block is connected/disconnected to eventually remove transactions that became invalid.

To keep the flow as efficient as possible, such function doesn't run all the checks available inside AcceptTxToMemoryPool() but only the ones that could really fail and depend on the context (like the chain height, or the state of a sidechain).

This PR includes a small fix to apply additional checks in case a hard fork activates, because the change in the set of validation rules may affect pending transactions (for instance, a shielding transaction will not be valid anymore after the activation of hard fork #11).

qa/pull-tester/rpc-tests.sh

@@ -164,6 +164,7 @@ testScripts=(
   'p2p_ignore_spent_tx.py',215,455
   'shieldedpooldeprecation_rpc.py',558,1794
   'mempool_size_limit.py',121,203
+  'mempool_hard_fork_cleaning.py',34,60
 );
 
 testScriptsExt=(

qa/rpc-tests/mempool_hard_fork_cleaning.py

@@ -0,0 +1,125 @@
+#!/usr/bin/env python3
+# Copyright (c) 2016 The Zcash developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+
+from test_framework.blockchainhelper import EXPECT_SUCCESS, BlockchainHelper, SidechainParameters
+from test_framework.test_framework import BitcoinTestFramework, ForkHeights
+from test_framework.util import assert_equal, assert_greater_than, initialize_chain_clean, mark_logs, \
+                                start_nodes, sync_blocks, wait_and_assert_operationid_status, wait_until
+
+DEBUG_MODE = 1
+NUMB_OF_NODES = 2
+Z_FEE = 0.0001
+
+class Test (BitcoinTestFramework):
+
+    def setup_chain(self):
+        print("Initializing test directory "+self.options.tmpdir)
+        initialize_chain_clean(self.options.tmpdir, NUMB_OF_NODES)
+
+    def setup_nodes(self):
+        self.nodes = start_nodes(NUMB_OF_NODES, self.options.tmpdir, [['-experimentalfeatures', '-zmergetoaddress', '-limitdebuglogsize=false']] * NUMB_OF_NODES)
+
+    def run_test (self):
+
+        '''
+        In this test we want to check that a tx becoming irrevocably invalid after hard-fork overcome
+        is actually removed from mempool.
+
+        The test is composed of two sections:
+        1. Reach sidechains fork, create a sidechain, disconnect one block and check that the sidechain creation is evicted
+        2. Reach a height close to the shielded pool deprecation fork, create a shielding tx, mine one block, check that the tx is evicted
+        '''
+
+        # ####################################################################################################
+        # Fork 8 - Sidechain hard fork
+        # ####################################################################################################
+
+        mark_logs("Node 0 starts mining to reach the block immediately before the sidechain hard fork activation", self.nodes, DEBUG_MODE)
+        sc_fork_height = ForkHeights['MINIMAL_SC']
+        blocks = self.nodes[0].generate(sc_fork_height - 1) # The mempool should accept sidechain txs one block before the hard fork
+        self.sync_all()
+
+        mark_logs("Node 0 creates a v0 sidechain", self.nodes, DEBUG_MODE)
+        test_helper = BlockchainHelper(self)
+        test_helper.create_sidechain("v0_sidechain", SidechainParameters["DEFAULT_SC_V0"], EXPECT_SUCCESS)
+        self.sync_all()
+
+        mark_logs("Check that the nodes discard the sidechain transaction after disconnecting one block", self.nodes, DEBUG_MODE)
+        for node in self.nodes:
+            assert_equal(len(node.getrawmempool()), 1)              # All nodes should have the sidechain creation transaction
+            node.invalidateblock(blocks[-1])                        # Let's disconnect the last block
+            assert_equal(node.getblockcount(), sc_fork_height - 2)  # At this point, the sidechain transaction can't be mined anymore
+            assert_equal(len(node.getrawmempool()), 0)              # Nodes should have evicted the sidechain creation transaction
+
+        mark_logs("RemoveStaleTransaction() function worked properly for block disconnection on a hard fork", self.nodes, DEBUG_MODE)
+
+
+
+
+
+        # ####################################################################################################
+        # Fork 11 - Shielded pool deprecation hard fork
+        # ####################################################################################################
+
+        mark_logs("Node 0 starts mining to get close to the Shieleded Pool Deprecation hard fork [Fork - 3]", self.nodes, DEBUG_MODE)
+        shielded_pool_deprecation_fork_height = ForkHeights['SHIELDED_POOL_DEPRECATION']
+        block_count = self.nodes[0].getblockcount()
+        self.nodes[1].generate(shielded_pool_deprecation_fork_height - block_count - 3)
+        self.sync_all()
+
+        mark_logs("Split the network", self.nodes, DEBUG_MODE)
+        self.split_network(0)
+
+        mark_logs("Node 0 creates a shielding transaction", self.nodes, DEBUG_MODE)
+        total_funds_before_shielding = sum(x['amount'] for x in self.nodes[0].listunspent(0))
+        opid = self.nodes[0].z_mergetoaddress(["ANY_TADDR"], self.nodes[0].z_getnewaddress(), Z_FEE, 2, 2)["opid"]
+        wait_and_assert_operationid_status(self.nodes[0], opid)
+
+        # Give time to the wallet to be notified and updated the balance (listunspent would otherwise return an outdated value)
+        wait_until(lambda: self.nodes[0].getmempoolinfo()['fullyNotified'] == True, 20)
+        assert_greater_than(total_funds_before_shielding, sum(x['amount'] for x in self.nodes[0].listunspent(0))) # Some less funds available
+
+        mark_logs("Join the network", self.nodes, DEBUG_MODE)
+        self.join_network(0)
+
+        mark_logs("Check that the shielding transaction is only available on node 0 (no rebroadcasting happens)", self.nodes, DEBUG_MODE)
+        assert_equal(self.nodes[0].getmempoolinfo()["size"], 1)
+        assert_equal(self.nodes[1].getmempoolinfo()["size"], 0)
+
+        mark_logs("Node 1 mines one block, new height is [Fork - 2]", self.nodes, DEBUG_MODE)
+        self.nodes[1].generate(1)
+        sync_blocks(self.nodes)
+
+        mark_logs("Check that the mempool situation is unchanged", self.nodes, DEBUG_MODE)
+        assert_equal(self.nodes[0].getblockcount(), shielded_pool_deprecation_fork_height - 2)
+        assert_equal(self.nodes[0].getmempoolinfo()["size"], 1)
+        assert_equal(self.nodes[1].getmempoolinfo()["size"], 0)
+
+        mark_logs("Node 1 mines another block, new height is [Fork - 1] (shielding transaction can't be mined in the next block)", self.nodes, DEBUG_MODE)
+        self.nodes[1].generate(1)
+        sync_blocks(self.nodes)
+
+        mark_logs("Check that the shielding transaction has been evicted", self.nodes, DEBUG_MODE)
+        assert_equal(self.nodes[0].getblockcount(), shielded_pool_deprecation_fork_height - 1)
+        assert_equal(self.nodes[0].getmempoolinfo()["size"], 0)
+        assert_equal(self.nodes[1].getmempoolinfo()["size"], 0)
+
+        mark_logs("Check that the shielding transaction is not rebroadcasted even in case of explicit request", self.nodes, DEBUG_MODE)
+        resenttxs = self.nodes[0].resendwallettransactions()
+        self.sync_all()
+        assert_equal(len(resenttxs), 0)
+
+        mark_logs("Check that the funds used for the shielding transaction are available again", self.nodes, DEBUG_MODE)
+        assert_equal(sum(x['amount'] for x in self.nodes[0].listunspent(0)), total_funds_before_shielding)
+        self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), total_funds_before_shielding, "", "", True)
+        assert_equal(self.nodes[0].getmempoolinfo()["size"], 1)
+        self.sync_all()
+
+        mark_logs("RemoveStaleTransaction() function worked properly for block connection on a hard fork", self.nodes, DEBUG_MODE)
+
+
+if __name__ == '__main__':
+    Test().main()

qa/rpc-tests/zcjoinsplit.py

@@ -4,14 +4,12 @@
 # Test joinsplit semantics
 #
 
-from test_framework.test_framework import BitcoinTestFramework, ForkHeights, sync_blocks
+from test_framework.test_framework import BitcoinTestFramework, ForkHeights
 from test_framework.authproxy import JSONRPCException
 from test_framework.util import assert_equal, start_node, assert_greater_than, \
     assert_greater_or_equal_than, gather_inputs, connect_nodes, disconnect_nodes
 from decimal import Decimal
 
-import sys
-
 RPC_VERIFY_REJECTED = -26
 
 class JoinSplitTest(BitcoinTestFramework):
@@ -76,21 +74,15 @@ def run_test(self):
                 # advance chain on node1 (so that shielded pool deprecation hard fork is active)
                 self.nodes[1].generate(fork_crossing_margin)
                 connect_nodes(self.nodes, 0, 1)
-                sync_blocks(self.nodes)
-                # make sure now deprecated tx is in node0 mempool but not in node1 mempool
-                assert_equal(self.nodes[0].getmempoolinfo()["size"], 1)
+                self.sync_all()
+
+                # The deprecated transaction is not in the mempool anymore:
+                # - Node 0 removed the transaction as "stale"
+                # - Node 1 never received the transaction
+                assert_equal(self.nodes[0].getmempoolinfo()["size"], 0)
                 assert_equal(self.nodes[1].getmempoolinfo()["size"], 0)
-                # mine a block including now deprecated tx
-                try:
-                    self.nodes[0].generate(1)
-                    assert(False)
-                except JSONRPCException as e:
-                    # failing at CreateNewBlock->TestBlockValidity
-                    # (very close to block validation failure a node receiving this block would incur into)
-                    print("Expected exception caught during testing due to deprecation (error=" + str(e.error["code"]) + ")")
-                    self.nodes[0].clearmempool()
-                    self.sync_all()
-                    continue # other steps of this round are skipped
+
+                continue # other steps of this round are skipped
 
             self.nodes[0].generate(1)
 
@@ -100,9 +92,9 @@ def run_test(self):
             # The pure joinsplit we create should be mined in the next block
             # despite other transactions being in the mempool.
             addrtest = self.nodes[0].getnewaddress()
-            for xx in range(0,10):
+            for _ in range(0,10):
                 self.nodes[0].generate(1)
-                for x in range(0,50):
+                for _ in range(0,50):
                     self.nodes[0].sendtoaddress(addrtest, 0.01)
 
             joinsplit_tx = self.nodes[0].createrawtransaction([], {})

src/main.cpp

@@ -4288,7 +4288,8 @@ bool static DisconnectTip(CValidationState &state) {
         mempool->removeWithAnchor(anchorBeforeDisconnect);
     }
 
-    mempool->removeStaleTransactions(pcoinsTip, dummyTxs, dummyCerts);
+    bool fHardForkCheckEnabled = ForkManager::getInstance().isCrossHardFork(pcoinsTip->GetHeight() + 1, pcoinsTip->GetHeight() + 2);
+    mempool->removeStaleTransactions(pcoinsTip, dummyTxs, dummyCerts, fHardForkCheckEnabled);
     mempool->removeStaleCertificates(pcoinsTip, dummyCerts);
 
     mempool->check(pcoinsTip);
@@ -4378,7 +4379,8 @@ bool static ConnectTip(CValidationState &state, CBlockIndex *pindexNew, CBlock *
     mempool->removeForBlock(pblock->vtx, pindexNew->nHeight, removedTxs,  removedCerts, !IsInitialBlockDownload());
     mempool->removeForBlock(pblock->vcert, pindexNew->nHeight, removedTxs, removedCerts);
 
-    mempool->removeStaleTransactions(pcoinsTip, removedTxs, removedCerts);
+    bool fHardForkCheckEnabled = ForkManager::getInstance().isCrossHardFork(pcoinsTip->GetHeight(), pcoinsTip->GetHeight() + 1);
+    mempool->removeStaleTransactions(pcoinsTip, removedTxs, removedCerts, fHardForkCheckEnabled);
     mempool->removeStaleCertificates(pcoinsTip, removedCerts);
 
     mempool->check(pcoinsTip);

src/txmempool.cpp

@@ -1110,8 +1110,8 @@ void CTxMemPool::removeConflicts(const CTransaction &tx, std::list<CTransaction>
     removeOutOfScBalanceCsw(pcoinsTip, removedTxs, removedCerts);
 }
 
-void CTxMemPool::removeStaleTransactions(const CCoinsViewCache * const pCoinsView,
-                                         std::list<CTransaction>& outdatedTxs, std::list<CScCertificate>& outdatedCerts)
+void CTxMemPool::removeStaleTransactions(const CCoinsViewCache * const pCoinsView, std::list<CTransaction>& outdatedTxs,
+                                         std::list<CScCertificate>& outdatedCerts, bool fHardForkCheckEnabled)
 {
     LOCK(cs);
     std::set<uint256> txesToRemove;
@@ -1162,6 +1162,14 @@ void CTxMemPool::removeStaleTransactions(const CCoinsViewCache * const pCoinsVie
                 continue;
             }
         }
+
+        if (fHardForkCheckEnabled)
+        {
+            CValidationState dummyState;
+            if(!tx.ContextualCheck(dummyState, pcoinsTip->GetHeight() + 1, 0)) {
+                txesToRemove.insert(tx.GetHash());
+            }
+        }
     }
 
     for(const auto& hash: txesToRemove)

src/txmempool.h

@@ -255,8 +255,8 @@ class CTxMemPool
                          std::list<CTransaction>& removedTxs, std::list<CScCertificate>& removedCerts);
     void removeOutOfScBalanceCsw(const CCoinsViewCache * const pCoinsView,
                                  std::list<CTransaction> &removedTxs, std::list<CScCertificate> &removedCerts);
-    void removeStaleTransactions(const CCoinsViewCache * const pCoinsView,
-                                 std::list<CTransaction>& outdatedTxs, std::list<CScCertificate>& outdatedCerts);
+    void removeStaleTransactions(const CCoinsViewCache * const pCoinsView, std::list<CTransaction>& outdatedTxs,
+                                 std::list<CScCertificate>& outdatedCerts, bool fHardForkCheckEnabled = false);
     // END OF UNCONFIRMED TRANSACTIONS CLEANUP METHODS
 
     // UNCONFIRMED CERTIFICATES CLEANUP METHODS

src/zen/forkmanager.cpp

@@ -45,6 +45,14 @@ void ForkManager::selectNetwork(const CBaseChainParams::Network network) {
     currentNetwork = network;
 }
 
+/**
+ * @brief determine if between two heights at least one hard-fork gets crossed
+ */
+bool ForkManager::isCrossHardFork(int fromHeight, int toHeight)
+{
+    return getForkAtHeight(fromHeight)->getHeight(currentNetwork) != getForkAtHeight(toHeight)->getHeight(currentNetwork);
+}
+
 /**
  * @brief getCommunityFundAddress returns the community fund address based on the passed in height and maxHeight
  * @param height the height

src/zen/forkmanager.h

@@ -40,6 +40,13 @@ class ForkManager
      */
     void selectNetwork(CBaseChainParams::Network network);
 
+    /**
+     * @brief determine if between two heights at least one hard-fork gets crossed
+     */
+    bool isCrossHardFork(int fromHeight, int toHeight);
+
+    // ---------- forks redefined methods ----------
+
     /**
      * @brief getCommunityFundAddress returns the community fund address based on the passed in height and maxHeight
      */
@@ -135,6 +142,8 @@ class ForkManager
      */
     bool isShieldingForbidden(int height) const;
 
+    // ---------- forks redefined methods ----------
+
 private:
 
     /**