[KVStore] Support changing protocol state ID in sealing segment #5656
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jordanschalm
changed the title
Base automatically changed from
yurii/5136-querying-kvstore
to
feature/protocol-state-kvstore
April 12, 2024 15:15
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/protocol-state-kvstore #5656 +/- ##
==================================================================
- Coverage 64.03% 63.48% -0.55%
==================================================================
Files 8 91 +83
Lines 798 8153 +7355
==================================================================
+ Hits 511 5176 +4665
- Misses 252 2690 +2438
- Partials 35 287 +252
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
we had many error types, but none were actually used.
durkmurder
reviewed
Apr 17, 2024
state/protocol/inmem/snapshot.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("could not get current epoch commit event: %w", err) | ||
| head := s.enc.SealingSegment.Highest() | ||
| entry, ok := s.enc.SealingSegment.ProtocolStateEntries[head.Payload.ProtocolStateID] |
durkmurder
reviewed
Apr 17, 2024
state/protocol/inmem/snapshot.go
Outdated
| } | ||
|
|
||
| func (s Snapshot) ProtocolState() (protocol.KVStoreReader, error) { | ||
| return kvstore.VersionedDecode(s.enc.KVStore.Version, s.enc.KVStore.Data) | ||
| head := s.enc.SealingSegment.Highest() | ||
| entry, ok := s.enc.SealingSegment.ProtocolStateEntries[head.Payload.ProtocolStateID] |
durkmurder
reviewed
Apr 17, 2024
|
|
||
| segment, err := state.Final().SealingSegment() | ||
| require.NoError(t, err) | ||
| assert.GreaterOrEqual(t, len(segment.ProtocolStateEntries), 2, "should have >2 distinct protocol state entries") |
durkmurder
reviewed
Apr 17, 2024
state/protocol/badger/state.go
Outdated
| @@ -588,6 +572,12 @@ func bootstrapEpoch( | |||
| } | |||
| } | |||
|
|
|||
| // insert epoch protocol state entry, which references above service events | |||
| err := epochProtocolStateSnapshots.StoreTx(richEntry.ID(), richEntry.ProtocolStateEntry)(tx) | |||
There was a problem hiding this comment.
You can also use SkipDuplicatesTx to avoid handling specific sentinel
seal into epoch 2 before stopping the epoch 1 VN
durkmurder
reviewed
Apr 17, 2024
durkmurder
reviewed
Apr 17, 2024
| // wait until we have sealed all blocks in epoch 1 before stopping the replaced container | ||
| // in particular, this avoids an edge case where sealing halts if we stop the single VN | ||
| // assigned in epoch 1 between finalizing and sealing the transition into epoch 2 | ||
| s.AwaitSealedView(s.Ctx, epoch1FinalView+1, time.Minute, 500*time.Millisecond) |
durkmurder
reviewed
Apr 17, 2024
model/flow/sealing_segment.go
Outdated
| // to a previously unseen protocol state entry. If it does, retrieves the state entry | ||
| // and persists it for inclusion in the resulting SealingSegment. | ||
| // Errors expected during normal operation: | ||
| // - InvalidSealingSegmentError if the added block would cause an invalid resulting segment |
There was a problem hiding this comment.
Not sure where this comes from? protocolStateLookup doesn't return expected errors.
Additionally confused how it can construct an invalid resulting segment
durkmurder
approved these changes
Apr 17, 2024
Merged
4 tasks
|
Created this WIP follow-up PR to address the outstanding feedback to remove |
cmd/util/common/checkpoint.go
Outdated
There was a problem hiding this comment.
minor nitpick: maybe we should rename the function GenerateProtocolSnapshotForCheckpointWithHeights to GenerateProtocolSnapshotForCheckpointWithHeight (remove "s" at the end), because we are only providing one height value.
AlexHentschel
approved these changes
Apr 26, 2024
Co-authored-by: Alexander Hentschel <[email protected]>
…go into jordan/5120-sealing-segment
jordanschalm
merged commit 48297e8
into
feature/protocol-state-kvstore
55 checks passed
This was referenced Apr 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR modifies the
SealingSegmentto support block segments containing different protocol state commitments (distinctPayload.ProtocolStateIDfields).SealingSegment.ProtocolStateEntriesto store all distinct protocol state entriesProtocolStateEntriesSnapshotAPI #5650SealingSegment: