From 5de2295f3c215303527f649461bc2ed7113efc7c Mon Sep 17 00:00:00 2001 From: Lee Clagett Date: Wed, 25 Mar 2020 03:02:28 -0400 Subject: [PATCH] Correct key image check in tx_pool --- src/cryptonote_core/tx_pool.cpp | 23 ++++--------- tests/core_tests/chaingen_main.cpp | 1 + tests/core_tests/tx_pool.cpp | 52 ++++++++++++++++++++++++++++++ tests/core_tests/tx_pool.h | 11 +++++++ 4 files changed, 71 insertions(+), 16 deletions(-) diff --git a/src/cryptonote_core/tx_pool.cpp b/src/cryptonote_core/tx_pool.cpp index 1bc475879..112873e4e 100644 --- a/src/cryptonote_core/tx_pool.cpp +++ b/src/cryptonote_core/tx_pool.cpp @@ -422,27 +422,18 @@ namespace cryptonote CHECKED_GET_SPECIFIC_VARIANT(in, const txin_to_key, txin, false); std::unordered_set& kei_image_set = m_spent_key_images[txin.k_image]; - /* If any existing key-image in the set is publicly visible AND this is - not forcibly "kept_by_block", then fail (duplicate key image). If all - existing key images are supposed to be hidden, we silently allow so - that the node doesn't leak knowledge of a local/stem tx. */ - bool visible = false; + // Only allow multiple txes per key-image if kept-by-block. Only allow + // the same txid if going from local/stem->fluff. + if (tx_relay != relay_method::block) { - for (const crypto::hash& other_id : kei_image_set) - visible |= m_blockchain.txpool_tx_matches_category(other_id, relay_category::legacy); - } - - CHECK_AND_ASSERT_MES(!visible, false, "internal error: tx_relay=" << unsigned(tx_relay) + const bool one_txid = + (kei_image_set.empty() || (kei_image_set.size() == 1 && *(kei_image_set.cbegin()) == id)); + CHECK_AND_ASSERT_MES(one_txid, false, "internal error: tx_relay=" << unsigned(tx_relay) << ", kei_image_set.size()=" << kei_image_set.size() << ENDL << "txin.k_image=" << txin.k_image << ENDL << "tx_id=" << id); + } - /* If adding a tx (hash) that already exists, fail only if the tx has - been publicly "broadcast" previously. This way, when a private tx is - received for the first time from a remote node, "this" node will - respond as-if it were seen for the first time. LMDB does the - "hard-check" on key-images, so the effect is overwriting the existing - tx_pool metadata and "first seen" time. */ const bool new_or_previously_private = kei_image_set.insert(id).second || !m_blockchain.txpool_tx_matches_category(id, relay_category::legacy); diff --git a/tests/core_tests/chaingen_main.cpp b/tests/core_tests/chaingen_main.cpp index 23f3170b8..014c7475b 100644 --- a/tests/core_tests/chaingen_main.cpp +++ b/tests/core_tests/chaingen_main.cpp @@ -161,6 +161,7 @@ int main(int argc, char* argv[]) GENERATE_AND_PLAY(txpool_spend_key_all); GENERATE_AND_PLAY(txpool_double_spend_norelay); GENERATE_AND_PLAY(txpool_double_spend_local); + GENERATE_AND_PLAY(txpool_double_spend_keyimage); // Double spend GENERATE_AND_PLAY(gen_double_spend_in_tx); diff --git a/tests/core_tests/tx_pool.cpp b/tests/core_tests/tx_pool.cpp index 537015dca..cc738c4ba 100644 --- a/tests/core_tests/tx_pool.cpp +++ b/tests/core_tests/tx_pool.cpp @@ -125,10 +125,12 @@ txpool_double_spend_base::txpool_double_spend_base() , m_no_relay_hashes() , m_all_hashes() , m_no_new_index(0) + , m_failed_index(0) , m_new_timestamp_index(0) , m_last_tx(crypto::hash{}) { REGISTER_CALLBACK_METHOD(txpool_double_spend_base, mark_no_new); + REGISTER_CALLBACK_METHOD(txpool_double_spend_base, mark_failed); REGISTER_CALLBACK_METHOD(txpool_double_spend_base, mark_timestamp_change); REGISTER_CALLBACK_METHOD(txpool_double_spend_base, timestamp_change_pause); REGISTER_CALLBACK_METHOD(txpool_double_spend_base, check_unchanged); @@ -143,6 +145,12 @@ bool txpool_double_spend_base::mark_no_new(cryptonote::core& /*c*/, size_t ev_in return true; } +bool txpool_double_spend_base::mark_failed(cryptonote::core& /*c*/, size_t ev_index, const std::vector& /*events*/) +{ + m_failed_index = ev_index + 1; + return true; +} + bool txpool_double_spend_base::mark_timestamp_change(cryptonote::core& /*c*/, size_t ev_index, const std::vector& /*events*/) { m_new_timestamp_index = ev_index + 1; @@ -483,6 +491,8 @@ bool txpool_double_spend_base::check_tx_verification_context(const cryptonote::t m_last_tx = cryptonote::get_transaction_hash(tx); if (m_no_new_index == event_idx) return !tvc.m_verifivation_failed && !tx_added; + else if (m_failed_index == event_idx) + return tvc.m_verifivation_failed;// && !tx_added; else return !tvc.m_verifivation_failed && tx_added; } @@ -559,3 +569,45 @@ bool txpool_double_spend_local::generate(std::vector& events) return true; } +bool txpool_double_spend_keyimage::generate(std::vector& events) const +{ + INIT_MEMPOOL_TEST(); + + DO_CALLBACK(events, "check_txpool_spent_keys"); + SET_EVENT_VISITOR_SETT(events, event_visitor_settings::set_local_relay); + DO_CALLBACK(events, "mark_no_new"); + + const std::size_t tx_index1 = events.size(); + MAKE_TX(events, tx_0, miner_account, bob_account, send_amount, blk_0); + + DO_CALLBACK(events, "increase_all_tx_count"); + DO_CALLBACK(events, "check_txpool_spent_keys"); + DO_CALLBACK(events, "mark_timestamp_change"); + DO_CALLBACK(events, "check_new_hidden"); + DO_CALLBACK(events, "timestamp_change_pause"); + DO_CALLBACK(events, "mark_no_new"); + const std::size_t tx_index2 = events.size(); + events.push_back(tx_0); + DO_CALLBACK(events, "check_txpool_spent_keys"); + DO_CALLBACK(events, "mark_timestamp_change"); + DO_CALLBACK(events, "check_unchanged"); + + // use same key image with different id + cryptonote::transaction tx_1; + { + auto events_copy = events; + events_copy.erase(events_copy.begin() + tx_index1); + events_copy.erase(events_copy.begin() + tx_index2 - 1); + MAKE_TX(events_copy, tx_temp, miner_account, bob_account, send_amount, blk_0); + tx_1 = tx_temp; + } + + // same key image + DO_CALLBACK(events, "timestamp_change_pause"); + DO_CALLBACK(events, "mark_failed"); + events.push_back(tx_1); + DO_CALLBACK(events, "check_unchanged"); + + return true; +} + diff --git a/tests/core_tests/tx_pool.h b/tests/core_tests/tx_pool.h index 996c76698..eb71dcf79 100644 --- a/tests/core_tests/tx_pool.h +++ b/tests/core_tests/tx_pool.h @@ -77,6 +77,7 @@ class txpool_double_spend_base : public txpool_base std::unordered_set m_no_relay_hashes; std::unordered_map m_all_hashes; size_t m_no_new_index; + size_t m_failed_index; size_t m_new_timestamp_index; crypto::hash m_last_tx; @@ -86,6 +87,7 @@ public: txpool_double_spend_base(); bool mark_no_new(cryptonote::core& c, size_t ev_index, const std::vector& events); + bool mark_failed(cryptonote::core& c, size_t ev_index, const std::vector& events); bool mark_timestamp_change(cryptonote::core& c, size_t ev_index, const std::vector& events); //! Pause for 1 second, so that `receive_time` for tx meta changes (tx hidden from public rpc being updated) @@ -116,3 +118,12 @@ struct txpool_double_spend_local : txpool_double_spend_base bool generate(std::vector& events) const; }; + +struct txpool_double_spend_keyimage : txpool_double_spend_base +{ + txpool_double_spend_keyimage() + : txpool_double_spend_base() + {} + + bool generate(std::vector& events) const; +};