From a8538395e236b589cdcb590cfdf470aabc96caff Mon Sep 17 00:00:00 2001 From: Doyle Date: Mon, 28 Oct 2019 14:03:13 +1100 Subject: [PATCH 1/2] Avoid checkpoint reorg-ing in the P2P thread; potential lockup Hold onto the invalid chain until you receive another alt block and cause a re-org on the block processing thread. --- src/cryptonote_core/blockchain.cpp | 101 ++++++++++++++++------------- src/cryptonote_core/blockchain.h | 2 +- tests/core_tests/loki_tests.cpp | 37 +++++++---- 3 files changed, 80 insertions(+), 60 deletions(-) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 46508b3a20f..7cead8b1a10 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -1431,7 +1431,7 @@ bool Blockchain::create_block_template(block& b, const crypto::hash *from_block, std::list alt_chain; block_verification_context bvc = boost::value_initialized(); std::vector timestamps; - if (!build_alt_chain(*from_block, alt_chain, timestamps, bvc, nullptr)) + if (!build_alt_chain(*from_block, alt_chain, timestamps, bvc, nullptr /*num_alt_checkpoints*/, nullptr /*num_checkpoints*/)) return false; if (parent_in_main) @@ -1651,14 +1651,20 @@ bool Blockchain::complete_timestamps_vector(uint64_t start_top_height, std::vect return true; } //------------------------------------------------------------------ -bool Blockchain::build_alt_chain(const crypto::hash &prev_id, std::list& alt_chain, std::vector ×tamps, block_verification_context& bvc, int *num_checkpoints) const +bool Blockchain::build_alt_chain(const crypto::hash &prev_id, + std::list &alt_chain, + std::vector ×tamps, + block_verification_context &bvc, + int *num_alt_checkpoints, + int *num_checkpoints) const { //build alternative subchain, front -> mainchain, back -> alternative head cryptonote::alt_block_data_t data; cryptonote::blobdata blob; timestamps.clear(); - int checkpoint_count = 0; + int alt_checkpoint_count = 0; + int checkpoint_count = 0; crypto::hash prev_hash = crypto::null_hash; block_extended_info bei = {}; blobdata checkpoint_blob; @@ -1667,31 +1673,36 @@ bool Blockchain::build_alt_chain(const crypto::hash &prev_id, std::listget_alt_block(prev_hash, &data, &blob, &checkpoint_blob)) { CHECK_AND_ASSERT_MES(cryptonote::parse_and_validate_block_from_blob(blob, bei.bl), false, "Failed to parse alt block"); - - if (data.checkpointed) + if (data.checkpointed) // Take checkpoint from blob stored alongside alt block { CHECK_AND_ASSERT_MES(t_serializable_object_from_blob(bei.checkpoint, checkpoint_blob), false, "Failed to parse alt checkpoint from blob"); - checkpoint_count++; + alt_checkpoint_count++; } - else + + // NOTE: If we receive or pre-define a checkpoint for a historical block + // that conflicts with current blocks on the blockchain, upon receipt of + // a new alt block, along this alt chain we should also double check all + // blocks that are checkpointed along this chain in m_checkpoints + + // This is particularly relevant for receiving checkpoints via P2P votes + // Which can form checkpoints retrospectively, that may conflict with + // your canonical chain. + bool height_is_checkpointed = false; + bool alt_block_matches_checkpoint = m_checkpoints.check_block(data.height, get_block_hash(bei.bl), &height_is_checkpointed, nullptr); + + if (height_is_checkpointed) { - // NOTE: If we receive or pre-define a checkpoint for a historical block - // that conflicts with current blocks on the blockchain, upon receipt of - // a new alt block, along this alt chain we should also count all blocks - // that are checkpointed along this chain in m_checkpoints if they were - // not specified at the time we received the alt block. - - // This is particularly relevant for receiving checkpoints via P2P votes - // Which can form checkpoints retrospectively, that may conflict with - // your canonical chain. - bool alt_block_is_checkpointed_in_main_db = false; - if (m_checkpoints.check_block(data.height, get_block_hash(bei.bl), &alt_block_is_checkpointed_in_main_db, nullptr) && - alt_block_is_checkpointed_in_main_db) + if (alt_block_matches_checkpoint) { - data.checkpointed = true; - CHECK_AND_ASSERT_MES(get_checkpoint(data.height, bei.checkpoint), false, "Unexpected failure to retrieve checkpoint after checking it existed"); - checkpoint_count++; + if (!data.checkpointed) + { + data.checkpointed = true; + CHECK_AND_ASSERT_MES(get_checkpoint(data.height, bei.checkpoint), false, "Unexpected failure to retrieve checkpoint after checking it existed"); + alt_checkpoint_count++; + } } + else + checkpoint_count++; // One of our stored-checkpoints references another block that's not this alt block. } bei.height = data.height; @@ -1706,6 +1717,7 @@ bool Blockchain::build_alt_chain(const crypto::hash &prev_id, std::list alt_chain; std::vector timestamps; int num_checkpoints_on_alt_chain = 0; - if (!build_alt_chain(b.prev_id, alt_chain, timestamps, bvc, &num_checkpoints_on_alt_chain)) + int num_checkpoints_on_chain = 0; + if (!build_alt_chain(b.prev_id, alt_chain, timestamps, bvc, &num_checkpoints_on_alt_chain, &num_checkpoints_on_chain)) return false; // verify that the block's timestamp is within the acceptable range @@ -1901,6 +1914,15 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id alt_data.already_generated_coins = get_outs_money_amount(b.miner_tx); alt_data.already_generated_coins += (alt_chain.size() ? prev_data.already_generated_coins : m_db->get_block_already_generated_coins(block_height - 1)); m_db->add_alt_block(id, alt_data, cryptonote::block_to_blob(b), checkpoint_blob.empty() ? nullptr : &checkpoint_blob); + + // Check current height for pre-existing checkpoint + bool height_is_checkpointed = false; + bool alt_block_matches_checkpoint = m_checkpoints.check_block(alt_data.height, id, &height_is_checkpointed, nullptr); + if (height_is_checkpointed) + { + if (!alt_block_matches_checkpoint) + num_checkpoints_on_chain++; + } } alt_chain.push_back(block_extended_info(alt_data, b, checkpoint)); @@ -1919,12 +1941,9 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id } } - uint64_t last_block_height = alt_chain.back().height; - std::vector const checkpoints = m_db->get_checkpoints_range(curr_blockchain_height, last_block_height, num_checkpoints_on_alt_chain + 1); - bool alt_chain_has_greater_pow = alt_data.cumulative_difficulty > main_chain_cumulative_difficulty; - bool alt_chain_has_more_checkpoints = (num_checkpoints_on_alt_chain > static_cast(checkpoints.size())); - bool alt_chain_has_equal_checkpoints = (num_checkpoints_on_alt_chain == static_cast(checkpoints.size())); - + bool alt_chain_has_greater_pow = alt_data.cumulative_difficulty > main_chain_cumulative_difficulty; + bool alt_chain_has_more_checkpoints = (num_checkpoints_on_alt_chain > num_checkpoints_on_chain); + bool alt_chain_has_equal_checkpoints = (num_checkpoints_on_alt_chain == num_checkpoints_on_chain); { std::vector txs; std::vector missed; @@ -1945,13 +1964,18 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id { if (alt_chain_has_more_checkpoints || (alt_chain_has_greater_pow && alt_chain_has_equal_checkpoints)) { + bool keep_alt_chain = false; if (alt_chain_has_more_checkpoints) + { MGINFO_GREEN("###### REORGANIZE on height: " << alt_chain.front().height << " of " << m_db->height() - 1 << ", checkpoint is found in alternative chain on height " << block_height); + } else + { + keep_alt_chain = true; MGINFO_GREEN("###### REORGANIZE on height: " << alt_chain.front().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 " << alt_data.cumulative_difficulty); + } - bool keep_alt_chain = (alt_chain_has_greater_pow && alt_chain_has_equal_checkpoints); - bool r = switch_to_alternative_blockchain(alt_chain, keep_alt_chain); + bool r = switch_to_alternative_blockchain(alt_chain, keep_alt_chain); if (r) bvc.m_added_to_main_chain = true; else @@ -4349,21 +4373,6 @@ bool Blockchain::update_checkpoints_from_json_file(const std::string& file_path) bool Blockchain::update_checkpoint(cryptonote::checkpoint_t const &checkpoint) { CRITICAL_REGION_LOCAL(m_blockchain_lock); - if (checkpoint.height < m_db->height() && !checkpoint.check(m_db->get_block_hash_from_height(checkpoint.height))) - { - uint8_t hf_version = get_current_hard_fork_version(); - if (hf_version >= cryptonote::network_version_13_enforce_checkpoints) - { - uint64_t blocks_to_pop = m_db->height() - checkpoint.height; - crypto::hash const reorg_hash = m_db->get_block_hash_from_height(checkpoint.height - 1); - MGINFO_GREEN("###### CHECKPOINTING REORGANIZE from height: " << m_db->height() << " to before checkpoint height: " << (checkpoint.height - 1) << " id: " << reorg_hash); - pop_blocks(blocks_to_pop); - } - } - - // NOTE: Rollback first, then add checkpoint otherwise we would delete the checkpoint during rollback and - // and if you are still syncing from the incorrect peer, you will re-receive the incorrect blocks again without the - // checkpointing stopping it. bool result = m_checkpoints.update_checkpoint(checkpoint); return result; } diff --git a/src/cryptonote_core/blockchain.h b/src/cryptonote_core/blockchain.h index c022e27760e..af92fc341eb 100644 --- a/src/cryptonote_core/blockchain.h +++ b/src/cryptonote_core/blockchain.h @@ -1255,7 +1255,7 @@ namespace cryptonote * * @return true on success, false otherwise */ - bool build_alt_chain(const crypto::hash &prev_id, std::list& alt_chain, std::vector ×tamps, block_verification_context& bvc, int *num_checkpoints) const; + bool build_alt_chain(const crypto::hash &prev_id, std::list& alt_chain, std::vector ×tamps, block_verification_context& bvc, int *num_alt_checkpoints, int *num_checkpoints) const; /** * @brief gets the difficulty requirement for a new block on an alternate chain diff --git a/tests/core_tests/loki_tests.cpp b/tests/core_tests/loki_tests.cpp index e40a05bc4c6..a97c02f698f 100644 --- a/tests/core_tests/loki_tests.cpp +++ b/tests/core_tests/loki_tests.cpp @@ -124,19 +124,28 @@ bool loki_checkpointing_alt_chain_handle_alt_blocks_at_tip::generate(std::vector fork.create_and_add_next_block(); fork.add_service_node_checkpoint(fork.height(), service_nodes::CHECKPOINT_MIN_VOTES); - // NOTE: Checkpoint should cause a reorg back to checkpoint height - 1. The - // alt block is still in the alt db because we don't trigger a chain switch - // until we receive a 2nd block that confirms the alt block. - loki_register_callback(events, "check_alt_block_count", [&events](cryptonote::core &c, size_t ev_index) + // NOTE: Though we receive a checkpoint via votes, the alt block is still in + // the alt db because we don't trigger a chain switch until we receive a 2nd + // block that confirms the alt block. + uint64_t curr_height = gen.height(); + crypto::hash curr_hash = get_block_hash(gen.top().block); + loki_register_callback(events, "check_alt_block_count", [&events, curr_height, curr_hash](cryptonote::core &c, size_t ev_index) { DEFINE_TESTS_ERROR_CONTEXT("check_alt_block_count"); + + uint64_t top_height; + crypto::hash top_hash; + c.get_blockchain_top(top_height, top_hash); + CHECK_EQ(top_height, curr_height); + CHECK_EQ(top_hash, curr_hash); CHECK_EQ(c.get_blockchain_storage().get_alternative_blocks_count(), 1); return true; }); // NOTE: We add a new block ontop that causes the alt block code path to run // again, and calculate that this alt chain now has 2 blocks on it with - // a greater cumulative difficulty, causing a chain switch at this point. + // now same difficulty but more checkpoints, causing a chain switch at this point. + gen.create_and_add_next_block(); fork.create_and_add_next_block(); crypto::hash expected_top_hash = cryptonote::get_block_hash(fork.top().block); loki_register_callback(events, "check_chain_reorged", [&events, expected_top_hash](cryptonote::core &c, size_t ev_index) @@ -307,6 +316,7 @@ bool loki_checkpointing_alt_chain_with_increasing_service_node_checkpoints::gene for (auto i = 0u; i < NUM_SERVICE_NODES; ++i) registration_txs[i] = gen.create_and_add_registration_tx(gen.first_miner()); gen.create_and_add_next_block(registration_txs); + gen.add_blocks_until_next_checkpointable_height(); // NOTE: Add blocks until we get to the first height that has a checkpointing quorum AND there are service nodes in the quorum. int const MAX_TRIES = 16; @@ -318,24 +328,25 @@ bool loki_checkpointing_alt_chain_with_increasing_service_node_checkpoints::gene if (quorum && quorum->validators.size()) break; } assert(tries != MAX_TRIES); + gen.add_n_blocks(service_nodes::CHECKPOINT_INTERVAL - 1); // Setup the two chains as follows, where C = checkpointed block, B = normal // block, the main chain should NOT reorg to the fork chain as they have the // same PoW-ish and equal number of checkpoints. // Main chain - C B B B B // Fork chain - B B B B C - loki_chain_generator fork = gen; - cryptonote::checkpoint_t gen_checkpoint = gen.create_service_node_checkpoint(gen.height(), service_nodes::CHECKPOINT_MIN_VOTES); - gen.create_and_add_next_block({}, &gen_checkpoint); + loki_chain_generator fork = gen; + gen.create_and_add_next_block(); + gen.add_service_node_checkpoint(gen.height(), service_nodes::CHECKPOINT_MIN_VOTES); fork.create_and_add_next_block(); - gen.add_blocks_until_next_checkpointable_height(); - gen.create_and_add_next_block(); + gen.add_n_blocks(service_nodes::CHECKPOINT_INTERVAL); + gen.add_service_node_checkpoint(gen.height(), service_nodes::CHECKPOINT_MIN_VOTES); - fork.add_blocks_until_next_checkpointable_height(); - cryptonote::checkpoint_t fork_first_checkpoint = fork.create_service_node_checkpoint(fork.height(), service_nodes::CHECKPOINT_MIN_VOTES); - fork.create_and_add_next_block({}, &fork_first_checkpoint); + fork.add_n_blocks(service_nodes::CHECKPOINT_INTERVAL); + fork.add_service_node_checkpoint(gen.height(), service_nodes::CHECKPOINT_MIN_VOTES); + fork.add_service_node_checkpoint(fork.height(), service_nodes::CHECKPOINT_MIN_VOTES); crypto::hash const gen_top_hash = cryptonote::get_block_hash(gen.top().block); loki_register_callback(events, "check_still_on_main_chain", [&events, gen_top_hash](cryptonote::core &c, size_t ev_index) { From 4c8d9e819353519af3f6c15ace000eec718e00ca Mon Sep 17 00:00:00 2001 From: Doyle Date: Mon, 28 Oct 2019 14:03:32 +1100 Subject: [PATCH 2/2] Bump v5 patch to 3 --- src/version.cpp.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/version.cpp.in b/src/version.cpp.in index 43e9ea9690c..48083951e63 100644 --- a/src/version.cpp.in +++ b/src/version.cpp.in @@ -1,6 +1,6 @@ #define DEF_LOKI_VERSION_MAJOR 5 #define DEF_LOKI_VERSION_MINOR 1 -#define DEF_LOKI_VERSION_PATCH 2 +#define DEF_LOKI_VERSION_PATCH 3 #define LOKI_STRINGIFY2(val) #val #define LOKI_STRINGIFY(val) LOKI_STRINGIFY2(val)