Merge pull request #137 from DeFiCh/fix/prevent_invalid_tx_in_mempool

Prevent bad tx propagation

src/masternodes/mn_checks.h

@@ -112,25 +112,25 @@ bool HasCollateralAuth(CTransaction const & tx, CCoinsViewCache const & coins, u
 Res ApplyCustomTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, const Consensus::Params& consensusParams, uint32_t height, uint32_t txn, bool isCheck = true, bool skipAuth = false);
 //! Deep check (and write)
 Res ApplyCreateMasternodeTx(CCustomCSView & mnview, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata);
-Res ApplyResignMasternodeTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata);
+Res ApplyResignMasternodeTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, bool skipAuth = false);
 
-Res ApplyCreateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
-Res ApplyUpdateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
-Res ApplyUpdateTokenAnyTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
+Res ApplyCreateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
+Res ApplyUpdateTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
+Res ApplyUpdateTokenAnyTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
 Res ApplyMintTokenTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
 
-Res ApplyCreatePoolPairTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
-Res ApplyUpdatePoolPairTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
-Res ApplyPoolSwapTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
-Res ApplyAddPoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
-Res ApplyRemovePoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
+Res ApplyCreatePoolPairTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
+Res ApplyUpdatePoolPairTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
+Res ApplyPoolSwapTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
+Res ApplyAddPoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
+Res ApplyRemovePoolLiquidityTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
 
 Res ApplyUtxosToAccountTx(CCustomCSView & mnview, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
 Res ApplyAccountToUtxosTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
-Res ApplyAccountToAccountTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
-Res ApplyAnyAccountsToAccountsTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
+Res ApplyAccountToAccountTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
+Res ApplyAnyAccountsToAccountsTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
 
-Res ApplySetGovernanceTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
+Res ApplySetGovernanceTx(CCustomCSView & mnview, CCoinsViewCache const & coins, CTransaction const & tx, uint32_t height, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams, bool skipAuth = false);
 
 ResVal<uint256> ApplyAnchorRewardTx(CCustomCSView & mnview, CTransaction const & tx, int height, uint256 const & prevStakeModifier, std::vector<unsigned char> const & metadata, Consensus::Params const & consensusParams);
 

src/miner.cpp

@@ -561,10 +561,10 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda
 
             // Only check custom TXs
             if (txType != CustomTxType::None) {
-                auto res = ApplyCustomTx(view, ::ChainstateActive().CoinsTip(), tx, chainparams.GetConsensus(), nHeight, std::numeric_limits<uint32_t>::max(), false, true);
+                auto res = ApplyCustomTx(view, ::ChainstateActive().CoinsTip(), tx, chainparams.GetConsensus(), nHeight, 0, false, true);
 
                 // Not okay invalidate, undo and skip
-                if (!res.ok && NotAllowedToFail(txType)) {
+                if (!res.ok) {
                     customTxPassed = false;
 
                     LogPrintf("%s: Failed %s TX %s: %s\n", __func__, ToString(txType), tx.GetHash().GetHex(), res.msg);

src/validation.cpp

@@ -561,6 +561,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
     {
         CCoinsView dummy;
         CCoinsViewCache view(&dummy);
+        CCustomCSView mnview(*pcustomcsview);
 
         LockPoints lp;
         CCoinsViewCache& coins_cache = ::ChainstateActive().CoinsTip();
@@ -601,11 +602,31 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
         // Bring the best block into scope
         view.GetBestBlock();
 
+        const auto height = GetSpendHeight(view);
+
+        // check for txs in mempool
+        for (const auto& e : mempool.mapTx.get<entry_time>()) {
+            const auto& tx = e.GetTx();
+            auto res = ApplyCustomTx(mnview, view, tx, chainparams.GetConsensus(), height, 0, false);
+            assert(res.ok || !(res.code & CustomTxErrCodes::Fatal)); // inconsistent mempool
+        }
+
         CAmount nFees = 0;
-        if (!Consensus::CheckTxInputs(tx, state, view, pcustomcsview.get(), GetSpendHeight(view), nFees, chainparams)) {
+        if (!Consensus::CheckTxInputs(tx, state, view, &mnview, height, nFees, chainparams)) {
             return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state));
         }
 
+        // NOTE Consensus::CheckTxInputs will check for NotAllowToFail
+        std::vector<unsigned char> metadata;
+        const auto txType = GuessCustomTxType(tx, metadata);
+
+        if (!NotAllowedToFail(txType)) {
+            auto res = ApplyCustomTx(mnview, view, tx, chainparams.GetConsensus(), height, 0, false);
+            if (!res.ok || (res.code & CustomTxErrCodes::Fatal)) {
+                return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INVALID, res.msg);
+            }
+        }
+
         // we have all inputs cached now, so switch back to dummy, so we don't need to keep lock on mempool
         view.SetBackend(dummy);
 

src/wallet/wallet.cpp

@@ -2152,8 +2152,19 @@ void CWallet::ReacceptWalletTransactions(interfaces::Chain::Lock& locked_chain)
     // Try to add wallet transactions to memory pool
     for (const std::pair<const int64_t, CWalletTx*>& item : mapSorted) {
         CWalletTx& wtx = *(item.second);
-        std::string unused_err_string;
-        wtx.SubmitMemoryPoolAndRelay(unused_err_string, false, locked_chain);
+        std::string err_string;
+        bool result = wtx.SubmitMemoryPoolAndRelay(err_string, false, locked_chain);
+
+        // err_string only set on mempool acceptance failure
+        if (!result && !err_string.empty()) {
+            std::vector<unsigned char> metadata;
+            CustomTxType txType = GuessCustomTxType(*item.second->tx, metadata);
+
+            // Abandon custom TXs that are rejected by mempool
+            if (txType != CustomTxType::None) {
+                AbandonTransaction(locked_chain, item.second->tx->GetHash());
+            }
+        }
     }
 }
 

test/functional/feature_account_mining.py

@@ -7,6 +7,7 @@
 
 from test_framework.test_framework import DefiTestFramework
 from test_framework.util import assert_equal
+from test_framework.authproxy import JSONRPCException
 
 class AccountMiningTest(DefiTestFramework):
     def set_test_params(self):
@@ -28,8 +29,13 @@ def run_test(self):
         assert_equal(node.getaccount(account)[0], "10.00000000@DFI")
 
         # Send double the amount we have in account
+        thrown = False
         for _ in range(100):
-            node.accounttoutxos(account, {destination: "2@DFI"})
+            try:
+                node.accounttoutxos(account, {destination: "2@DFI"})
+            except JSONRPCException:
+                thrown = True
+        assert_equal(thrown, True)
 
         # Store block height
         blockcount = node.getblockcount()
@@ -40,8 +46,40 @@ def run_test(self):
         # Check the blockchain has extended as expected
         assert_equal(node.getblockcount(), blockcount + 1)
 
+        # Generate 10 more blocks
+        node.generate(10)
+
+        # Check the blockchain has extended as expected
+        assert_equal(node.getblockcount(), blockcount + 11)
+
         # Account should now be empty
         assert_equal(node.getaccount(account), [])
 
+        # Update block height
+        blockcount = node.getblockcount()
+
+        # Send more UTXOs to account
+        node.utxostoaccount({account: "1@0"})
+        node.generate(1)
+
+        # Update block height
+        blockcount = node.getblockcount()
+
+        # Test mixture of account TXs
+        thrown = False
+        for _ in range(10):
+            try:
+                node.accounttoaccount(account, {destination: "1@DFI"})
+                node.accounttoutxos(account, {destination: "1@DFI"})
+            except JSONRPCException:
+                thrown = True
+        assert_equal(thrown, True)
+
+        # Generate 1 more blocks
+        node.generate(1)
+
+        # Check the blockchain has extended as expected
+        assert_equal(node.getblockcount(), blockcount + 1)
+
 if __name__ == '__main__':
     AccountMiningTest().main ()

test/functional/feature_prevent_bad_tx_propagation.py

@@ -0,0 +1,57 @@
+#!/usr/bin/env python3
+# Copyright (c) 2014-2019 The Bitcoin Core developers
+# Copyright (c) DeFi Blockchain Developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test account mining behaviour"""
+
+from test_framework.authproxy import JSONRPCException
+from test_framework.test_framework import DefiTestFramework
+from test_framework.util import assert_equal
+
+class AccountMiningTest(DefiTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+        self.setup_clean_chain = True
+        self.extra_args = [['-txnotokens=0', '-amkheight=50']]
+
+    def run_test(self):
+        node = self.nodes[0]
+        node.generate(120)
+
+        # Get addresses and set up account
+        account = node.getnewaddress()
+        destination = node.getnewaddress()
+
+        node.utxostoaccount({account: "5@0"})
+
+        node.generate(1)
+
+        # Check we have expected balance
+        assert_equal(node.getaccount(account)[0], "5.00000000@DFI")
+
+        # Corrent account to utxo tx - entering mempool
+        node.accounttoutxos(account, {destination: "4@DFI"})
+
+        try:
+            # Not enough amount - rejected
+            node.accounttoutxos(account, {destination: "2@DFI"})
+        except JSONRPCException as e:
+            errorString = e.error['message']
+
+        assert('bad-txns-customtx' in errorString)
+
+        # Store block height
+        blockcount = node.getblockcount()
+
+        # One minted block with correct tx
+        node.generate(1)
+
+        # Check the blockchain height
+        assert_equal(node.getblockcount(), blockcount + 1)
+
+        # Account should have 1@DFI
+        assert_equal(node.getaccount(account)[0], "1.00000000@DFI")
+
+if __name__ == '__main__':
+    AccountMiningTest().main ()

test/functional/feature_tokens_multisig.py

@@ -98,13 +98,12 @@ def run_test(self):
         signed_rawtx_1 = self.nodes[0].signrawtransactionwithkey(rawtx_1, [owner_1_privkey], [{"txid":txid_owner_1,"vout":vout_owner_1,"scriptPubKey":owner_1_scriptpubkey}])
         assert_equal(signed_rawtx_1['complete'], True)
 
-        # Send first name change TX
-        self.nodes[0].sendrawtransaction(signed_rawtx_1['hex'])
-        self.nodes[0].generate(1)
-
-        # Check that name has not changed
-        t128 = self.nodes[0].gettoken(128)
-        assert_equal(t128['128']['name'], "shiny")
+        # Send should fail as transaction is invalid
+        try:
+            self.nodes[0].sendrawtransaction(signed_rawtx_1['hex'])
+        except JSONRPCException as e:
+            errorString = e.error['message']
+        assert("tx must have at least one input from token owner" in errorString)
 
         # Test that multisig TXs can change names
         rawtx_1 = self.nodes[0].createrawtransaction([{"txid":txid_1,"vout":vout_1}], [{"data":name_change_1},{owner_1:0.9999}])

test/functional/test_runner.py

@@ -233,6 +233,7 @@
     'rpc_help.py',
     'feature_help.py',
     'feature_shutdown.py',
+    'feature_prevent_bad_tx_propagation.py'
     # Don't append tests at the end to avoid merge conflicts
     # Put them in a random line within the section that fits their approximate run-time
 ]