From f33a88cfc17177e3a079ee2226df9674dfa2d138 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Fri, 25 Dec 2015 22:13:38 +0000 Subject: [PATCH] blockchain: fix a few block addition bugs If the block reward was too high, the verification failed flag was set, but the function continued. The code which was supposed to trap this flag and return failure failed to trap it, and, while the block was not added to the chain, the function would return success. The reason for avoiding returning when the block reward problem was detected was to be able to return any transactions to the pool if needed. This is now mooted by moving the transaction return code to a separate function, which is now called at all appropriate points, making the logic much simpler, and hopefully correct now. We also move the hard fork version check after the prev_id check, as block which does not go on the top of the chain might not have the expected version there, without being invalid just for this reason. Last, we trap the case where a block fails to be added due to using already spent key images, to set the verification failed flag. --- src/cryptonote_core/blockchain.cpp | 81 +++++++++++++++++------------- src/cryptonote_core/blockchain.h | 1 + 2 files changed, 47 insertions(+), 35 deletions(-) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 03bf1df6..71a2b884 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -2363,6 +2363,23 @@ bool Blockchain::check_block_timestamp(const block& b) const return check_block_timestamp(timestamps, b); } //------------------------------------------------------------------ +void Blockchain::return_tx_to_pool(const std::vector &txs) +{ + for (auto& tx : txs) + { + cryptonote::tx_verification_context tvc = AUTO_VAL_INIT(tvc); + // We assume that if they were in a block, the transactions are already + // known to the network as a whole. However, if we had mined that block, + // that might not be always true. Unlikely though, and always relaying + // these again might cause a spike of traffic as many nodes re-relay + // all the transactions in a popped block when a reorg happens. + if (!m_tx_pool.add_tx(tx, tvc, true, true)) + { + LOG_PRINT_L0("Failed to return taken transaction with hash: " << get_transaction_hash(tx) << " to tx_pool"); + } + } +} +//------------------------------------------------------------------ // Needs to validate the block and acquire each transaction from the // transaction mem_pool, then pass the block and transactions to // m_db->add_block() @@ -2374,16 +2391,17 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash& CRITICAL_REGION_LOCAL(m_blockchain_lock); TIME_MEASURE_START(t1); + if(bl.prev_id != get_tail_id()) + { + LOG_PRINT_L1("Block with id: " << id << std::endl << "has wrong prev_id: " << bl.prev_id << std::endl << "expected: " << get_tail_id()); + return false; + } + // this is a cheap test if (!m_hardfork->check(bl)) { LOG_PRINT_L1("Block with id: " << id << std::endl << "has old version: " << (unsigned)bl.major_version << std::endl << "current: " << (unsigned)m_hardfork->get_current_version()); - return false; - } - - if(bl.prev_id != get_tail_id()) - { - LOG_PRINT_L1("Block with id: " << id << std::endl << "has wrong prev_id: " << bl.prev_id << std::endl << "expected: " << get_tail_id()); + bvc.m_verifivation_failed = true; return false; } @@ -2498,7 +2516,6 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash& uint64_t t_exists = 0; uint64_t t_pool = 0; uint64_t t_dblspnd = 0; - bool add_tx_to_pool = false; TIME_MEASURE_FINISH(t3); // XXX old code adds miner tx here @@ -2520,7 +2537,8 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash& { LOG_PRINT_L1("Block with id: " << id << " attempting to add transaction already in blockchain with id: " << tx_id); bvc.m_verifivation_failed = true; - break; + return_tx_to_pool(txs); + return false; } TIME_MEASURE_FINISH(aa); @@ -2532,7 +2550,8 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash& { LOG_PRINT_L1("Block with id: " << id << " has at least one unknown transaction with id: " << tx_id); bvc.m_verifivation_failed = true; - break; + return_tx_to_pool(txs); + return false; } TIME_MEASURE_FINISH(bb); @@ -2568,8 +2587,8 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash& add_block_as_invalid(bl, id); LOG_PRINT_L1("Block with id " << id << " added as invalid because of wrong inputs in transactions"); bvc.m_verifivation_failed = true; - add_tx_to_pool = true; - break; + return_tx_to_pool(txs); + return false; } } #if defined(PER_BLOCK_CHECKPOINT) @@ -2584,8 +2603,8 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash& add_block_as_invalid(bl, id); LOG_PRINT_L1("Block with id " << id << " added as invalid because of wrong inputs in transactions"); bvc.m_verifivation_failed = true; - add_tx_to_pool = true; - break; + return_tx_to_pool(txs); + return false; } } #endif @@ -2600,10 +2619,12 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash& TIME_MEASURE_START(vmt); uint64_t base_reward = 0; uint64_t already_generated_coins = m_db->height() ? m_db->get_block_already_generated_coins(m_db->height() - 1) : 0; - if(!validate_miner_transaction(bl, cumulative_block_size, fee_summary, base_reward, already_generated_coins)) + if(!validate_miner_transaction(bl, cumulative_block_size, fee_summary, base_reward, already_generated_coins, bvc.m_partial_block_reward)) { LOG_PRINT_L1("Block with id: " << id << " has incorrect miner transaction"); bvc.m_verifivation_failed = true; + return_tx_to_pool(txs); + return false; } TIME_MEASURE_FINISH(vmt); @@ -2623,40 +2644,30 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash& TIME_MEASURE_START(addblock); uint64_t new_height = 0; - bool add_success = true; if (!bvc.m_verifivation_failed) { try { new_height = m_db->add_block(bl, block_size, cumulative_difficulty, already_generated_coins, txs); } + catch (const KEY_IMAGE_EXISTS& e) + { + LOG_ERROR("Error adding block with hash: " << id << " to blockchain, what = " << e.what()); + bvc.m_verifivation_failed = true; + return_tx_to_pool(txs); + return false; + } catch (const std::exception& e) { //TODO: figure out the best way to deal with this failure LOG_ERROR("Error adding block with hash: " << id << " to blockchain, what = " << e.what()); - add_success = false; + return_tx_to_pool(txs); + return false; } } - - // if we failed for any reason to verify the block, return taken - // transactions to the tx_pool. - if ((bvc.m_verifivation_failed && add_tx_to_pool) || !add_success) + else { - // return taken transactions to transaction pool - for (auto& tx : txs) - { - cryptonote::tx_verification_context tvc = AUTO_VAL_INIT(tvc); - // We assume that if they were in a block, the transactions are already - // known to the network as a whole. However, if we had mined that block, - // that might not be always true. Unlikely though, and always relaying - // these again might cause a spike of traffic as many nodes re-relay - // all the transactions in a popped block when a reorg happens. - if (!m_tx_pool.add_tx(tx, tvc, true, true)) - { - LOG_PRINT_L0("Failed to return taken transaction with hash: " << get_transaction_hash(tx) << " to tx_pool"); - } - } - return false; + LOG_ERROR("Blocks that failed verification should not reach here"); } TIME_MEASURE_FINISH(addblock); diff --git a/src/cryptonote_core/blockchain.h b/src/cryptonote_core/blockchain.h index 381236db..2f9287e8 100644 --- a/src/cryptonote_core/blockchain.h +++ b/src/cryptonote_core/blockchain.h @@ -269,6 +269,7 @@ namespace cryptonote uint64_t get_adjusted_time() const; bool complete_timestamps_vector(uint64_t start_height, std::vector& timestamps); bool update_next_cumulative_size_limit(); + void return_tx_to_pool(const std::vector &txs); bool check_for_double_spend(const transaction& tx, key_images_container& keys_this_block) const; void get_timestamp_and_difficulty(uint64_t ×tamp, difficulty_type &difficulty, const int offset) const;