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

API for deleting StorageNodes #7405

Open
turadg opened this issue Apr 12, 2023 · 6 comments · May be fixed by #8063
Open

API for deleting StorageNodes #7405

turadg opened this issue Apr 12, 2023 · 6 comments · May be fixed by #8063
Assignees
Labels
enhancement New feature or request performance Performance related issues

Comments

@turadg
Copy link
Member

turadg commented Apr 12, 2023

What is the Problem Being Solved?

Once a StorageNode is made, there's no clear way to delete it from RPC. #5945 describes setValue(undefined) but that will only delete if sequence: set whereas the contracts all use sequence: append presently.

Description of the Design

An API method to delete the node.

A way to trigger that from a wrapping Recorder.

Security Considerations

Scaling Considerations

Test Plan

@turadg turadg added enhancement New feature or request performance Performance related issues labels Apr 12, 2023
@ivanlei ivanlei added the vaults_triage DO NOT USE label Apr 27, 2023
@gibson042
Copy link
Member

Question: Is there a need for recursive delete, or just a single storage node?

@mhofman
Copy link
Member

mhofman commented Sep 20, 2023

@michaelfig @gibson042, thinking about this, should a delete of a storage node with sequence: true just set a marker for deletion in the current block's cell, and (for optimization) we maintain a list of path that can be removed in the next block?

@gibson042
Copy link
Member

Ugh, that would bring in all kinds of complexity (e.g., write then delete then write again). I'd really prefer to keep stomping on any writes that precede delete().

@mhofman
Copy link
Member

mhofman commented Sep 20, 2023

I'm really concerned that contract code does not have the concept of what the block boundary is, and they may expect the write performed in a previous delivery, but executed in the same block, to be visible off-chain.

@michaelfig
Copy link
Member

Keep in mind that basically every write is accompanied by a Tendermint CometBFT event containing the write's data, which will be indexed even if the state is deleted in that block. We may need to update off-chain client libraries to use these events correctly, but then the worst that can happen is only if we relied on provable queries, which we don't.

I may be convinced to handle delete specially, but only if somebody presents clear requirements and semantics for how it should behave in the face of "write then delete then write again".

@mhofman
Copy link
Member

mhofman commented Sep 26, 2023

First, a clarification. My understanding is that vstorage has 2 features:

  • Provable update events
  • hierarchical storage with support for breadth first iteration

vstorage accomplishes the first by offering a sequence: true mode where all updates within a block are written combined (with tracking of the block height in which the updates were made). If multiple updates in a block are generated for a sequence: false node, the events may not be provable (is there a use case for generating events from sequence: false? maybe for any feature implementing its own concatenation?)

I believe that a node should not switch semantics over time. If a node has sequence: true, I assume the consumers would like to be able to always prove all update events. If we introduce delete for those nodes, it should similarly be provable.

I would expect that a deletion of a sequence: true node would actually record the deletion as a special value. My understanding is that currently an empty string value is currently not allowed for an append, so we could use the empty string value as a deletion marker for append nodes, or we could do the right thing and change the definition of StreamCell to Values []*string so that each value entry is null or a string when JSON encoded. Any existing cell will parse just fine with this new definition for the golang/chain side, but I'm less sure how casting clients would handle new cells with a null value.

Then there is the question of intent for a delete of an append node. I think that's where my previous observation comes in that we could optimize our storage cost by actually removing the cell in the subsequent block (if the last value of the cell is a deletion). From what I understand of the vstorage model, we could theoretically generalize this and always rewrite all cell nodes that were just updated the previous block to the latest value from the cell only (low level write, without events generated), but given how an IAVL DB works, that may actually be counter productive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance Performance related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants