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

Feature: Dynamic Protocol State #5336

Merged
merged 575 commits into from
Feb 1, 2024

Conversation

jordanschalm
Copy link
Member

@jordanschalm jordanschalm commented Feb 1, 2024

This PR includes all changes from the Dynamic Protocol State feature branch and conflict resolution from #5325.

Rather than merging jordan/sync-dyn-proto-state--master->feature/dynamic-protocol-state, then feature/dynamic-protocol-state->master, this PR merges jordan/sync-dyn-proto-state--master->master directly. This is because conflicts resolved in the sync PR re-appeared in the feature branch PR; but since the feature branch is protected we can't resolve conflicts there directly (same problem as #5071).

It replaces #5158.

durkmurder and others added 30 commits October 18, 2023 21:47
• revised goDoc of protocol state implementation
• changed `ProtocolStateEntry.PreviousEpoch` to pointer (consistent with `ProtocolStateEntry.NextEpoch`)
• There should be no significant algorithmic changes. Though, I have switched the order of the if and else branches when processing an Epoch Setup event. For me, this is much more in line with the intuitive order of documentation.

state/protocol/protocol_state/updater.go:
• notable updates and revisions of goDoc
• tried to address concerns around inconsistent handling of invariances
• updated code to work with `ProtocolStateEntry.PreviousEpoch` being potentially a nil pointer

storage/badger/protocol_state_test.go:
• detailed goDoc revisions for test `assertRichProtocolStateValidity`
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
… ordering assumptions in the system smart contracts vs the core protocol layer.
…`Updater.ProcessEpochSetup()` such that the documentation and implementation are in the same order
…. Updated finalize command to correctly read prepared data.
It is commented out on HEAD, uncommented on master.

Causes following error when uncommented here:

```
 CGO_CFLAGS="" mockgen -destination=storage/mocks/storage.go -package=mocks github.com/onflow/flow-go/storage Blocks,Headers,Payloads,Collections,Commits,Events,ServiceEvents,TransactionResults
 # MERGE: Line below commented out on HEAD
 CGO_CFLAGS="" mockgen -destination=module/mocks/network.go -package=mocks github.com/onflow/flow-go/module Local,Requester
 2024/01/30 10:30:40 Failed to format generated source code: module/mocks/network.go:67:65: missing ',' in type argument list (and 3 more errors)
```
This version includes ProtocolStateID and all changes to date on master
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2024

Codecov Report

Attention: 531 lines in your changes are missing coverage. Please review.

Comparison is base (f1f4ec5) 56.79% compared to head (321a9e2) 55.93%.

Files Patch % Lines
model/flow/protocol_state.go 62.50% 70 Missing and 14 partials ⚠️
model/flow/identity.go 61.80% 44 Missing and 11 partials ⚠️
model/flow/identity_list.go 68.18% 48 Missing and 1 partial ⚠️
state/protocol/badger/state.go 77.51% 31 Missing and 16 partials ⚠️
state/protocol/badger/mutator.go 63.86% 31 Missing and 12 partials ⚠️
state/protocol/badger/params.go 59.59% 30 Missing and 10 partials ⚠️
consensus/hotstuff/committees/static.go 0.00% 36 Missing ⚠️
cmd/bootstrap/cmd/rootblock.go 75.22% 19 Missing and 9 partials ⚠️
cmd/scaffold.go 0.00% 12 Missing ⚠️
cmd/bootstrap/cmd/finalize.go 64.51% 8 Missing and 3 partials ⚠️
... and 45 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5336      +/-   ##
==========================================
- Coverage   56.79%   55.93%   -0.87%     
==========================================
  Files         757     1017     +260     
  Lines       75303    97998   +22695     
==========================================
+ Hits        42767    54812   +12045     
- Misses      29196    39082    +9886     
- Partials     3340     4104     +764     
Flag Coverage Δ
unittests 55.93% <67.48%> (-0.87%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

🎉

Copy link
Member

@durkmurder durkmurder left a comment

Choose a reason for hiding this comment

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

🚢

@jordanschalm jordanschalm added this pull request to the merge queue Feb 1, 2024
Merged via the queue into master with commit 568129d Feb 1, 2024
50 of 51 checks passed
@jordanschalm jordanschalm deleted the jordan/sync-dyn-proto-state--master branch February 1, 2024 18:43
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