Merge pull request #10716

196078e Refactored tx verification NIC logic (SChernykh)

src/cryptonote_core/blockchain.cpp

@@ -2056,7 +2056,7 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id
 
     // Add pool supplement txs to the main mempool with relay_method::block
     CRITICAL_REGION_LOCAL(m_tx_pool);
-    for (auto& extra_block_tx : extra_block_txs.txs_by_txid)
+    for (auto& extra_block_tx : extra_block_txs)
     {
       const crypto::hash& txid = extra_block_tx.first;
       transaction& tx = extra_block_tx.second.first;
@@ -2083,8 +2083,7 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id
           .weight = get_transaction_weight(tx),
           .res = true}});
     }
-    extra_block_txs.txs_by_txid.clear();
-    extra_block_txs.nic_verified_hf_version = 0;
+    extra_block_txs.clear();
 
     bei.block_cumulative_weight = cryptonote::get_transaction_weight(b.miner_tx);
     for (const crypto::hash &txid: b.tx_hashes)
@@ -4173,15 +4172,15 @@ leave:
     bool find_tx_failure{!found_tx_in_pool};
     if (!found_tx_in_pool) // if not in mempool:
     {
-      const auto extra_txs_it{extra_block_txs.txs_by_txid.find(tx_id)};
-      if (extra_txs_it != extra_block_txs.txs_by_txid.end()) // if in block supplement:
+      const auto extra_txs_it{extra_block_txs.find(tx_id)};
+      if (extra_txs_it != extra_block_txs.end()) // if in block supplement:
       {
         tx = std::move(extra_txs_it->second.first);
         txblob = std::move(extra_txs_it->second.second);
         tx_weight = tx.pruned ? get_pruned_transaction_weight(tx) : get_transaction_weight(tx, txblob.size());
         fee = get_tx_fee(tx);
         pruned = tx.pruned;
-        extra_block_txs.txs_by_txid.erase(extra_txs_it);
+        extra_block_txs.erase(extra_txs_it);
         txpool_events.emplace_back(txpool_event{tx, tx_id, txblob.size(), tx_weight, true});
         find_tx_failure = false;
       }
@@ -4194,7 +4193,7 @@ leave:
     // txs twice.
     if (find_tx_failure) // did not find txid in mempool or provided extra block txs
     {
-      const bool fully_supplemented_block = extra_block_txs.txs_by_txid.size() >= bl.tx_hashes.size();
+      const bool fully_supplemented_block = extra_block_txs.size() >= bl.tx_hashes.size();
       if (fully_supplemented_block)
         MERROR_VER("Block with id: " << id  << " has at least one unknown transaction with id: " << tx_id);
       else

src/cryptonote_core/cryptonote_core.cpp

@@ -1460,7 +1460,7 @@ namespace cryptonote
     // saving each block, then it doesn't matter either way: cleanup_handle_incoming_blocks()
     // always triggers a sync.
     size_t block_total_bytes = block_blob.size();
-    for (const auto &t : extra_block_txs.txs_by_txid)
+    for (const auto &t : extra_block_txs)
       block_total_bytes += t.second.second.size();
 
     CRITICAL_REGION_LOCAL(m_incoming_tx_lock);

src/cryptonote_core/tx_verification_utils.cpp

@@ -469,7 +469,7 @@ bool ver_non_input_consensus(const transaction& tx, tx_verification_context& tvc
     return ver_non_input_consensus_templated(&tx, &tx + 1, tvc, hf_version);
 }
 
-bool ver_non_input_consensus(const pool_supplement& ps, tx_verification_context& tvc,
+bool ver_non_input_consensus(pool_supplement& ps, tx_verification_context& tvc,
     const std::uint8_t hf_version)
 {
     // We already verified the pool supplement for this hard fork version! Yippee!

src/cryptonote_core/tx_verification_utils.h

@@ -89,12 +89,46 @@ bool ver_mixed_rct_semantics(std::vector<const rct::rctSig*> rvv);
  */
 struct pool_supplement
 {
+public:
+    using txs_by_txid_t = std::unordered_map<crypto::hash, std::pair<transaction, blobdata>>;
+
+    bool add_tx(const crypto::hash& id, transaction&& tx, const blobdata& blob)
+    {
+        // Any new tx invalidates previous verification
+        nic_verified_hf_version = 0;
+
+        return txs_by_txid.emplace(id, std::make_pair(std::move(tx), blob)).second;
+    }
+
+    void clear() {
+        txs_by_txid.clear();
+
+        // Empty set can technically stay validated, but reset it to 0 anyway.
+        nic_verified_hf_version = 0;
+    }
+
+    txs_by_txid_t::iterator begin() { return txs_by_txid.begin(); }
+    txs_by_txid_t::iterator end()   { return txs_by_txid.end(); }
+
+    txs_by_txid_t::const_iterator begin() const { return txs_by_txid.begin(); }
+    txs_by_txid_t::const_iterator end()   const { return txs_by_txid.end(); }
+
+    txs_by_txid_t::iterator find(const crypto::hash& id) { return txs_by_txid.find(id); }
+
+    txs_by_txid_t::iterator erase(txs_by_txid_t::iterator it) { return txs_by_txid.erase(it); }
+
+    size_t size() const { return txs_by_txid.size(); }
+
+private:
+    // The only function which can set nic_verified_hf_version to non-zero value
+    friend bool ver_non_input_consensus(pool_supplement& ps, tx_verification_context& tvc, std::uint8_t hf_version);
+
     // Map of supplemental tx info that we might need to validate a block
     // Maps TXID -> transaction and blob
-    std::unordered_map<crypto::hash, std::pair<transaction, blobdata>> txs_by_txid;
+    txs_by_txid_t txs_by_txid;
     // If non-zero, then consider all the txs' non-input consensus (NIC) rules verified for this
     // hard fork. User: If you add an unverified transaction to txs_by_txid, set this field to zero!
-    mutable std::uint8_t nic_verified_hf_version = 0;
+    std::uint8_t nic_verified_hf_version = 0;
 };
 
 /**
@@ -124,7 +158,7 @@ struct pool_supplement
 bool ver_non_input_consensus(const transaction& tx, tx_verification_context& tvc,
     std::uint8_t hf_version);
 
-bool ver_non_input_consensus(const pool_supplement& ps, tx_verification_context& tvc,
+bool ver_non_input_consensus(pool_supplement& ps, tx_verification_context& tvc,
     std::uint8_t hf_version);
 
 } // namespace cryptonote

src/cryptonote_protocol/cryptonote_protocol_handler.inl

@@ -91,8 +91,6 @@ namespace cryptonote
     const bool allow_pruned,
     cryptonote::pool_supplement& pool_supplement)
   {
-    pool_supplement.nic_verified_hf_version = 0;
-
     if (tx_entries.size() > blk_tx_hashes.size())
     {
       MERROR("Failed to make pool supplement: Too many transaction blobs!");
@@ -140,7 +138,7 @@ namespace cryptonote
         return false;
       }
 
-      pool_supplement.txs_by_txid.emplace(tx_hash, std::make_pair(std::move(tx), tx_entry.blob));
+      pool_supplement.add_tx(tx_hash, std::move(tx), tx_entry.blob);
     }
 
     return true;