3SF & Fork-choice #24

kamilsa · 2025-09-26T06:20:12Z

This pull request introduces a new fork choice rule implementation and integrates foundational infrastructure for fork choice logic in the blockchain module. The main changes include the addition of the ForkChoiceStore and related APIs, a new FCBlockTree class, and updates to the state transition function. There are also improvements to the timeline logic and node initialization to support the new fork choice mechanism.

Known issues

No persistency
Genesis time is set as next multiple of 12 seconds

Signed-off-by: turuslan <[email protected]>

…figurations

…nt FCBlockTree class for fork choice logic

…t retrieval; refactor BlockBody to use variable size containers

Copilot

Pull Request Overview

This PR implements a fork choice rule mechanism and integrates foundational infrastructure for fork choice logic in the blockchain module. The changes introduce the ForkChoiceStore class with vote processing, block production, and head selection capabilities, along with a new FCBlockTree implementation and updates to support interval-based timeline execution.

Key changes:

Implements ForkChoiceStore with comprehensive fork choice algorithms including vote processing, safe target computation, and block production
Adds FCBlockTree as a fork choice-aware block tree implementation
Updates timeline logic to support interval-based execution with slot subdivision
Integrates fork choice store into production and networking modules

Reviewed Changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/blockchain/fork_choice.hpp/cpp	Core fork choice store implementation with vote processing and block production
src/blockchain/impl/fc_block_tree.hpp/cpp	Fork choice-aware block tree implementation
src/modules/production/production.cpp	Integration of fork choice for block and vote production
src/modules/networking/networking.cpp	Integration of fork choice for attestation processing
src/app/impl/timeline_impl.cpp	Enhanced timeline with interval-based slot execution
tests/unit/blockchain/fork_choice_test.cpp	Comprehensive test suite for fork choice functionality

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-26T06:21:44Z

src/blockchain/fork_choice.cpp

+    };
+    store.blocks_.emplace(anchor_root, std::move(anchor_block));
+    store.states_.emplace(anchor_root, std::move(anchor_state));
+    std::cout << "Genesis (anchor) root " << anchor_root.toHex() << "\n";


Using std::cout for output in production code violates logging conventions. This should use the proper logging system (SL_INFO, SL_DEBUG, etc.) or be removed entirely.

src/modules/networking/networking.cpp

Copilot · 2025-09-26T06:21:46Z

src/executable/lean_node.cpp

+      std::chrono::duration_cast<std::chrono::milliseconds>(
+          std::chrono::system_clock::now().time_since_epoch())
+          .count();
+  genesis_time += 12000 - (genesis_time % 12000); // 1758719218000


The magic number 12000 should be replaced with a named constant (e.g., GENESIS_TIME_INTERVAL_MS) to improve code readability and maintainability. The comment with the hardcoded timestamp should also be removed.

… update constructors and tests

Copilot

Pull Request Overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 6 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/executable/lean_node.cpp

src/blockchain/fork_choice.cpp

src/app/impl/timeline_impl.cpp

turuslan · 2025-09-27T04:49:43Z

src/blockchain/fork_choice.hpp

+
+   private:
+    STF stf_;
+    std::shared_ptr<clock::SystemClock> clock_;


time_ counter tracks time observed and incremented by ForkChoiceStore state machine.
It's not expected to be real clock, and not mockable.

Suggested change

std::shared_ptr<clock::SystemClock> clock_;

Interval time_ = 0;

May add separate slotFromClock method

turuslan · 2025-09-27T05:04:20Z

src/blockchain/fork_choice.cpp

+    } else {
+      // forkchoice should be correctly ticked to current time before importing
+      // gossiped attestations
+      if (vote.slot > getCurrentSlot() + 1) {


getCurrentSlot() should return slot from time_, not clock.now()

Suggested change

if (vote.slot > getCurrentSlot() + 1) {

if (vote.slot > getCurrentSlot()) {

https://github.com/leanEthereum/leanSpec/blob/fd029cb1bd13a847c350a7e4aafc02bed69194e9/src/lean_spec/subspecs/forkchoice/store.py#L175-L177

https://github.com/leanEthereum/leanSpec/blob/fd029cb1bd13a847c350a7e4aafc02bed69194e9/docs/client/forkchoice.md?plain=1#L303-L306

src/blockchain/fork_choice.cpp

Co-authored-by: Copilot <[email protected]>

Copilot

Pull Request Overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

src/modules/shared/prodution_types.tmp.hpp:1

The filename contains a typo: 'prodution_types.tmp.hpp' should be 'production_types.tmp.hpp' (missing 'c' in 'production').

/**

src/blockchain/fork_choice.cpp:1

Large block of commented debug code should be removed from production code. If debugging functionality is needed, consider using a proper logging mechanism with debug levels instead.

/**

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/executable/lean_node.cpp

Copilot · 2025-09-29T05:04:31Z

src/blockchain/fork_choice.cpp

+    blocks_.emplace(block_hash, final_block);
+    states_.emplace(block_hash, std::move(final_state));
+
+    // update head (not in spec)


The comment '(not in spec)' indicates a deviation from specification. This should be properly documented with rationale for the deviation and potential implications.

Suggested change

// update head (not in spec)

/**

* Deviation from specification: update head_ to the newly produced block.

*

* Rationale:

* The specification does not require the fork choice head to be updated immediately

* upon block production. However, in this implementation, we update head_ here to

* ensure that subsequent queries to getHead() reflect the most recently produced block.

* This can simplify block production logic and improve responsiveness for clients

* querying the current head.

*

* Potential implications:

* - May cause temporary divergence from the canonical fork choice rule if other

* blocks are produced or imported concurrently.

* - Should be reviewed to ensure it does not affect consensus safety or liveness.

* - If strict spec compliance is required, consider removing or gating this update.

*/

…ot notifications and improve time management

…to feature/fork_choice_timeline

…vote handling

Copilot

Pull Request Overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/blockchain/fork_choice.cpp:1

Remove commented-out debug code. This clutters the codebase and should be replaced with proper logging if needed.

/**

src/blockchain/state_transition_function.cpp:1

Remove commented-out code. If this assignment is intentionally disabled, add a comment explaining why.

/**

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/blockchain/impl/fc_block_tree.cpp

src/blockchain/fork_choice.hpp

src/executable/lean_node.cpp

src/loaders/impl/production_loader.hpp

src/modules/production/production.cpp

src/modules/shared/prodution_types.tmp.hpp

…vote handling

src/app/impl/timeline_impl.cpp

src/blockchain/fork_choice.hpp

turuslan · 2025-09-30T03:07:21Z

src/blockchain/fork_choice.cpp

+    } else {
+      // forkchoice should be correctly ticked to current time before importing
+      // gossiped attestations
+      if (vote.slot > getCurrentSlot() + 1) {


https://github.com/leanEthereum/leanSpec/blob/fd029cb1bd13a847c350a7e4aafc02bed69194e9/src/lean_spec/subspecs/forkchoice/store.py#L175-L177

https://github.com/leanEthereum/leanSpec/blob/fd029cb1bd13a847c350a7e4aafc02bed69194e9/docs/client/forkchoice.md?plain=1#L303-L306

turuslan · 2025-09-30T03:16:11Z

src/blockchain/fork_choice.cpp

+    auto block_hash = block.hash();
+    blocks_.emplace(block_hash, block);
+    states_.emplace(block_hash, std::move(state));
+
+    // update head (not in spec)
+    head_ = block_hash;


Suggested change

auto block_hash = block.hash();

blocks_.emplace(block_hash, block);

states_.emplace(block_hash, std::move(state));

// update head (not in spec)

head_ = block_hash;

BOOST_OUTCOME_TRY(onBlock(block));

…_timeline # Conflicts: # src/executable/lean_node.cpp

src/blockchain/fork_choice.cpp

xDimon · 2025-09-30T06:55:26Z

src/injector/node_injector.cpp

+    AnchorState genesis_state = STF::generateGenesisState(*genesis_config);
+    AnchorBlock genesis_block = STF::genesisBlock(genesis_state);


I'd like to refactor it to use only injections. AnchorState inherited on State and having ctor by genesis_config. And the same for AnchorBlock with ctor by AnchorState.

src/blockchain/fork_choice.cpp

turuslan and others added 13 commits September 19, 2025 13:13

fork choice

6ce87e1

Signed-off-by: turuslan <[email protected]>

test

a23818b

Signed-off-by: turuslan <[email protected]>

test

352214f

Signed-off-by: turuslan <[email protected]>

test

d4eb551

Signed-off-by: turuslan <[email protected]>

fix gcc

7fc3609

Signed-off-by: turuslan <[email protected]>

pr comment

dbbe9bf

Signed-off-by: turuslan <[email protected]>

pr comment

b30b5ae

Signed-off-by: turuslan <[email protected]>

Add slot interval start events and corresponding handlers

9ec3687

Add genesis configuration support to node injector

f1a01c3

Add fork choice store integration in networking and module instances

73d7f73

Integrate ForkChoiceStore into various modules and update related con…

f59b1f0

…figurations

Refactor Block and BlockBody to use variable size containers; impleme…

d899917

…nt FCBlockTree class for fork choice logic

Enhance fork choice logic: implement block production and current slo…

e399d8e

…t retrieval; refactor BlockBody to use variable size containers

Copilot AI review requested due to automatic review settings September 26, 2025 06:20

Copilot AI reviewed Sep 26, 2025

View reviewed changes

kamilsa added 4 commits September 26, 2025 16:34

ForkChoiceStore constructor

11a5679

Return result in produce block

56210c1

Small cleaning

b91ca19

Refactor ForkChoiceStore: integrate manual clock for time management;…

ee903e7

… update constructors and tests

Copilot AI review requested due to automatic review settings September 26, 2025 13:50

Copilot AI reviewed Sep 26, 2025

View reviewed changes

src/executable/lean_node.cpp Outdated Show resolved Hide resolved

src/blockchain/fork_choice.cpp Outdated Show resolved Hide resolved

turuslan reviewed Sep 27, 2025

View reviewed changes

Genesis interval ms const

578bc74

Co-authored-by: Copilot <[email protected]>

Copilot AI review requested due to automatic review settings September 29, 2025 05:03

Update comment

2058702

Co-authored-by: Copilot <[email protected]>

Copilot AI reviewed Sep 29, 2025

View reviewed changes

kamilsa added 3 commits September 29, 2025 10:22

Refactor timeline implementation: calculate absolute intervals for sl…

6e9125e

…ot notifications and improve time management

Merge remote-tracking branch 'origin/feature/fork_choice_timeline' in…

23297c3

…to feature/fork_choice_timeline

Refactor ForkChoiceStore: simplify block production logic and update …

bc088b5

…vote handling

Copilot AI review requested due to automatic review settings September 29, 2025 09:46

Copilot AI reviewed Sep 29, 2025

View reviewed changes

xDimon reviewed Sep 29, 2025

View reviewed changes

Refactor ForkChoiceStore: simplify block production logic and update …

6dfb116

…vote handling

xDimon reviewed Sep 29, 2025

View reviewed changes

src/app/impl/timeline_impl.cpp Outdated Show resolved Hide resolved

turuslan approved these changes Sep 30, 2025

View reviewed changes

kamilsa added 6 commits September 30, 2025 09:14

Review fixes 1

1495cf8

Fix test (autogenerate)

f55b1bd

Fix cmake

87438b6

Fix lean node

e4c378c

Merge remote-tracking branch 'origin/master' into feature/fork_choice…

0e14bc6

…_timeline # Conflicts: # src/executable/lean_node.cpp

Small fixes

0ecc5f5

xDimon reviewed Sep 30, 2025

View reviewed changes

qdrvm deleted a comment from Copilot AI Sep 30, 2025

Small fixes

d4e2f06

kamilsa enabled auto-merge (squash) September 30, 2025 07:39

turuslan reviewed Sep 30, 2025

View reviewed changes

src/blockchain/fork_choice.cpp Outdated Show resolved Hide resolved

qdrvm deleted a comment from Copilot AI Sep 30, 2025

Review fixes 2

8e32159

xDimon self-requested a review October 1, 2025 06:31

xDimon approved these changes Oct 1, 2025

View reviewed changes

kamilsa merged commit 6765a12 into master Oct 1, 2025
2 checks passed

kamilsa deleted the feature/fork_choice_timeline branch October 1, 2025 06:37

	std::shared_ptr<clock::SystemClock> clock_;
	Interval time_ = 0;

	if (vote.slot > getCurrentSlot() + 1) {
	if (vote.slot > getCurrentSlot()) {

-    // update head (not in spec)
+    /**
+     * Deviation from specification: update head_ to the newly produced block.
+     *
+     * Rationale:
+     * The specification does not require the fork choice head to be updated immediately
+     * upon block production. However, in this implementation, we update head_ here to
+     * ensure that subsequent queries to getHead() reflect the most recently produced block.
+     * This can simplify block production logic and improve responsiveness for clients
+     * querying the current head.
+     *
+     * Potential implications:
+     * - May cause temporary divergence from the canonical fork choice rule if other
+     *   blocks are produced or imported concurrently.
+     * - Should be reviewed to ensure it does not affect consensus safety or liveness.
+     * - If strict spec compliance is required, consider removing or gating this update.
+     */

		AnchorState genesis_state = STF::generateGenesisState(*genesis_config);
		AnchorBlock genesis_block = STF::genesisBlock(genesis_state);

3SF & Fork-choice #24

3SF & Fork-choice #24

Uh oh!

Conversation

kamilsa commented Sep 26, 2025

Known issues

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

turuslan Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

turuslan Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

turuslan Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

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!

turuslan Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

turuslan Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xDimon Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development