[Cadence 0.x] Add ProtocolStateVersionUpgrade service event
#411
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
added a commit
to onflow/flow-go
that referenced
this pull request
Feb 28, 2024
| /// as a service event during the next heartbeat. | ||
| pub fun setPendingProtocolStateVersionUpgrade(newProtocolVersion: UInt64, activeView: UInt64) { | ||
| let pendingUpgrade = NodeVersionBeacon.ViewActivatedVersion(version: newProtocolVersion, activeView: activeView) | ||
| NodeVersionBeacon.storePendingProtocolStateVersionUpgrade(upgrade: pendingUpgrade) |
There was a problem hiding this comment.
Just to confirm my understanding:
The reason we don't just emit the event here is that the service events are only observed from the service transaction.
There was a problem hiding this comment.
Exactly. The idea was to be as restrictive as possible for what qualifies as a service event, because they are so security-sensitive, at the cost of convenience in this kind of case.
joshuahannan
approved these changes
Mar 13, 2024
| return | ||
| } | ||
| emit NodeVersionBeacon.ProtocolStateVersionUpgrade( | ||
| newProtocolVersion: pendingUpgrade!.version, |
There was a problem hiding this comment.
It doesn't look like it from looking at the other code, but I just want to confirm that there is no way that this optional can be nil here
There was a problem hiding this comment.
I'm pretty sure it cannot be nil, because of the if statement above:
if pendingUpgrade == nil {
return
}
Comment on lines
21
to
24
| // post { | ||
| // NodeVersionBeacon.peekPendingProtocolStateVersionUpgrade().version! == newProtocolVersion : "version not updated" | ||
| // NodeVersionBeacon.peekPendingProtocolStateVersionUpgrade().activeView! == activeView : "active view not updated" | ||
| // } |
There was a problem hiding this comment.
Should these be commented out or removed?
jordanschalm
commented
Mar 15, 2024
transactions/nodeVersionBeacon/admin/set_protocol_state_version.cdc
Outdated
Show resolved
Hide resolved
transactions/nodeVersionBeacon/admin/set_protocol_state_version.cdc
Outdated
Show resolved
Hide resolved
joshuahannan
approved these changes
Mar 24, 2024
|
Are you planning on doing this upgrade before or after the Cadence 1.0 upgrade? If the answer is after, we should make these changes based on the |
After Cadence 1.0 -> 👍 I'll do that. |
jordanschalm
changed the title
ProtocolStateVersionUpgrade service eventProtocolStateVersionUpgrade service event
2 tasks
jordanschalm
added a commit
to onflow/flow-go
that referenced
this pull request
Apr 29, 2024
* bump core-contracts version point to onflow/flow-core-contracts#411 * crash upon seeing upgrade event * add test outline * cleanup * tidy * lint: remove unused type * lint * bump core-contracts version to f59efe400a8bd77c29a6124f797b466ae4927c15 * tidy * proto state changes for integration test - require consecutive versions - bootstrap with v0 rather than v1 - ignore invalid service events * use v0 model in bootstrapping - don't panic on unknown service events when determining metrics - added todo to clean up metrics * fix bug - using wrong lookup method * add transaction script for proto version upgrade test * open dbs in read mode in tests * extend blockstate util * update upgrade test * clarify seq number getter name * add todos where code needs to be changed * add protocol entries to sealing segment (builder, struct) * add protostate entries while building segment * add fixture methods for default pstate id in tests * sketch changes in state bootstrapping * fix first half of sealing segment tests * re-enable all existing sealing segment tests * add segment tests * update docs * add root protocol state entry to snapshot constructor * update consensus tests * store all protocol state entries in bootstrapping * refactor: all epoch bootstrapping together * naming consistency * re-enable epoch integration tests * re-enable badger state tests * lint * lint disable rule which flags use of deprecated types * correct error in snapshot generation (consensus tests) * remove unused helper method * check for changing pstate ID in tests * add note about linter config change * consolidate error types in sealing segment we had many error types, but none were actually used. * Update state/protocol/protocol_state/kvstore.go * lint * use sealing segment for ProtocolState methods on snapshot * remove protocol state fields from encodable snapshot * fix test * lint * move KeyValueStoreData to flow package * lint * replace 2 fields with flow.kvdata * remove special handling for snapshots spanning epoch transitions * fix an edge case with vn epoch join/leave test seal into epoch 2 before stopping the epoch 1 VN * address review feedback * use snapshot to check version changes in test * tidy * specify v0 kvstore in upgrade test * remove InvalidEpochTransitionAttempted field from kvstore * shorten test * add documentation * cleanup/documentation * lint * update mocks * lint * review feedback * fix state commitment fixtures * goimport * goimport * goimports the `-local github.com/onflow/flow-go` flag treats some non-flow-go repos differently than others, if they happen to have the prefix flow-go (like flow-go-sdk).
|
Closing in favour of #419. (Keeping the branch open for now as it's pinned by a feature branch.) |
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 adds a minimal version of a service event to upgrade the protocol state version (see also onflow/flow-go#5428).
There is an integration test validating the new service event is processed in onflow/flow-go#5477.
For now, I have added this service event to the
NodeVersionBeaconcontract, which has some benefits and drawbacks:ProtocolStateVersionUpgradedoes something similar to the existing Version Beacon, so it makes sense for their logic to be togetherProtocolStateVersionUpgradecleanly difficult