collections permit Presences/vrefs in keyShape/valueShape, but doesn't refcount them

[warner]

Describe the bug

Our virtual/durable collections (i.e. makeScalarBigMapStore) accept optional keyShape and valueShape patterns, to restrict the kinds of keys and values that can be stored in each one. The patterns are defined with a constraint language, with terms both both categories of data (M.number() accepts any Number), and specific values (using 4 in a pattern will only accept the value 4, no other number). There is an M.remotable(), which matches things like (imported) Presences and exported objects created with Far, or virtual/durable Representatives. And if you include a specific Remotable in the pattern, then (apparently) that will only match against the specific instance.

import { matches, M } from '@agoric/store';
const thing1 = Far('thing', {});
const thing2 = Far('thing', {});
const pattern1 = M.remotable();
const pattern2 = thing1;
matches(thing1, pattern1); // true
matches(thing2, pattern1); // true
matches(thing1, thing1); // true
matches(thing1, thing2); // false

A specific Remotable could be used in a keyShape or valueShape too:

const type1 = [thing1, M.number()];
const type2 = [thing2, M.string()];
const valueShape = M.or(type1, type2);
const store = makeScalarBigMapStore('map', { valueShape });

However, using a specific Remotable like that is going to be problematic. Both keyShape and valueShape are serialized and stored in a special per-collection vatstore key named |schemata, so that later vat incarnations (or later reanimations of the collection within the same vat incarnation) will be subject to the constraints as well. But if the shape had Remotables (Presences, Far()-generated ephemerals, or virtual/durable Representatives), the serialized form will have a vref, and we don't perform refcounting on those vrefs like we do for virtual-object state or collection entries.

Attempting to unserialize the schemata later might or might not find the corresponding object to be present in slotToVal. If the object was ephemeral, and it had been garbage-collected since the last usage, the unserialize will fail.

This was less obvious before, because the schemata was held in RAM for the lifetime of the incarnation, so it would only show up in a post-upgrade access to the Remotable-in-valueShape-using collection. We didn't really acknowlege the high cardinality of collections at first, and have places where we hold per-collection data in RAM when it really ought to live primarily in the DB. One consequence of this is a syscall sensitivity to GC of collections, which is being addressed in #6360. That will cause more frequent unserialization of |schemata, making this problem become visible within even the current vat incarnation.

The Fix

collectionManager.js should assert that serializedSchema.slots.length === 0 while creating the collection the first time.

As this means serializedSchema.body contains the complete description of the schema, I'm tempted to go further and only record .body, rather than our current JSON.stringify(serializedSchema) (including both .body and .slots). Then, on restore, do unserialize({ body: vatstoreGet(..|schemata), slots: [] }). That would make the saved data slightly smaller and simpler, but also completely excludes the possibility of unserializing vrefs in this context. Doing this would cause us backwards-compatibiltiy problems if we ever decided to find a way to lift this restriction, though.

[warner]

cc @erights

[warner]

@erights reports that we need this, "For example, the ledger uses an AmountPattern with the brand being the specific brand for that currency", and #3167 / #6432 depend upon it. Indeed, when I implemented the prohibition, I got a unit test failure from packages/ERTP/basicFunctionality/test-basicFunctionality.js › test splitPayments, among others.

To fix it properly, we'll need to increment the vom.rc.${baseref} counters when creating the collection, and decrement them again when the collection is deleted. We have helper functions in virtualReferenceManager.js to do this.

The creation of the collection happens in response to a specific userspace action (it calls makeScalarBigMapStore or similar). However the deletion does not, and we must make sure that the vatstore writes only happen during the BOYD pass. I think is equivalent to making sure that deleteCollection() is only called from that context, and I'm guessing that the fact that it's registered with vrm.registerKind() will satisfy that requirement, but we'll need to double-check.