From 24b3e9007a8ad0684edcee51df444b21a033e4ba Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sun, 27 Mar 2016 12:35:36 +0100 Subject: [PATCH] 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. --- src/cryptonote_core/blockchain.cpp | 15 ++++++--- src/cryptonote_core/blockchain.h | 9 ++++-- src/cryptonote_core/cryptonote_core.cpp | 1 + src/cryptonote_core/tx_pool.cpp | 18 +++++++++-- src/cryptonote_core/verification_context.h | 7 +++++ src/rpc/core_rpc_server.cpp | 36 ++++++++++++++++------ src/rpc/core_rpc_server_commands_defs.h | 18 +++++++++++ src/simplewallet/simplewallet.cpp | 6 ++++ src/wallet/wallet2.cpp | 2 +- src/wallet/wallet_errors.h | 9 +++++- 10 files changed, 98 insertions(+), 23 deletions(-) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 0f6afe74..61cf064b 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::vectorget_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 393faef2..db930cf9 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 c31be5ac..20b9f0b0 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 5d67acdd..829caaf0 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 fcfd2a3e..e58291ea 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 6b625e77..a01d04df 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 72e399ec..343c8a08 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 1856118f..0579b1a4 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 &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 1c7187cf..228e3b9a 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 6074e085..b6e583d2 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