[Dynamic Protocol State] Querying of KV Store using Snapshot API
#5650
Conversation
…s after last changes
durkmurder
requested review from
peterargue,
zhangchiqing and
ramtinms
as code owners
There was a problem hiding this comment.
Looks great.
As you mentioned in the PR description, the ProtocolState naming remains a bit confusing, as what was previously "the whole protocol state" is now just a part of it, and stored separately from the rest. I am OK with addressing this when we move the epoch state into the KVStore, I've added a couple suggestions for noting this path in the comments / renaming a few other fields.
| // ProtocolState returns the dynamic protocol state that the Head block commits to. The | ||
| // compliance layer guarantees that only valid blocks are appended to the protocol state. | ||
| // EpochProtocolState returns the epoch part of dynamic protocol state that the Head block commits to. | ||
| // The compliance layer guarantees that only valid blocks are appended to the protocol state. |
There was a problem hiding this comment.
| // The compliance layer guarantees that only valid blocks are appended to the protocol state. | |
| // The compliance layer guarantees that only valid blocks are appended to the protocol state. | |
| // TODO: Conceptually the epoch protocol state is part of the overall Dynamic Protocol State | |
| // (`ProtocolState` below), but it is separate here for historical reasons. Eventually we should | |
| // move the epoch protocol state data into the protocol state KV Store. |
There was a problem hiding this comment.
regarding your suggestion @jordanschalm:
- As I understand your suggestion, you are commenting on a future revision of the implementation. It is not a suggestion to change the API (
Snapshotinterface), right? - Therefore, I would suggest to move your suggested
TODOcomment intoEpochProtocolState()'s body (before line 332)
Given the importance of the Epoch portion of the protocol state, I feel it is very reasonable to retain the dedicated method EpochProtocolState() as a convenience function in the (Snapshot interface).
There was a problem hiding this comment.
No, It is a suggestion to change the API interface in a future revision (remove the top-level EpochProtocolState method).
I agree the Epoch portion of the protocol state is important, but at that point the data will be duplicated in 3 places in the Snapshot: .Epochs, .EpochProtocolState, and (for example) .KVStore.EpochState.
In general we should aim to have a collection of data in 1 place in the Snapshot structure, adding a shortcut in exceptional cases. Maybe 2 for deeply nested fields, definitely not 3 😅.
There was a problem hiding this comment.
📝 added item to issue 5666
| // GetInvalidEpochTransitionAttempted returns a flag indicating whether epoch | ||
| // fallback mode has been tentatively triggered on this fork. | ||
| // Errors: | ||
| // - ErrKeyNotSupported if the respective entry does not exist in the | ||
| // Protocol State Snapshot that is backing the `Reader` interface. | ||
| GetInvalidEpochTransitionAttempted() (bool, error) |
There was a problem hiding this comment.
I added this originally mainly as a way to have two different versions we could test with right away. But we still have the InvalidEpochTransitionAttempted field in the EpochStateContainer (where is was originally) and this is what is used for triggering EFM in the state machine.
I don't think we have an immediate plan to use this version of the flag instead, so I think we should remove it to avoid confusion.
However, it's still necessary for several tests (in particular the integration test in #5477) to have multiple versions to transition between before releasing the feature. Not sure what the best course of action is.
Suggestion: At the moment, I'm leaning toward having a v1 model version which is otherwise identical to v0, but returns a different version number.
This comment isn't really related to this PR - feel free to address here, otherwise I can in #5477.
There was a problem hiding this comment.
I will leave this to you as you will be able to check what is the best course of action when you will be working on the integration test.
There was a problem hiding this comment.
📝 added item to issue 5666
| // ProtocolState returns the dynamic protocol state that the Head block commits to. The | ||
| // compliance layer guarantees that only valid blocks are appended to the protocol state. | ||
| // EpochProtocolState returns the epoch part of dynamic protocol state that the Head block commits to. | ||
| // The compliance layer guarantees that only valid blocks are appended to the protocol state. |
There was a problem hiding this comment.
regarding your suggestion @jordanschalm:
- As I understand your suggestion, you are commenting on a future revision of the implementation. It is not a suggestion to change the API (
Snapshotinterface), right? - Therefore, I would suggest to move your suggested
TODOcomment intoEpochProtocolState()'s body (before line 332)
Given the importance of the Epoch portion of the protocol state, I feel it is very reasonable to retain the dedicated method EpochProtocolState() as a convenience function in the (Snapshot interface).
state/protocol/protocol_state.go
Outdated
There was a problem hiding this comment.
Probably beyond the scope of the PR, but I am a bit worried about about the safety of this API.
-
The
DynamicProtocolState.Entry()method does not return the most recent dynamic protocol state, which is easily overlooked. The documentation is clear, but I am not sure that is enough.For example, I would guess that this code here does not take this into account. I am worried that this implementation would forget the "dynamic status" of ejected nodes, because it uses the initial status at the beginning of the spork!
I think the easiest (at least temporary) mitigation would be to rename the DynamicProtocolState.Entry() method to something more explicit like InitialEpochState()
state/protocol/badger/state.go
Outdated
| rootProtocolStateEntry := rootEpochState.Entry().ProtocolStateEntry | ||
| rootEpochStateID := rootProtocolStateEntry.ID() | ||
| err := protocolState.StoreTx(rootEpochStateID, rootProtocolStateEntry)(tx) |
There was a problem hiding this comment.
I am worried that his implementation is incorrect (more specifically, I think it doesn't ejected nodes correctly):
- lets assume we bootstrap with a sealing segment where:
- Bob was an authorized participant at the beginning of the epoch
- However, during the epoch Bob got ejected (slashing or maybe self-ejection)
- below (line 283), we index every block with the root state.
I guess in the end, this is more a flag for @jordanschalm's, that this code needs refactoring and to not use Snapshot.EpochProtocolState().Entry() as this always returns the state at the beginning of the Epoch (thereby "forgetting" ejection events).
There was a problem hiding this comment.
not use Snapshot.EpochProtocolState().Entry() as this always returns the state at the beginning of the Epoch
I don't think this is true.
epochPSForB := Snapshot.EpochProtocolState()returns theDynamicProtocolStatefor the snapshot's reference blockBpsEntryForB := DynamicProtocolState.Entry()returns theProtocolStateEntryassociated withBpsEntryForBis the data structure representing the complete but minimal dynamic protocol state, including block-by-block state changes- In particular,
ProtocolStateEntrycontainsEpochStateContainerwhich tracks the block-by-block identity lists in the fieldActiveIdentities DynamicIdentityEntryList
The structure is confusing, though, because Entry() is part of InitialProtocolState, whose documentation states that it "returns constant data for given epoch", which is not true of Entry().
There was a problem hiding this comment.
+1 on Jordan's point, all instances of DynamicProtocolState are obtained using a concrete block:
Both of those places commit to a concrete block so they always return state at reference block.
There is no way to obtain InitialProtocolState right now.
That is a good argument to refactor it later.
There was a problem hiding this comment.
Thanks. I am glad the implementations are doing the right thing. However, I would like to emphasize that this is super confusing given the current API design:
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.
There was a problem hiding this comment.
📝 added item to issue 5666
#5316
Context
This PR exposes KV Store using the existing
SnapshotAPI which allows querying data from Dynamic Protocol State by block ID.Access to the KV Store has been structured to mirror existing implementation for epochs+identity table. In future we might want to consolidate them in one interface which could be a facade to the Dynamic Protocol State and allows querying sub-states using it. For now both
KV StoreandEpochs+IdentityTableare exposes equally inSnapshotinterface.I have renamed
ProtocolStatetoEpochProtocolStatein some places but I believe we should also address the naming as part of future refactoring.Most of the tests that were previously disabled were enabled in this PR, but there are some tests still broken due to
SealingSegmentlimitations(details in #5120).