[Dynamic Protocol State] InstanceParams refactoring
#5113
Conversation
… on creating state
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/dynamic-protocol-state #5113 +/- ##
==================================================================
- Coverage 56.46% 54.68% -1.78%
==================================================================
Files 987 236 -751
Lines 92546 23048 -69498
==================================================================
- Hits 52252 12603 -39649
+ Misses 36449 9391 -27058
+ Partials 3845 1054 -2791
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
This PR has some overlap with #5089, I would prefer if it gets merged first so I can resolve conflicts in bootstrapping process |
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
…nflow/flow-go into yurii/instance-params-refactoring
… in the `GlobalParams` interface (` state/protocol/params.go`)
… to uniformly return functors `func(*transaction.Tx) error`
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
…nflow/flow-go into yurii/instance-params-refactoring
…suggestions # Conflicts: # state/protocol/badger/state.go
… indicate in the variable names that these are the **latest** values
… variable anymore
There was a problem hiding this comment.
Very nice. This is a great cleanup.
There are a few places, where I think our goDoc should be revised, which I implemented in PR #5132 (targeting your branch), alongside a few other revisions. Most (but not all) of my comments should be addressed by my PR
| metrics.BlockSealed(lastSealed) | ||
| metrics.SealedHeight(lastSealed.Header.Height) | ||
| metrics.FinalizedHeight(lastFinalized.Header.Height) | ||
| for _, block := range segment.Blocks { | ||
| state.metrics.BlockFinalized(block) | ||
| metrics.BlockFinalized(block) | ||
| } |
There was a problem hiding this comment.
what do you think about moving these lines into this function
flow-go/state/protocol/badger/state.go
Line 850 in 4099164
? It would declutter the high-level code a bit.
There was a problem hiding this comment.
Reporting those metrics relies on SealingSegment and to my understanding we want to report them once, only when we bootstrap. updateEpochMetrics on other side is called every time we open a state(on each node startup) we probably don't want to report again those metrics. Additionally I would suggest to think if we actually need those metrics at that place, maybe it's safe to remove them?
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Context
This PR updates design and implementation of
InstanceParamsto be more modular, remove circular dependency ofState->InstanceParams->Stateand improves overall bootstrapping process.Previously our bootstrapping process was creating a state, then bootstraping it and then populating internal cache. I've updated the process to be more clear, now we bootstrap everything only after that we try to open boostrapped state.