[Dynamic Protocol State] Read-only interfaces of protocol state #4579
Conversation
…otocol state read-only interface
…to protocol state
…ps://github.com/onflow/flow-go into yurii/5513-read-only-identity-table
…constant and should be always accesible
…ps://github.com/onflow/flow-go into yurii/5513-read-only-identity-table
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## feature/dynamic-protocol-state #4579 +/- ##
==================================================================
- Coverage 54.82% 53.94% -0.89%
==================================================================
Files 919 722 -197
Lines 85870 71641 -14229
==================================================================
- Hits 47080 38645 -8435
+ Misses 35178 30142 -5036
+ Partials 3612 2854 -758
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. |
…m/onflow/flow-go into yurii/5513-read-only-identity-table
|
I've accidentally merged PR for state updater, now it's in this PR. Sorry for such confusion. Closed PR has diff and adds changes very locally, so hopefully it isn't too bad. |
| func (s *UpdaterSuite) TestProcessEpochSetupHappyPath() { | ||
| setup := unittest.EpochSetupFixture(func(setup *flow.EpochSetup) { | ||
| setup.Counter = s.parentProtocolState.CurrentEpochSetup.Counter + 1 | ||
| setup.Participants[0].InitialWeight = 13 |
There was a problem hiding this comment.
| setup.Participants[0].InitialWeight = 13 |
Is this needed?
…urii/5513-read-only-identity-table
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
|
|
||
| // DynamicProtocolStateAdapter implements protocol.DynamicProtocolState by wrapping an InitialProtocolStateAdapter. | ||
| type DynamicProtocolStateAdapter struct { | ||
| *InitialProtocolStateAdapter |
There was a problem hiding this comment.
I think we could change this into a value type:
- this should never be nil
- improves memory locality
- InitialProtocolStateAdapter is a pointer to
flow.RichProtocolStateEntry, so its hardly any data to copy
There was a problem hiding this comment.
I would be inclined to extend RichProtocolStateEntry such that is directly implements the interfaces protocol.DynamicProtocolState and protocol.InitialProtocolState. In other words, merge the implementation of InitialProtocolStateAdapter and DynamicProtocolStateAdapter into RichProtocolStateEntry
| // Entry Returns low-level protocol state entry that was used to initialize this object. | ||
| // It shouldn't be used by high-level logic, it is useful for some cases such as bootstrapping. | ||
| // Prefer using other methods to access protocol state. | ||
| Entry() *flow.RichProtocolStateEntry |
There was a problem hiding this comment.
I would prefer, if we could remove this method from the API. Presumably, we will use this very rarely in higher-level business logic. But it is dangerous to expose broadly, because it could incentive engineers to use it incorrectly by accident.
There was a problem hiding this comment.
Added a point here. It's dangerous but has it's own usages. I would suggest to implement everything and then try to get rid of it when we see how everything comes together.
…urii/5513-read-only-identity-table
…nflow/flow-go into yurii/5513-read-only-identity-table
FVM Benchstat comparisonThis branch with compared with the base branch onflow:feature/dynamic-protocol-state commit 620d901 The command Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
https://github.com/dapperlabs/flow-go/issues/5513
Context
In this PR I've introduced read-only interfaces for accessing the protocol state. Initially we have planned to support two types of queries:
ByEpoch- returns information that is static for duration of epoch, can be queried by epoch counter.ByBlockID- returns information which can change from block to block during the epoch duration, can be queried by block ID.I have implemented only
ByBlockIDsince it's unclear ifByEpochis needed at this point at all. The only place where we query by-epoch is from consensus committee, which already has a logic for caching epochs on it's own.Why we can't simplify consensus committee to use protocol state? Replacing existing logic with protocol state is not very practical since the backbone of consensus committee is mapping views to epoch counters. This information is not available in protocol state. With protocol state we can only change from where the data will be fetched:
To summarize, I don't see too much value in using
ByEpochin consensus committee, so I have omitted it's implementation at this point since it requires building an extra index in database for storingepoch_counter->protocol_state_id.As part of this PR I've also changed how
GlobalParamswork, in current implementation it doesn't return errors anymore since all data must be accessible at any point from any place.GlobalParamswill be served only from protocol state when we change implementation ofprotocol.Snapshot.