Refactor: block tree and fork choice #51

xDimon · 2025-12-26T09:29:59Z

Take to use block tree and block storage.
Extract block+state collection into block tree/storage
Implement storing of block states.
Fix logging system tuning over cli.
Some minor fixes and improvements.

remaining:
to fix fork-choice test
to refactor outcome handling (avoid exception at getting outcome-value without checks) and other ways to have exceptions.

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

src/types/block.hpp

src/types/block_index.hpp

Copilot

Pull request overview

This pull request refactors the block tree and fork choice implementation, extracting block and state collections into dedicated storage, implementing state persistence, and improving the CLI logging configuration system.

Key Changes

Refactored block tree architecture by removing FCBlockTree and enhancing BlockTreeImpl with integrated fork choice
Introduced state storage alongside blocks, replacing justification-based finalization with state-based approach
Added LRU caching for blockchain states to improve performance
Enhanced CLI logging system with improved argument parsing and validation

Reviewed changes

Copilot reviewed 57 out of 58 changed files in this pull request and generated 25 comments.

Show a summary per file

File	Description
src/blockchain/fork_choice.cpp	Major refactoring to use BlockTree/BlockStorage instead of internal maps; added state caching
src/blockchain/impl/block_tree_impl.cpp	Updated finalize method to remove justification parameter; renamed getNumberByHash to getSlotByHash
src/blockchain/impl/block_storage_impl.cpp	Replaced justification storage methods with state storage methods
src/blockchain/impl/anchor_block_impl.cpp	New implementation for anchor block based on anchor state
src/blockchain/impl/anchor_state_impl.cpp	New implementation for anchor state from genesis config
src/utils/lru_cache.hpp	New LRU cache implementation for performance optimization
src/log/logger.cpp	Enhanced logging configuration with better CLI argument parsing
tests/unit/blockchain/fork_choice_test.cpp	Tests need updates for new constructor signatures
src/types/justification.hpp	File deleted - justification concept replaced with state storage
src/blockchain/impl/fc_block_tree.cpp	File deleted - functionality merged into BlockTreeImpl
example/0-single/genesis/config.yaml	Updated genesis configuration with validator settings
CMakeLists.txt	Added Boost.DI configuration macros

Comments suppressed due to low confidence (1)

tests/unit/blockchain/fork_choice_test.cpp:132

The makeBlockMap function returns a ForkChoiceStore::Blocks type which has been commented out in the fork_choice.hpp (lines 64-65). This will cause a compilation error. The function and its usages need to be refactored to work with the new block tree-based storage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/blockchain/impl/block_storage_initializer.cpp

src/utils/lru_cache.hpp

CMakeLists.txt

src/types/config.hpp

Copilot · 2025-12-29T05:05:25Z

src/blockchain/fork_choice.cpp

+        // latest_finalized_(latest_finalized),
+        // blocks_(std::move(blocks)),
+        // states_(std::move(states)),


Commented-out code and parameters should be removed. This reduces code clarity and makes the constructor signature harder to understand.

tests/unit/blockchain/state_transition_function_test.cpp

src/blockchain/fork_choice.cpp

src/blockchain/block_storage.hpp

src/blockchain/fork_choice.cpp

src/blockchain/impl/block_storage_initializer.cpp

CMakeLists.txt

src/blockchain/fork_choice.hpp

src/blockchain/impl/genesis_block_header_impl.cpp

src/log/logger.hpp

src/storage/spaces.hpp

src/blockchain/fork_choice.cpp

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

Copilot

Pull request overview

Copilot reviewed 49 out of 50 changed files in this pull request and generated 23 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-13T09:17:31Z

tests/unit/blockchain/fork_choice_test.cpp

  return ForkChoiceStore(time,
                         testutil::prepareLoggers(),
                         std::make_shared<lean::metrics::MetricsMock>(),
                         config_param,
                         head,
                         safe_target,
                         latest_justified,
-                         latest_finalized,
-                         blocks,
-                         states,
-                         latest_known_attestations,
-                         latest_new_attestations,
+                         // latest_finalized,
+                         std::move(latest_known_attestations),
+                         std::move(latest_new_attestations),
                         validator_index,
                         validator_registry,
                         validator_keys_manifest,
-                         xmss_provider);
+                         xmss_provider,
+                         block_tree,
+                         block_storage);


The parameter latest_finalized is removed from the function signature but wasn't used in the function body. However, this appears to be an issue because the ForkChoiceStore now relies on BlockTree to track the last finalized block. The test constructor should ensure that the BlockTree mock is properly initialized with the finalized block information, otherwise tests may fail when the fork choice store tries to access block_tree_->lastFinalized().

src/types/state.hpp

src/types/block.hpp

Copilot · 2026-01-13T09:17:32Z

src/utils/lru_cache.hpp

+      if constexpr (std::is_same_v<ValueArg, Value>) {
+        auto it = std::ranges::find_if(
+            cache_.begin(), cache_.end(), [&](const auto &item) {
+              return *item.value == value;
+            });
+        if (it != cache_.end()) {
+          if (cache_.size() >= kMaxSize) {
+            auto min = std::min_element(cache_.begin(), cache_.end());
+            cache_.erase(min);
+          }
+          auto &entry = cache_.emplace_back(CacheEntry{key, it->value, ticks_});
+          return entry.value;
+        }
+        auto &entry = cache_.emplace_back(
+            CacheEntry{key,
+                       std::make_shared<Value>(std::forward<ValueArg>(value)),
+                       ticks_});
+        return entry.value;
+
+      } else {
+        auto value_sptr =
+            std::make_shared<Value>(std::forward<ValueArg>(value));
+        auto it = std::ranges::find_if(
+            cache_.begin(), cache_.end(), [&](const auto &item) {
+              return *item.value == *value_sptr;
+            });
+        if (it != cache_.end()) {
+          if (cache_.size() >= kMaxSize) {
+            auto min = std::min_element(cache_.begin(), cache_.end());
+            cache_.erase(min);
+          }
+          auto &entry = cache_.emplace_back(CacheEntry{key, it->value, ticks_});
+          return entry.value;
+        }
+        auto &entry =
+            cache_.emplace_back(CacheEntry{key, std::move(value_sptr), ticks_});
+        return entry.value;
+      }


The logic in the put method has a potential issue. When an existing value is found in the cache (lines 119 or 140), the code checks if cache_.size() >= kMaxSize and then erases the minimum element before emplacing a new entry. This can lead to the cache exceeding its maximum size.

The issue is that when a matching value is found, the code adds a new entry with the same value but a different key without removing the old entry first. This means you could end up with duplicate values in the cache under different keys, and the cache size could grow beyond kMaxSize.

The logic should either:

Update the existing entry's key and tick instead of creating a duplicate, or

Remove the old entry before adding the new one, or

Only erase when size equals kMaxSize (not >=) since we're about to add one entry

Copilot · 2026-01-13T09:17:32Z

src/modules/production/read_config_yaml.hpp

+    auto yaml_genesis_validators = yaml["VALIDATORS"];
    if (not yaml_genesis_validators.IsSequence()) {
-      return ConfigYamlError::INVALID;
+      yaml_genesis_validators = yaml["GENESIS_VALIDATORS"];
+      if (not yaml_genesis_validators.IsSequence()) {
+        return ConfigYamlError::INVALID;
+      }
    }


The YAML config key has been changed from "GENESIS_VALIDATORS" to "VALIDATORS" with a fallback to the old key name. While this maintains backward compatibility, the inconsistency in naming could be confusing. Consider documenting this change or standardizing on one key name. The fallback logic is good for migration, but it's unclear if "VALIDATORS" is the intended new standard or if both should be supported long-term.

Copilot · 2026-01-13T09:17:36Z

src/blockchain/fork_choice.cpp

+    auto source_block_slot = getBlockSlot(data.source.root);
+    auto target_block_slot = getBlockSlot(data.target.root);
+    if (source_block_slot != data.source.slot) {
      return Error::INVALID_ATTESTATION;
    }
-    if (target_block.slot != data.target.slot) {
+    if (target_block_slot != data.target.slot) {
      return Error::INVALID_ATTESTATION;
    }


The validateAttestationData method uses direct comparisons with optional values at lines 318 and 321 without proper unwrapping. The code compares source_block_slot != data.source.slot, but source_block_slot is of type std::optional<Slot> (returned by getBlockSlot), not Slot. This comparison will always use the optional's comparison operators which may not produce the intended result. The optional should be unwrapped first with .value() or checked for has_value() before comparison.

Copilot · 2026-01-13T09:17:36Z

src/blockchain/fork_choice.cpp

+                 current_slot);
+
+        auto res = updateSafeTarget();
+        BOOST_ASSERT(res.has_value());


The assertion BOOST_ASSERT(res.has_value()) at line 760 should be replaced with proper error handling. If updateSafeTarget() returns an error, the assertion will fail in debug mode but cause undefined behavior in release builds. Since this is in production code path (handling slot intervals), errors should be handled gracefully rather than relying on assertions.

Suggested change

BOOST_ASSERT(res.has_value());

if (not res.has_value()) {

SL_ERROR(logger_,

"Failed to update safe target for slot {}: {}",

current_slot,

res.error());

}

Copilot · 2026-01-13T09:17:37Z

src/blockchain/fork_choice.cpp


-    auto validator_count = getState(head_).validatorCount();
+    auto head_state_res = getState(head_.root);
+    BOOST_ASSERT(head_state_res.has_value());


The code uses BOOST_ASSERT at line 651 to verify that getting the head state succeeds. In a production environment, this could fail due to database errors or other issues. The assertion will only catch this in debug builds; in release builds it will cause undefined behavior. This should use proper error handling with early return or error propagation.

Suggested change

BOOST_ASSERT(head_state_res.has_value());

if (!head_state_res.has_value()) {

SL_ERROR(logger_,

"Failed to get head state for root {}: {}",

head_.root,

head_state_res.error());

return {};

}

Copilot · 2026-01-13T09:17:37Z

src/blockchain/block_storage.hpp


-    // -- justification --
+    // -- state --
+    // TODO: refactoring is needed - make special separated storage for states


The comment at line 173 says "TODO: refactoring is needed - make special separated storage for states". This indicates that the current implementation of storing states in the same BlockStorage as blocks is considered temporary. This technical debt should be tracked and addressed, especially since state storage has different access patterns and retention requirements than block storage.

Suggested change

// TODO: refactoring is needed - make special separated storage for states

// Note: state is currently accessed via BlockStorage; if a dedicated state

// storage is introduced in the future, this interface should be updated

// to reflect that separation.

Copilot · 2026-01-13T09:17:37Z

src/utils/lru_cache.hpp

+/**
+ * Copyright Quadrivium LLC
+ * All Rights Reserved
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#pragma once
+
+#include <algorithm>
+#include <cstdint>
+#include <functional>
+#include <memory>
+#include <optional>
+#include <vector>
+
+#include <boost/assert.hpp>
+#include <qtils/outcome.hpp>
+
+#include "utils/retain_if.hpp"
+
+namespace lean {
+
+  template <template <bool> class Lockable, bool IsLockable>
+  struct LockGuard {
+    inline LockGuard(const Lockable<IsLockable> &) {}
+  };
+
+  template <template <bool> class Lockable>
+  struct LockGuard<Lockable, true> {
+    LockGuard(const Lockable<true> &protected_object)
+        : protected_object_(protected_object) {
+      protected_object_.lock();
+    }
+    ~LockGuard() {
+      protected_object_.unlock();
+    }
+
+   private:
+    const Lockable<true> &protected_object_;
+  };
+
+  template <bool IsLockable>
+  class Lockable {
+   protected:
+    friend struct LockGuard<Lockable, IsLockable>;
+    inline void lock() const {}
+    inline void unlock() const {}
+  };
+
+  template <>
+  class Lockable<true> {
+   protected:
+    friend struct LockGuard<Lockable, true>;
+    inline void lock() const {
+      mutex_.lock();
+    }
+    inline void unlock() const {
+      mutex_.unlock();
+    }
+    mutable std::mutex mutex_;
+  };
+
+  /**
+   * LRU cache designed for small amounts of data (as its get() is O(N))
+   */
+  template <typename Key,
+            typename Value,
+            bool ThreadSafe = false,
+            typename PriorityType = uint64_t>
+  struct SmallLruCache final : public Lockable<ThreadSafe> {
+   public:
+    static_assert(std::is_unsigned_v<PriorityType>);
+
+    using Mutex = Lockable<ThreadSafe>;
+
+    struct CacheEntry {
+      Key key;
+      std::shared_ptr<Value> value;
+      PriorityType latest_use_tick_;
+
+      bool operator<(const CacheEntry &rhs) const {
+        return latest_use_tick_ < rhs.latest_use_tick_;
+      }
+    };
+
+    SmallLruCache(size_t max_size) : kMaxSize{max_size} {
+      BOOST_ASSERT(kMaxSize > 0);
+      cache_.reserve(kMaxSize);
+    }
+
+    std::optional<std::shared_ptr<const Value>> get(const Key &key) {
+      LockGuard lg(*this);
+      if (++ticks_ == 0) {
+        handleTicksOverflow();
+      }
+      for (auto &entry : cache_) {
+        if (entry.key == key) {
+          entry.latest_use_tick_ = ticks_;
+          return entry.value;
+        }
+      }
+      return std::nullopt;
+    }
+
+    template <typename ValueArg>
+    std::shared_ptr<const Value> put(const Key &key, ValueArg &&value) {
+      LockGuard lg(*this);
+      static_assert(std::is_convertible_v<ValueArg, Value>
+                    || std::is_constructible_v<ValueArg, Value>);
+      if (++ticks_ == 0) {
+        handleTicksOverflow();
+      }
+
+      if constexpr (std::is_same_v<ValueArg, Value>) {
+        auto it = std::ranges::find_if(
+            cache_.begin(), cache_.end(), [&](const auto &item) {
+              return *item.value == value;
+            });
+        if (it != cache_.end()) {
+          if (cache_.size() >= kMaxSize) {
+            auto min = std::min_element(cache_.begin(), cache_.end());
+            cache_.erase(min);
+          }
+          auto &entry = cache_.emplace_back(CacheEntry{key, it->value, ticks_});
+          return entry.value;
+        }
+        auto &entry = cache_.emplace_back(
+            CacheEntry{key,
+                       std::make_shared<Value>(std::forward<ValueArg>(value)),
+                       ticks_});
+        return entry.value;
+
+      } else {
+        auto value_sptr =
+            std::make_shared<Value>(std::forward<ValueArg>(value));
+        auto it = std::ranges::find_if(
+            cache_.begin(), cache_.end(), [&](const auto &item) {
+              return *item.value == *value_sptr;
+            });
+        if (it != cache_.end()) {
+          if (cache_.size() >= kMaxSize) {
+            auto min = std::min_element(cache_.begin(), cache_.end());
+            cache_.erase(min);
+          }
+          auto &entry = cache_.emplace_back(CacheEntry{key, it->value, ticks_});
+          return entry.value;
+        }
+        auto &entry =
+            cache_.emplace_back(CacheEntry{key, std::move(value_sptr), ticks_});
+        return entry.value;
+      }
+    }
+
+    outcome::result<std::shared_ptr<const Value>> get_else(
+        const Key &key, const std::function<outcome::result<Value>()> &func) {
+      if (auto opt = get(key); opt.has_value()) {
+        return opt.value();
+      }
+      auto res = func();
+      if (res.has_value()) {
+        return put(key, std::move(res.value()));
+      }
+      return res.as_failure();
+    }
+
+    void erase(const Key &key) {
+      LockGuard lg(*this);
+      auto it = std::ranges::find_if(
+          cache_.begin(), cache_.end(), [&](const auto &item) {
+            return item.key == key;
+          });
+      if (it != cache_.end()) {
+        cache_.erase(it);
+      }
+    }
+
+    void erase_if(const std::function<bool(const Key &key, const Value &value)>
+                      &predicate) {
+      LockGuard lg(*this);
+      retain_if(cache_, [&](const CacheEntry &item) {
+        return not predicate(item.key, *item.value);
+      });
+    }
+
+   private:
+    void handleTicksOverflow() {
+      // 'compress' timestamps of entries in the cache (works because we care
+      // only about their order, not actual timestamps)
+      std::sort(cache_.begin(), cache_.end());
+      for (auto &entry : cache_) {
+        entry.latest_use_tick_ = ticks_;
+        ticks_++;
+      }
+    }
+
+    const size_t kMaxSize;
+    // an abstract representation of time to implement 'recency' without
+    // depending on real time. Incremented on each cache access
+    PriorityType ticks_{};
+    std::vector<CacheEntry> cache_;
+  };
+
+  template <typename Key,
+            typename Value,
+            bool ThreadSafe = true,
+            typename PriorityType = uint64_t>
+  using LruCache = SmallLruCache<Key, Value, ThreadSafe, PriorityType>;
+
+}  // namespace lean


The new LruCache class in src/utils/lru_cache.hpp lacks test coverage. This is a critical utility class with complex logic for cache management, thread safety options, and tick overflow handling. Given that it's now used in the ForkChoiceStore for state caching, it should have comprehensive unit tests covering:

Basic put/get operations

Cache eviction when full

LRU ordering behavior

Thread safety (if ThreadSafe=true)

Tick overflow handling

Edge cases like cache size of 1

Consider adding dedicated tests for this class before it's used in production.

xDimon added 5 commits December 21, 2025 10:37

feature: anchor block and state

9a44c3b

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

refactor: changes generating some files

48705c1

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

draft

2597140

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

draft

ab14d7a

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

refactor: log config over cli

64113fc

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

xDimon requested review from kamilsa and turuslan December 26, 2025 09:29

turuslan approved these changes Dec 26, 2025

View reviewed changes

src/types/block.hpp Show resolved Hide resolved

src/types/block_index.hpp Outdated Show resolved Hide resolved

kamilsa requested a review from Copilot December 29, 2025 04:58

Copilot started reviewing on behalf of kamilsa December 29, 2025 04:58 View session

Copilot AI reviewed Dec 29, 2025

View reviewed changes

kamilsa reviewed Dec 29, 2025

View reviewed changes

xDimon added 3 commits December 29, 2025 11:37

fix: review issues

06cfe1a

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

draft

adfb692

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

draft

8b665f2

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

turuslan changed the title ~~Refactor: block tree and forck choice~~ Refactor: block tree and fork choice Dec 31, 2025

xDimon added 2 commits January 13, 2026 16:25

git: Merge branch 'master' into refactor/block_tree_and_forck_choice

c935b3e

fix: test

186a4a6

xDimon requested a review from kamilsa January 13, 2026 08:58

kamilsa requested a review from Copilot January 13, 2026 09:09

Copilot started reviewing on behalf of kamilsa January 13, 2026 09:09 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

xDimon added 2 commits January 13, 2026 19:16

more logs

193479d

fix: receiving and import blocks with correct

16597ea

-        BOOST_ASSERT(res.has_value());
+        if (not res.has_value()) {
+          SL_ERROR(logger_,
+                   "Failed to update safe target for slot {}: {}",
+                   current_slot,
+                   res.error());
+        }

-    BOOST_ASSERT(head_state_res.has_value());
+    if (!head_state_res.has_value()) {
+      SL_ERROR(logger_,
+               "Failed to get head state for root {}: {}",
+               head_.root,
+               head_state_res.error());
+      return {};
+    }

-    // TODO: refactoring is needed - make special separated storage for states
+    // Note: state is currently accessed via BlockStorage; if a dedicated state
+    // storage is introduced in the future, this interface should be updated
+    // to reflect that separation.

Refactor: block tree and fork choice #51

Are you sure you want to change the base?

Refactor: block tree and fork choice #51

Uh oh!

Conversation

xDimon commented Dec 26, 2025

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants