RFC for account deletion #51
Conversation
| new insertions | ||
|
|
||
| ``` | ||
| H(H(A,B),H(C,D)) free = [[Right; Left]] |
There was a problem hiding this comment.
Indexes are already dense, shouldn't we use those?
There was a problem hiding this comment.
Do you mean using the Bigstring view of the same information, that is, Merkle_address ?
There was a problem hiding this comment.
The actual implementation will use Location.t, which may or may not be a compact representation since we are within the functor's body. The current instance of this functor does use a compact representation for indexes. I chose the Right, Left way of wording the location because I thought it was easier to follow, especially in a tree-like drawing.
- more precision in Merkle tree explanation - expand upond transaction logic
- more precision in Merkle tree explanation
There was a problem hiding this comment.
Can you update this to be in line with the template? More comments to follow
00xx-account-deletion.md
Outdated
| A B C D | ||
| ``` | ||
|
|
||
| Upon removal of `C`, the structure would evolve as follows, with `x` a |
There was a problem hiding this comment.
This is the only mention of this placeholder value. The details here seem important, e.g. we don't want a deleted account to continue to use the same amount of memory, otherwise we shouldn't refund the creation fee.
There was a problem hiding this comment.
Indeed. Let me be more clear here.
There was a problem hiding this comment.
Would you maybe prefer also to use symbol o in place of x since it actually is meant to mark the empty account
|
|
||
| The support for deletion at the ledger level now needs to be lifted within the | ||
| transaction logic for account updates. | ||
| The key idea here is to handle removal as a specific kind of account update for |
There was a problem hiding this comment.
How does the transaction logic differ from what's present today? E.g.
- How should the API to signal that we're deleting an account look?
- How should the native version of that work?
- How should the snark version of that work?
- What happens in the case of failure? Is it already handled?
- How is the global supply of Mina tracked faithfully?
|
|
||
| To illustrate option 1, let us assume we have the following Merkle tree of depth | ||
| 2, with an empty free list. This Merkle tree has one location marked with `x` - | ||
| this marks the empty account. |
There was a problem hiding this comment.
I don't think this is the design we want in practice: if we just replace it with the empty account, we don't actually save any space. We would really like to delete the account entirely (at least, on disk).
There was a problem hiding this comment.
I see your point. We don't really need to store the empty account though, we just need to act as if it were stored in the leaf. I will rephrase that bit.
In response to #51 (comment)
- in response to #51 (comment)
A more thorough discussion about where storing the free list on-disk might fail.
00xx-account-deletion.md
Outdated
|
|
||
| Deleting an account $a₀$ results in two actions: | ||
| 1. Actual removal of account $a₀$ of the storage layer; | ||
| 2. Return of balance of MINA tokens from $a₉$ and initial creation fee to a specified $a₁$ account. |
There was a problem hiding this comment.
This isn't how we want it in practice. Instead, we want to return the balance to the 'floating balance' for the zkApp txn, and the rest of the txn logic handles the other account. We should not specify the a_1 account (and indeed that might be several accounts in actual usage).
There was a problem hiding this comment.
What @mrmr1993 says here is correct. Luckily, it's very easy (in fact, easier than what is described in the RFC as of now): you just need to track the initial creation fee in the fee excess, no other changes will be required.
There was a problem hiding this comment.
Updated the document to reflect this discussion
00xx-account-deletion.md
Outdated
| [bheap](https://opam.ocaml.org/packages/bheap/) or | ||
| [CCHeap](https://c-cube.github.io/ocaml-containers/last/containers/CCHeap/index.html)) | ||
| for this implementation, that is, the free list is an ordered set of locations, | ||
| from which we always allocate the biggest one. |
There was a problem hiding this comment.
There are 2 things in conflict here: coalescing the free space to the right while also inserting to the left to keep it dense. You've addressed one, I was addressing the other. Please talk to both in the RFC. It seems as though either option probably require us to deserialise the whole list (unless we have head and bottom peeking).
Explictly state what we gain when permitting batch allocations.
rbonichon
force-pushed
the
account-deletion
branch
from
85b5f51
to
b597847
Compare
| 5. reserialize the new value of the heap. | ||
|
|
||
| **Simple improvement on batch allocations**. Furthermore, we will implement a | ||
| decoding function where one can specify how many locations we ideally want to |
There was a problem hiding this comment.
I agree, but at a certain point it's simpler to deserialise the whole list, take as many locations as are needed, push any new locations onto the list, and then reserialise the final list.
Update the text to try to answer comments in #51 (comment)
This document describes the account deletion feature, its motivation and proposed solution as well as possible culprits.