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

InitialProtocolState refactor and epoch protocol sub-state rename #5968

Merged
merged 27 commits into from
May 29, 2024

Conversation

jordanschalm
Copy link
Member

@jordanschalm jordanschalm commented May 22, 2024

This PR addresses the following item from #5666:

I find it super confusing / misleading that Snapshot.EpochProtocolState().Entry() is defined through the InitialProtocolState interface. The InitialProtocolState is very clear in its documentation that it (quote) "returns constant data for given epoch." Therefore, the only possible conclusion is also that Entry returns the DynamicProtocolState as specified initially by the Epoch Setup and Commit events. However, the implementations always return the DynamicProtocolState as of the specified block. (for further details and discussion, see #5650)

Suggestions: (discussion April 15th, Yurii, Jordan, Alex)

  • merge InitialProtocolState interface into DynamicProtocolState EpochProtocolState
  • whenever we are referring to Epoch-sub-state, we should explicitly refer to it using "Epoch..."

Changes

  • Combines InitialProtocolState with DynamicProtocolState, renames combined type to EpochProtocolState
  • Broadly speaking, renames types and variablesg which were previously referred to as "Protocol State", to "Epoch Protocol State"

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

Attention: Patch coverage is 71.15385% with 60 lines in your changes are missing coverage. Please review.

Project coverage is 55.79%. Comparing base (1ac121d) to head (3cc4dde).
Report is 19 commits behind head on master.

Files Patch % Lines
cmd/scaffold.go 0.00% 21 Missing ⚠️
utils/unittest/fixtures.go 0.00% 11 Missing ⚠️
model/flow/protocol_state.go 41.17% 10 Missing ⚠️
storage/badger/epoch_protocol_state.go 76.47% 8 Missing ⚠️
state/protocol/inmem/epoch_protocol_state.go 87.50% 2 Missing and 2 partials ⚠️
integration/testnet/container.go 0.00% 2 Missing ⚠️
state/protocol/badger/snapshot.go 87.50% 2 Missing ⚠️
state/protocol/protocol_state/epochs/factory.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5968      +/-   ##
==========================================
- Coverage   55.82%   55.79%   -0.04%     
==========================================
  Files        1129     1129              
  Lines       89260    89330      +70     
==========================================
+ Hits        49829    49840      +11     
- Misses      34677    34735      +58     
- Partials     4754     4755       +1     
Flag Coverage Δ
unittests 55.79% <71.15%> (-0.04%) ⬇️

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.

@jordanschalm jordanschalm changed the title InitialProtocolState refactor InitialProtocolState refactor and epoch protocol sub-state rename May 22, 2024
@jordanschalm jordanschalm marked this pull request as ready for review May 22, 2024 17:36
@jordanschalm jordanschalm requested a review from peterargue as a code owner May 22, 2024 17:36
@jordanschalm jordanschalm requested review from durkmurder, AlexHentschel and kc1116 and removed request for peterargue May 22, 2024 17:36
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.

Thank you. This refactoring removes a large amount of ambiguity in my opinion. Great work 👏 .

Unfortunately, there are some remaining areas, where we are actually referring to the epoch sub-state (of the overall protocol state) but Interface and method inputs still only talk about the "Protocol State". I think this is particularly dangerous in case of IDs because we have a the filed ProtocolStateID in the block Payload, which is identical naming to the epoch sub-state's ID for the storage layer and therefore very easily confused. I tried to add extensive comments in all places that I found. Though, if you could take a second look that I didn't miss anything that would be appreciated.

Not sure, but it might be best to first merge this PR and then follow up with a second one doing the renaming, because there are a lot of files changed already in this PR. If we do that in two PRs, I am happy to take a second pass. But I am also totally happy if you just do the renaming at the end of this PR before merging. It doesn't have to be perfect, as long as most places in the respective storage layer abstraction actually talk about the epoch sub-state, I think it is fine if we missed a few occurrences that still use "protocol state". It will be clear from context.

Happy to help with the renaming ... it might touch a lot of code to get it consistent.

model/flow/protocol_state.go Outdated Show resolved Hide resolved
state/protocol/protocol_state.go Show resolved Hide resolved
state/protocol/protocol_state.go Outdated Show resolved Hide resolved
state/protocol/protocol_state.go Show resolved Hide resolved
state/protocol/protocol_state.go Show resolved Hide resolved
storage/badger/operation/protocol_state_test.go Outdated Show resolved Hide resolved
storage/badger/operation/protocol_state_test.go Outdated Show resolved Hide resolved
storage/badger/protocol_state.go Outdated Show resolved Hide resolved
storage/badger/protocol_state_test.go Outdated Show resolved Hide resolved
utils/unittest/fixtures.go Outdated Show resolved Hide resolved
@jordanschalm jordanschalm requested a review from ramtinms as a code owner May 28, 2024 21:51
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.

Very nice PR, extremely solid refactoring which brings consistency in the code base. Well done!

@jordanschalm jordanschalm enabled auto-merge May 29, 2024 15:27
@jordanschalm jordanschalm added this pull request to the merge queue May 29, 2024
Merged via the queue into master with commit 29bf6de May 29, 2024
55 checks passed
@jordanschalm jordanschalm deleted the jord/initial-proto-state-refactor branch May 29, 2024 16:12
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.

[EPIC] [Dynamic Protocol State | M2] ToDos, suggested revisions and tech debt
4 participants