ZCF registerFeeMint fails reincarnation

[turadg]

Describe the bug

If a contract calls registerFeeMint then in its reincarnation (restartContract) it will fail with the following error,

Error#1: no kind info for 21 ( o+d21/1 ); check deserialize preceeding kind definitions

To Reproduce

git checkout 7466-psm-upgrade
cd packages/inter-protocol
yarn test test/swingsetTests/psmUpgrade/

It works before restart,

About to registerFeeMint {
  feeMintAccess: Object [Alleged: FeeMint feeMintAccess] {},
  initialPoserInvitation: Object [Alleged: Zoe Invitation payment] {},
  marshaller: Object [Alleged: Board readonlyMarshaller] {},
  storageNode: Object [Alleged: ChainStorageNode] {}
}
----- BootPSMUpg.3  5 BOOT buildV1 started instance
----- BootPSMUpg.3  6 testFunctionality1

but fails in restart,

----- BootPSMUpg.3  7 BOOT nullUpgradeV1 start
----- BootPSMUpg.3  8 BOOT nullUpgradeV1 upgradeContract
UnhandledPromiseRejectionWarning: (Error#1)
Nested error under Error#1
Error#1: no kind info for 21 ( o+d21/1 ); check deserialize preceeding kind definitions
…
  at makeZCFMintFactory (.../zoe/src/contractFacet/zcfMint.js:167:11)
  at initSeatMgrAndMintFactory (.../zoe/src/contractFacet/zcfZygote.js:335:22)
  at Object.restartContract (.../zoe/src/contractFacet/zcfZygote.js:420:7)

Note the ZCF fix that makes this problem detected sooner: 7e3986a

Expected behavior

Contracts using registerFeeMint are restartable

[warner]

Here's the sequence of VRM registerKind() and reanimate() calls during the "perform null upgrade" portion of that test:

  # [v9] registerKind(1)
UnhandledPromiseRejectionWarning: (Error#1)
  # [v9] registerKind(2)
  # [v9] registerKind(3)
  # [v9] registerKind(4)
  # [v9] registerKind(5)
  # [v9] registerKind(6)
  # [v9] registerKind(7)
  # [v9] registerKind(8)
  # [v9] registerKind(9)
   # [v9] reanimate(o+d6/1) true
   # [v9] reanimate(o+d6/3) true
   # [v9] reanimate(o+d6/4) true
   # [v9] reanimate(o+d1/10) true
  # [v9] registerKind(10)
   # [v9] reanimate(o+d8/5) true
   # [v9] reanimate(o+d1/11) true
  # [v9] registerKind(11)
   # [v9] reanimate(o+d1/12) true
  # [v9] registerKind(12)
   # [v9] reanimate(o+d1/13) true
  # [v9] registerKind(13)
   # [v9] reanimate(o+d7/6) true
   # [v9] reanimate(o+d7/7) true
   # [v9] reanimate(o+d1/14) true
  # [v9] registerKind(14)
   # [v9] reanimate(o+d7/8) true
   # [v9] reanimate(o+d1/15) true
  # [v9] registerKind(15)
   # [v9] reanimate(o+d1/16) true
  # [v9] registerKind(16)
   # [v9] reanimate(o+d1/17) true
  # [v9] registerKind(17)
   # [v9] reanimate(o+d17/1) true
   # [v9] reanimate(o+d6/9) true
   # [v9] reanimate(o+d15/1) true
   # [v9] reanimate(o+d7/10) true
   # [v9] reanimate(o+d6/11) true
   # [v9] reanimate(o+d7/12) true
   # [v9] reanimate(o+d1/18) true
  # [v9] registerKind(18)
   # [v9] reanimate(o+d1/19) true
  # [v9] registerKind(19)
   # [v9] reanimate(o+d19/1) true
   # [v9] reanimate(o+d8/13) true
   # [v9] reanimate(o+d1/20) true
  # [v9] registerKind(20)
   # [v9] reanimate(o+d20/1) true
   # [v9] reanimate(o+d21/1) false

It looks like something in the buildRootObject is attempting to deserialize (reanimate) an instance of Kind 21 before it performs a defineDurableKind for that ID.

I think the "check deserialize preceeding kind definitions" error message should cite defineDurableKind, to make it more clear what userspace needs to do differently.

If this test lends itself to a debugger, try putting a breakpoint on calls to defineDurableKind and let's figure out the relative ordering of them.

[warner]

note to self: I am seeing an unexpected reanimateDurableKindID of Kind 10 (which was registered with a tag of SeatHandle) during the initial incarnation, meaning something deserialized the KindHandle vref and didn't find the handle already present in valToSlot. It's unexpected because we hold the KindHandles strongly, as keys of kindHandleToID, so that incarnation should have created the handle with makeKindHandle and then never dropped it: so how did deserialization fail to find the pre-existing Representative?

(nevermind, I misread my logs)

[warner]

Ok, so I think there's an initialization inversion in zcfMint.js. Here's the story:

zcfMint.js manages a factory of mints. Each mint has a Keyword. The factory and all mints are durable. They all live inside the contract vat, as part of ZCF.

We have one Kind for the factory (of which there is only ever one instance), and another Kind for each mint/keyword (of which there is only one instance per keyword). So call those the 1+N Kinds

Mints are created at runtime, some arbitrarily long time after the contract vat is first created.

When the contract is upgraded, we must re-initialize (call defineDurableKind for) all 1+N Kinds. We must re-initialize each Kind before deserializing any instances of that Kind: that's the SwingSet rule. The error message printed above (check deserialize preceeding kind definitions) indicates the rule has been violated.

The top-level export of zcfMint.js is a function named makeZCFMintFactory(zcfBaggage), which creates a "ZCF Mint Factory", which keeps its state in three-ish keys of zcfBaggage:

• zcfBaggage.get('baggageSet') holds a durable Set, known to the code as a variable named zcfMintBaggageSet
• zcfMintFactory and some related key are used for the prepareExo (one key for the Kind handle, a second for the single instance of that kind), whose maker function is known to the code as zcfMintFactory

Each time someone calls zcfMintFactory.makeZCFMintInternal(), it creates a new zcfMintBaggage (one per keyword), uses it in a call to makeDurableZcfMint(), and stores the resulting mint in zcfMintBaggageSet. The zcfMintBaggage itself is closed over by the zcfMint, but not stored anywhere, which is weird.

makeDurableZcfMint() uses prepareSingleton to define a new Kind (whose tag is zcfMint), and then make an instance of that kind. zcfMintBaggage will hold both the Kind Handle and the instance.

The end of makeZCFMintFactory() has a loop which wants to re-define all Kinds, but the first incarnation doesn't have anything to loop over yet:

  for (const zcfMintBaggage of zcfMintBaggageSet.values()) {
    provideDurableZcfMint(zcfMintBaggage);
  }

Baggage contents after ZCF initialization

When makeZCFMintFactory finishes, our baggage tree will look like:

• zcfBaggage
  • [baggageSet] = DurableSetStore
  • [zcfMintFactory_kindHandle] = KindHandle(zcfMintFactory)
  • [the_zcfMintFactory] = KindHandle(zcfMintFactory)

Baggage contents after first mint created

After someone calls zcfMintFactory.makeZCFMintInternal() for keyword A, the tree will look like:

• zcfBaggage
  • [baggageSet] = DurableSetStore
    • ZCFMint (for A), an instance of Kind 'zcfMint' (for A)
      • this closes over a zcfMintBaggage (for A), with the following keys:
        • keyword: keyword (probably "A")
        • zoeMint: zoeMint (maybe specific to A)
        • zcfMint_kindHandle = KindHandle(zcfMint) (for A)
        • the_zcfMint = ZCFMint (for A)
  • [zcfMintFactory_kindHandle] = KindHandle(zcfMintFactory)
  • [the_zcfMintFactory] = KindHandle(zcfMintFactory)

Baggage contents after second mint created

Then after someone calls it again for keyword B, we get:

• zcfBaggage
  • [baggageSet] = DurableSetStore
    • ZCFMint (for A), an instance of Kind 'zcfMint' (for A)
      • this closes over a zcfMintBaggage (for A), with the following keys:
        • keyword: keyword (probably "A")
        • zoeMint: zoeMint (maybe specific to A)
        • zcfMint_kindHandle = KindHandle(zcfMint) (for A)
        • the_zcfMint = ZCFMint (for A)
    • ZCFMint (for B), an instance of Kind 'zcfMint' (for B)
      • this closes over a zcfMintBaggage (for B), with the following keys:
        • keyword: keyword (probably "B")
        • zoeMint: zoeMint (maybe specific to B)
        • zcfMint_kindHandle = KindHandle(zcfMint) (for B)
        • the_zcfMint = ZCFMint (for B)
  • [zcfMintFactory_kindHandle] = KindHandle(zcfMintFactory)
  • [the_zcfMintFactory] = KindHandle(zcfMintFactory)

Upgrde / Kind redefinition

Now an upgrade happens, and our loop wants to redefine everything:

  for (const zcfMintBaggage of zcfMintBaggageSet.values()) {
    provideDurableZcfMint(zcfMintBaggage);
  }

The .values() deserializes the first entry of zcfMintBaggageSet, which should be a ZCFMint (for A). But it cannot deserialize that until the Kind has been redefined, which won't happen until provideDurableZcfMint is called.

Problems

Based on the name, I think zcfMintBaggageSet was meant to hold the zcfMintBaggage. But it looks like it's storing the zcfMint objects themselves. If makeZCFMintInternal were changed from:

      async makeZCFMintInternal(keyword, zoeMint) {
        const zcfMintBaggage = makeScalarBigMapStore('zcfMintBaggage', {
          durable: true,
        });
        const zcfMint = await makeDurableZcfMint(
          keyword,
          zoeMint,
          zcfMintBaggage,
        );
        zcfMintBaggageSet.add(zcfMint); // <-- HERE
        return zcfMint;
      },

to:

        zcfMintBaggageSet.add(zcfMintBaggage); // <-- HERE

then the post-upgrade sequence would be:

• deserialize zcfMintBaggage (for A), which produces a DurableMapStore
• provideDurableZcfMint() extracts keyword and zoeMint from it
  • it then calls prepareSingleton
    • which extracts zcfMint_kindHandle, and does defineDurableKind with it
    • then extracts the_zcfMint from zcfMintBaggage, which works because the Kind is defined already

Other Notes

• provideKindHandle uses kindName as a prefix in the baggage, but prepareExo uses kindName as a suffix. This could lead to collisions, especially if kindName is equal to the or kindHandle. They should be changed to use the same pattern.
• the comments on zcfMintFactory claim that the zoeMint argument is "the promise returned by a makeZoeMint() call"
  • but zoeMint is stored in a DurableMapStore, which means it cannot be a promise