Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track unrealized justification/finalization in forkchoice #10658

Merged
merged 6 commits into from
May 21, 2022

Conversation

potuz
Copy link
Contributor

@potuz potuz commented May 8, 2022

Tracks unrealized justification and finalization in forkchoice. It does not introduce runtime changes. Pure functions with their respective unit tests.

@potuz potuz requested a review from a team as a code owner May 8, 2022 14:47
@potuz potuz requested review from kasey, terencechain and rkapka May 8, 2022 14:47
@terencechain terencechain added Blocked Blocked by research or external factors and removed Ready For Review labels May 9, 2022
Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

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

After trying to grok some of the unit tests here, I have a concern about the function signature of:

func (f *ForkChoice) InsertOptimisticBlock(
	ctx context.Context,
	slot types.Slot,
	blockRoot, parentRoot, payloadHash [fieldparams.RootLength]byte,
	justifiedEpoch, finalizedEpoch types.Epoch,
) 

It feels very likely someone will mix up the last two function arguments, and even the first three. Highly recommend we switch to a struct of func arguments as an alternative. It will also make unit tests more understandable

@potuz potuz merged commit e7991b9 into develop May 21, 2022
@delete-merged-branch delete-merged-branch bot deleted the unrealized_justification_forkchoice branch May 21, 2022 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Blocked by research or external factors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants