Remove children from AccountUpdate: RFC and Implementation

[mitschabaude]

On top of #1384, polish up the system introduced there for keeping track of zkapp children, use it everywhere, and rip out the previous system.

This comes with a proposal to update or remove some existing public APIs:

RFC: Refactor representation of account update layout during transaction construction

Motivation: It was a mistake to store children on account updates. Doing that entangles account updates to the specific context of a transaction, when really they should be an atomic type that can be passed between different contexts. It's cleaner if a separate, higher-level structure stores the parent-child relationships.

Remove AccountUpdate.children and AccountUpdate.parent

• This PR makes the breaking change of removing both of these properties from AccountUpdate.

New APIs to hold the transaction layout

Since AccountUpdate no longer holds information about the transaction layout, we need new APIs as a replacement.

• AccountUpdateTree and AccountUpdateForest are the new public types to represent account updates together with their children:

type AccountUpdateTree = { accountUpdate: Hashed<AccountUpdate>; children: AccountUpdateForest };
type AccountUpdateForest = MerkleList<AccountUpdateTree>;

• approve() should accept these as input to help construct the tree below an update:

AccountUpdate.approve(input: AccountUpdate | Hashed<AccountUpdate> | AccountUpdateTree | AccountUpdateForest): void;

• I think we no longer need the second argument to approve(), an AccountUpdateLayout, to specify the layout of children to be witnessed. Removing it will get rid of some complex code, and it should be superseded by operation on types like AccountUpdateForest which represent a dynamic layout of children

New API to have a token contract approve the results of a zkApp call

Previously, an important pattern was to call a zkApp and after that take its account update off of zkApp.self, and pass that to a token contract to approve. I.e.

// inside caller method
callee.someMethod();
tokenContract.approveAccountUpdate(callee.self);

This ended up creating a tree like the following:

caller
   |
token contract
   |
callee
   |
... callee's children ...

I called this the "manual callback" flow. It was equivalent to passing an Experiment.Callback with the callee method to the token contract.

• With the proposed changes, callee.self no longer contains the callee's children, so we need a new API to obtain them. Maybe something like this:

// inside caller method
callee.someMethod();
tokenContract.approveAccountUpdate(callee.extractTree());

Where extractTree() collects the AccountUpdateTree of the callee and its children, and explicitly causes them from being disattached from their current location in the transaction (previously, we had a hack that disattached account updates when they are passed to another method).

SmartContract.extractTree(): AccountUpdateTree;

Side note: Of course it would be MUCH nicer to simply get the tree produced by a method as the result of the method call. But we can't have this since we can't change method signatures with decorators (even though we can change their implementation) -- except if we require every method to explicitly return its resulting tree. Not a bad idea IMO but out of scope of v1 for sure.

Remove redundant Callback API

Ever since #428, I didn't like the Experimental.Callback API because it's redundant with, and less explicit than, the approach described above. I propose to remove it. Also, remove Experimental.createChildAccountUpdate (redundant with AccountUpdate.create()).

This is pretty much orthogonal to the rest of this RFC and will go into a separate PR.

Internal changes

Internally, #1384 introduced a global UnfinishedForest structure which holds a zkApp's children during the run of a smart contract method, and is updated by operations like this.send(). After the zkApp method runs, the UnfinishedForest is "finalized" to an AccountUpdateForest, whose hash is compared with the public input.

This PR pulls the trigger to actually use this system, instead of the old system based on AccountUpdate.children, to compute the public input.

Similarly, a top-level UnfinishedForest is maintained during Mina.transaction(), and finalized into a flat list AccountUpdate[] with call-depths, which is the source of truth of the overall transaction contents.

TODOs

• fix integration tests after making UnfinishedForest responsible for hashing
• figure out reasons for the scary amount of constraint reductions
  • I didn't figure out the reason, but after getting everything to work the constraints are back at expected levels
• actually get rid of children
• API that replaces obtaining .self and passing it to a token contract - idea: have a method return an AccountUpdateTree as the new canonical type for an update + its children, which are the result of a zkapp call

[mitschabaude]

Note: I plan to add a nicer API on top of extractTree(), but will leave that to a follow-up PR which also removes callbacks

[MartinMinkov]

Lots of code in this change 🎉 Question: if one was to forget to call approve on a generated AU, what happens? Is there an error that will show up when proving?

In a case like this:

let tree = AccountUpdateTree.from(payerUpdate);
tree.approve(receiverUpdate);
this.approve(tree);

If you forget to call tree.approve or this.approve, what happens?

[MartinMinkov]

Do you think it would be helpful to print the ID of the AU that wasn't found? Not sure if it would actually be helpful for debugging 🤷

[mitschabaude]

Printing the label would probably help! I use labels all the time these days for debugging

[mitschabaude]

I'm leaving this out for now since it's definitely not an error I expect will be encountered - we can improve the message if someone hits this

[mitschabaude]

If you forget to call tree.approve or this.approve, what happens?

Great question!

For AccountUpdate, there are different ways to create them, some automatically attach the update to the transaction at the expected location (those that we generally highlight, like AccountUpdate.create()), and some just create an update which doesn't end up anywhere without approving (lower level methods, like new AccountUpdate() or AccountUpdate.defaultAccountUpdate()).

In the case of AccountUpdateTree and AccountUpdateForest, I think of them as being more similar to the lower-level, explicit methods on account updates, used by advanced people that want to control the tx layout precisely. That's why both of these are not put anywhere by default.

I was going for intuitive behaviour in the sense that approving one update explicitly overrides whatever we did with it automatically before that. So in the mentioned code example, the following happens at each step (see comments):

@method depositUsingTree() {

  let payerUpdate = AccountUpdate.createSigned(this.sender);
  // now `payerUpdate` is a child of the `depositUsingTree()` zkapp AU

  let receiverUpdate = AccountUpdate.create(this.address);
  // now also `receiverUpdate` is a child of the zkapp
  
  payerUpdate.send({ to: receiverUpdate, amount: UInt64.one });
  // nothing happened here, send doesn't change the layout when we pass AUs

  let tree = AccountUpdateTree.from(payerUpdate);
  // a tree was created but nothing else happened. `payerUpdate` is still a child of the zkapp
  
  tree.approve(receiverUpdate);
  // `payerUpdate` is still a child of the zkapp, and `receiverUpdate` is disattached (but part of the tree)
  // that's because `approve` overrides the previous attachment of `receiverUpdate`
  // and the tree wasn't attached anywhere yet
  
  this.approve(tree);
  // this explicitly puts the tree, containing `payerUpdate` under the zkapp, without duplicating the existing `payerUpdate` at the same level
  // so we now have `zkapp` parent of `payerUpdate` parent of `receiverUpdate`
}

Note that the end result wouldn't change if we had created payerUpdate and receiverUpdate in ways that wouldn't have attached them already.