[KV Store] `ProtocolStateVersionUpgrade` Service Event #5428

jordanschalm · 2024-02-21T18:33:58Z

This PR adds the service event type for ProtocolStateVersionUpgrade. I have provisionally added the new service event to NodeVersionBeacon contract (see also onflow/flow-core-contracts#411), which has some benefits and drawbacks:

Conceptually ProtocolStateVersionUpgrade does something similar to the existing Version Beacon, so it makes sense for their logic to be together
The Version Beacon contract already has a Heartbeat resource hooked up to the system chunk, which is necessary to emit service events
The existing version beacon code is written in a way that suggests the version it is operating on is global, rather than specifically scoped to the Execution State Machine
- This, combined with the limitations of upgrading contracts, makes adding ProtocolStateVersionUpgrade cleanly difficult

Other Changes

Changes marshalling logic for all service events, to prevent data loss when unmarshalling in some cases using JSON (9741a9a)
Removes unused service event fixtures

Change to encode/decode passes tests, still requires cleanup: - adds encode/decode for new event type - adds tests for new event type - changes how decoding is implemented to prevent a bug. Previously we would re-encode, then re-decode the entire service event, after first decoding it into a weakly typed map. In particular with JSON, this could cause the data to change between encode/decode cycles, I think because of numeric type sizes.

model/flow/service_event.go

codecov-commenter · 2024-02-22T16:08:10Z

Codecov Report

Attention: Patch coverage is 58.24742% with 81 lines in your changes are missing coverage. Please review.

Project coverage is 55.96%. Comparing base (b4f1bcf) to head (334aaf8).

Files	Patch %	Lines
model/convert/service_event.go	56.33%	24 Missing and 7 partials ⚠️
model/flow/service_event.go	53.57%	25 Missing and 1 partial ⚠️
utils/unittest/service_events_fixtures.go	67.34%	14 Missing and 2 partials ⚠️
utils/unittest/fixtures.go	0.00%	5 Missing ⚠️
model/flow/version_beacon.go	62.50%	3 Missing ⚠️

Additional details and impacted files

@@                        Coverage Diff                         @@
##           feature/protocol-state-kvstore    #5428      +/-   ##
==================================================================
- Coverage                           55.97%   55.96%   -0.02%     
==================================================================
  Files                                1018     1018              
  Lines                               97894    98055     +161     
==================================================================
+ Hits                                54800    54872      +72     
- Misses                              38994    39078      +84     
- Partials                             4100     4105       +5

Flag	Coverage Δ
unittests	`55.96% <58.24%> (-0.02%)`	⬇️

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.

fvm/systemcontracts/system_contracts.go

AlexHentschel

Thank you so much for adding documentation for existing functions. Amazing!

Most of my comments are of stylistic nature.
Though there us one conceptual aspect that worries me: the convention of when the ProtocolStateVersionUpgrade takes effect. I have added a detailed explanation of my concern in this comment.

fvm/systemcontracts/system_contracts.go

model/convert/service_event.go

AlexHentschel · 2024-03-05T00:20:55Z

model/convert/service_event.go

+					foundFieldCount++
+					activeViewValue = cdcEvent.Fields[i]
+				}
+			}


Do we need a default clause in the switch statement?

At the moment the conversion allows for extra fields. If they are unknown, then they are ignored. This simplifies migration (for example, it saved some work and complexity when migrating the EpochSetup event to the FLIP 204 format, which adds fields).

See also https://github.com/onflow/flow-go/pull/5428/files#r1513023070.

This approach of "ignoring unexpected fields" is in my opinion a security vulnerability waiting to happen. We know precisely what we are expecting but accept huge set of binary-different messages. We have gone through great length in the networking layer that it de-duplicates binary identical messages and reduces the trust score of peers should they try to spam us with the same (binary) message repeatedly. But we allow to include extra garbage in the encoding, i.e. it becomes trivial for a node to send me 1 million times the same message, all looking different on a binary level to the networking layer ...
The malleability surface in the code base is quite similar in nature in my opinion.

I am ok with continuing the existing approach in this PR. Though a non-unique encoding scheme is just generally not suitable for a BFT system. It creates too many edge cases that we cannot reasonably anticipate and manage in my opinion. I would like us to move away from this as soon as possible.

cc @janezpodhostnik for visibility

same discussion as below I guess

Completely agree in general, however these methods are specifically for use only on trusted inputs. Independent of whether or not we accept unknown fields, we cannot pass untrusted inputs from the network into these methods -- it would be trivial to crash the node or cause other problems. See this comment:

// CAUTION: This function must only be used for input events computed locally, by an // Execution or Verification Node; it is not resilient to malicious inputs.

Because of this, I don't think we improve security by being more restrictive about event fields here.

thanks 👍

I think I get it now: this conversion is for the Execution Node or Verification node, to convert a service event, which was produced by local computation, from flow.Event (Compact Cadence Format [ccf]) to flow.ServiceEvent. I think the conversion from ccf is generally not BFT, right?

I think the conversion from ccf is generally not BFT, right?

I don't think that is specifically true about the CCF format in general. And here we are operating on everything in memory as Cadence Go types: it hasn't been encoded, so CCF doesn't play a role here. The key part is that the inputs to the function are produced strictly by local computation.

model/flow/service_event.go

model/flow/version_beacon.go

AlexHentschel · 2024-03-05T01:53:16Z

model/flow/version_beacon.go

+// NOTE: The above implies that, if the `ProtocolStateVersionUpgrade` event is sealed in a
+// block with view >= `ActiveView`, then `NewProtocolStateVersion` takes effect immediately.
+//
+// Otherwise, the node software stops processing blocks, until it is manually updated


Side node:

A cmd-tool would be great which allows us to override the protocol version requirement when the node starts up (maybe as simple as --allowIncompatibleProtocol=True). Just in case we mess something up that we (more precisely a supermajority of the node operators) can manually recover the protocol.

If you agree, would you be so kind to create an issue please, so that we don't forget?

I don't think we should immediately make an issue, but it's worth considering. I will add it as a discussion point to the FLIP/discussion post (at the moment I can't edit the post...).

The reason I don't think we should necessarily pre-emptively do this:

Adding a flag that disable startup version checking would be a relatively small change, I think. Not having this in place preemptively would not be a big impediment to recovery.

If we do need this, my guess is that it would be necessary but not sufficient. We will need to evaluate how the different versions will interact, and possibly make some changes to the software in one or the other version, before recovering.

thanks for the explanation. Agree with all of your points.

model/convert/service_event.go

janezpodhostnik · 2024-03-05T15:36:15Z

model/flow/service_event.go

+// Event portion of a ServiceEvent into a specific typed structure.
+// Forwards errors from the underlying marshaller (treat errors as you would from eg. json.Unmarshal)
+func unmarshalWrapped[E any](b []byte, marshaller marshallerImpl) (*E, error) {
+	eventWrapper := serviceEventUnmarshalWrapper[E]{}


This is the only usage of the type serviceEventUnmarshalWrapper. It might be easier to read if the type is defined inside this function.

model/flow/service_event.go

model/flow/version_beacon.go

fvm/systemcontracts/system_contracts.go

Co-authored-by: Alexander Hentschel <[email protected]>

model/convert/service_event.go

durkmurder · 2024-03-07T13:18:51Z

model/convert/service_event.go

+	}
+
+	versionUpgrade, err := DecodeCadenceValue("ProtocolStateVersionUpgrade payload", payload,
+		func(cdcEvent cadence.Event) (flow.ProtocolStateVersionUpgrade, error) {


Can we simplify it a bit by returning a pointer? In the end we still convert to pointer. Not sure if the api of DecodeCadenceValue allows it though.

Good point - we can return pointer directly 👍. Done in ec6e878.

model/convert/service_event_test.go

fvm/systemcontracts/system_contracts.go

AlexHentschel · 2024-03-09T01:51:14Z

model/convert/service_event.go

+					foundFieldCount++
+					activeViewValue = cdcEvent.Fields[i]
+				}
+			}


This approach of "ignoring unexpected fields" is in my opinion a security vulnerability waiting to happen. We know precisely what we are expecting but accept huge set of binary-different messages. We have gone through great length in the networking layer that it de-duplicates binary identical messages and reduces the trust score of peers should they try to spam us with the same (binary) message repeatedly. But we allow to include extra garbage in the encoding, i.e. it becomes trivial for a node to send me 1 million times the same message, all looking different on a binary level to the networking layer ...
The malleability surface in the code base is quite similar in nature in my opinion.

I am ok with continuing the existing approach in this PR. Though a non-unique encoding scheme is just generally not suitable for a BFT system. It creates too many edge cases that we cannot reasonably anticipate and manage in my opinion. I would like us to move away from this as soon as possible.

cc @janezpodhostnik for visibility

AlexHentschel

Amazing work. Appreciate the detailed documentation, and specifications.

Added a few suggestions to further clarify a few details, but all concerns are addressed. Thank you.

model/flow/version_beacon.go

Co-authored-by: Alexander Hentschel <[email protected]>

durkmurder

Great stuff, looking forward incorporating those changes into upgrade framework.

jordanschalm added 5 commits February 20, 2024 12:20

add proto state version upgrade event

635f6e2

docs, remove unused funcs

5a1cc20

make (un)marshal funcs private

acb426d

add service event to fvm

8d1ed7b

AlexHentschel reviewed Feb 21, 2024

View reviewed changes

model/flow/service_event.go Outdated Show resolved Hide resolved

jordanschalm added 2 commits February 21, 2024 15:43

wip

f8d1ce8

complete event conversion function

a02c986

jordanschalm added 3 commits February 22, 2024 08:31

remove unused fixtures

95d9108

add fixtures, encode test for new event

761fb39

change location of new event to VersionBeacon

33ee6c6

jordanschalm commented Feb 22, 2024

View reviewed changes

fvm/systemcontracts/system_contracts.go Outdated Show resolved Hide resolved

jordanschalm added 2 commits February 22, 2024 08:49

cleanup, docs

920c0da

notes about errors from marshalling funcs

82f7a39

jordanschalm changed the title ~~Jordan/5351 upgrade service event~~ [KV Store] ProtocolStateVersionUpgrade Service Event Feb 22, 2024

jordanschalm added 6 commits February 22, 2024 09:13

service event conversion error docs

e74a4e3

lint

1df183a

lint

0407725

add active view field to upgrade event

5b1a2b9

update conversion

23f217c

remove debug logs

48d4cbb

jordanschalm marked this pull request as ready for review February 26, 2024 22:58

jordanschalm requested review from ramtinms and janezpodhostnik as code owners February 26, 2024 22:58

jordanschalm requested review from kc1116 and durkmurder and removed request for ramtinms February 26, 2024 22:58

jordanschalm mentioned this pull request Feb 28, 2024

[Cadence 0.x] Add ProtocolStateVersionUpgrade service event onflow/flow-core-contracts#411

Closed

AlexHentschel reviewed Mar 5, 2024

View reviewed changes

janezpodhostnik reviewed Mar 5, 2024

View reviewed changes

Apply suggestions from code review

05c2c30

Co-authored-by: Alexander Hentschel <[email protected]>

durkmurder reviewed Mar 7, 2024

View reviewed changes

model/convert/service_event.go Outdated Show resolved Hide resolved

durkmurder reviewed Mar 7, 2024

View reviewed changes

model/convert/service_event_test.go Show resolved Hide resolved

jordanschalm added 6 commits March 7, 2024 12:02

catch error in event conversion

e725e5e

review comments for model/convert/service_event.go

a996b12

service event wrapper cleanup

a2437c5

update SafetyThreshold documentation

c952e18

minor docs

b5cafcb

use ptr in service event conversion

ec6e878

AlexHentschel reviewed Mar 9, 2024

View reviewed changes

AlexHentschel approved these changes Mar 9, 2024

View reviewed changes

model/flow/version_beacon.go Outdated Show resolved Hide resolved

model/flow/version_beacon.go Outdated Show resolved Hide resolved

model/flow/version_beacon.go Outdated Show resolved Hide resolved

Apply suggestions from code review

334aaf8

Co-authored-by: Alexander Hentschel <[email protected]>

This was referenced Mar 11, 2024

Use more restrictive decode mode in global CBOR Decoder #5534

Open

Modify Service Event encoding to be compatible with CBOR "strict" mode #5535

Open

durkmurder approved these changes Mar 11, 2024

View reviewed changes

jordanschalm merged commit 4d20066 into feature/protocol-state-kvstore Mar 11, 2024
50 of 51 checks passed

jordanschalm deleted the jordan/5351-upgrade-service-event branch March 11, 2024 21:01

jordanschalm mentioned this pull request Apr 8, 2024

[Cadence 1.0] Add ProtocolStateVersionUpgrade service event onflow/flow-core-contracts#419

Closed

2 tasks

franklywatson linked an issue Apr 26, 2024 that may be closed by this pull request

[Dynamic Protocol State | M2] KV Store upgrade event #5351

Closed

jordanschalm mentioned this pull request Oct 28, 2024

Add ProtocolStateVersionUpgrade service event onflow/flow-core-contracts#464

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KV Store] `ProtocolStateVersionUpgrade` Service Event #5428

[KV Store] `ProtocolStateVersionUpgrade` Service Event #5428

jordanschalm commented Feb 21, 2024 •

edited

Loading

codecov-commenter commented Feb 22, 2024 •

edited

Loading

AlexHentschel left a comment •

edited

Loading

AlexHentschel Mar 5, 2024

jordanschalm Mar 7, 2024

AlexHentschel Mar 9, 2024 •

edited

Loading

AlexHentschel Mar 9, 2024

jordanschalm Mar 11, 2024 •

edited

Loading

AlexHentschel Mar 11, 2024 •

edited

Loading

jordanschalm Mar 12, 2024

AlexHentschel Mar 5, 2024

jordanschalm Mar 7, 2024

AlexHentschel Mar 9, 2024

janezpodhostnik Mar 5, 2024

durkmurder Mar 7, 2024

jordanschalm Mar 7, 2024

AlexHentschel Mar 9, 2024 •

edited

Loading

AlexHentschel left a comment

durkmurder left a comment

[KV Store] ProtocolStateVersionUpgrade Service Event #5428

[KV Store] ProtocolStateVersionUpgrade Service Event #5428

Conversation

jordanschalm commented Feb 21, 2024 • edited Loading

Other Changes

codecov-commenter commented Feb 22, 2024 • edited Loading

Codecov Report

AlexHentschel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel Mar 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

AlexHentschel Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Side node:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel Mar 9, 2024 • edited Loading

Choose a reason for hiding this comment

AlexHentschel left a comment

Choose a reason for hiding this comment

durkmurder left a comment

Choose a reason for hiding this comment

[KV Store] `ProtocolStateVersionUpgrade` Service Event #5428

[KV Store] `ProtocolStateVersionUpgrade` Service Event #5428

jordanschalm commented Feb 21, 2024 •

edited

Loading

codecov-commenter commented Feb 22, 2024 •

edited

Loading

AlexHentschel left a comment •

edited

Loading

AlexHentschel Mar 9, 2024 •

edited

Loading

jordanschalm Mar 11, 2024 •

edited

Loading

AlexHentschel Mar 11, 2024 •

edited

Loading

AlexHentschel Mar 9, 2024 •

edited

Loading