From 496c3daea49384dc15c091952600a5e0cc84d839 Mon Sep 17 00:00:00 2001 From: canepat <16927169+canepat@users.noreply.github.com> Date: Mon, 11 Sep 2023 09:47:49 +0200 Subject: [PATCH 1/8] node: revalidate canonical chain with one forward cycle at startup node: handle already existing canonical block in main chain verification --- silkworm/node/stagedsync/execution_engine.cpp | 13 +- .../node/stagedsync/execution_pipeline.cpp | 24 +- .../node/stagedsync/execution_pipeline.hpp | 11 +- silkworm/node/stagedsync/forks/main_chain.cpp | 211 +++++++++++++++--- silkworm/node/stagedsync/forks/main_chain.hpp | 11 +- .../node/stagedsync/forks/main_chain_test.cpp | 25 ++- 6 files changed, 222 insertions(+), 73 deletions(-) diff --git a/silkworm/node/stagedsync/execution_engine.cpp b/silkworm/node/stagedsync/execution_engine.cpp index 08f228e7d3..62061e65e4 100644 --- a/silkworm/node/stagedsync/execution_engine.cpp +++ b/silkworm/node/stagedsync/execution_engine.cpp @@ -125,7 +125,7 @@ std::optional ExecutionEngine::find_forking_point( if (path.forking_point == main_chain_.last_chosen_head()) return {std::move(path)}; // search remaining path on main chain - if (main_chain_.is_canonical(path.forking_point)) return {std::move(path)}; + if (main_chain_.is_finalized_canonical(path.forking_point)) return {std::move(path)}; auto forking_point = main_chain_.find_forking_point(path.forking_point.hash); if (!forking_point) return {}; // not found @@ -152,7 +152,7 @@ concurrency::AwaitableFuture ExecutionEngine::verify_chain(H auto fork = find_fork_by_head(forks_, head_block_hash); if (fork == forks_.end()) { - if (main_chain_.is_canonical(head_block_hash)) { + if (main_chain_.is_finalized_canonical(head_block_hash)) { SILK_DEBUG << "ExecutionEngine: chain " << head_block_hash.to_hex() << " already verified"; concurrency::AwaitablePromise promise{io_context_.get_executor()}; promise.set_value(ValidChain{last_fork_choice_}); @@ -176,13 +176,16 @@ bool ExecutionEngine::notify_fork_choice_update(Hash head_block_hash, std::optio if (!updated) return false; last_fork_choice_ = main_chain_.last_chosen_head(); - fork_tracking_active_ = true; + if (head_block_hash == main_chain_.current_head().hash and node_settings_.parallel_fork_tracking_enabled) { + log::Info("ExecutionEngine") << "activate parallel fork tracking at head " << head_block_hash.to_hex(); + fork_tracking_active_ = true; + } } else { // chose the fork with the given head auto f = find_fork_by_head(forks_, head_block_hash); if (f == forks_.end()) { - if (main_chain_.is_canonical(head_block_hash)) { + if (main_chain_.is_finalized_canonical(head_block_hash)) { SILK_DEBUG << "ExecutionEngine: chain " << head_block_hash.to_hex() << " already chosen"; return true; } else { @@ -291,7 +294,7 @@ std::optional ExecutionEngine::get_block_number(Hash header_hash) cons } bool ExecutionEngine::is_canonical(Hash header_hash) const { - return main_chain_.is_canonical(header_hash); + return main_chain_.is_finalized_canonical(header_hash); } } // namespace silkworm::stagedsync diff --git a/silkworm/node/stagedsync/execution_pipeline.cpp b/silkworm/node/stagedsync/execution_pipeline.cpp index 45c5c07225..2ac461efda 100644 --- a/silkworm/node/stagedsync/execution_pipeline.cpp +++ b/silkworm/node/stagedsync/execution_pipeline.cpp @@ -50,17 +50,11 @@ class ExecutionPipeline::LogTimer : public Timer { : Timer{ pipeline->node_settings_->asio_context.get_executor(), pipeline->node_settings_->sync_loop_log_interval_seconds * 1'000, - [this] { return execute(); }, - true}, - pipeline_{pipeline} { - start(); - } - - ~LogTimer() { - stop(); - } + [this] { return expired(); }, + /*.auto_start=*/true}, + pipeline_{pipeline} {} - bool execute() { + bool expired() { if (pipeline_->is_stopping()) { log::Info(pipeline_->get_log_prefix()) << "stopping ..."; return false; @@ -162,7 +156,7 @@ void ExecutionPipeline::load_stages() { db::stages::kExecutionKey, db::stages::kSendersKey, db::stages::kBlockBodiesKey, - db::stages::kBlockHashesKey, // Decanonify block hashes + db::stages::kBlockHashesKey, // De-canonify block hashes db::stages::kHeadersKey, }); } @@ -214,15 +208,15 @@ Stage::Result ExecutionPipeline::forward(db::RWTxn& cycle_txn, BlockNum target_h if (stage_result != Stage::Result::kSuccess) { /* clang-format off */ auto result_description = std::string(magic_enum::enum_name(stage_result)); - log::Error(get_log_prefix(), {"op", "Forward", "returned", }); + log::Error(get_log_prefix(), {"op", "Forward", "returned", result_description}); log::Error("ExecPipeline") << "Forward interrupted due to stage " << current_stage_->first << " failure"; return stage_result; } /* clang-format on */ - auto stage_head_number_ = db::stages::read_stage_progress(cycle_txn, current_stage_->first); - if (stage_head_number_ != target_height) { + auto stage_head_number = db::stages::read_stage_progress(cycle_txn, current_stage_->first); + if (stage_head_number != target_height) { throw std::logic_error("Sync pipeline: stage returned success with an height different from target=" + - to_string(target_height) + " reached= " + to_string(stage_head_number_)); + to_string(target_height) + " reached= " + to_string(stage_head_number)); } auto [_, stage_duration] = stages_stop_watch.lap(); diff --git a/silkworm/node/stagedsync/execution_pipeline.hpp b/silkworm/node/stagedsync/execution_pipeline.hpp index eff97b2740..7b1f06b3d6 100644 --- a/silkworm/node/stagedsync/execution_pipeline.hpp +++ b/silkworm/node/stagedsync/execution_pipeline.hpp @@ -45,12 +45,13 @@ class ExecutionPipeline : public Stoppable { silkworm::NodeSettings* node_settings_; std::unique_ptr sync_context_; // context shared across stages - using Stage_Container = std::map>; - Stage_Container stages_; + using StageContainer = std::map>; + StageContainer stages_; + StageContainer::iterator current_stage_; - Stage_Container::iterator current_stage_; - std::vector stages_forward_order_; - std::vector stages_unwind_order_; + using StageNames = std::vector; + StageNames stages_forward_order_; + StageNames stages_unwind_order_; std::atomic current_stages_count_{0}; std::atomic current_stage_number_{0}; diff --git a/silkworm/node/stagedsync/forks/main_chain.cpp b/silkworm/node/stagedsync/forks/main_chain.cpp index 600904ee22..51907ca851 100644 --- a/silkworm/node/stagedsync/forks/main_chain.cpp +++ b/silkworm/node/stagedsync/forks/main_chain.cpp @@ -18,6 +18,8 @@ #include +#include + #include #include #include @@ -54,15 +56,26 @@ MainChain::MainChain(boost::asio::io_context& ctx, NodeSettings& ns, const db::R } else last_fork_choice_ = last_finalized_head_; - if (canonical_chain_.current_head() == last_fork_choice_) { - canonical_head_status_ = ValidChain{canonical_chain_.current_head()}; - } - tx_.commit_and_stop(); } void MainChain::open() { tx_.reopen(*db_access_); // comply to mdbx limitation: tx must be used from its creation thread + + // Revalidate chain by executing forward cycle up to the canonical current head at startup: + // - if last cycle completed successfully, this will simply do nothing (no hurt) + // - if last cycle was executed partially (i.e. not all stages are at the same height), this will do a cleanup cycle + const auto& canonical_head{canonical_chain_.current_head()}; + SILK_INFO << "Revalidate chain up to the canonical current head number=" << canonical_head.number << " hash=" << to_hex(canonical_head.hash); + + forward(canonical_head.number, canonical_head.hash); + + // If forward cleanup cycle has not produced a valida chain, then we need to unwind + if (not std::holds_alternative(canonical_head_status_)) { + const auto unwind_point{pipeline_.unwind_point()}; + ensure_invariant(unwind_point.has_value(), "unwind point from pipeline requested when forward fails"); + unwind(*unwind_point); + } } void MainChain::close() { @@ -99,7 +112,7 @@ std::optional MainChain::find_forking_point(const Hash& header_hash) co return find_forking_point(*header, header_hash); } -bool MainChain::is_canonical(BlockId block) const { +bool MainChain::is_finalized_canonical(BlockId block) const { if (block.number > last_fork_choice_.number) return false; return (canonical_chain_.get_hash(block.number) == block.hash); } @@ -134,26 +147,48 @@ void MainChain::insert_block(const Block& block) { block_count = 0; StopWatch timing{StopWatch::kStart}; tx_.commit_and_renew(); - SILK_INFO << "MainChain: commit " << kInsertedBlockBatch << " blocks up to " << block.header.number + SILK_INFO << "MainChain::insert_block commit " << kInsertedBlockBatch << " blocks up to " << block.header.number << " took " << StopWatch::format(timing.since_start()); } } -VerificationResult MainChain::verify_chain(Hash head_block_hash) { - SILK_TRACE << "MainChain: verifying chain head=" << head_block_hash.to_hex(); - - // retrieve the head header - auto head_header = get_header(head_block_hash); - ensure_invariant(head_header.has_value(), "header to verify not found"); +VerificationResult MainChain::verify_chain(Hash block_hash) { + SILK_TRACE << "MainChain: verifying chain block=" << block_hash.to_hex(); + + // Retrieve the block header to validate + const auto block_header = get_header(block_hash); + ensure_invariant(block_header.has_value(), "header to verify not found"); + + // Check if incoming block already exists as canonical block + if (is_canonical(block_header->number, block_hash)) { + // The incoming block matches a block already on the canonical chain, verification is not always needed + if (block_header->number <= last_fork_choice_.number) { + // Last FCU block is greater than or equal incoming canonical block, chain is valid up to last FCU block + return ValidChain{last_fork_choice_.number, last_fork_choice_.hash}; + } else if (std::holds_alternative(canonical_head_status_)) { + // Chain is valid up to canonical head + return ValidChain{canonical_chain_.current_head().number, canonical_chain_.current_head().hash}; + } else if (std::holds_alternative(canonical_head_status_)) { + // Chain is valid up to unwind point + const auto& invalid_chain{std::get(canonical_head_status_)}; + if (block_header->number <= invalid_chain.unwind_point.number) { + // Unwind point is greater than or equal incoming canonical block, chain is valid up to unwind point + return ValidChain{invalid_chain.unwind_point.number, invalid_chain.unwind_point.hash}; + } else { + // Incoming canonical block is greater than unwind point, so chain is invalid + return invalid_chain; + } + } + } // db commit policy bool commit_at_each_stage = is_first_sync_; if (!commit_at_each_stage) tx_.disable_commit(); // the new head is on a new fork? - BlockId forking_point = canonical_chain_.find_forking_point(*head_header, head_block_hash); // the forking origin + BlockId forking_point = canonical_chain_.find_forking_point(*block_header, block_hash); // the forking origin - if (head_block_hash != canonical_chain_.current_head().hash && // if the new head is not the current head + if (block_hash != canonical_chain_.current_head().hash && // if the new head is not the current head forking_point.number < canonical_chain_.current_head().number) { // and if the forking is behind the head // we need to do unwind to change canonical auto unwind_result = pipeline_.unwind(tx_, forking_point.number); @@ -163,10 +198,11 @@ VerificationResult MainChain::verify_chain(Hash head_block_hash) { } // update canonical up to header_hash - canonical_chain_.update_up_to(head_header->number, head_block_hash); + canonical_chain_.update_up_to(block_header->number, block_hash); // forward - Stage::Result forward_result = pipeline_.forward(tx_, head_header->number); + Stage::Result forward_result = pipeline_.forward(tx_, block_header->number); + SILK_INFO << "MainChain::verify_chain commit forward_result=" << magic_enum::enum_name<>(forward_result); // evaluate result VerificationResult verify_result; @@ -176,6 +212,7 @@ VerificationResult MainChain::verify_chain(Hash head_block_hash) { pipeline_.head_header_hash() == canonical_chain_.current_head().hash, "forward succeeded with pipeline head not aligned with canonical head"); verify_result = ValidChain{pipeline_.head_header_number(), pipeline_.head_header_hash()}; + SILK_INFO << "MainChain::verify_chain commit verify_result index=" << verify_result.index(); break; } case Stage::Result::kWrongFork: @@ -203,25 +240,79 @@ VerificationResult MainChain::verify_chain(Hash head_block_hash) { // finish tx_.enable_commit(); - if (commit_at_each_stage) tx_.commit_and_renew(); + if (commit_at_each_stage and std::holds_alternative(verify_result)) { + StopWatch timing{StopWatch::kStart}; + SILK_INFO << "MainChain::verify_chain commit BEFORE"; + tx_.commit_and_renew(); + SILK_INFO << "MainChain::verify_chain commit at hash=" << block_hash.to_hex() + << " took " << StopWatch::format(timing.since_start()); + } return verify_result; } bool MainChain::notify_fork_choice_update(Hash head_block_hash, std::optional finalized_block_hash) { - if (canonical_chain_.current_head().hash != head_block_hash) { - // usually update_fork_choice must follow verify_chain with the same header except when: - // 1) (PoS) CL is syncing so head_block_hash is referring to a previous valid head - // 2) (PoW) previous verify_chain returned InvalidChain so CL is issuing a fcu with a previous valid head + if (finalized_block_hash and not canonical_chain_.has(*finalized_block_hash)) { + return false; // finalized block not found + } - if (canonical_chain_.has(head_block_hash) && - std::holds_alternative(canonical_head_status_)) return true; + const auto head_block_number{get_block_number(head_block_hash)}; + ensure_invariant(head_block_number.has_value(), "unknown block number for head block hash"); + if (is_canonical_head_ancestor(head_block_hash) and head_block_number <= last_fork_choice_.number) { + // FCU selects an old canonical block already targeted by a previous FCU + return true; + } - auto verification = verify_chain(head_block_hash); // this will reset canonical chain to head_block_hash + // Usually FCU must follow verify_chain with the same header except when: + // 1) (PoS) CL is syncing so head_block_hash is referring to a previous valid head + // 2) (PoW) previous verify_chain returned InvalidChain so CL is issuing a FCU with a previous valid head + // When FCU selects a non-canonical block or our last canonical is not valid, we need to verify the resulting chain + if (not canonical_chain_.has(head_block_hash) or not std::holds_alternative(canonical_head_status_)) { + verify_chain(head_block_hash); // this will reset canonical chain to head_block_hash ensure_invariant(canonical_chain_.current_head().hash == head_block_hash, "canonical head not aligned with fork choice"); } + if (!std::holds_alternative(canonical_head_status_)) { + return false; // canonical head is not valid + } + + const auto valid_chain = std::get(canonical_head_status_); + ensure_invariant(canonical_chain_.current_head() == valid_chain.current_head, + "canonical head not aligned with saved head status"); + + last_fork_choice_.number = *head_block_number; + last_fork_choice_.hash = head_block_hash; + + db::write_last_head_block(tx_, last_fork_choice_.hash); + if (finalized_block_hash) { + db::write_last_finalized_block(tx_, *finalized_block_hash); + + const auto finalized_block_number = get_block_number(*finalized_block_hash); + last_finalized_head_.number = *finalized_block_number; + last_finalized_head_.hash = std::move(*finalized_block_hash); + } + + tx_.commit_and_renew(); + + is_first_sync_ = false; + + return true; +} + +bool MainChain::notify_fork_choice_update2(Hash head_block_hash, std::optional finalized_block_hash) { + if (canonical_chain_.current_head().hash != head_block_hash) { + // usually update_fork_choice must follow verify_chain with the same header except when: + // 1) (PoS) CL is syncing so head_block_hash is referring to a previous valid head + // 2) (PoW) previous verify_chain returned InvalidChain so CL is issuing a fcu with a previous valid head + + if (not canonical_chain_.has(head_block_hash) or not std::holds_alternative(canonical_head_status_)) { + auto verification = verify_chain(head_block_hash); // this will reset canonical chain to head_block_hash + ensure_invariant(canonical_chain_.current_head().hash == head_block_hash, + "canonical head not aligned with fork choice"); + } + } + if (!std::holds_alternative(canonical_head_status_)) return false; // head is not valid auto valid_chain = std::get(canonical_head_status_); @@ -250,6 +341,14 @@ bool MainChain::notify_fork_choice_update(Hash head_block_hash, std::optional MainChain::collect_bad_headers(db::RWTxn& tx, InvalidChain& invalid_chain) { if (!invalid_chain.bad_block) return {}; + const auto bad_count{canonical_chain_.current_head().number - invalid_chain.unwind_point.number}; + SILK_INFO << "MainChain::collect_bad_headers bad_count=" << bad_count << " skip=" << (bad_count > 10); + + // Do not collect too many headers, rather skip + if (bad_count > 10) { + return {}; + } + std::set bad_headers; for (BlockNum current_height = canonical_chain_.current_head().number; current_height > invalid_chain.unwind_point.number; current_height--) { @@ -377,7 +476,7 @@ bool MainChain::extends(BlockId block, BlockId supposed_parent) const { return false; } -bool MainChain::is_canonical(Hash block_hash) const { +bool MainChain::is_finalized_canonical(Hash block_hash) const { auto header = get_header(block_hash); if (!header) return false; if (header->number > last_fork_choice_.number) return false; @@ -385,16 +484,62 @@ bool MainChain::is_canonical(Hash block_hash) const { return canonical_hash_at_same_height == block_hash; } -/* -ChainHead MainChain::get_canonical_head_from_db() { - auto [height, hash] = db::read_canonical_head(tx_); +bool MainChain::is_canonical(BlockNum block_height, const Hash& block_hash) const { + // Check if specified block already exists as canonical block + return canonical_chain_.get_hash(block_height) == block_hash; +} + +bool MainChain::is_canonical_head_ancestor(const Hash& block_hash) const { + return canonical_chain_.has(block_hash) and canonical_chain_.current_head().hash != block_hash; +} - std::optional td = db::read_total_difficulty(tx_, height, hash); - ensure_invariant(td.has_value(), - "total difficulty of canonical hash at height " + std::to_string(height) + " not found in db"); +void MainChain::forward(BlockNum head_height, const Hash& head_hash) { + // update canonical up to header_hash + canonical_chain_.update_up_to(head_height, head_hash); + + // forward + Stage::Result forward_result = pipeline_.forward(tx_, head_height); - return {height, hash, *td}; + // evaluate result + VerificationResult verify_result; + switch (forward_result) { + case Stage::Result::kSuccess: { + ensure_invariant(pipeline_.head_header_number() == canonical_chain_.current_head().number && + pipeline_.head_header_hash() == canonical_chain_.current_head().hash, + "forward succeeded with pipeline head not aligned with canonical head"); + verify_result = ValidChain{pipeline_.head_header_number(), pipeline_.head_header_hash()}; + break; + } + case Stage::Result::kWrongFork: + case Stage::Result::kInvalidBlock: + case Stage::Result::kWrongStateRoot: { + ensure_invariant(pipeline_.unwind_point().has_value(), + "unwind point from pipeline requested when forward fails"); + InvalidChain invalid_chain; + invalid_chain.unwind_point.number = *pipeline_.unwind_point(); + invalid_chain.unwind_point.hash = *canonical_chain_.get_hash(*pipeline_.unwind_point()); + if (pipeline_.bad_block()) { + invalid_chain.bad_block = pipeline_.bad_block(); + invalid_chain.bad_headers = collect_bad_headers(tx_, invalid_chain); + } + verify_result = invalid_chain; + break; + } + case Stage::Result::kStoppedByEnv: + verify_result = ValidChain{pipeline_.head_header_number(), pipeline_.head_header_hash()}; + break; + default: + verify_result = ValidationError{pipeline_.head_header_number(), pipeline_.head_header_hash()}; + } + canonical_head_status_ = verify_result; +} + +void MainChain::unwind(BlockNum unwind_point) { + const auto unwind_result = pipeline_.unwind(tx_, unwind_point); + success_or_throw(unwind_result); // unwind must complete with success + + // Remove last part of canonical chain + canonical_chain_.delete_down_to(unwind_point); } -*/ } // namespace silkworm::stagedsync diff --git a/silkworm/node/stagedsync/forks/main_chain.hpp b/silkworm/node/stagedsync/forks/main_chain.hpp index 531c7def41..0679ac06aa 100644 --- a/silkworm/node/stagedsync/forks/main_chain.hpp +++ b/silkworm/node/stagedsync/forks/main_chain.hpp @@ -54,17 +54,19 @@ class MainChain { void reintegrate_fork(ExtendingFork&); // reintegrate fork into the main chain std::optional find_forking_point(const BlockHeader& header, const Hash& header_hash) const; std::optional find_forking_point(const Hash& header_hash) const; - bool is_canonical(BlockId block) const; + bool is_finalized_canonical(BlockId block) const; // verification // verify chain up to head_block_hash VerificationResult verify_chain(Hash head_block_hash); // accept the current chain up to head_block_hash bool notify_fork_choice_update(Hash head_block_hash, std::optional finalized_block_hash = std::nullopt); + bool notify_fork_choice_update2(Hash head_block_hash, std::optional finalized_block_hash = std::nullopt); // state BlockId last_chosen_head() const; // set by notify_fork_choice_update(), is always valid BlockId last_finalized_head() const; + BlockId current_head() const; // header/body retrieval BlockNum get_block_progress() const; @@ -75,7 +77,7 @@ class MainChain { bool extends_last_fork_choice(BlockNum, Hash) const; bool extends(BlockId block, BlockId supposed_parent) const; bool is_ancestor(BlockId supposed_parent, BlockId block) const; - bool is_canonical(Hash) const; + bool is_finalized_canonical(Hash) const; // Warning: this getters use kHeaderNumbers so will return only header processed by the pipeline std::optional get_header(Hash) const; std::optional get_header_td(Hash) const; @@ -88,8 +90,11 @@ class MainChain { protected: Hash insert_header(const BlockHeader&); void insert_body(const Block&, const Hash& block_hash); + void forward(BlockNum head_height, const Hash& head_hash); + void unwind(BlockNum unwind_point); - BlockId current_head() const; // private state, it is implementation dependent, this head can be invalid + bool is_canonical(BlockNum block_height, const Hash& block_hash) const; + bool is_canonical_head_ancestor(const Hash& block_hash) const; std::set collect_bad_headers(db::RWTxn& tx, InvalidChain& invalid_chain); diff --git a/silkworm/node/stagedsync/forks/main_chain_test.cpp b/silkworm/node/stagedsync/forks/main_chain_test.cpp index 61751b2ca7..6fb0e1e82f 100644 --- a/silkworm/node/stagedsync/forks/main_chain_test.cpp +++ b/silkworm/node/stagedsync/forks/main_chain_test.cpp @@ -176,13 +176,13 @@ TEST_CASE("MainChain") { REQUIRE(!present_in_canonical); final_canonical_head = main_chain.current_head(); - REQUIRE(final_canonical_head == initial_canonical_head); - REQUIRE(main_chain.canonical_chain_.current_head() == initial_canonical_head); - REQUIRE(main_chain.last_chosen_head() == block0_id); // not changed + CHECK(final_canonical_head == block1_id); // still block1 even if invalid + CHECK(main_chain.canonical_chain_.current_head() == block1_id); // still block1 even if invalid + CHECK(main_chain.last_chosen_head() == block0_id); // not changed current_status = main_chain.canonical_head_status_; - REQUIRE(holds_alternative(current_status)); - REQUIRE(std::get(current_status).current_head == block0_id); + CHECK(holds_alternative(current_status)); + CHECK(std::get(current_status).unwind_point == block0_id); } SECTION("one valid body after the genesis") { @@ -274,23 +274,24 @@ TEST_CASE("MainChain") { extends_canonical = main_chain.extends_last_fork_choice(block3.header.number, block3.header.hash()); CHECK(extends_canonical); + // TODO(canepat) seems broken, fixme - START // reverting the chain simulating invalid block main_chain.canonical_head_status_ = InvalidChain{BlockId{1, block1_hash}}; updated = main_chain.notify_fork_choice_update(*header0_hash); CHECK(updated); // checking the status - present_in_canonical = main_chain.get_canonical_hash(block1.header.number); - REQUIRE(!present_in_canonical); + CHECK(main_chain.get_canonical_hash(block1.header.number)); // block1 still in canonical even if invalid final_canonical_head = main_chain.current_head(); - REQUIRE(final_canonical_head == initial_canonical_head); - REQUIRE(main_chain.canonical_chain_.current_head() == initial_canonical_head); - REQUIRE(main_chain.last_chosen_head() == block0_id); + CHECK(final_canonical_head == block1_id); + CHECK(main_chain.canonical_chain_.current_head() == block1_id); + // CHECK(main_chain.last_chosen_head() == block0_id); current_status = main_chain.canonical_head_status_; - REQUIRE(holds_alternative(current_status)); - REQUIRE(std::get(current_status).current_head == block0_id); + // CHECK(holds_alternative(current_status)); + // CHECK(std::get(current_status).unwind_point == block1_id); + // TODO(canepat) seems broken, fixme - END } SECTION("diverting the head") { From d4e0fc1b9e0e7fe96605c03813db869a698bce55 Mon Sep 17 00:00:00 2001 From: GitHub Date: Mon, 11 Sep 2023 07:48:12 +0000 Subject: [PATCH 2/8] make fmt --- silkworm/node/stagedsync/forks/main_chain_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/silkworm/node/stagedsync/forks/main_chain_test.cpp b/silkworm/node/stagedsync/forks/main_chain_test.cpp index 6fb0e1e82f..49c58d33a9 100644 --- a/silkworm/node/stagedsync/forks/main_chain_test.cpp +++ b/silkworm/node/stagedsync/forks/main_chain_test.cpp @@ -176,9 +176,9 @@ TEST_CASE("MainChain") { REQUIRE(!present_in_canonical); final_canonical_head = main_chain.current_head(); - CHECK(final_canonical_head == block1_id); // still block1 even if invalid + CHECK(final_canonical_head == block1_id); // still block1 even if invalid CHECK(main_chain.canonical_chain_.current_head() == block1_id); // still block1 even if invalid - CHECK(main_chain.last_chosen_head() == block0_id); // not changed + CHECK(main_chain.last_chosen_head() == block0_id); // not changed current_status = main_chain.canonical_head_status_; CHECK(holds_alternative(current_status)); From 01ec28a1dac1f77740b1e64ddb8c52123ca074c4 Mon Sep 17 00:00:00 2001 From: canepat <16927169+canepat@users.noreply.github.com> Date: Mon, 11 Sep 2023 11:44:58 +0200 Subject: [PATCH 3/8] add missing file --- silkworm/node/common/settings.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/silkworm/node/common/settings.hpp b/silkworm/node/common/settings.hpp index bfe9f9e152..78d3daaf30 100644 --- a/silkworm/node/common/settings.hpp +++ b/silkworm/node/common/settings.hpp @@ -51,6 +51,7 @@ struct NodeSettings { uint32_t sync_loop_throttle_seconds{0}; // Minimum interval amongst sync cycle uint32_t sync_loop_log_interval_seconds{30}; // Interval for sync loop to emit logs std::string node_name; // The node identifying name + bool parallel_fork_tracking_enabled{false}; // Whether to track multiple parallel forks at head }; } // namespace silkworm From d7726e9a02956049a418ab15e92c39966c262ec6 Mon Sep 17 00:00:00 2001 From: canepat <16927169+canepat@users.noreply.github.com> Date: Mon, 11 Sep 2023 11:48:51 +0200 Subject: [PATCH 4/8] remove unused code --- silkworm/node/stagedsync/forks/main_chain.cpp | 38 ------------------- silkworm/node/stagedsync/forks/main_chain.hpp | 1 - 2 files changed, 39 deletions(-) diff --git a/silkworm/node/stagedsync/forks/main_chain.cpp b/silkworm/node/stagedsync/forks/main_chain.cpp index 51907ca851..85682d1b02 100644 --- a/silkworm/node/stagedsync/forks/main_chain.cpp +++ b/silkworm/node/stagedsync/forks/main_chain.cpp @@ -300,44 +300,6 @@ bool MainChain::notify_fork_choice_update(Hash head_block_hash, std::optional finalized_block_hash) { - if (canonical_chain_.current_head().hash != head_block_hash) { - // usually update_fork_choice must follow verify_chain with the same header except when: - // 1) (PoS) CL is syncing so head_block_hash is referring to a previous valid head - // 2) (PoW) previous verify_chain returned InvalidChain so CL is issuing a fcu with a previous valid head - - if (not canonical_chain_.has(head_block_hash) or not std::holds_alternative(canonical_head_status_)) { - auto verification = verify_chain(head_block_hash); // this will reset canonical chain to head_block_hash - ensure_invariant(canonical_chain_.current_head().hash == head_block_hash, - "canonical head not aligned with fork choice"); - } - } - - if (!std::holds_alternative(canonical_head_status_)) return false; // head is not valid - - auto valid_chain = std::get(canonical_head_status_); - ensure_invariant(canonical_chain_.current_head() == valid_chain.current_head, - "canonical head not aligned with recorded head status"); - - if (finalized_block_hash && !canonical_chain_.has(*finalized_block_hash)) return false; // finalized block not found - // we need a way to disambiguate this "false" from the one above - - db::write_last_head_block(tx_, head_block_hash); - if (finalized_block_hash) db::write_last_finalized_block(tx_, *finalized_block_hash); - - tx_.commit_and_renew(); - - last_fork_choice_ = canonical_chain_.current_head(); - if (finalized_block_hash) { - auto finalized_header = get_header(*finalized_block_hash); - last_finalized_head_ = {finalized_header->number, *finalized_block_hash}; - } - - is_first_sync_ = false; - - return true; -} - std::set MainChain::collect_bad_headers(db::RWTxn& tx, InvalidChain& invalid_chain) { if (!invalid_chain.bad_block) return {}; diff --git a/silkworm/node/stagedsync/forks/main_chain.hpp b/silkworm/node/stagedsync/forks/main_chain.hpp index 0679ac06aa..e469d2598c 100644 --- a/silkworm/node/stagedsync/forks/main_chain.hpp +++ b/silkworm/node/stagedsync/forks/main_chain.hpp @@ -61,7 +61,6 @@ class MainChain { VerificationResult verify_chain(Hash head_block_hash); // accept the current chain up to head_block_hash bool notify_fork_choice_update(Hash head_block_hash, std::optional finalized_block_hash = std::nullopt); - bool notify_fork_choice_update2(Hash head_block_hash, std::optional finalized_block_hash = std::nullopt); // state BlockId last_chosen_head() const; // set by notify_fork_choice_update(), is always valid From fc969770e2117113362904a44f0b2117bbc2a136 Mon Sep 17 00:00:00 2001 From: canepat <16927169+canepat@users.noreply.github.com> Date: Mon, 11 Sep 2023 11:49:22 +0200 Subject: [PATCH 5/8] add log traces --- silkworm/node/stagedsync/server.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/silkworm/node/stagedsync/server.cpp b/silkworm/node/stagedsync/server.cpp index a7914dd1c4..2b274eebba 100644 --- a/silkworm/node/stagedsync/server.cpp +++ b/silkworm/node/stagedsync/server.cpp @@ -34,12 +34,14 @@ bool Server::stop() { } void Server::execution_loop() { + SILK_TRACE << "execution::Server::execution_loop enter"; exec_engine_.open(); asio::executor_work_guard work{io_context_.get_executor()}; io_context_.run(); exec_engine_.close(); + SILK_TRACE << "execution::Server::execution_loop exit"; } void Server::handle_exception(const std::exception_ptr& e) { From 2e2cf417b97d647b158535f04ed2cd99f4c14984 Mon Sep 17 00:00:00 2001 From: canepat <16927169+canepat@users.noreply.github.com> Date: Mon, 11 Sep 2023 11:49:22 +0200 Subject: [PATCH 6/8] add log traces --- silkworm/node/stagedsync/local_client.cpp | 1 + silkworm/node/stagedsync/server.cpp | 2 ++ 2 files changed, 3 insertions(+) diff --git a/silkworm/node/stagedsync/local_client.cpp b/silkworm/node/stagedsync/local_client.cpp index 249a3a1636..44f5e8f2fd 100644 --- a/silkworm/node/stagedsync/local_client.cpp +++ b/silkworm/node/stagedsync/local_client.cpp @@ -13,6 +13,7 @@ See the License for the specific language governing permissions and limitations under the License. */ + #include "local_client.hpp" #include diff --git a/silkworm/node/stagedsync/server.cpp b/silkworm/node/stagedsync/server.cpp index a7914dd1c4..2b274eebba 100644 --- a/silkworm/node/stagedsync/server.cpp +++ b/silkworm/node/stagedsync/server.cpp @@ -34,12 +34,14 @@ bool Server::stop() { } void Server::execution_loop() { + SILK_TRACE << "execution::Server::execution_loop enter"; exec_engine_.open(); asio::executor_work_guard work{io_context_.get_executor()}; io_context_.run(); exec_engine_.close(); + SILK_TRACE << "execution::Server::execution_loop exit"; } void Server::handle_exception(const std::exception_ptr& e) { From 5cd93627255028c13de12ea14dd83cd2f8f2e5c3 Mon Sep 17 00:00:00 2001 From: canepat <16927169+canepat@users.noreply.github.com> Date: Mon, 11 Sep 2023 11:54:03 +0200 Subject: [PATCH 7/8] improve log message --- silkworm/node/stagedsync/forks/main_chain.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/silkworm/node/stagedsync/forks/main_chain.cpp b/silkworm/node/stagedsync/forks/main_chain.cpp index 85682d1b02..a0e34f30eb 100644 --- a/silkworm/node/stagedsync/forks/main_chain.cpp +++ b/silkworm/node/stagedsync/forks/main_chain.cpp @@ -66,7 +66,7 @@ void MainChain::open() { // - if last cycle completed successfully, this will simply do nothing (no hurt) // - if last cycle was executed partially (i.e. not all stages are at the same height), this will do a cleanup cycle const auto& canonical_head{canonical_chain_.current_head()}; - SILK_INFO << "Revalidate chain up to the canonical current head number=" << canonical_head.number << " hash=" << to_hex(canonical_head.hash); + SILK_INFO << "Revalidate canonical chain up to number=" << canonical_head.number << " hash=" << to_hex(canonical_head.hash); forward(canonical_head.number, canonical_head.hash); From 6a5f4214e26c4e13816371305b56d5a89a557a06 Mon Sep 17 00:00:00 2001 From: canepat <16927169+canepat@users.noreply.github.com> Date: Mon, 11 Sep 2023 18:24:25 +0200 Subject: [PATCH 8/8] node: avoid committing valid chains during first cycle in chain verification node: refactor staged sync main chain node: avoid committing changes for invalid block in Execution stage node: fix main chain unit test --- silkworm/node/stagedsync/execution_engine.cpp | 6 +-- .../node/stagedsync/execution_pipeline.cpp | 2 +- silkworm/node/stagedsync/forks/main_chain.cpp | 26 ++++-------- silkworm/node/stagedsync/forks/main_chain.hpp | 2 +- .../node/stagedsync/forks/main_chain_test.cpp | 42 ++++++++----------- silkworm/node/stagedsync/stages/stage.hpp | 13 +++--- .../stagedsync/stages/stage_execution.cpp | 9 +--- 7 files changed, 36 insertions(+), 64 deletions(-) diff --git a/silkworm/node/stagedsync/execution_engine.cpp b/silkworm/node/stagedsync/execution_engine.cpp index 62061e65e4..11974827f9 100644 --- a/silkworm/node/stagedsync/execution_engine.cpp +++ b/silkworm/node/stagedsync/execution_engine.cpp @@ -272,17 +272,17 @@ std::optional ExecutionEngine::get_body(Hash header_hash) const { } std::optional ExecutionEngine::get_canonical_header(BlockNum bn) const { - auto hash = main_chain_.get_canonical_hash(bn); + auto hash = main_chain_.get_finalized_canonical_hash(bn); if (!hash) return {}; return main_chain_.get_header(*hash); } std::optional ExecutionEngine::get_canonical_hash(BlockNum bn) const { - return main_chain_.get_canonical_hash(bn); + return main_chain_.get_finalized_canonical_hash(bn); } std::optional ExecutionEngine::get_canonical_body(BlockNum bn) const { - auto hash = main_chain_.get_canonical_hash(bn); + auto hash = main_chain_.get_finalized_canonical_hash(bn); if (!hash) return {}; return main_chain_.get_body(*hash); } diff --git a/silkworm/node/stagedsync/execution_pipeline.cpp b/silkworm/node/stagedsync/execution_pipeline.cpp index 2ac461efda..4d706385dd 100644 --- a/silkworm/node/stagedsync/execution_pipeline.cpp +++ b/silkworm/node/stagedsync/execution_pipeline.cpp @@ -151,7 +151,7 @@ void ExecutionPipeline::load_stages() { db::stages::kTxLookupKey, db::stages::kLogIndexKey, db::stages::kHistoryIndexKey, - db::stages::kHashStateKey, // Needs to happen before unwinding Execution + db::stages::kHashStateKey, db::stages::kIntermediateHashesKey, // Needs to happen after unwinding HashState db::stages::kExecutionKey, db::stages::kSendersKey, diff --git a/silkworm/node/stagedsync/forks/main_chain.cpp b/silkworm/node/stagedsync/forks/main_chain.cpp index a0e34f30eb..caec22a270 100644 --- a/silkworm/node/stagedsync/forks/main_chain.cpp +++ b/silkworm/node/stagedsync/forks/main_chain.cpp @@ -70,7 +70,7 @@ void MainChain::open() { forward(canonical_head.number, canonical_head.hash); - // If forward cleanup cycle has not produced a valida chain, then we need to unwind + // If forward cleanup cycle has not produced a valid chain, then we need to unwind if (not std::holds_alternative(canonical_head_status_)) { const auto unwind_point{pipeline_.unwind_point()}; ensure_invariant(unwind_point.has_value(), "unwind point from pipeline requested when forward fails"); @@ -163,7 +163,7 @@ VerificationResult MainChain::verify_chain(Hash block_hash) { if (is_canonical(block_header->number, block_hash)) { // The incoming block matches a block already on the canonical chain, verification is not always needed if (block_header->number <= last_fork_choice_.number) { - // Last FCU block is greater than or equal incoming canonical block, chain is valid up to last FCU block + // Last FCU block is greater than or equal to incoming canonical block, chain is valid up to last FCU block return ValidChain{last_fork_choice_.number, last_fork_choice_.hash}; } else if (std::holds_alternative(canonical_head_status_)) { // Chain is valid up to canonical head @@ -184,17 +184,15 @@ VerificationResult MainChain::verify_chain(Hash block_hash) { // db commit policy bool commit_at_each_stage = is_first_sync_; if (!commit_at_each_stage) tx_.disable_commit(); + auto _ = gsl::finally([&]() { tx_.enable_commit(); }); // the new head is on a new fork? BlockId forking_point = canonical_chain_.find_forking_point(*block_header, block_hash); // the forking origin if (block_hash != canonical_chain_.current_head().hash && // if the new head is not the current head forking_point.number < canonical_chain_.current_head().number) { // and if the forking is behind the head - // we need to do unwind to change canonical - auto unwind_result = pipeline_.unwind(tx_, forking_point.number); - success_or_throw(unwind_result); // unwind must complete with success - // remove last part of canonical - canonical_chain_.delete_down_to(forking_point.number); + // We need to do unwind to change canonical + unwind(forking_point.number); } // update canonical up to header_hash @@ -202,7 +200,7 @@ VerificationResult MainChain::verify_chain(Hash block_hash) { // forward Stage::Result forward_result = pipeline_.forward(tx_, block_header->number); - SILK_INFO << "MainChain::verify_chain commit forward_result=" << magic_enum::enum_name<>(forward_result); + SILK_INFO << "MainChain::verify_chain forward_result=" << magic_enum::enum_name<>(forward_result); // evaluate result VerificationResult verify_result; @@ -212,7 +210,6 @@ VerificationResult MainChain::verify_chain(Hash block_hash) { pipeline_.head_header_hash() == canonical_chain_.current_head().hash, "forward succeeded with pipeline head not aligned with canonical head"); verify_result = ValidChain{pipeline_.head_header_number(), pipeline_.head_header_hash()}; - SILK_INFO << "MainChain::verify_chain commit verify_result index=" << verify_result.index(); break; } case Stage::Result::kWrongFork: @@ -238,15 +235,6 @@ VerificationResult MainChain::verify_chain(Hash block_hash) { } canonical_head_status_ = verify_result; - // finish - tx_.enable_commit(); - if (commit_at_each_stage and std::holds_alternative(verify_result)) { - StopWatch timing{StopWatch::kStart}; - SILK_INFO << "MainChain::verify_chain commit BEFORE"; - tx_.commit_and_renew(); - SILK_INFO << "MainChain::verify_chain commit at hash=" << block_hash.to_hex() - << " took " << StopWatch::format(timing.since_start()); - } return verify_result; } @@ -377,7 +365,7 @@ std::optional MainChain::get_header(BlockNum header_height, Hash he return header; } -std::optional MainChain::get_canonical_hash(BlockNum height) const { +std::optional MainChain::get_finalized_canonical_hash(BlockNum height) const { if (height > last_fork_choice_.number) return {}; return canonical_chain_.get_hash(height); } diff --git a/silkworm/node/stagedsync/forks/main_chain.hpp b/silkworm/node/stagedsync/forks/main_chain.hpp index e469d2598c..dea25a25ad 100644 --- a/silkworm/node/stagedsync/forks/main_chain.hpp +++ b/silkworm/node/stagedsync/forks/main_chain.hpp @@ -70,7 +70,7 @@ class MainChain { // header/body retrieval BlockNum get_block_progress() const; std::optional get_header(BlockNum, Hash) const; - std::optional get_canonical_hash(BlockNum) const; + std::optional get_finalized_canonical_hash(BlockNum) const; std::optional get_header_td(BlockNum, Hash) const; std::vector get_last_headers(uint64_t limit) const; bool extends_last_fork_choice(BlockNum, Hash) const; diff --git a/silkworm/node/stagedsync/forks/main_chain_test.cpp b/silkworm/node/stagedsync/forks/main_chain_test.cpp index 49c58d33a9..5a28989302 100644 --- a/silkworm/node/stagedsync/forks/main_chain_test.cpp +++ b/silkworm/node/stagedsync/forks/main_chain_test.cpp @@ -164,7 +164,7 @@ TEST_CASE("MainChain") { REQUIRE(holds_alternative(current_status)); // check canonical - auto present_in_canonical = main_chain.get_canonical_hash(block1.header.number); + auto present_in_canonical = main_chain.get_finalized_canonical_hash(block1.header.number); REQUIRE(!present_in_canonical); // reverting the chain @@ -172,13 +172,12 @@ TEST_CASE("MainChain") { CHECK(updated); // checking the status - present_in_canonical = main_chain.get_canonical_hash(block1.header.number); + present_in_canonical = main_chain.get_finalized_canonical_hash(block1.header.number); REQUIRE(!present_in_canonical); final_canonical_head = main_chain.current_head(); - CHECK(final_canonical_head == block1_id); // still block1 even if invalid - CHECK(main_chain.canonical_chain_.current_head() == block1_id); // still block1 even if invalid - CHECK(main_chain.last_chosen_head() == block0_id); // not changed + CHECK(final_canonical_head == block1_id); // still block1 even if invalid + CHECK(main_chain.last_chosen_head() == block0_id); // not changed current_status = main_chain.canonical_head_status_; CHECK(holds_alternative(current_status)); @@ -235,7 +234,7 @@ TEST_CASE("MainChain") { bool present = db::read_body(tx, block1_hash, block1.header.number, saved_body); REQUIRE(present); - auto present_in_canonical = main_chain.get_canonical_hash(block1.header.number); + auto present_in_canonical = main_chain.get_finalized_canonical_hash(block1.header.number); REQUIRE(!present_in_canonical); // not yet // confirming the chain @@ -243,12 +242,11 @@ TEST_CASE("MainChain") { CHECK(updated); // checking the status - present_in_canonical = main_chain.get_canonical_hash(block1.header.number); + present_in_canonical = main_chain.get_finalized_canonical_hash(block1.header.number); REQUIRE(present_in_canonical); final_canonical_head = main_chain.current_head(); REQUIRE(final_canonical_head == block1_id); - REQUIRE(main_chain.canonical_chain_.current_head() == block1_id); REQUIRE(main_chain.last_chosen_head() == block1_id); // testing other methods @@ -274,24 +272,20 @@ TEST_CASE("MainChain") { extends_canonical = main_chain.extends_last_fork_choice(block3.header.number, block3.header.hash()); CHECK(extends_canonical); - // TODO(canepat) seems broken, fixme - START - // reverting the chain simulating invalid block - main_chain.canonical_head_status_ = InvalidChain{BlockId{1, block1_hash}}; - updated = main_chain.notify_fork_choice_update(*header0_hash); - CHECK(updated); - - // checking the status - CHECK(main_chain.get_canonical_hash(block1.header.number)); // block1 still in canonical even if invalid + // Testing a mini re-org + Block new_block1 = generateSampleChildrenBlock(*header0); + const auto new_block1_hash{new_block1.header.hash()}; + BlockId new_block1_id{1, new_block1_hash}; - final_canonical_head = main_chain.current_head(); - CHECK(final_canonical_head == block1_id); - CHECK(main_chain.canonical_chain_.current_head() == block1_id); - // CHECK(main_chain.last_chosen_head() == block0_id); + // inserting & verifying the block + main_chain.insert_block(new_block1); + const auto new_verification1 = main_chain.verify_chain(new_block1_hash); + CHECK(holds_alternative(new_verification1)); + CHECK(std::get(new_verification1).current_head == new_block1_id); - current_status = main_chain.canonical_head_status_; - // CHECK(holds_alternative(current_status)); - // CHECK(std::get(current_status).unwind_point == block1_id); - // TODO(canepat) seems broken, fixme - END + // confirming the chain + const auto new_block1_updated = main_chain.notify_fork_choice_update(new_block1_hash); + CHECK(new_block1_updated); } SECTION("diverting the head") { diff --git a/silkworm/node/stagedsync/stages/stage.hpp b/silkworm/node/stagedsync/stages/stage.hpp index af9471cb83..bb418c93fa 100644 --- a/silkworm/node/stagedsync/stages/stage.hpp +++ b/silkworm/node/stagedsync/stages/stage.hpp @@ -54,23 +54,20 @@ struct SyncContext { class Stage : public Stoppable { public: enum class [[nodiscard]] Result{ - kSuccess, // + kSuccess, // valid chain kUnknownChainId, // kUnknownProtocolRuleSet, // - kBadBlockHash, // kBadChainSequence, // - kInvalidRange, // kInvalidProgress, // - kInvalidBlock, // + kInvalidBlock, // invalid chain kInvalidTransaction, // kDecodingError, // - kWrongFork, // The persisted canonical chain must be changed - kWrongStateRoot, // + kWrongFork, // invalid chain: the persisted canonical chain must be changed + kWrongStateRoot, // invalid chain kUnexpectedError, // - kUnknownError, // kDbError, // kAborted, // - kStoppedByEnv, // Encountered "STOP_BEFORE_STAGE" env var + kStoppedByEnv, // valid chain: encountered "STOP_BEFORE_STAGE" env var kUnspecified, }; diff --git a/silkworm/node/stagedsync/stages/stage_execution.cpp b/silkworm/node/stagedsync/stages/stage_execution.cpp index ec52713259..9f1bcfea98 100644 --- a/silkworm/node/stagedsync/stages/stage_execution.cpp +++ b/silkworm/node/stagedsync/stages/stage_execution.cpp @@ -105,9 +105,7 @@ Stage::Result Execution::forward(db::RWTxn& txn) { prune_receipts)}; // If we return with success we must persist data - // Though counterintuitive we also must persist on KInvalidBlock to allow subsequent unwind - if (execution_result != Stage::Result::kSuccess && - execution_result != Stage::Result::kInvalidBlock) { + if (execution_result != Stage::Result::kSuccess) { throw StageError(execution_result); } @@ -122,11 +120,6 @@ Stage::Result Execution::forward(db::RWTxn& txn) { auto [_, duration]{commit_stopwatch.stop()}; log::Info(log_prefix_ + " commit", {"batch time", StopWatch::format(duration)}); - // If an invalid block returned now can throw - if (execution_result == Stage::Result::kInvalidBlock) { - ret = execution_result; - break; - } block_num_++; }