From 1701c267502c32af64212d8e496cbdb6f3bc00df Mon Sep 17 00:00:00 2001 From: warptangent Date: Sun, 11 Jan 2015 16:59:59 -0800 Subject: [PATCH 1/5] Use block index when obtaining block's difficulty for log statement Use last block id, not number of blocks (off-by-one error). Fixes error at start of blockchain reorganization: "Attempt to get cumulative difficulty from height failed -- difficulty not in db" --- src/cryptonote_core/blockchain.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 48e6543e..2a8eb672 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -1216,8 +1216,8 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id { //do reorganize! LOG_PRINT_GREEN("###### REORGANIZE on height: " - << alt_chain.front()->second.height << " of " << m_db->height() - << " with cum_difficulty " << m_db->get_block_cumulative_difficulty(m_db->height()) + << alt_chain.front()->second.height << " of " << m_db->height() - 1 + << " with cum_difficulty " << m_db->get_block_cumulative_difficulty(m_db->height() - 1) << std::endl << " alternative blockchain size: " << alt_chain.size() << " with cum_difficulty " << bei.cumulative_difficulty, LOG_LEVEL_0 ); From 4d0a94b20cd6e3b03657dc4ca49bc248a1e7f230 Mon Sep 17 00:00:00 2001 From: warptangent Date: Sun, 11 Jan 2015 18:04:04 -0800 Subject: [PATCH 2/5] Complete implementation of transaction removal Complete method BlockchainLMDB::remove_output() - use output index as the key for: m_output_indices, m_output_txs, m_output_keys - call new method BlockchainLMDB::remove_amount_output_index() Add method to remove amount output index. - BlockchainLMDB::remove_amount_output_index() - for m_output_amounts This also fixes the segfault when blockchain reorganization is attempted. --- .../BlockchainDB_impl/db_lmdb.cpp | 109 ++++++++++++++---- .../BlockchainDB_impl/db_lmdb.h | 7 +- src/cryptonote_core/blockchain_db.cpp | 6 +- src/cryptonote_core/blockchain_db.h | 2 +- 4 files changed, 99 insertions(+), 25 deletions(-) diff --git a/src/cryptonote_core/BlockchainDB_impl/db_lmdb.cpp b/src/cryptonote_core/BlockchainDB_impl/db_lmdb.cpp index 835b9248..4fa16b23 100644 --- a/src/cryptonote_core/BlockchainDB_impl/db_lmdb.cpp +++ b/src/cryptonote_core/BlockchainDB_impl/db_lmdb.cpp @@ -262,7 +262,7 @@ void BlockchainLMDB::add_transaction_data(const crypto::hash& blk_hash, const tr throw0(DB_ERROR("Failed to add tx unlock time to db transaction")); } -void BlockchainLMDB::remove_transaction_data(const crypto::hash& tx_hash) +void BlockchainLMDB::remove_transaction_data(const crypto::hash& tx_hash, const transaction& tx) { LOG_PRINT_L3("BlockchainLMDB::" << __func__); check_open(); @@ -279,7 +279,7 @@ void BlockchainLMDB::remove_transaction_data(const crypto::hash& tx_hash) if (mdb_del(*m_write_txn, m_tx_heights, &val_h, NULL)) throw1(DB_ERROR("Failed to add removal of tx block height to db transaction")); - remove_tx_outputs(tx_hash); + remove_tx_outputs(tx_hash, tx); if (mdb_del(*m_write_txn, m_tx_outputs, &val_h, NULL)) throw1(DB_ERROR("Failed to add removal of tx outputs to db transaction")); @@ -330,8 +330,9 @@ void BlockchainLMDB::add_output(const crypto::hash& tx_hash, const tx_out& tx_ou m_num_outputs++; } -void BlockchainLMDB::remove_tx_outputs(const crypto::hash& tx_hash) +void BlockchainLMDB::remove_tx_outputs(const crypto::hash& tx_hash, const transaction& tx) { + LOG_PRINT_L3("BlockchainLMDB::" << __func__); lmdb_cur cur(*m_write_txn, m_tx_outputs); @@ -356,7 +357,8 @@ void BlockchainLMDB::remove_tx_outputs(const crypto::hash& tx_hash) for (uint64_t i = 0; i < num_elems; ++i) { - remove_output(*(const uint64_t*)v.mv_data); + const tx_out tx_output = tx.vout[i]; + remove_output(*(const uint64_t*)v.mv_data, tx_output.amount); if (i < num_elems - 1) { mdb_cursor_get(cur, &k, &v, MDB_NEXT_DUP); @@ -367,17 +369,19 @@ void BlockchainLMDB::remove_tx_outputs(const crypto::hash& tx_hash) cur.close(); } +// TODO: probably remove this function void BlockchainLMDB::remove_output(const tx_out& tx_output) { + LOG_PRINT_L3("BlockchainLMDB::" << __func__ << " (unused version - does nothing)"); return; } -void BlockchainLMDB::remove_output(const uint64_t& out_index) +void BlockchainLMDB::remove_output(const uint64_t& out_index, const uint64_t amount) { LOG_PRINT_L3("BlockchainLMDB::" << __func__); check_open(); - MDB_val k; + MDB_val_copy k(out_index); MDB_val v; /****** Uncomment if ever outputs actually need to be stored in this manner @@ -400,29 +404,94 @@ void BlockchainLMDB::remove_output(const uint64_t& out_index) throw1(DB_ERROR("Error adding removal of output to db transaction")); *********************************************************************/ - auto result = mdb_del(*m_write_txn, m_output_indices, &v, NULL); - if (result != 0 && result != MDB_NOTFOUND) - throw1(DB_ERROR("Error adding removal of output tx index to db transaction")); - - result = mdb_del(*m_write_txn, m_output_txs, &v, NULL); - if (result != 0 && result != MDB_NOTFOUND) - throw1(DB_ERROR("Error adding removal of output tx hash to db transaction")); - - result = mdb_del(*m_write_txn, m_output_amounts, &v, NULL); - if (result != 0 && result != MDB_NOTFOUND) - throw1(DB_ERROR("Error adding removal of output amount to db transaction")); - - result = mdb_del(*m_write_txn, m_output_keys, &v, NULL); + auto result = mdb_del(*m_write_txn, m_output_indices, &k, NULL); if (result == MDB_NOTFOUND) { - LOG_PRINT_L2("Removing output, no public key found."); + LOG_PRINT_L0("Unexpected: global output index not found in m_output_indices"); + } + else if (result) + { + throw1(DB_ERROR("Error adding removal of output tx index to db transaction")); + } + + result = mdb_del(*m_write_txn, m_output_txs, &k, NULL); + // if (result != 0 && result != MDB_NOTFOUND) + // throw1(DB_ERROR("Error adding removal of output tx hash to db transaction")); + if (result == MDB_NOTFOUND) + { + LOG_PRINT_L0("Unexpected: global output index not found in m_output_txs"); + } + else if (result) + { + throw1(DB_ERROR("Error adding removal of output tx hash to db transaction")); + } + + result = mdb_del(*m_write_txn, m_output_keys, &k, NULL); + if (result == MDB_NOTFOUND) + { + LOG_PRINT_L0("Unexpected: global output index not found in m_output_keys"); } else if (result) throw1(DB_ERROR("Error adding removal of output pubkey to db transaction")); + remove_amount_output_index(amount, out_index); + m_num_outputs--; } +void BlockchainLMDB::remove_amount_output_index(const uint64_t amount, const uint64_t global_output_index) +{ + LOG_PRINT_L3("BlockchainLMDB::" << __func__); + check_open(); + + lmdb_cur cur(*m_write_txn, m_output_amounts); + + MDB_val_copy k(amount); + MDB_val v; + + auto result = mdb_cursor_get(cur, &k, &v, MDB_SET); + if (result == MDB_NOTFOUND) + throw1(OUTPUT_DNE("Attempting to get an output index by amount and amount index, but amount not found")); + else if (result) + throw0(DB_ERROR("DB error attempting to get an output")); + + size_t num_elems = 0; + mdb_cursor_count(cur, &num_elems); + + mdb_cursor_get(cur, &k, &v, MDB_FIRST_DUP); + + uint64_t amount_output_index = 0; + uint64_t goi = 0; + bool found_index = false; + for (uint64_t i = 0; i < num_elems; ++i) + { + mdb_cursor_get(cur, &k, &v, MDB_GET_CURRENT); + goi = *(const uint64_t *)v.mv_data; + if (goi == global_output_index) + { + amount_output_index = i; + found_index = true; + break; + } + mdb_cursor_get(cur, &k, &v, MDB_NEXT_DUP); + } + if (found_index) + { + // found the amount output index + // now delete it + result = mdb_cursor_del(cur, 0); + if (result) + throw0(DB_ERROR(std::string("Error deleting amount output index ").append(boost::lexical_cast(amount_output_index)).c_str())); + } + else + { + // not found + cur.close(); + throw1(OUTPUT_DNE("Failed to find amount output index")); + } + cur.close(); +} + void BlockchainLMDB::add_spent_key(const crypto::key_image& k_image) { LOG_PRINT_L3("BlockchainLMDB::" << __func__); diff --git a/src/cryptonote_core/BlockchainDB_impl/db_lmdb.h b/src/cryptonote_core/BlockchainDB_impl/db_lmdb.h index d95d1901..2513aa48 100644 --- a/src/cryptonote_core/BlockchainDB_impl/db_lmdb.h +++ b/src/cryptonote_core/BlockchainDB_impl/db_lmdb.h @@ -190,15 +190,16 @@ private: virtual void add_transaction_data(const crypto::hash& blk_hash, const transaction& tx); - virtual void remove_transaction_data(const crypto::hash& tx_hash); + virtual void remove_transaction_data(const crypto::hash& tx_hash, const transaction& tx); virtual void add_output(const crypto::hash& tx_hash, const tx_out& tx_output, const uint64_t& local_index); virtual void remove_output(const tx_out& tx_output); - void remove_tx_outputs(const crypto::hash& tx_hash); + void remove_tx_outputs(const crypto::hash& tx_hash, const transaction& tx); - void remove_output(const uint64_t& out_index); + void remove_output(const uint64_t& out_index, const uint64_t amount); + void remove_amount_output_index(const uint64_t amount, const uint64_t global_output_index); virtual void add_spent_key(const crypto::key_image& k_image); diff --git a/src/cryptonote_core/blockchain_db.cpp b/src/cryptonote_core/blockchain_db.cpp index 439cf5de..5e781af7 100644 --- a/src/cryptonote_core/blockchain_db.cpp +++ b/src/cryptonote_core/blockchain_db.cpp @@ -107,6 +107,9 @@ void BlockchainDB::remove_transaction(const crypto::hash& tx_hash) { transaction tx = get_tx(tx_hash); + // TODO: This loop calling remove_output() should be removed. It doesn't do + // anything (function is empty), and the outputs were already intended to be + // removed later as part of remove_transaction_data(). for (const tx_out& tx_output : tx.vout) { remove_output(tx_output); @@ -120,7 +123,8 @@ void BlockchainDB::remove_transaction(const crypto::hash& tx_hash) } } - remove_transaction_data(tx_hash); + // need tx as tx.vout has the tx outputs, and the output amounts are needed + remove_transaction_data(tx_hash, tx); } } // namespace cryptonote diff --git a/src/cryptonote_core/blockchain_db.h b/src/cryptonote_core/blockchain_db.h index b498320a..db56c7c0 100644 --- a/src/cryptonote_core/blockchain_db.h +++ b/src/cryptonote_core/blockchain_db.h @@ -274,7 +274,7 @@ private: virtual void add_transaction_data(const crypto::hash& blk_hash, const transaction& tx) = 0; // tells the subclass to remove data about a transaction - virtual void remove_transaction_data(const crypto::hash& tx_hash) = 0; + virtual void remove_transaction_data(const crypto::hash& tx_hash, const transaction& tx) = 0; // tells the subclass to store an output virtual void add_output(const crypto::hash& tx_hash, const tx_out& tx_output, const uint64_t& local_index) = 0; From 909ea810671e8da74b0c0d92641eee8dd798e0b3 Mon Sep 17 00:00:00 2001 From: warptangent Date: Sun, 11 Jan 2015 18:19:01 -0800 Subject: [PATCH 3/5] Remove a have_block() check so alternate block can be processed Remove have_block() check from Blockchain::handle_block_to_main_chain(). Add logging to have_block(). This allows blockchain reorganization to proceed further. have_block() check here causes an error after a blockchain reorganize begins with error: "Attempting to add block to main chain, but it's already either there or in an alternate chain." While reorganizing to become the main chain, a block in the alternative chain would be refused due to have_block() rightfully finding it in the alternative chain. The reorganization would end in rollback, restoring to previous blockchain. Original implementation didn't call it here, and it doesn't appear necessary to be called from here in this implementation either. When needed, it appears it's called prior to handle_block_to_main_chain(). --- src/cryptonote_core/blockchain.cpp | 34 ++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 2a8eb672..7015383f 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -1699,13 +1699,22 @@ bool Blockchain::have_block(const crypto::hash& id) const CRITICAL_REGION_LOCAL(m_blockchain_lock); if(m_db->block_exists(id)) + { + LOG_PRINT_L3("block exists in main chain"); return true; + } if(m_alternative_chains.count(id)) + { + LOG_PRINT_L3("block found in m_alternative_chains"); return true; + } if(m_invalid_blocks.count(id)) + { + LOG_PRINT_L3("block found in m_invalid_blocks"); return true; + } return false; } @@ -2010,14 +2019,25 @@ bool Blockchain::check_block_timestamp(const block& b) const bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash& id, block_verification_context& bvc) { LOG_PRINT_L3("Blockchain::" << __func__); + + // NOTE: Omitting check below with have_block() It causes an error after a + // blockchain reorganize begins with error: "Attempting to add block to main + // chain, but it's already either there or in an alternate" + // + // A block in the alternative chain, desired to become the main chain, never + // makes it due to have_block finding it in he alternative chain. + // + // Original implementation didn't use it here, and it doesn't appear + // necessary to be called from here in this implementation either. + // if we already have the block, return false - if (have_block(id)) - { - LOG_PRINT_L0("Attempting to add block to main chain, but it's already either there or in an alternate chain. hash: " << id); - bvc.m_verifivation_failed = true; - return false; - } - + // if (have_block(id)) + // { + // LOG_PRINT_L0("Attempting to add block to main chain, but it's already either there or in an alternate chain. hash: " << id); + // bvc.m_verifivation_failed = true; + // return false; + // } + TIME_MEASURE_START(block_processing_time); CRITICAL_REGION_LOCAL(m_blockchain_lock); if(bl.prev_id != get_tail_id()) From 63051bea1c5f37b922a50af444e039d5ea33c09d Mon Sep 17 00:00:00 2001 From: warptangent Date: Sun, 11 Jan 2015 18:46:08 -0800 Subject: [PATCH 4/5] Fix comparison between main and alternate chain's cumulative difficulty. This fixes the continual reorganization between a main and alternate chain, using the same two latest blocks from each. The check that cumulative difficulty of the alternate chain is bigger than main's was not using main's last block, but incorrectly using the passed-in block's previous block. main_chain_cumulative_difficulty was being used in two different ways. This has been split up to keep use of main_chain_cumulative_difficulty consistent. --- src/cryptonote_core/blockchain.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 7015383f..c052e394 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -1188,8 +1188,16 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id // FIXME: // this brings up an interesting point: consider allowing to get block // difficulty both by height OR by hash, not just height. - auto main_chain_cumulative_difficulty = m_db->get_block_cumulative_difficulty(m_db->get_block_height(b.prev_id)); - bei.cumulative_difficulty = alt_chain.size() ? it_prev->second.cumulative_difficulty : main_chain_cumulative_difficulty; + difficulty_type main_chain_cumulative_difficulty = m_db->get_block_cumulative_difficulty(m_db->height() - 1); + if (alt_chain.size()) + { + bei.cumulative_difficulty = it_prev->second.cumulative_difficulty; + } + else + { + // passed-in block's previous block's cumulative difficulty, found on the main chain + bei.cumulative_difficulty = m_db->get_block_cumulative_difficulty(m_db->get_block_height(b.prev_id)); + } bei.cumulative_difficulty += current_diff; // add block to alternate blocks storage, From 0840c2fd7eb28aabcf793c2ec59824fd354a936c Mon Sep 17 00:00:00 2001 From: warptangent Date: Sun, 11 Jan 2015 19:07:27 -0800 Subject: [PATCH 5/5] Fix height assertion in Blockchain::handle_alternative_block() It expects the total number of blocks of main chain, not last block id (off-by-one error). This again behaves like the same height assertion done in original implementation in blockchain_storage::handle_alternative_block(). This allows a reorganization to proceed after an alternative block has been added. --- src/cryptonote_core/blockchain.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index c052e394..4fa868ab 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -1113,7 +1113,7 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id if(alt_chain.size()) { // make sure alt chain doesn't somehow start past the end of the main chain - CHECK_AND_ASSERT_MES(m_db->height() - 1 > alt_chain.front()->second.height, false, "main blockchain wrong height"); + CHECK_AND_ASSERT_MES(m_db->height() > alt_chain.front()->second.height, false, "main blockchain wrong height"); // make sure that the blockchain contains the block that should connect // this alternate chain with it.