AccountStorage refactor from Smt to sequential hash
#811
Conversation
phklive
changed the title
AccountStorage refactor from Smt to sequential hash
|
This is a first pass at tackling this issue:
@bobbinth, I would like to know if this seems like a general good route of if you would advise for another way of tackling this problem. |
|
Also, these commits coming from the previous PR I built on which was merged in next: #763 
Why are they appearing here? Is there something I did wrong or a merge conflict? |
| /// Returns a new instance of account storage initialized with the provided slots. | ||
| pub fn new(slots: &[StorageSlot]) -> Self { |
There was a problem hiding this comment.
Any reason to pass in a slice instead of a Vec as a parameter here?
| /// Returns an item from the storage at the specified index. | ||
| /// | ||
| /// If the item is not present in the storage, [crate::EMPTY_WORD] is returned. | ||
| pub fn get_map_item(&self, index: u8, key: Word) -> Result<Word, AccountError> { | ||
| let storage_map = self.maps.get(&index).ok_or(AccountError::StorageMapNotFound(index))?; | ||
|
|
||
| Ok(storage_map.get_value(&Digest::from(key))) | ||
| } | ||
|
|
||
| /// Returns a reference to the Sparse Merkle Tree that backs the storage slots. | ||
| pub fn slots(&self) -> &SimpleSmt<STORAGE_TREE_DEPTH> { | ||
| &self.slots | ||
| } | ||
|
|
||
| /// Returns layout info for this storage. | ||
| pub fn layout(&self) -> &[StorageSlotType] { | ||
| &self.layout | ||
| } | ||
|
|
||
| /// Returns a commitment to the storage layout. | ||
| pub fn layout_commitment(&self) -> Digest { | ||
| layout_commitment(&self.layout) | ||
| } | ||
|
|
||
| /// Returns the storage maps for this storage. | ||
| pub fn maps(&self) -> &BTreeMap<u8, StorageMap> { | ||
| &self.maps | ||
| pub fn get_item(&self, index: u8) -> Digest { | ||
| match self.slots.get(index as usize) { | ||
| Some(storage_slot) => match storage_slot { | ||
| StorageSlot::Value(word) => word.into(), | ||
| StorageSlot::Map(map) => map.root(), | ||
| }, | ||
| None => EMPTY_WORD.into(), | ||
| } |
There was a problem hiding this comment.
A few comments here:
get_item()should return aWord.get_item()should panic if the index is out of bounds.- We also need to have
get_map_item()(similar to how we had it before).
We need to have some accessors to get info about the storage. Specifically, we need to be able to learn:
- How many slots are in the storage.
- What types these slots have.
These could be done via separate methods or via a single layout() method (similar to what we had before).
| /// Updates the value of the storage slot at the specified index. | ||
| /// | ||
| /// This method should be used only to update simple value slots. For updating values | ||
| /// in storage maps, please see [AccountStorage::set_map_item()]. | ||
| /// | ||
| /// # Errors | ||
| /// Returns an error if: | ||
| /// - The index specifies a reserved storage slot. | ||
| /// - The update tries to set a slot of type array. | ||
| /// - The update has a value arity different from 0. | ||
| pub fn set_item(&mut self, index: u8, value: Word) -> Result<Word, AccountError> { | ||
| // layout commitment slot cannot be updated | ||
| if index == Self::SLOT_LAYOUT_COMMITMENT_INDEX { | ||
| return Err(AccountError::StorageSlotIsReserved(index)); | ||
| } | ||
| pub fn set_item(&mut self, index: u8, storage_slot: StorageSlot) { |
There was a problem hiding this comment.
We should keep the signature of this method as it was before. Basically, we want to keep mutators parallel to their counterparts in the MASM code.
| pub fn set_map_item( | ||
| &mut self, | ||
| index: u8, | ||
| key: Word, | ||
| value: Word, | ||
| ) -> Result<(Word, Word), AccountError> { |
There was a problem hiding this comment.
Similar to the previous comment: we should keep this method.
| impl Serializable for StorageSlot { | ||
| fn write_into<W: vm_core::utils::ByteWriter>(&self, target: &mut W) { | ||
| target.write_u16(self.into()); | ||
| target.write(self); | ||
| } | ||
| } |
There was a problem hiding this comment.
Would this not lead to infinite recursion?
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum StorageSlot { | ||
| Value(Word), | ||
| Map(StorageMap), | ||
| } |
There was a problem hiding this comment.
I think we may still need to keep StorageSlotType enum. For example, it would be used in AccountStorage::get_slot_type() or AccountStorage::layout().
We could also have methods like StorageSlotType::as_element() and StorageSlotType::as_word() to simplify conversion of storage into a vector of elements.
| _ => false, | ||
| } | ||
| } | ||
| impl StorageSlot { |
There was a problem hiding this comment.
We also probably need something like
StorageSlot::get_type() -> StorageSlotTypeStorageSlot::to_word() -> Word. This would return the value for simple value slots, and the root for maps.
| match storage_slot { | ||
| StorageSlot::Value(word) => { | ||
| for element in word { | ||
| elements.push(*element) | ||
| } | ||
| for _ in 0..4 { | ||
| elements.push(ZERO) | ||
| } | ||
| }, | ||
| StorageSlot::Map(map) => { | ||
| let smt_root = map.root(); | ||
|
|
||
| for element in smt_root.as_elements() { | ||
| elements.push(*element) | ||
| } | ||
|
|
||
| elements.push(ONE); | ||
|
|
||
| for _ in 0..3 { | ||
| elements.push(ZERO) | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
If we have methods mentioned in the previous comment, this could be simplified quite a bit. For example:
elements.extend_from_slice(&slot.to_word());
elements.extend_from_slice(&slot.get_type().as_word());There was a problem hiding this comment.
I don't think we need to modify the deltas as a part of this PR. The current delta structs should be sufficient for the new account storage structure and we'll do a more general refactoring as a part of #798.
Hmm - not sure. I don't mind having them here as they'll go away once we do "squash and merge", but also if there is an easy way to git rid of them here, that would be even better. |
In this PR I propose to refactor the
AccountStoragefromSmtto a sequential hash.Closes: #802