New storage write API #2355
New storage write API #2355
Conversation
brentstone
force-pushed
the
brent/new-storage-write
branch
from
80f2ca1
to
0d5cfdc
Compare
brentstone
force-pushed
the
brent/new-storage-write
branch
4 times, most recently
from
b27bc60
to
5a6c6f8
Compare
brentstone
force-pushed
the
brent/new-storage-write
branch
from
c8bcb96
to
1798f81
Compare
brentstone
force-pushed
the
brent/new-storage-write
branch
2 times, most recently
from
03c5977
to
fd7e817
Compare
brentstone
force-pushed
the
brent/new-storage-write
branch
from
aaca999
to
7207603
Compare
brentstone
force-pushed
the
brent/new-storage-write
branch
from
71833cd
to
a668827
Compare
tzemanovic
force-pushed
the
brent/new-storage-write
branch
from
eea0a55
to
ef90ed1
Compare
|
|
||
| // If not persisting the diffs, remove the existing diffs from two block | ||
| // heights earlier than `height` | ||
| if !persist_diffs { |
There was a problem hiding this comment.
I don't think this works. This deletes a key with height height - 2. But say that we write a non-diff key at height 5 and than the next write happens at height 7, than we'll try to delete the keys 6/old/ and 6/new which don't exist. Instead we should be deleting the keys 5/old and 5/new. This can build up and we can end up with quite a few stale keys in storage
|
|
||
| // If not persisting the diffs, remove the existing diffs from two block | ||
| // heights earlier than `height` | ||
| if !persist_diffs { |
| batch.0.put_cf(cf, new_val_key, new_value); | ||
| } | ||
|
|
||
| // If not persisting the diffs, remove the last diffs. |
There was a problem hiding this comment.
Seems like the same code as above? Can we abstract this into a function?
There was a problem hiding this comment.
unfortunately they don't have the same interface - I think we'll want to get rid of the non-batch methods though
crates/state/src/lib.rs
Outdated
| } | ||
|
|
||
| /// Write with diffs but no merklization | ||
| pub fn write_without_merkl( |
| StorageModification::Write { | ||
| ref value, | ||
| action: _, | ||
| } => len as i64 - value.len() as i64, |
There was a problem hiding this comment.
What happens here if value.len() > len? Do we charge negative gas? Is this intended?
There was a problem hiding this comment.
This is only used for size_diff and we don't do anything with it on the caller side(s). We don't subtract from gas cost
| StorageModification::Write { | ||
| ref value, | ||
| action: _, | ||
| } => len as i64 - value.len() as i64, |
There was a problem hiding this comment.
Same question (what happens if len - value.len() < 0 ?)
tzemanovic
force-pushed
the
brent/new-storage-write
branch
from
9ca40ed
to
89bb67d
Compare
* brent/new-storage-write: rename new fns prune non-persisted DB diffs No merkle/diffs for remaining masp keys changelog: add #2355 ibc: storage keys refactor handle non-merklized values in `State` methods some TODOs and another comment remake wasms for tests ibc writes without merklization or diffs tests for writing without merklization or diffs use consts instead of string literals abstract instances of `write_bytes` to `write` masp writes without merklization or diffs storage write/delete options for inclusion in merkle tree and diffs
|
closing in favor of #2438 |
Describe your changes
StorageWrite API changes.
Some notes:
writeanddeletefunctionsmasp/note_commitment_anchor/<hash>masp/commitment_treemasp/convert_anchormasp/nullifiers/<hash>masp/pin-<id>ibc/clients/counteribc/connections/counteribc/channelEnds/counterwrite_bytesare abstracted abstracted to a different write API callIndicate on which release or other PRs this topic is based on
v0.30.1
Checklist before merging to
draft