[pallet_contracts] Modify the storage host function benchmarks to run on an unbalanced storage trie. #5036
Conversation
smiasojed
added
the
T7-smart_contracts
|
bot bench substrate-pallet --pallet=pallet_contracts |
|
@smiasojed https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6713254 was started for your command Comment |
|
We are migrating the command bot to be a GitHub Action |
|
@smiasojed Command |
|
bot bench substrate-pallet --pallet=pallet_contracts |
|
@smiasojed https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6714033 was started for your command Comment |
|
We are migrating the command bot to be a GitHub Action |
…=dev --target_dir=substrate --pallet=pallet_contracts
|
@smiasojed Command |
|
bot bench substrate-pallet --pallet=pallet_contracts |
|
@smiasojed https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6715024 was started for your command Comment |
|
We are migrating the command bot to be a GitHub Action |
…=dev --target_dir=substrate --pallet=pallet_contracts
|
@smiasojed Command |
| /// Update a storage entry into a contract's kv storage. | ||
| /// Function used in benchmarks, which can simulate prefix collision in keys. | ||
| #[cfg(feature = "runtime-benchmarks")] | ||
| pub fn bench_write_raw( |
There was a problem hiding this comment.
Instead of this call we could add and use Key::Raw variant, but I am not sure if it is better option, WDYT?
There was a problem hiding this comment.
wouldn't call child::put_raw enough for the benchmark?
There was a problem hiding this comment.
I suggest keeping it this way. We can't use child::put_raw directly because that wouldn't include the read which is also part of every write due to storage deposits.
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
bot bench substrate-pallet --pallet=pallet_contracts |
|
@smiasojed Command |
smiasojed
force-pushed
the
sm/storage-pov
branch
from
7dd3b3a
to
2259be8
Compare
smiasojed
changed the title
|
bot bench substrate-pallet --pallet=pallet_contracts |
|
@smiasojed https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6728111 was started for your command Comment |
|
We are migrating the command bot to be a GitHub Action |
…=dev --target_dir=substrate --pallet=pallet_contracts
|
@smiasojed Command |
| result = child::get_raw(&child_trie_info, &key); | ||
| } | ||
|
|
||
| assert_eq!(result, Some(value)); |
There was a problem hiding this comment.
can we somehow verify that we did hit unbalanced_trie_layer nodes
There was a problem hiding this comment.
Now that we are providing a key as a parameter, we can be sure that we will hit it.
There was a problem hiding this comment.
I mean is there a way to assert that hitting this storage is traversing 20 nodes?
There was a problem hiding this comment.
We are constructing the trie around the provided key. Assuming the trie construction is done correctly, we should hit 20 layers by writing to this key. I am not sure if we can assert this somehow.
There was a problem hiding this comment.
right looks like it can't be done, the weights results make sense, and I just doubled check by writing the proof to disk that the storage_full has indeed 20 keys more
| This PR modifies the storage host function benchmarks. Previously, they were run | ||
| on an empty storage trie. Now, they are run on an unbalanced storage trie | ||
| to reflect the worst-case scenario. This approach increases the storage host | ||
| function weights and decreases the probability of DoS attacks. |
There was a problem hiding this comment.
Contracts can write in storage in a way that produces an unbalanced trie? I expected this to be impossible/prevented.
There was a problem hiding this comment.
All the keys for stored values are hashed using Blake2, so it is unlikely that this will happen in normal use. Main issue here is that we used empty trie for benchmarks.
There was a problem hiding this comment.
A contract can't create unbalanced tries because we hash the key from the contract. However, a contract could have a balanced but very big trie. The unbalanced trie is just use to simulate a deep trie for benchmarking.
| child::put_raw(&child_trie_info, &key, &value); | ||
| for l in 0..UNBALANCED_TRIE_LAYERS { | ||
| let pos = l as usize / 2; | ||
| let mut key_new = key.to_vec(); | ||
| for i in 0u8..16 { | ||
| key_new[pos] = if l % 2 == 0 { | ||
| (key_new[pos] & 0xF0) | i | ||
| } else { | ||
| (key_new[pos] & 0x0F) | (i << 4) | ||
| }; | ||
|
|
||
| if key == &key_new { | ||
| continue | ||
| } | ||
| child::put_raw(&child_trie_info, &key_new, &value); | ||
| } | ||
| } | ||
| Ok(contract) |
There was a problem hiding this comment.
This is quite hard to scan, might be nice to add some explanation on how you construct the tree
| /// Update a storage entry into a contract's kv storage. | ||
| /// Function used in benchmarks, which can simulate prefix collision in keys. | ||
| #[cfg(feature = "runtime-benchmarks")] | ||
| pub fn bench_write_raw( |
There was a problem hiding this comment.
I suggest keeping it this way. We can't use child::put_raw directly because that wouldn't include the read which is also part of every write due to storage deposits.
| This PR modifies the storage host function benchmarks. Previously, they were run | ||
| on an empty storage trie. Now, they are run on an unbalanced storage trie | ||
| to reflect the worst-case scenario. This approach increases the storage host | ||
| function weights and decreases the probability of DoS attacks. |
There was a problem hiding this comment.
A contract can't create unbalanced tries because we hash the key from the contract. However, a contract could have a balanced but very big trie. The unbalanced trie is just use to simulate a deep trie for benchmarking.
… on an unbalanced storage trie. (paritytech#5036) This PR modifies the storage host function benchmarks. Previously, they were run on an empty storage trie. Now, they are run on an unbalanced storage trie to reflect worst case scenario. This approach increases the storage host function weights and decreases the probability of DoS attacks. --------- Co-authored-by: command-bot <> Co-authored-by: PG Herveou <[email protected]>
This PR modifies the storage host function benchmarks. Previously, they were run on an empty storage trie.
Now, they are run on an unbalanced storage trie to reflect worst case scenario.
This approach increases the storage host function weights and decreases the probability of DoS attacks.