Convey tx verification failure reasons to the RPC client

This allows appropriate action to be taken, like displaying
the reason to the user.

Do just that in simplewallet, which should help a lot in
determining why users fail to send.

Also make it so a tx which is accepted but not relayed is
seen as a success rather than a failure.

10 files changed, 101 insertions, 26 deletions

diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp
index 0f6afe740..61cf064b9 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 393faef2a..db930cf97 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
@@ -874,11 +876,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 5d67acdd2..829caaf0f 100644
--- a/src/cryptonote_core/tx_pool.cpp
+++ b/src/cryptonote_core/tx_pool.cpp
@@ -97,6 +97,7 @@ namespace cryptonote
     if(!check_inputs_types_supported(tx))
     {
       tvc.m_verifivation_failed = true;
+      tvc.m_invalid_input = true;
       return false;
     }
 
@@ -113,6 +114,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;
     }
 
@@ -124,6 +126,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;
     }
 
@@ -132,6 +135,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;
     }
 
@@ -142,21 +146,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
@@ -480,7 +486,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);
@@ -496,7 +503,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 6b625e77b..a01d04dfe 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 72e399ec4..343c8a082 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;
-
-      BEGIN_KV_SERIALIZE_MAP()
-        KV_SERIALIZE(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 1856118f7..0579b1a40 100644
--- a/src/simplewallet/simplewallet.cpp
+++ b/src/simplewallet/simplewallet.cpp
@@ -2160,6 +2160,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)
   {
@@ -2307,6 +2310,9 @@ bool simple_wallet::sweep_dust(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 1c7187cf0..228e3b9af 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -1958,7 +1958,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 6074e0858..b6e583d2f 100644
--- a/src/wallet/wallet_errors.h
+++ b/src/wallet/wallet_errors.h
@@ -457,15 +457,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
       {
@@ -473,12 +475,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