Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgraded stateShape may be incompatible with existing state #7337

Closed
Tracked by #7588
mhofman opened this issue Apr 5, 2023 · 9 comments
Closed
Tracked by #7588

Upgraded stateShape may be incompatible with existing state #7337

mhofman opened this issue Apr 5, 2023 · 9 comments
Labels
bug Something isn't working endo liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet

Comments

@mhofman
Copy link
Member

mhofman commented Apr 5, 2023

Describe the bug

An ExoClass accept a stateShape argument that validates the state at get/set (including init).

If a new incarnation provides a stateShape that is incompatible with the state created in previous incarnations, the existing instances will fail when they try to read their state.

Expected behavior

Rewiring the kinds with stateShape checks should enforce that the new stateShape is "compatible" with the previous one, aka that all existing states pass the new stateShape check. If incompatible, redefining the ExoClass should fail.

I'm not sure if removing a stateShape is possible, but if it is, it should be the equivalent to setting the new stateShape to M.any(), aka any further restriction is impossible.

@mhofman mhofman added bug Something isn't working SwingSet package: SwingSet endo liveslots requires vat-upgrade to deploy changes labels Apr 5, 2023
@mhofman
Copy link
Member Author

mhofman commented Apr 5, 2023

@erights suggests that the Exo class definition could take an upgradeHook along stateShape, which is called when:

  • The representative is first recreated (which must be a non gc sensitive event), and
  • The instance's state correspond to a previous stateShape, and
  • The instance's state does not match against the new stateShape

This would require keeping track of which stateShape each instance was last associated with, either individually or by segmenting the VO definitions by incarnations.

This also implies that upgradeHook would have to support reading from any previous state, not just the N-1 state shape. This is basically the centralized equivalent of the current status quo where behavior methods must handle state from previous incarnations if no shape checks exist.

@erights
Copy link
Member

erights commented Apr 5, 2023

Since we can test whether two stateShape patterns are literally identical, rather than segmenting VO storage by incarnation, I suggested (and prefer) to segment it by distinct stateShape. But this is probably not an observable difference anyway.

In order to compress a given instance by the stateShape that was current when the instance was last stored, we'd need such segmentation anyway.

Only durable kinds need support either segmentation or upgradeHook. Virtual-non-durable kinds are single incarnation anyway, so the issue does not arise.

@erights
Copy link
Member

erights commented Apr 5, 2023

Also suggest that similar schema upgrade concerns will apply to change of keyShape or valueShape for the durable stores.

Similarly, only durable stores need support for such schema change. Virtual-non-durable stores are single incarnation anyway, so the issue does not arise.

warner added a commit that referenced this issue Apr 9, 2023
Save a serialized copy of the Kind's `stateShape` option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337
warner added a commit that referenced this issue Apr 10, 2023
Save a serialized copy of the Kind's `stateShape` option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337
warner added a commit that referenced this issue Apr 10, 2023
Save a serialized copy of the Kind's `stateShape` option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337
warner added a commit that referenced this issue Apr 10, 2023
Save a serialized copy of the Kind's `stateShape` option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337
warner added a commit that referenced this issue Apr 11, 2023
Save a serialized copy of the Kind's `stateShape` option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337
warner added a commit that referenced this issue Apr 12, 2023
Save a serialized copy of the Kind's `stateShape` option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337
warner added a commit that referenced this issue Apr 12, 2023
When a vat upgrade provides a new definition for a pre-existing
durable Kind, it can supply a `stateShape` option that might not be
compatible with one established by the previous incarnation.

The long-term issue is how to manage "schema upgrades", as the
permissible/desired shape of the virtual-object `state` data changes
over versions. For now, our main concern is that userspace doesn't
create a situation where reading a `state` property causes an error,
because the new `stateShape` rejects values that were recorded by an
earlier version.

The short-term fix is to insist that each new incarnation defines the
Kind with exactly the same `stateShape` as its predecessor. This check
is performed by comparing the serialized/marshalled capdata of
`stateShape` against that of the earlier version, which is convenient
but overly strict (e.g. the properties must be defined in the same
order).

If violated, the new-version `buildRootObject` will throw an error as
it calls `defineDurableKind`.

refs #7337
warner added a commit that referenced this issue Apr 12, 2023
Save a serialized copy of the Kind's `stateShape` option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337
warner added a commit that referenced this issue Apr 12, 2023
When a vat upgrade provides a new definition for a pre-existing
durable Kind, it can supply a `stateShape` option that might not be
compatible with one established by the previous incarnation.

The long-term issue is how to manage "schema upgrades", as the
permissible/desired shape of the virtual-object `state` data changes
over versions. For now, our main concern is that userspace doesn't
create a situation where reading a `state` property causes an error,
because the new `stateShape` rejects values that were recorded by an
earlier version.

The short-term fix is to insist that each new incarnation defines the
Kind with exactly the same `stateShape` as its predecessor. This check
is performed by comparing the serialized/marshalled capdata of
`stateShape` against that of the earlier version, which is convenient
but overly strict (e.g. the properties must be defined in the same
order).

If violated, the new-version `buildRootObject` will throw an error as
it calls `defineDurableKind`.

refs #7337
warner added a commit that referenced this issue Apr 12, 2023
Save a serialized copy of the Kind's `stateShape` option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337
warner added a commit that referenced this issue Apr 12, 2023
When a vat upgrade provides a new definition for a pre-existing
durable Kind, it can supply a `stateShape` option that might not be
compatible with one established by the previous incarnation.

The long-term issue is how to manage "schema upgrades", as the
permissible/desired shape of the virtual-object `state` data changes
over versions. For now, our main concern is that userspace doesn't
create a situation where reading a `state` property causes an error,
because the new `stateShape` rejects values that were recorded by an
earlier version.

The short-term fix is to insist that each new incarnation defines the
Kind with exactly the same `stateShape` as its predecessor. This check
is performed by comparing the serialized/marshalled capdata of
`stateShape` against that of the earlier version, which is convenient
but overly strict (e.g. the properties must be defined in the same
order).

If violated, the new-version `buildRootObject` will throw an error as
it calls `defineDurableKind`.

refs #7337
turadg pushed a commit that referenced this issue Apr 12, 2023
Save a serialized copy of the Kind's `stateShape` option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337
turadg pushed a commit that referenced this issue Apr 12, 2023
When a vat upgrade provides a new definition for a pre-existing
durable Kind, it can supply a `stateShape` option that might not be
compatible with one established by the previous incarnation.

The long-term issue is how to manage "schema upgrades", as the
permissible/desired shape of the virtual-object `state` data changes
over versions. For now, our main concern is that userspace doesn't
create a situation where reading a `state` property causes an error,
because the new `stateShape` rejects values that were recorded by an
earlier version.

The short-term fix is to insist that each new incarnation defines the
Kind with exactly the same `stateShape` as its predecessor. This check
is performed by comparing the serialized/marshalled capdata of
`stateShape` against that of the earlier version, which is convenient
but overly strict (e.g. the properties must be defined in the same
order).

If violated, the new-version `buildRootObject` will throw an error as
it calls `defineDurableKind`.

refs #7337
mergify bot pushed a commit that referenced this issue Apr 13, 2023
Save a serialized copy of the Kind's `stateShape` option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337
mergify bot pushed a commit that referenced this issue Apr 13, 2023
When a vat upgrade provides a new definition for a pre-existing
durable Kind, it can supply a `stateShape` option that might not be
compatible with one established by the previous incarnation.

The long-term issue is how to manage "schema upgrades", as the
permissible/desired shape of the virtual-object `state` data changes
over versions. For now, our main concern is that userspace doesn't
create a situation where reading a `state` property causes an error,
because the new `stateShape` rejects values that were recorded by an
earlier version.

The short-term fix is to insist that each new incarnation defines the
Kind with exactly the same `stateShape` as its predecessor. This check
is performed by comparing the serialized/marshalled capdata of
`stateShape` against that of the earlier version, which is convenient
but overly strict (e.g. the properties must be defined in the same
order).

If violated, the new-version `buildRootObject` will throw an error as
it calls `defineDurableKind`.

refs #7337
@warner
Copy link
Member

warner commented Apr 17, 2023

In #7407 I wrote up this upgrade process with a bit more detail, specifically for durable Kinds (perhaps we should merge the tickets?). In doing so, I came to the conclusion that we really should use a distinct version number of some kind, instead of relying only on shape changes. Please take a look at the arguments there and see if you agree.

@mhofman
Copy link
Member Author

mhofman commented Apr 17, 2023

I think these are not entirely mutually exclusive. If a user provides an existing version number, we need to check that the shape is the same as the one that was recorded for that version.

But besides that, I agree we would no longer need to compare the stateShape before and after to decide if it's different, and create a new segment. Only the user provided version would create new segments.

@warner
Copy link
Member

warner commented Apr 19, 2023

We landed the "stateShape must be identical" change in e47f456 as part of PR #7365.

Let's define this ticket to mean "accept different stateShape as long as it is a (non-strict) superset of the previous version".

If we implement #7407 upgrades first, then we don't need to implement this ticket:

  • when the new version uses a new version number, we'll accept entirely incompatible state shapes, and the user's migration function will be responsible for dealing with the changes
  • when the new version uses a pre-existing version number, we'll demand that the "new" stateShape is identical to the previous one

@mhofman
Copy link
Member Author

mhofman commented Apr 19, 2023

I would say that #7407 is the likely approach to be taken, and that the immediate concern with incompatible state shape changes has been satisfied in #7365, so this can be closed. We can always re-open if we decide not to do #7407 .

@gibson042
Copy link
Member

Specific need: adding/removing/changing an annotation-only Remotable/Promise matcher label—cf. endojs/endo#1933 (comment)

@warner
Copy link
Member

warner commented Jun 5, 2024

I'll agree that #7407 is the likely approach (merely relaxing the schema is harder to measure implement, and would make compression more difficult), and that we can close this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working endo liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet
Projects
None yet
Development

No branches or pull requests

4 participants