tx relay v2: more improvements

src/blockchain_utilities/blockchain_import.cpp

@@ -175,13 +175,15 @@ int check_flush(cryptonote::core &core, std::vector<block_complete_entry> &block
       tx_verification_context tvc = AUTO_VAL_INIT(tvc);
       CHECK_AND_ASSERT_THROW_MES(tx_blob.prunable_hash == crypto::null_hash,
         "block entry must not contain pruned txs");
+      cryptonote::transaction tx;
       crypto::hash txid;
-      core.handle_incoming_tx(tx_blob.blob, tvc, relay_method::block, true, txid);
-      if(tvc.m_verifivation_failed)
+      const bool parse_success = cryptonote::parse_and_validate_tx_from_blob(tx_blob.blob, tx, txid, true);
+      if (!parse_success
+        || !core.handle_incoming_tx(tx_blob.blob, tx, txid, tvc, relay_method::block, true)
+        || tvc.m_verifivation_failed)
       {
-        cryptonote::transaction transaction;
-        if (cryptonote::parse_and_validate_tx_from_blob(tx_blob.blob, transaction))
-          MERROR("Transaction verification failed, tx_id = " << cryptonote::get_transaction_hash(transaction));
+        if (parse_success)
+          MERROR("Transaction verification failed, tx_id = " << txid);
         else
           MERROR("Transaction verification failed, transaction is unparsable");
         core.cleanup_handle_incoming_blocks();

src/cryptonote_basic/cryptonote_format_utils.cpp

@@ -156,6 +156,13 @@ namespace cryptonote
     return tx.vout.size();
   }
   //---------------------------------------------------------------
+  bool passes_max_size_check(const bool max_size_check, const blobdata_ref &tx_blob)
+  {
+    if (!max_size_check)
+      return true;
+    return tx_blob.size() <= get_max_tx_size();
+  }
+  //---------------------------------------------------------------
 }
 
 namespace cryptonote
@@ -254,8 +261,9 @@ namespace cryptonote
     return true;
   }
   //---------------------------------------------------------------
-  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx)
+  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx, const bool max_size_check)
   {
+    CHECK_AND_ASSERT_MES(passes_max_size_check(max_size_check, tx_blob), false, "Tx blob too big");
     binary_archive<false> ba{epee::strspan<std::uint8_t>(tx_blob)};
     bool r = ::serialization::serialize(ba, tx);
     CHECK_AND_ASSERT_MES(r, false, "Failed to parse transaction from blob");
@@ -265,8 +273,9 @@ namespace cryptonote
     return true;
   }
   //---------------------------------------------------------------
-  bool parse_and_validate_tx_base_from_blob(const blobdata_ref& tx_blob, transaction& tx)
+  bool parse_and_validate_tx_base_from_blob(const blobdata_ref& tx_blob, transaction& tx, const bool max_size_check)
   {
+    CHECK_AND_ASSERT_MES(passes_max_size_check(max_size_check, tx_blob), false, "Tx blob too big");
     binary_archive<false> ba{epee::strspan<std::uint8_t>(tx_blob)};
     bool r = tx.serialize_base(ba);
     CHECK_AND_ASSERT_MES(r, false, "Failed to parse transaction from blob");
@@ -275,16 +284,18 @@ namespace cryptonote
     return true;
   }
   //---------------------------------------------------------------
-  bool parse_and_validate_tx_prefix_from_blob(const blobdata_ref& tx_blob, transaction_prefix& tx)
+  bool parse_and_validate_tx_prefix_from_blob(const blobdata_ref& tx_blob, transaction_prefix& tx, const bool max_size_check)
   {
+    CHECK_AND_ASSERT_MES(passes_max_size_check(max_size_check, tx_blob), false, "Tx blob too big");
     binary_archive<false> ba{epee::strspan<std::uint8_t>(tx_blob)};
     bool r = ::serialization::serialize_noeof(ba, tx);
     CHECK_AND_ASSERT_MES(r, false, "Failed to parse transaction prefix from blob");
     return true;
   }
   //---------------------------------------------------------------
-  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx, crypto::hash& tx_hash)
+  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx, crypto::hash& tx_hash, const bool max_size_check)
   {
+    CHECK_AND_ASSERT_MES(passes_max_size_check(max_size_check, tx_blob), false, "Tx blob too big");
     binary_archive<false> ba{epee::strspan<std::uint8_t>(tx_blob)};
     bool r = ::serialization::serialize(ba, tx);
     CHECK_AND_ASSERT_MES(r, false, "Failed to parse transaction from blob");
@@ -296,9 +307,9 @@ namespace cryptonote
     return get_transaction_hash(tx, tx_hash);
   }
   //---------------------------------------------------------------
-  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx, crypto::hash& tx_hash, crypto::hash& tx_prefix_hash)
+  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx, crypto::hash& tx_hash, crypto::hash& tx_prefix_hash, const bool max_size_check)
   {
-    if (!parse_and_validate_tx_from_blob(tx_blob, tx, tx_hash))
+    if (!parse_and_validate_tx_from_blob(tx_blob, tx, tx_hash, max_size_check))
       return false;
     get_transaction_prefix_hash(tx, tx_prefix_hash);
     return true;

src/cryptonote_basic/cryptonote_format_utils.h

@@ -55,12 +55,13 @@ namespace cryptonote
   crypto::hash get_transaction_prefix_hash(const transaction_prefix& tx, hw::device &hwdev);
   void get_transaction_prefix_hash(const transaction_prefix& tx, crypto::hash& h);
   crypto::hash get_transaction_prefix_hash(const transaction_prefix& tx);
-  bool parse_and_validate_tx_prefix_from_blob(const blobdata_ref& tx_blob, transaction_prefix& tx);
   bool expand_transaction_1(transaction &tx, bool base_only);
-  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx, crypto::hash& tx_hash, crypto::hash& tx_prefix_hash);
-  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx, crypto::hash& tx_hash);
-  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx);
-  bool parse_and_validate_tx_base_from_blob(const blobdata_ref& tx_blob, transaction& tx);
+  /* The size check is useful before parsing non-coinbase txs from untrusted sources. Coinbase blob sizes may be uncapped. */
+  bool parse_and_validate_tx_prefix_from_blob(const blobdata_ref& tx_blob, transaction_prefix& tx, const bool max_size_check = false);
+  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx, crypto::hash& tx_hash, crypto::hash& tx_prefix_hash, const bool max_size_check = false);
+  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx, crypto::hash& tx_hash, const bool max_size_check = false);
+  bool parse_and_validate_tx_from_blob(const blobdata_ref& tx_blob, transaction& tx, const bool max_size_check = false);
+  bool parse_and_validate_tx_base_from_blob(const blobdata_ref& tx_blob, transaction& tx, const bool max_size_check = false);
   bool is_v1_tx(const blobdata_ref& tx_blob);
   bool is_v1_tx(const blobdata& tx_blob);
 

src/cryptonote_core/blockchain.cpp

@@ -5689,7 +5689,7 @@ bool Blockchain::prepare_handle_incoming_blocks(const std::vector<block_complete
       crypto::hash &tx_prefix_hash = txes[tx_index].second;
       ++tx_index;
 
-      if (!parse_and_validate_tx_base_from_blob(tx_blob.blob, tx))
+      if (!parse_and_validate_tx_base_from_blob(tx_blob.blob, tx, true))
         SCAN_TABLE_QUIT("Could not parse tx from incoming blocks.");
       cryptonote::get_transaction_prefix_hash(tx, tx_prefix_hash);
 

src/cryptonote_core/cryptonote_core.cpp

@@ -781,7 +781,7 @@ namespace cryptonote
     return true;
   }
   //-----------------------------------------------------------------------------------------------
-  bool core::handle_incoming_tx(const blobdata& tx_blob, tx_verification_context& tvc, relay_method tx_relay, bool relayed, crypto::hash& txid)
+  bool core::handle_incoming_tx(const blobdata& tx_blob, transaction& tx, const crypto::hash& txid, tx_verification_context& tvc, relay_method tx_relay, bool relayed)
   {
     tvc = {};
 
@@ -797,14 +797,6 @@ namespace cryptonote
       return false;
     }
 
-    transaction tx;
-    if (!parse_and_validate_tx_from_blob(tx_blob, tx, txid))
-    {
-      LOG_PRINT_L1("Incoming transactions failed to parse, rejected");
-      tvc.m_verifivation_failed = true;
-      return false;
-    }
-
     const uint64_t tx_weight = get_transaction_weight(tx, tx_blob.size());
     if (!add_new_tx(tx, txid, tx_blob, tx_weight, tvc, tx_relay, relayed))
       return false;
@@ -1217,7 +1209,7 @@ namespace cryptonote
 
     for (std::size_t i = 0; i < tx_blobs.size(); ++i)
     {
-      if (!parse_and_validate_tx_from_blob(tx_blobs[i], txs[i], tx_hashes[i]))
+      if (!parse_and_validate_tx_from_blob(tx_blobs[i], txs[i], tx_hashes[i], true))
       {
         LOG_ERROR("Failed to parse relayed transaction");
         return;

src/cryptonote_core/cryptonote_core.h

@@ -116,18 +116,19 @@ namespace cryptonote
      /**
       * @brief handles an incoming transaction
       *
-      * Parses an incoming transaction and, if nothing is obviously wrong,
+      * Processes an incoming transaction and, if nothing is obviously wrong,
       * passes it along to the transaction pool
       *
       * @param tx_blob the tx to handle
+      * @param tx the parsed tx to handle (may expand the tx)
+      * @param txid the tx hash to handle
       * @param tvc metadata about the transaction's validity
       * @param tx_relay how the transaction was received
       * @param relayed whether or not the transaction was relayed to us
-      * @param txid return by reference
       *
       * @return true if the transaction was accepted, false otherwise
       */
-     bool handle_incoming_tx(const blobdata& tx_blob, tx_verification_context& tvc, relay_method tx_relay, bool relayed, crypto::hash& txid);
+     bool handle_incoming_tx(const blobdata& tx_blob, transaction& tx, const crypto::hash& txid, tx_verification_context& tvc, relay_method tx_relay, bool relayed);
 
     /**
       * @brief handles a single incoming block

src/cryptonote_core/tx_pool.cpp

@@ -1422,8 +1422,10 @@ namespace cryptonote
   //---------------------------------------------------------------------------------
   bool tx_memory_pool::get_transaction(const crypto::hash& id, cryptonote::blobdata& txblob, relay_category tx_category) const
   {
-    CRITICAL_REGION_LOCAL(m_transactions_lock);
-    CRITICAL_REGION_LOCAL1(m_blockchain);
+    // WARNING: this function does not take m_blockchain_lock, and thus should only call read only
+    // m_db functions which do not depend on one another (ie, no getheight + gethash(height-1), as
+    // well as not accessing class members, even read only (ie, m_invalid_blocks). The caller must
+    // lock if it is otherwise needed.
     try
     {
       return m_blockchain.get_txpool_tx_blob(id, txblob, tx_category);
@@ -1452,8 +1454,10 @@ namespace cryptonote
   //---------------------------------------------------------------------------------
   bool tx_memory_pool::have_tx(const crypto::hash &id, relay_category tx_category) const
   {
-    CRITICAL_REGION_LOCAL(m_transactions_lock);
-    CRITICAL_REGION_LOCAL1(m_blockchain);
+    // WARNING: this function does not take m_blockchain_lock, and thus should only call read only
+    // m_db functions which do not depend on one another (ie, no getheight + gethash(height-1), as
+    // well as not accessing class members, even read only (ie, m_invalid_blocks). The caller must
+    // lock if it is otherwise needed.
     return m_blockchain.get_db().txpool_has_tx(id, tx_category);
   }
   //---------------------------------------------------------------------------------

src/cryptonote_core/tx_sanity_check.cpp

@@ -43,7 +43,7 @@ bool tx_sanity_check(const cryptonote::blobdata &tx_blob, uint64_t rct_outs_avai
 {
   cryptonote::transaction tx;
 
-  if (!cryptonote::parse_and_validate_tx_from_blob(tx_blob, tx))
+  if (!cryptonote::parse_and_validate_tx_from_blob(tx_blob, tx, true))
   {
     MERROR("Failed to parse transaction");
     return false;

src/cryptonote_protocol/cryptonote_protocol_defs.h

@@ -200,11 +200,13 @@ namespace cryptonote
       std::vector<blobdata>   txs;
       std::string _; // padding
       bool dandelionpp_fluff; //zero initialization defaults to stem mode
+      uint64_t nonce; // if responding to NOTIFY_REQUEST_TX_POOL_TXS, includes nonce in resp
 
       BEGIN_KV_SERIALIZE_MAP()
         KV_SERIALIZE(txs)
         KV_SERIALIZE(_)
         KV_SERIALIZE_OPT(dandelionpp_fluff, true) // backwards compatible mode is fluff
+        KV_SERIALIZE_OPT(nonce, (uint64_t)0)
       END_KV_SERIALIZE_MAP()
     };
     typedef epee::misc_utils::struct_init<request_t> request;
@@ -445,9 +447,11 @@ namespace cryptonote
 
     struct request_t
     {
+      uint64_t n; // request nonce
       std::vector<crypto::hash> t;
 
       BEGIN_KV_SERIALIZE_MAP()
+        KV_SERIALIZE(n)
         KV_SERIALIZE_CONTAINER_POD_AS_BLOB(t)
       END_KV_SERIALIZE_MAP()
     };

src/cryptonote_protocol/cryptonote_protocol_handler.h

@@ -176,14 +176,14 @@ namespace cryptonote
     bool should_ask_for_pruned_data(cryptonote_connection_context& context, uint64_t first_block_height, uint64_t nblocks, bool check_block_weights) const;
     void drop_connection(cryptonote_connection_context &context, bool add_fail, bool flush_all_spans);
     void drop_connection_with_score(cryptonote_connection_context &context, unsigned int score, bool flush_all_spans);
-    void drop_connection(const boost::uuids::uuid&);
+    void drop_connection(const boost::uuids::uuid&, bool add_fail);
     void drop_connections(const epee::net_utils::network_address address);
     bool kick_idle_peers();
     bool check_standby_peers();
     bool update_sync_search();
-    void send_txs_request(cryptonote_connection_context &context, std::vector<crypto::hash> &&tx_hashes);
-    std::mutex m_check_tx_request_queue_mutex;
+    void send_txs_request(cryptonote_connection_context &context, request_manager::tx_request_t &&tx_req);
     bool check_tx_request_queue();
+    void fly_available_requests_in_queue(const std::unordered_set<boost::uuids::uuid> &ignore_peers = {});
     int try_add_next_blocks(cryptonote_connection_context &context);
     void notify_new_stripe(cryptonote_connection_context &context, uint32_t stripe);
     size_t skip_unneeded_hashes(cryptonote_connection_context& context, bool check_block_queue) const;