From babf25d2ec133833dcad0b42121a187463ecccb0 Mon Sep 17 00:00:00 2001 From: Lee Clagett Date: Thu, 26 Mar 2020 12:24:11 +0000 Subject: [PATCH 1/2] Allow unrestricted rpc calls to get full txpool info --- src/cryptonote_core/cryptonote_core.cpp | 8 +++---- src/cryptonote_core/cryptonote_core.h | 6 +++-- src/rpc/core_rpc_server.cpp | 19 +++++++++------ .../functional_tests/functional_tests_rpc.py | 14 ++++++++--- tests/functional_tests/txpool.py | 24 ++++++++++++------- utils/python-rpc/framework/daemon.py | 5 ++-- 6 files changed, 49 insertions(+), 27 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& backlog) const + bool core::get_txpool_backlog(std::vector& 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& backlog) const; + bool get_txpool_backlog(std::vector& 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/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 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/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 = { From 571546067f1b1d0f51a2a796e2509c93133e1cf4 Mon Sep 17 00:00:00 2001 From: Lee Clagett Date: Thu, 26 Mar 2020 12:24:45 +0000 Subject: [PATCH 2/2] Always reject duplicate key-images from second txid --- src/cryptonote_core/tx_pool.cpp | 22 +++++++------ src/cryptonote_core/tx_pool.h | 6 ++-- tests/functional_tests/transfer.py | 52 ++++++++++++++++++++++-------- 3 files changed, 55 insertions(+), 25 deletions(-) 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/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')