Merge pull request #6445

5715460 Always reject duplicate key-images from second txid (vtnerd)
babf25d Allow unrestricted rpc calls to get full txpool info (vtnerd)

9 files changed, 104 insertions, 52 deletions

diff --git a/src/cryptonote_core/cryptonote_core.cpp b/src/cryptonote_core/cryptonote_core.cpp
index 10bbff457..3ff3c77e2 100644
--- a/src/cryptonote_core/cryptonote_core.cpp
+++ b/src/cryptonote_core/cryptonote_core.cpp
@@ -428,9 +428,9 @@ namespace cryptonote
     return m_blockchain_storage.get_split_transactions_blobs(txs_ids, txs, missed_txs);
   }
   //-----------------------------------------------------------------------------------------------
-  bool core::get_txpool_backlog(std::vector<tx_backlog_entry>& backlog) const
+  bool core::get_txpool_backlog(std::vector<tx_backlog_entry>& backlog, bool include_sensitive_txes) const
   {
-    m_mempool.get_transaction_backlog(backlog);
+    m_mempool.get_transaction_backlog(backlog, include_sensitive_txes);
     return true;
   }
   //-----------------------------------------------------------------------------------------------
@@ -1544,9 +1544,9 @@ namespace cryptonote
     return m_blockchain_storage.get_db().get_block_cumulative_difficulty(height);
   }
   //-----------------------------------------------------------------------------------------------
-  size_t core::get_pool_transactions_count() const
+  size_t core::get_pool_transactions_count(bool include_sensitive_txes) const
   {
-    return m_mempool.get_transactions_count();
+    return m_mempool.get_transactions_count(include_sensitive_txes);
   }
   //-----------------------------------------------------------------------------------------------
   bool core::have_block(const crypto::hash& id) const
diff --git a/src/cryptonote_core/cryptonote_core.h b/src/cryptonote_core/cryptonote_core.h
index 79a846de1..255645efc 100644
--- a/src/cryptonote_core/cryptonote_core.h
+++ b/src/cryptonote_core/cryptonote_core.h
@@ -469,10 +469,11 @@ namespace cryptonote
 
      /**
       * @copydoc tx_memory_pool::get_txpool_backlog
+      * @param include_sensitive_txes include private transactions
       *
       * @note see tx_memory_pool::get_txpool_backlog
       */
-     bool get_txpool_backlog(std::vector<tx_backlog_entry>& backlog) const;
+     bool get_txpool_backlog(std::vector<tx_backlog_entry>& backlog, bool include_sensitive_txes = false) const;
      
      /**
       * @copydoc tx_memory_pool::get_transactions
@@ -514,10 +515,11 @@ namespace cryptonote
 
      /**
       * @copydoc tx_memory_pool::get_transactions_count
+      * @param include_sensitive_txes include private transactions
       *
       * @note see tx_memory_pool::get_transactions_count
       */
-     size_t get_pool_transactions_count() const;
+     size_t get_pool_transactions_count(bool include_sensitive_txes = false) const;
 
      /**
       * @copydoc Blockchain::get_total_transactions
diff --git a/src/cryptonote_core/tx_pool.cpp b/src/cryptonote_core/tx_pool.cpp
index c49a3dabc..a7b2e4422 100644
--- a/src/cryptonote_core/tx_pool.cpp
+++ b/src/cryptonote_core/tx_pool.cpp
@@ -210,7 +210,7 @@ namespace cryptonote
     // TODO: Investigate why not?
     if(!kept_by_block)
     {
-      if(have_tx_keyimges_as_spent(tx))
+      if(have_tx_keyimges_as_spent(tx, id))
       {
         mark_double_spend(tx);
         LOG_PRINT_L1("Transaction with id= "<< id << " used already spent key images");
@@ -253,7 +253,7 @@ namespace cryptonote
         meta.last_relayed_time = time(NULL);
         meta.relayed = relayed;
         meta.set_relay_method(tx_relay);
-        meta.double_spend_seen = have_tx_keyimges_as_spent(tx);
+        meta.double_spend_seen = have_tx_keyimges_as_spent(tx, id);
         meta.pruned = tx.pruned;
         meta.bf_padding = 0;
         memset(meta.padding, 0, sizeof(meta.padding));
@@ -1098,30 +1098,32 @@ namespace cryptonote
     return m_blockchain.get_db().txpool_has_tx(id, tx_category);
   }
   //---------------------------------------------------------------------------------
-  bool tx_memory_pool::have_tx_keyimges_as_spent(const transaction& tx) const
+  bool tx_memory_pool::have_tx_keyimges_as_spent(const transaction& tx, const crypto::hash& txid) const
   {
     CRITICAL_REGION_LOCAL(m_transactions_lock);
     CRITICAL_REGION_LOCAL1(m_blockchain);
     for(const auto& in: tx.vin)
     {
       CHECKED_GET_SPECIFIC_VARIANT(in, const txin_to_key, tokey_in, true);//should never fail
-      if(have_tx_keyimg_as_spent(tokey_in.k_image))
+      if(have_tx_keyimg_as_spent(tokey_in.k_image, txid))
          return true;
     }
     return false;
   }
   //---------------------------------------------------------------------------------
-  bool tx_memory_pool::have_tx_keyimg_as_spent(const crypto::key_image& key_im) const
+  bool tx_memory_pool::have_tx_keyimg_as_spent(const crypto::key_image& key_im, const crypto::hash& txid) const
   {
     CRITICAL_REGION_LOCAL(m_transactions_lock);
-    bool spent = false;
     const auto found = m_spent_key_images.find(key_im);
-    if (found != m_spent_key_images.end())
+    if (found != m_spent_key_images.end() && !found->second.empty())
     {
-      for (const crypto::hash& tx_hash : found->second)
-        spent |= m_blockchain.txpool_tx_matches_category(tx_hash, relay_category::broadcasted);
+      // If another tx is using the key image, always return as spent.
+      // See `insert_key_images`.
+      if (1 < found->second.size() || *(found->second.cbegin()) != txid)
+        return true;
+      return m_blockchain.txpool_tx_matches_category(txid, relay_category::broadcasted);
     }
-    return spent;
+    return false;
   }
   //---------------------------------------------------------------------------------
   void tx_memory_pool::lock() const
diff --git a/src/cryptonote_core/tx_pool.h b/src/cryptonote_core/tx_pool.h
index ca0e50415..292d427e2 100644
--- a/src/cryptonote_core/tx_pool.h
+++ b/src/cryptonote_core/tx_pool.h
@@ -470,10 +470,11 @@ namespace cryptonote
      * @brief check if a transaction in the pool has a given spent key image
      *
      * @param key_im the spent key image to look for
+     * @param txid hash of the new transaction where `key_im` was seen.
      *
      * @return true if the spent key image is present, otherwise false
      */
-    bool have_tx_keyimg_as_spent(const crypto::key_image& key_im) const;
+    bool have_tx_keyimg_as_spent(const crypto::key_image& key_im, const crypto::hash& txid) const;
 
     /**
      * @brief check if any spent key image in a transaction is in the pool
@@ -484,10 +485,11 @@ namespace cryptonote
      * @note see tx_pool::have_tx_keyimg_as_spent
      *
      * @param tx the transaction to check spent key images of
+     * @param txid hash of `tx`.
      *
      * @return true if any spent key images are present in the pool, otherwise false
      */
-    bool have_tx_keyimges_as_spent(const transaction& tx) const;
+    bool have_tx_keyimges_as_spent(const transaction& tx, const crypto::hash& txid) const;
 
     /**
      * @brief forget a transaction's spent key images
diff --git a/src/rpc/core_rpc_server.cpp b/src/rpc/core_rpc_server.cpp
index 1fd0c037b..9b0eeb1f1 100644
--- a/src/rpc/core_rpc_server.cpp
+++ b/src/rpc/core_rpc_server.cpp
@@ -438,7 +438,7 @@ namespace cryptonote
     store_difficulty(m_core.get_blockchain_storage().get_difficulty_for_next_block(), res.difficulty, res.wide_difficulty, res.difficulty_top64);
     res.target = m_core.get_blockchain_storage().get_difficulty_target();
     res.tx_count = m_core.get_blockchain_storage().get_total_transactions() - res.height; //without coinbase
-    res.tx_pool_size = m_core.get_pool_transactions_count();
+    res.tx_pool_size = m_core.get_pool_transactions_count(!restricted);
     res.alt_blocks_count = restricted ? 0 : m_core.get_blockchain_storage().get_alternative_blocks_count();
     uint64_t total_conn = restricted ? 0 : m_p2p.get_public_connections_count();
     res.outgoing_connections_count = restricted ? 0 : m_p2p.get_public_outgoing_connections_count();
@@ -1119,6 +1119,8 @@ namespace cryptonote
     }
     res.sanity_check_failed = false;
 
+    const bool restricted = m_restricted && ctx;
+
     tx_verification_context tvc{};
     if(!m_core.handle_incoming_tx({tx_blob, crypto::null_hash}, tvc, (req.do_not_relay ? relay_method::none : relay_method::local), false) || tvc.m_verifivation_failed)
     {
@@ -1423,12 +1425,13 @@ namespace cryptonote
 
     const bool restricted = m_restricted && ctx;
     const bool request_has_rpc_origin = ctx != NULL;
+    const bool allow_sensitive = !request_has_rpc_origin || !restricted;
 
-    size_t n_txes = m_core.get_pool_transactions_count();
+    size_t n_txes = m_core.get_pool_transactions_count(allow_sensitive);
     if (n_txes > 0)
     {
       CHECK_PAYMENT_SAME_TS(req, res, n_txes * COST_PER_TX);
-      m_core.get_pool_transactions_and_spent_keys_info(res.transactions, res.spent_key_images, !request_has_rpc_origin || !restricted);
+      m_core.get_pool_transactions_and_spent_keys_info(res.transactions, res.spent_key_images, allow_sensitive);
       for (tx_info& txi : res.transactions)
         txi.tx_blob = epee::string_tools::buff_to_hex_nodelimer(txi.tx_blob);
     }
@@ -1448,12 +1451,13 @@ namespace cryptonote
 
     const bool restricted = m_restricted && ctx;
     const bool request_has_rpc_origin = ctx != NULL;
+    const bool allow_sensitive = !request_has_rpc_origin || !restricted;
 
-    size_t n_txes = m_core.get_pool_transactions_count();
+    size_t n_txes = m_core.get_pool_transactions_count(allow_sensitive);
     if (n_txes > 0)
     {
       CHECK_PAYMENT_SAME_TS(req, res, n_txes * COST_PER_POOL_HASH);
-      m_core.get_pool_transaction_hashes(res.tx_hashes, !request_has_rpc_origin || !restricted);
+      m_core.get_pool_transaction_hashes(res.tx_hashes, allow_sensitive);
     }
 
     res.status = CORE_RPC_STATUS_OK;
@@ -1471,13 +1475,14 @@ namespace cryptonote
 
     const bool restricted = m_restricted && ctx;
     const bool request_has_rpc_origin = ctx != NULL;
+    const bool allow_sensitive = !request_has_rpc_origin || !restricted;
 
-    size_t n_txes = m_core.get_pool_transactions_count();
+    size_t n_txes = m_core.get_pool_transactions_count(allow_sensitive);
     if (n_txes > 0)
     {
       CHECK_PAYMENT_SAME_TS(req, res, n_txes * COST_PER_POOL_HASH);
       std::vector<crypto::hash> tx_hashes;
-      m_core.get_pool_transaction_hashes(tx_hashes, !request_has_rpc_origin || !restricted);
+      m_core.get_pool_transaction_hashes(tx_hashes, allow_sensitive);
       res.tx_hashes.reserve(tx_hashes.size());
       for (const crypto::hash &tx_hash: tx_hashes)
         res.tx_hashes.push_back(epee::string_tools::pod_to_hex(tx_hash));
diff --git a/tests/functional_tests/functional_tests_rpc.py b/tests/functional_tests/functional_tests_rpc.py
index 5f2a3d077..42d14e91a 100755
--- a/tests/functional_tests/functional_tests_rpc.py
+++ b/tests/functional_tests/functional_tests_rpc.py
@@ -34,8 +34,8 @@ try:
 except:
   tests = DEFAULT_TESTS
 
-N_MONERODS = 2
-N_WALLETS = 4
+N_MONERODS = 3
+N_WALLETS = 7
 WALLET_DIRECTORY = builddir + "/functional-tests-directory"
 DIFFICULTY = 10
 
@@ -43,9 +43,17 @@ monerod_base = [builddir + "/bin/monerod", "--regtest", "--fixed-difficulty", st
 monerod_extra = [
   [],
   ["--rpc-payment-address", "44SKxxLQw929wRF6BA9paQ1EWFshNnKhXM3qz6Mo3JGDE2YG3xyzVutMStEicxbQGRfrYvAAYxH6Fe8rnD56EaNwUiqhcwR", "--rpc-payment-difficulty", str(DIFFICULTY), "--rpc-payment-credits", "5000", "--data-dir", builddir + "/functional-tests-directory/monerod1"],
+  ["--rpc-restricted-bind-port", "18482", "--data-dir", builddir + "/functional-tests-directory/monerod2"]
 ]
-wallet_base = [builddir + "/bin/monero-wallet-rpc", "--wallet-dir", WALLET_DIRECTORY, "--rpc-bind-port", "wallet_port", "--disable-rpc-login", "--rpc-ssl", "disabled", "--daemon-ssl", "disabled", "--daemon-port", "18180", "--log-level", "1"]
+wallet_base = [builddir + "/bin/monero-wallet-rpc", "--wallet-dir", WALLET_DIRECTORY, "--rpc-bind-port", "wallet_port", "--disable-rpc-login", "--rpc-ssl", "disabled", "--daemon-ssl", "disabled", "--log-level", "1"]
 wallet_extra = [
+  ["--daemon-port", "18180"],
+  ["--daemon-port", "18180"],
+  ["--daemon-port", "18180"],
+  ["--daemon-port", "18180"],
+  ["--daemon-port", "18182"],
+  ["--daemon-port", "18182"],
+  ["--daemon-port", "18182"]
 ]
 
 command_lines = []
diff --git a/tests/functional_tests/transfer.py b/tests/functional_tests/transfer.py
index c3d71aa9c..f7a39fa0c 100755
--- a/tests/functional_tests/transfer.py
+++ b/tests/functional_tests/transfer.py
@@ -55,7 +55,7 @@ class TransferTest():
 
     def reset(self):
         print('Resetting blockchain')
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
         res = daemon.get_height()
         daemon.pop_blocks(res.height - 1)
         daemon.flush_txpool()
@@ -69,7 +69,7 @@ class TransferTest():
         ]
         self.wallet = [None] * len(seeds)
         for i in range(len(seeds)):
-            self.wallet[i] = Wallet(idx = i)
+            self.wallet[i] = Wallet(idx = i + 4)
             # close the wallet if any, will throw if none is loaded
             try: self.wallet[i].close_wallet()
             except: pass
@@ -77,7 +77,7 @@ class TransferTest():
 
     def mine(self):
         print("Mining some blocks")
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
 
         res = daemon.get_info()
         height = res.height
@@ -89,7 +89,7 @@ class TransferTest():
             assert res.height == height + 80
 
     def transfer(self):
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
 
         print("Creating transfer to self")
 
@@ -508,7 +508,7 @@ class TransferTest():
     def check_get_bulk_payments(self):
         print('Checking get_bulk_payments')
 
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
         res = daemon.get_info()
         height = res.height
 
@@ -544,7 +544,7 @@ class TransferTest():
     def check_get_payments(self):
         print('Checking get_payments')
 
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
         res = daemon.get_info()
         height = res.height
 
@@ -587,7 +587,8 @@ class TransferTest():
             assert len(res.tx_blob_list) == 1
             txes[i][1] = res.tx_blob_list[0]
 
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
+        restricted_daemon = Daemon(idx = 2, restricted_rpc = True)
         res = daemon.send_raw_transaction(txes[0][1])
         assert res.not_relayed == False
         assert res.low_mixin == False
@@ -598,6 +599,18 @@ class TransferTest():
         assert res.overspend == False
         assert res.fee_too_low == False
 
+        res = restricted_daemon.send_raw_transaction(txes[0][1])
+        assert res.not_relayed == False
+        assert res.low_mixin == False
+        assert res.double_spend == False
+        assert res.invalid_input == False
+        assert res.invalid_output == False
+        assert res.too_big == False
+        assert res.overspend == False
+        assert res.fee_too_low == False
+
+        res = restricted_daemon.get_transactions([txes[0][0]])
+        assert not 'txs' in res or len(res.txs) == 0
         res = daemon.get_transactions([txes[0][0]])
         assert len(res.txs) >= 1
         tx = [tx for tx in res.txs if tx.tx_hash == txes[0][0]][0]
@@ -615,6 +628,19 @@ class TransferTest():
         assert res.fee_too_low == False
         assert res.too_few_outputs == False
 
+        res = restricted_daemon.send_raw_transaction(txes[1][1])
+        assert res.not_relayed == False
+        assert res.low_mixin == False
+        assert res.double_spend == True
+        assert res.invalid_input == False
+        assert res.invalid_output == False
+        assert res.too_big == False
+        assert res.overspend == False
+        assert res.fee_too_low == False
+        assert res.too_few_outputs == False
+
+        res = restricted_daemon.get_transactions([txes[0][0]])
+        assert not 'txs' in res or len(res.txs) == 0
         res = daemon.get_transactions([txes[0][0]])
         assert len(res.txs) >= 1
         tx = [tx for tx in res.txs if tx.tx_hash == txes[0][0]][0]
@@ -623,13 +649,13 @@ class TransferTest():
 
     def sweep_dust(self):
         print("Sweeping dust")
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
         self.wallet[0].refresh()
         res = self.wallet[0].sweep_dust()
         assert not 'tx_hash_list' in res or len(res.tx_hash_list) == 0 # there's just one, but it cannot meet the fee
 
     def sweep_single(self):
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
 
         print("Sending single output")
 
@@ -685,7 +711,7 @@ class TransferTest():
         assert len([t for t in res.transfers if t.key_image == ki]) == 1
 
     def check_destinations(self):
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
 
         print("Checking transaction destinations")
 
@@ -741,7 +767,7 @@ class TransferTest():
                 self.wallet[0].refresh()
 
     def check_tx_notes(self):
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
 
         print('Testing tx notes')
         res = self.wallet[0].get_transfers()
@@ -758,7 +784,7 @@ class TransferTest():
         assert res.notes == ['out txid', 'in txid']
 
     def check_rescan(self):
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
 
         print('Testing rescan_spent')
         res = self.wallet[0].incoming_transfers(transfer_type = 'all')
@@ -798,7 +824,7 @@ class TransferTest():
             assert sorted(old_t_out, key = lambda k: k['txid']) == sorted(new_t_out, key = lambda k: k['txid'])
 
     def check_is_key_image_spent(self):
-        daemon = Daemon()
+        daemon = Daemon(idx = 2)
 
         print('Testing is_key_image_spent')
         res = self.wallet[0].incoming_transfers(transfer_type = 'all')
diff --git a/tests/functional_tests/txpool.py b/tests/functional_tests/txpool.py
index 27ae89764..2d7f69f3c 100755
--- a/tests/functional_tests/txpool.py
+++ b/tests/functional_tests/txpool.py
@@ -45,14 +45,14 @@ class TransferTest():
 
     def reset(self):
         print('Resetting blockchain')
-        daemon = Daemon()
+        daemon = Daemon(idx=2)
         res = daemon.get_height()
         daemon.pop_blocks(res.height - 1)
         daemon.flush_txpool()
 
     def create(self):
         print('Creating wallet')
-        wallet = Wallet()
+        wallet = Wallet(idx = 4)
         # close the wallet if any, will throw if none is loaded
         try: wallet.close_wallet()
         except: pass
@@ -61,8 +61,8 @@ class TransferTest():
 
     def mine(self):
         print("Mining some blocks")
-        daemon = Daemon()
-        wallet = Wallet()
+        daemon = Daemon(idx = 2)
+        wallet = Wallet(idx = 4)
 
         daemon.generateblocks('42ey1afDFnn4886T7196doS9GPMzexD9gXpsZJDwVjeRVdFCSoHnv7KPbBeGpzJBzHRCAs9UxqeoyFQMYbqSWYTfJJQAWDm', 80)
         wallet.refresh()
@@ -70,8 +70,8 @@ class TransferTest():
     def create_txes(self, address, ntxes):
         print('Creating ' + str(ntxes) + ' transactions')
 
-        daemon = Daemon()
-        wallet = Wallet()
+        daemon = Daemon(idx = 2)
+        wallet = Wallet(idx = 4)
 
         dst = {'address': address, 'amount': 1000000000000}
 
@@ -83,8 +83,10 @@ class TransferTest():
         return txes
 
     def check_empty_pool(self):
-        daemon = Daemon()
+        self.check_empty_rpc_pool(Daemon(idx = 2))
+        self.check_empty_rpc_pool(Daemon(idx = 2, restricted_rpc = True))
 
+    def check_empty_rpc_pool(self, daemon):
         res = daemon.get_transaction_pool_hashes()
         assert not 'tx_hashes' in res or len(res.tx_hashes) == 0
         res = daemon.get_transaction_pool_stats()
@@ -103,8 +105,9 @@ class TransferTest():
         assert res.pool_stats.num_double_spends == 0
 
     def check_txpool(self):
-        daemon = Daemon()
-        wallet = Wallet()
+        daemon = Daemon(idx = 2)
+        restricted_daemon = Daemon(idx = 2, restricted_rpc = True)
+        wallet = Wallet(idx = 4)
 
         res = daemon.get_info()
         height = res.height
@@ -117,6 +120,7 @@ class TransferTest():
         res = daemon.get_info()
         assert res.tx_pool_size == txpool_size + 5
         txpool_size = res.tx_pool_size
+        self.check_empty_rpc_pool(restricted_daemon)
 
         res = daemon.get_transaction_pool()
         assert len(res.transactions) == txpool_size
@@ -160,6 +164,7 @@ class TransferTest():
         print('Flushing 2 transactions')
         txes_keys = list(txes.keys())
         daemon.flush_txpool([txes_keys[1], txes_keys[3]])
+        self.check_empty_rpc_pool(restricted_daemon)
         res = daemon.get_transaction_pool()
         assert len(res.transactions) == txpool_size - 2
         assert len([x for x in res.transactions if x.id_hash == txes_keys[1]]) == 0
@@ -210,6 +215,7 @@ class TransferTest():
         print('Flushing unknown transactions')
         unknown_txids = ['1'*64, '2'*64, '3'*64]
         daemon.flush_txpool(unknown_txids)
+        self.check_empty_rpc_pool(restricted_daemon)
         res = daemon.get_transaction_pool()
         assert len(res.transactions) == txpool_size - 2
 
diff --git a/utils/python-rpc/framework/daemon.py b/utils/python-rpc/framework/daemon.py
index 749d9ed88..074f8de37 100644
--- a/utils/python-rpc/framework/daemon.py
+++ b/utils/python-rpc/framework/daemon.py
@@ -32,10 +32,11 @@ from .rpc import JSONRPC
 
 class Daemon(object):
 
-    def __init__(self, protocol='http', host='127.0.0.1', port=0, idx=0):
+    def __init__(self, protocol='http', host='127.0.0.1', port=0, idx=0, restricted_rpc = False):
+        base = 18480 if restricted_rpc else 18180
         self.host = host
         self.port = port
-        self.rpc = JSONRPC('{protocol}://{host}:{port}'.format(protocol=protocol, host=host, port=port if port else 18180+idx))
+        self.rpc = JSONRPC('{protocol}://{host}:{port}'.format(protocol=protocol, host=host, port=port if port else base+idx))
 
     def getblocktemplate(self, address, prev_block = "", client = ""):
         getblocktemplate = {