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

[Follower] Skipping proposals too far ahead of locally finalized view #4118

Merged
merged 3 commits into from
Apr 5, 2023

Conversation

durkmurder
Copy link
Member

Context

This PR removes compliance.Config from follower.Core and moves it to follower.Engine which uses SkipNewProposalsThreshold to drop proposals that are too far ahead in future.

@@ -347,6 +346,7 @@ func (builder *FlowAccessNodeBuilder) buildFollowerEngine() *FlowAccessNodeBuild
node.Storage.Headers,
builder.Finalized,
core,
followereng.WithComplianceConfigOpt(modulecompliance.WithSkipNewProposalsThreshold(node.ComplianceConfig.SkipNewProposalsThreshold)),
Copy link
Member

@jordanschalm jordanschalm Mar 29, 2023

Choose a reason for hiding this comment

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

It doesn't make a huge difference IMO whether we use height or view here, but:

  • The Follower uses view
  • The Participant uses height
  • The documentation mentions height
    // SkipNewProposalsThreshold defines the threshold for dropping blocks that are too far in
    // the future. Formally, let `H` be the height of the latest finalized block known to this
    // node. A new block `B` is dropped without further processing, if
    // B.Height > H + SkipNewProposalsThreshold
    SkipNewProposalsThreshold uint64

There is inconsistency between the config, and the two implementations.

Suggestion

The Participant engine has easy access to the finalized view, and the view of the header being considered.

finalView := c.finalizedView.Value()

We should:

  • change the documentation for SkipNewProposalsThreshold to refer to view rather than height
  • change this line in Participant engine to use finalized view instead of height

Then the two implementations, and config documentation, will all be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have considered it and think that the name doesn't refer to view or height and can be used for both depending on context, in the end it's part of config which could be used differently by concrete compliance engine implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, the name doesn't refer to view or height - but the documentation for the config (used in both implementations) explicitly refers to height.

Formally, let H be the height of the latest finalized block known to this node. A new block B is dropped without further processing, if B.Height > H + SkipNewProposalsThreshold

Copy link
Member

@AlexHentschel AlexHentschel Apr 4, 2023

Choose a reason for hiding this comment

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

+1 on Jordan's suggestion to make it consistent. I have a mild inclination to using view, because that is the canonical measure for time in our protocol. For example, if we use view, we could in the future eliminate an additional edge case in the follower:

// Note: we could eliminate this edge case by dropping future blocks, iff their _view_
// is strictly larger than `V + EpochCommitSafetyThreshold`, where `V` denotes
// the latest finalized block known to this node.

Btw, totally fine to leave this for a later PR.

Base automatically changed from yurii/6493-follower-core to master April 3, 2023 09:55
Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

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

Very nice.

I had to push myself to approve the PR 😉, because I still can't get over this construction 😱:

followereng.WithComplianceConfigOpt(modulecompliance.WithSkipNewProposalsThreshold(node.ComplianceConfig.SkipNewProposalsThreshold)),

I have some thoughts how we can remove this ugly code, which I added to issue #4125.

@durkmurder
Copy link
Member Author

bors merge

@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2023

Codecov Report

Merging #4118 (dd4c28f) into master (243a5e3) will decrease coverage by 8.59%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #4118      +/-   ##
==========================================
- Coverage   60.32%   51.73%   -8.59%     
==========================================
  Files         236      522     +286     
  Lines       22344    47056   +24712     
==========================================
+ Hits        13479    24346   +10867     
- Misses       7875    20689   +12814     
- Partials      990     2021    +1031     
Flag Coverage Δ
unittests 51.73% <0.00%> (-8.59%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/execution_builder.go 0.00% <0.00%> (ø)
cmd/verification_builder.go 0.00% <0.00%> (ø)

... and 612 files with indirect coverage changes

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

@bors bors bot merged commit 018512f into master Apr 5, 2023
@bors bors bot deleted the yurii/6493-skipping-blocks-too-far-in-future branch April 5, 2023 12:46
Sign up for free to 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.

4 participants