Skip to content

fork-choice interface - #767

Closed
S1nus wants to merge 23 commits into
mainfrom
connor/fork-choice-interface
Closed

fork-choice interface#767
S1nus wants to merge 23 commits into
mainfrom
connor/fork-choice-interface

Conversation

@S1nus

Copy link
Copy Markdown
Contributor

Fork-choice package added to the repo. It supports defining any fork-choice rule as a func ([]*types.Block) (*types.Block, bool), it works similarly to the DA interface.

The PR also integrates the fork-choice interface into BlockManager.

This PR also includes an implementation of the FirstOrderedPure fork-choice rule, and a placeholder for the FirstOrderedCentralized, whose implementation is currently blocked by #735.

@gupadhyaya

Copy link
Copy Markdown
Contributor

@S1nus i think you will have to use default FCR in all the tests that create newFullNode. otherwise, there won't be any FCR, hence leading to nil pointer error.

@S1nus

Copy link
Copy Markdown
ContributorAuthor

Newest commit changes FCRs from a function signature to an interface type. Certain schemes will want to have state, hence why it makes sense to represent the FCR as a struct. There might also be other functions besides Apply().

@S1nus

Copy link
Copy Markdown
ContributorAuthor

another idea: the fork-choice rule configuration option may be better suited as a parameter in the genesis block, rather than Rollkit's config

@codecov-commenter

codecov-commenter commented Mar 16, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 90.00% and project coverage change: +0.08 🎉

Comparison is base (1104628) 54.99% compared to head (fa46759) 55.08%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@ Coverage Diff @@## main #767 +/- ##
==========================================
+ Coverage 54.99% 55.08% +0.08% 
==========================================
Files 54 56 +2 Lines 9904 9927 +23 ==========================================
+ Hits 5447 5468 +21 - Misses 3662 3663 +1 - Partials 795 796 +1 
Impacted FilesCoverage Δ
block/manager.go65.84% <76.92%> (+0.16%)⬆️
config/config.go90.90% <100.00%> (+0.58%)⬆️
fork_choice/first_ordered_pure.go100.00% <100.00%> (ø)
fork_choice/rule.go100.00% <100.00%> (ø)
node/header_exchange.go51.96% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment threadblock/manager.go

fcr, ok := fork_choice.GetRule(conf.ForkChoiceRule)
if !ok {
fcr, ok = fork_choice.GetRule(config.DefaultNodeConfig.BlockManagerConfig.ForkChoiceRule)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiple calls to GetRule is not needed. when !ok simply assign firstOrderedPure to fcr?

Comment threadblock/manager.go
"hash", block.Hash(),
)
m.syncCache[block.SignedHeader.Header.BaseHeader.Height] = block
if m.syncCache[block.SignedHeader.Header.BaseHeader.Height] == nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

height := block.SignedHeader.Header.BaseHeader.Height and use height in multiple places

Comment threadblock/manager.go Outdated
@S1nus
S1nusforce-pushed the connor/fork-choice-interface branch from 95b08e7 to 8f01985CompareMarch 23, 2023 09:31
@S1nus

S1nus commented Jun 7, 2023

Copy link
Copy Markdown
ContributorAuthor

Closing the PR, we will re-visit as part of the P2P Blocksync project #983

@S1nusS1nus closed this Jun 7, 2023
@tac0turtle
tac0turtle deleted the connor/fork-choice-interface branch March 20, 2025 11:57
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@S1nus@gupadhyaya@codecov-commenter