Merge pull request #767

24b3e90 Convey tx verification failure reasons to the RPC client (moneromooo-monero)

10 files changed, 98 insertions, 23 deletions

diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp
index da14d7575..34810d983 100644
--- a/src/cryptonote_core/blockchain.cpp
+++ b/src/cryptonote_core/blockchain.cpp
@@ -1991,7 +1991,7 @@ bool Blockchain::get_tx_outputs_gindexs(const crypto::hash& tx_id, std::vector<u
 // This function overloads its sister function with
 // an extra value (hash of highest block that holds an output used as input)
 // as a return-by-reference.
-bool Blockchain::check_tx_inputs(const transaction& tx, uint64_t& max_used_block_height, crypto::hash& max_used_block_id, bool kept_by_block)
+bool Blockchain::check_tx_inputs(const transaction& tx, uint64_t& max_used_block_height, crypto::hash& max_used_block_id, tx_verification_context &tvc, bool kept_by_block)
 {
   LOG_PRINT_L3("Blockchain::" << __func__);
   CRITICAL_REGION_LOCAL(m_blockchain_lock);
@@ -2013,7 +2013,7 @@ bool Blockchain::check_tx_inputs(const transaction& tx, uint64_t& max_used_block
 #endif
 
   TIME_MEASURE_START(a);
-  bool res = check_tx_inputs(tx, &max_used_block_height);
+  bool res = check_tx_inputs(tx, tvc, &max_used_block_height);
   TIME_MEASURE_FINISH(a);
   crypto::hash tx_prefix_hash = get_transaction_prefix_hash(tx);
   if(m_show_time_stats)
@@ -2032,7 +2032,7 @@ bool Blockchain::check_tx_inputs(const transaction& tx, uint64_t& max_used_block
   return true;
 }
 //------------------------------------------------------------------
-bool Blockchain::check_tx_outputs(const transaction& tx)
+bool Blockchain::check_tx_outputs(const transaction& tx, tx_verification_context &tvc)
 {
   LOG_PRINT_L3("Blockchain::" << __func__);
   CRITICAL_REGION_LOCAL(m_blockchain_lock);
@@ -2041,6 +2041,7 @@ bool Blockchain::check_tx_outputs(const transaction& tx)
   if (m_hardfork->get_current_version() >= 2) {
     for (auto &o: tx.vout) {
       if (!is_valid_decomposed_amount(o.amount)) {
+        tvc.m_invalid_output = true;
         return false;
       }
     }
@@ -2066,7 +2067,7 @@ bool Blockchain::have_tx_keyimges_as_spent(const transaction &tx) const
 //        check_tx_input() rather than here, and use this function simply
 //        to iterate the inputs as necessary (splitting the task
 //        using threads, etc.)
-bool Blockchain::check_tx_inputs(const transaction& tx, uint64_t* pmax_used_block_height)
+bool Blockchain::check_tx_inputs(const transaction& tx, tx_verification_context &tvc, uint64_t* pmax_used_block_height)
 {
   LOG_PRINT_L3("Blockchain::" << __func__);
   size_t sig_index = 0;
@@ -2113,11 +2114,13 @@ bool Blockchain::check_tx_inputs(const transaction& tx, uint64_t* pmax_used_bloc
       if (n_unmixable == 0)
       {
         LOG_PRINT_L1("Tx " << get_transaction_hash(tx) << " has too low mixin (" << mixin << "), and no unmixable inputs");
+        tvc.m_low_mixin = true;
         return false;
       }
       if (n_mixable > 1)
       {
         LOG_PRINT_L1("Tx " << get_transaction_hash(tx) << " has too low mixin (" << mixin << "), and more than one mixable input with unmixable inputs");
+        tvc.m_low_mixin = true;
         return false;
       }
     }
@@ -2176,6 +2179,7 @@ bool Blockchain::check_tx_inputs(const transaction& tx, uint64_t* pmax_used_bloc
     if(have_tx_keyimg_as_spent(in_to_key.k_image))
     {
       LOG_PRINT_L1("Key image already spent in blockchain: " << epee::string_tools::pod_to_hex(in_to_key.k_image));
+      tvc.m_double_spend = true;
       return false;
     }
 
@@ -2667,7 +2671,8 @@ leave:
 #endif
     {
       // validate that transaction inputs and the keys spending them are correct.
-      if(!check_tx_inputs(tx))
+      tx_verification_context tvc;
+      if(!check_tx_inputs(tx, tvc))
       {
         LOG_PRINT_L1("Block with id: " << id  << " has at least one transaction (id: " << tx_id << ") with wrong inputs.");
 
diff --git a/src/cryptonote_core/blockchain.h b/src/cryptonote_core/blockchain.h
index 6bae0364d..a62487d1e 100644
--- a/src/cryptonote_core/blockchain.h
+++ b/src/cryptonote_core/blockchain.h
@@ -472,11 +472,12 @@ namespace cryptonote
      * @param tx the transaction to validate
      * @param pmax_used_block_height return-by-reference block height of most recent input
      * @param max_used_block_id return-by-reference block hash of most recent input
+     * @param tvc returned information about tx verification
      * @param kept_by_block whether or not the transaction is from a previously-verified block
      *
      * @return false if any input is invalid, otherwise true
      */
-    bool check_tx_inputs(const transaction& tx, uint64_t& pmax_used_block_height, crypto::hash& max_used_block_id, bool kept_by_block = false);
+    bool check_tx_inputs(const transaction& tx, uint64_t& pmax_used_block_height, crypto::hash& max_used_block_id, tx_verification_context &tvc, bool kept_by_block = false);
 
     /**
      * @brief check that a transaction's outputs conform to current standards
@@ -486,10 +487,11 @@ namespace cryptonote
      * written out would have only one non-zero digit in base 10).
      *
      * @param tx the transaction to check the outputs of
+     * @param tvc returned info about tx verification
      *
      * @return false if any outputs do not conform, otherwise true
      */
-    bool check_tx_outputs(const transaction& tx);
+    bool check_tx_outputs(const transaction& tx, tx_verification_context &tvc);
 
     /**
      * @brief gets the blocksize limit based on recent blocks
@@ -883,11 +885,12 @@ namespace cryptonote
      * transaction.
      *
      * @param tx the transaction to validate
+     * @param tvc returned information about tx verification
      * @param pmax_related_block_height return-by-pointer the height of the most recent block in the input set
      *
      * @return false if any validation step fails, otherwise true
      */
-    bool check_tx_inputs(const transaction& tx, uint64_t* pmax_used_block_height = NULL);
+    bool check_tx_inputs(const transaction& tx, tx_verification_context &tvc, uint64_t* pmax_used_block_height = NULL);
 
     /**
      * @brief performs a blockchain reorganization according to the longest chain rule
diff --git a/src/cryptonote_core/cryptonote_core.cpp b/src/cryptonote_core/cryptonote_core.cpp
index c31be5acf..20b9f0b0b 100644
--- a/src/cryptonote_core/cryptonote_core.cpp
+++ b/src/cryptonote_core/cryptonote_core.cpp
@@ -489,6 +489,7 @@ namespace cryptonote
     {
       LOG_PRINT_L1("WRONG TRANSACTION BLOB, too big size " << tx_blob.size() << ", rejected");
       tvc.m_verifivation_failed = true;
+      tvc.m_too_big = true;
       return false;
     }
 
diff --git a/src/cryptonote_core/tx_pool.cpp b/src/cryptonote_core/tx_pool.cpp
index 49fa4aa57..a06826163 100644
--- a/src/cryptonote_core/tx_pool.cpp
+++ b/src/cryptonote_core/tx_pool.cpp
@@ -102,6 +102,7 @@ namespace cryptonote
     if(!check_inputs_types_supported(tx))
     {
       tvc.m_verifivation_failed = true;
+      tvc.m_invalid_input = true;
       return false;
     }
 
@@ -118,6 +119,7 @@ namespace cryptonote
     {
       LOG_PRINT_L1("transaction use more money then it has: use " << print_money(outputs_amount) << ", have " << print_money(inputs_amount));
       tvc.m_verifivation_failed = true;
+      tvc.m_overspend = true;
       return false;
     }
 
@@ -130,6 +132,7 @@ namespace cryptonote
     {
       LOG_PRINT_L1("transaction fee is not enough: " << print_money(fee) << ", minimum fee: " << print_money(needed_fee));
       tvc.m_verifivation_failed = true;
+      tvc.m_fee_too_low = true;
       return false;
     }
 
@@ -138,6 +141,7 @@ namespace cryptonote
     {
       LOG_PRINT_L1("transaction is too big: " << blob_size << " bytes, maximum size: " << tx_size_limit);
       tvc.m_verifivation_failed = true;
+      tvc.m_too_big = true;
       return false;
     }
 
@@ -150,21 +154,23 @@ namespace cryptonote
       {
         LOG_PRINT_L1("Transaction with id= "<< id << " used already spent key images");
         tvc.m_verifivation_failed = true;
+        tvc.m_double_spend = true;
         return false;
       }
     }
 
-    if (!m_blockchain.check_tx_outputs(tx))
+    if (!m_blockchain.check_tx_outputs(tx, tvc))
     {
       LOG_PRINT_L1("Transaction with id= "<< id << " has at least one invalid outout");
       tvc.m_verifivation_failed = true;
+      tvc.m_invalid_output = true;
       return false;
     }
 
     crypto::hash max_used_block_id = null_hash;
     uint64_t max_used_block_height = 0;
 #if BLOCKCHAIN_DB == DB_LMDB
-    bool ch_inp_res = m_blockchain.check_tx_inputs(tx, max_used_block_height, max_used_block_id, kept_by_block);
+    bool ch_inp_res = m_blockchain.check_tx_inputs(tx, max_used_block_height, max_used_block_id, tvc, kept_by_block);
 #else
     bool ch_inp_res = m_blockchain.check_tx_inputs(tx, max_used_block_height, max_used_block_id);
 #endif
@@ -495,7 +501,8 @@ namespace cryptonote
       if(txd.last_failed_id != null_hash && m_blockchain.get_current_blockchain_height() > txd.last_failed_height && txd.last_failed_id == m_blockchain.get_block_id_by_height(txd.last_failed_height))
         return false;//we already sure that this tx is broken for this height
 
-      if(!m_blockchain.check_tx_inputs(txd.tx, txd.max_used_block_height, txd.max_used_block_id))
+      tx_verification_context tvc;
+      if(!m_blockchain.check_tx_inputs(txd.tx, txd.max_used_block_height, txd.max_used_block_id, tvc))
       {
         txd.last_failed_height = m_blockchain.get_current_blockchain_height()-1;
         txd.last_failed_id = m_blockchain.get_block_id_by_height(txd.last_failed_height);
@@ -511,7 +518,12 @@ namespace cryptonote
         if(txd.last_failed_id == m_blockchain.get_block_id_by_height(txd.last_failed_height))
           return false;
         //check ring signature again, it is possible (with very small chance) that this transaction become again valid
+#if BLOCKCHAIN_DB == DB_LMDB
+        tx_verification_context tvc;
+        if(!m_blockchain.check_tx_inputs(txd.tx, txd.max_used_block_height, txd.max_used_block_id, tvc))
+#else
         if(!m_blockchain.check_tx_inputs(txd.tx, txd.max_used_block_height, txd.max_used_block_id))
+#endif
         {
           txd.last_failed_height = m_blockchain.get_current_blockchain_height()-1;
           txd.last_failed_id = m_blockchain.get_block_id_by_height(txd.last_failed_height);
diff --git a/src/cryptonote_core/verification_context.h b/src/cryptonote_core/verification_context.h
index fcfd2a3e2..e58291ea9 100644
--- a/src/cryptonote_core/verification_context.h
+++ b/src/cryptonote_core/verification_context.h
@@ -40,6 +40,13 @@ namespace cryptonote
     bool m_verifivation_failed; //bad tx, should drop connection
     bool m_verifivation_impossible; //the transaction is related with an alternative blockchain
     bool m_added_to_pool; 
+    bool m_low_mixin;
+    bool m_double_spend;
+    bool m_invalid_input;
+    bool m_invalid_output;
+    bool m_too_big;
+    bool m_overspend;
+    bool m_fee_too_low;
   };
 
   struct block_verification_context
diff --git a/src/rpc/core_rpc_server.cpp b/src/rpc/core_rpc_server.cpp
index 5350b6fe0..2d2727a29 100644
--- a/src/rpc/core_rpc_server.cpp
+++ b/src/rpc/core_rpc_server.cpp
@@ -355,24 +355,40 @@ namespace cryptonote
 
     cryptonote_connection_context fake_context = AUTO_VAL_INIT(fake_context);
     tx_verification_context tvc = AUTO_VAL_INIT(tvc);
-    if(!m_core.handle_incoming_tx(tx_blob, tvc, false, false))
+    if(!m_core.handle_incoming_tx(tx_blob, tvc, false, false) || tvc.m_verifivation_failed)
     {
-      LOG_PRINT_L0("[on_send_raw_tx]: Failed to process tx");
-      res.status = "Failed";
-      return true;
-    }
-
-    if(tvc.m_verifivation_failed)
-    {
-      LOG_PRINT_L0("[on_send_raw_tx]: tx verification failed");
+      if (tvc.m_verifivation_failed)
+      {
+        LOG_PRINT_L0("[on_send_raw_tx]: tx verification failed");
+      }
+      else
+      {
+        LOG_PRINT_L0("[on_send_raw_tx]: Failed to process tx");
+      }
       res.status = "Failed";
+      if ((res.low_mixin = tvc.m_low_mixin))
+        res.reason = "mixin too low";
+      if ((res.double_spend = tvc.m_double_spend))
+        res.reason = "double spend";
+      if ((res.invalid_input = tvc.m_invalid_input))
+        res.reason = "invalid input";
+      if ((res.invalid_output = tvc.m_invalid_output))
+        res.reason = "invalid output";
+      if ((res.too_big = tvc.m_too_big))
+        res.reason = "too big";
+      if ((res.overspend = tvc.m_overspend))
+        res.reason = "overspend";
+      if ((res.fee_too_low = tvc.m_fee_too_low))
+        res.reason = "fee too low";
       return true;
     }
 
     if(!tvc.m_should_be_relayed)
     {
       LOG_PRINT_L0("[on_send_raw_tx]: tx accepted, but not relayed");
-      res.status = "Not relayed";
+      res.reason = "Not relayed";
+      res.not_relayed = true;
+      res.status = CORE_RPC_STATUS_OK;
       return true;
     }
 
diff --git a/src/rpc/core_rpc_server_commands_defs.h b/src/rpc/core_rpc_server_commands_defs.h
index 6d4dd1252..52885dd34 100644
--- a/src/rpc/core_rpc_server_commands_defs.h
+++ b/src/rpc/core_rpc_server_commands_defs.h
@@ -234,9 +234,27 @@ namespace cryptonote
     struct response
     {
       std::string status;
+      std::string reason;
+      bool not_relayed;
+      bool low_mixin;
+      bool double_spend;
+      bool invalid_input;
+      bool invalid_output;
+      bool too_big;
+      bool overspend;
+      bool fee_too_low;
 
       BEGIN_KV_SERIALIZE_MAP()
         KV_SERIALIZE(status)
+        KV_SERIALIZE(reason)
+        KV_SERIALIZE(not_relayed)
+        KV_SERIALIZE(low_mixin)
+        KV_SERIALIZE(double_spend)
+        KV_SERIALIZE(invalid_input)
+        KV_SERIALIZE(invalid_output)
+        KV_SERIALIZE(too_big)
+        KV_SERIALIZE(overspend)
+        KV_SERIALIZE(fee_too_low)
       END_KV_SERIALIZE_MAP()
     };
   };
diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp
index 46e705673..d05b5ea4c 100644
--- a/src/simplewallet/simplewallet.cpp
+++ b/src/simplewallet/simplewallet.cpp
@@ -2201,6 +2201,9 @@ bool simple_wallet::transfer_main(bool new_algorithm, const std::vector<std::str
   catch (const tools::error::tx_rejected& e)
   {
     fail_msg_writer() << (boost::format(tr("transaction %s was rejected by daemon with status: ")) % get_transaction_hash(e.tx())) << e.status();
+    std::string reason = e.reason();
+    if (!reason.empty())
+      fail_msg_writer() << tr("Reason: ") << reason;
   }
   catch (const tools::error::tx_sum_overflow& e)
   {
@@ -2358,6 +2361,9 @@ bool simple_wallet::sweep_unmixable(const std::vector<std::string> &args_)
   catch (const tools::error::tx_rejected& e)
   {
     fail_msg_writer() << (boost::format(tr("transaction %s was rejected by daemon with status: ")) % get_transaction_hash(e.tx())) << e.status();
+    std::string reason = e.reason();
+    if (!reason.empty())
+      fail_msg_writer() << tr("Reason: ") << reason;
   }
   catch (const tools::error::tx_sum_overflow& e)
   {
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 363665024..95a160434 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -1965,7 +1965,7 @@ void wallet2::commit_tx(pending_tx& ptx)
   m_daemon_rpc_mutex.unlock();
   THROW_WALLET_EXCEPTION_IF(!r, error::no_connection_to_daemon, "sendrawtransaction");
   THROW_WALLET_EXCEPTION_IF(daemon_send_resp.status == CORE_RPC_STATUS_BUSY, error::daemon_busy, "sendrawtransaction");
-  THROW_WALLET_EXCEPTION_IF(daemon_send_resp.status != CORE_RPC_STATUS_OK, error::tx_rejected, ptx.tx, daemon_send_resp.status);
+  THROW_WALLET_EXCEPTION_IF(daemon_send_resp.status != CORE_RPC_STATUS_OK, error::tx_rejected, ptx.tx, daemon_send_resp.status, daemon_send_resp.reason);
 
   txid = get_transaction_hash(ptx.tx);
   crypto::hash payment_id = cryptonote::null_hash;
diff --git a/src/wallet/wallet_errors.h b/src/wallet/wallet_errors.h
index 652b19499..3de97f49d 100644
--- a/src/wallet/wallet_errors.h
+++ b/src/wallet/wallet_errors.h
@@ -458,15 +458,17 @@ namespace tools
     //----------------------------------------------------------------------------------------------------
     struct tx_rejected : public transfer_error
     {
-      explicit tx_rejected(std::string&& loc, const cryptonote::transaction& tx, const std::string& status)
+      explicit tx_rejected(std::string&& loc, const cryptonote::transaction& tx, const std::string& status, const std::string& reason)
         : transfer_error(std::move(loc), "transaction was rejected by daemon")
         , m_tx(tx)
         , m_status(status)
+        , m_reason(reason)
       {
       }
 
       const cryptonote::transaction& tx() const { return m_tx; }
       const std::string& status() const { return m_status; }
+      const std::string& reason() const { return m_reason; }
 
       std::string to_string() const
       {
@@ -474,12 +476,17 @@ namespace tools
         ss << transfer_error::to_string() << ", status = " << m_status << ", tx:\n";
         cryptonote::transaction tx = m_tx;
         ss << cryptonote::obj_to_json_str(tx);
+        if (!m_reason.empty())
+        {
+          ss << " (" << m_reason << ")";
+        }
         return ss.str();
       }
 
     private:
       cryptonote::transaction m_tx;
       std::string m_status;
+      std::string m_reason;
     };
     //----------------------------------------------------------------------------------------------------
     struct tx_sum_overflow : public transfer_error