Skip to content

Commit

Permalink
fix: correct read mode for storage_has_key (#8968)
Browse files Browse the repository at this point in the history
In `storage_has_key` we were setting `KeyLookupMode::FlatStorage` by default. This is correct for most reads, but not for reads inside contracts, as it impacts chunk cache and contract call costs.

We must read from flat storage deterministically and derive lookup mode from protocol version.
  • Loading branch information
Longarithm authored Apr 25, 2023
1 parent d752d74 commit c271196
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 11 deletions.
15 changes: 8 additions & 7 deletions runtime/near-vm-logic/src/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,14 +200,14 @@ pub trait External {
/// # Example
/// ```
/// # use near_vm_logic::mocks::mock_external::MockedExternal;
/// # use near_vm_logic::External;
/// # use near_vm_logic::{External, StorageGetMode};
///
/// # let mut external = MockedExternal::new();
/// external.storage_set(b"key1", b"value1337").unwrap();
/// external.storage_set(b"key2", b"value1337").unwrap();
/// assert_eq!(external.storage_remove_subtree(b"key"), Ok(()));
/// assert!(!external.storage_has_key(b"key1").unwrap());
/// assert!(!external.storage_has_key(b"key2").unwrap());
/// assert!(!external.storage_has_key(b"key1", StorageGetMode::Trie).unwrap());
/// assert!(!external.storage_has_key(b"key2", StorageGetMode::Trie).unwrap());
/// ```
fn storage_remove_subtree(&mut self, prefix: &[u8]) -> Result<()>;

Expand All @@ -218,6 +218,7 @@ pub trait External {
/// # Arguments
///
/// * `key` - a key to check
/// * `mode`- whether the lookup will be performed through flat storage or trie
///
/// # Errors
///
Expand All @@ -226,16 +227,16 @@ pub trait External {
/// # Example
/// ```
/// # use near_vm_logic::mocks::mock_external::MockedExternal;
/// # use near_vm_logic::External;
/// # use near_vm_logic::{External, StorageGetMode};
///
/// # let mut external = MockedExternal::new();
/// external.storage_set(b"key42", b"value1337").unwrap();
/// // Returns value if exists
/// assert_eq!(external.storage_has_key(b"key42"), Ok(true));
/// assert_eq!(external.storage_has_key(b"key42", StorageGetMode::Trie), Ok(true));
/// // Returns None if there was no value
/// assert_eq!(external.storage_has_key(b"no_value_key"), Ok(false));
/// assert_eq!(external.storage_has_key(b"no_value_key", StorageGetMode::Trie), Ok(false));
/// ```
fn storage_has_key(&mut self, key: &[u8]) -> Result<bool>;
fn storage_has_key(&mut self, key: &[u8], mode: StorageGetMode) -> Result<bool>;

fn generate_data_id(&mut self) -> CryptoHash;

Expand Down
8 changes: 7 additions & 1 deletion runtime/near-vm-logic/src/logic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2643,7 +2643,13 @@ impl<'a> VMLogic<'a> {
}
self.gas_counter.pay_per(storage_has_key_byte, key.len() as u64)?;
let nodes_before = self.ext.get_trie_nodes_count();
let res = self.ext.storage_has_key(&key);
let read_mode =
if checked_feature!("stable", FlatStorageReads, self.current_protocol_version) {
StorageGetMode::FlatStorage
} else {
StorageGetMode::Trie
};
let res = self.ext.storage_has_key(&key, read_mode);
let nodes_delta = self
.ext
.get_trie_nodes_count()
Expand Down
2 changes: 1 addition & 1 deletion runtime/near-vm-logic/src/mocks/mock_external.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl External for MockedExternal {
Ok(())
}

fn storage_has_key(&mut self, key: &[u8]) -> Result<bool> {
fn storage_has_key(&mut self, key: &[u8], _mode: StorageGetMode) -> Result<bool> {
Ok(self.fake_trie.contains_key(key))
}

Expand Down
8 changes: 6 additions & 2 deletions runtime/runtime/src/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,14 @@ impl<'a> External for RuntimeExt<'a> {
Ok(())
}

fn storage_has_key(&mut self, key: &[u8]) -> ExtResult<bool> {
fn storage_has_key(&mut self, key: &[u8], mode: StorageGetMode) -> ExtResult<bool> {
let storage_key = self.create_storage_key(key);
let mode = match mode {
StorageGetMode::FlatStorage => KeyLookupMode::FlatStorage,
StorageGetMode::Trie => KeyLookupMode::Trie,
};
self.trie_update
.get_ref(&storage_key, KeyLookupMode::FlatStorage)
.get_ref(&storage_key, mode)
.map(|x| x.is_some())
.map_err(wrap_storage_error)
}
Expand Down

0 comments on commit c271196

Please sign in to comment.