-
Notifications
You must be signed in to change notification settings - Fork 699
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
[pallet_contracts] Add support for transient storage in contracts host functions #4566
Conversation
bot bench substrate-pallet --pallet=pallet_contracts |
@smiasojed https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6494983 was started for your command Comment |
…=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/6497164 was started for your command Comment |
…=dev --target_dir=substrate --pallet=pallet_contracts
let value = Some(vec![42u8; max_value_len as _]); | ||
let mut setup = CallSetup::<T>::default(); | ||
let (mut ext, _) = setup.ext(); | ||
CallSetup::<T>::with_transient_storage(&mut ext, T::MaxTransientStorageSize::get())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be a bit nicer to have with_transient_storage
be a method of the builder
let mut setup = CallSetup::<T>::default();
setup.transient_storage_size(T::MaxTransientStorageSize::get())
let (mut ext, _) = setup.ext();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
let amount = ext.transient_storage().meter().current().amount; | ||
let limit = ext.transient_storage().meter().current().limit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe
let &MeterEntry { amount, limit } = ext.transient_storage().meter().current();
// cost_write!(name, a, b, c) -> T::WeightInfo::name(a, b, c).saturating_add(T::WeightInfo::rollback_transient_storage()) | ||
// .saturating_add(T::WeightInfo::set_transient_storage_full().saturating_sub(T::WeightInfo::set_transient_storage_empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear to me why the cost is full - empty, can we explain the reasoning here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
full - empty
represents the weight of adding an item to a full BTreeMap. All transient storage host function benchmarks are done with an empty transient storage (BTreeMap), so this represents the worst-case scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow up question, should we refund the cost of rollback_transient_storage in the happy path since you pre-charge it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still not clear to me why we need to add the weight of
T::WeightInfo::set_transient_storage_full() - T::WeightInfo::set_transient_storage_empty()
when we do a ClearTransientStorage for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clear_transient_storage function modifies the storage. The benchmark is currently done for one item in the BTreeMap, but we would like to consider the worst-case scenario, where the modification is done on a fully filled BTreeMap.
I am treating it as a constant cost, similar to DBWrite, which is 100µs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense then, we should add that to the comment to make it clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm as well, added a few more nits and a question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Can you do one additional thing in this PR: Since we are bumping the major version of the uapi crate it would be a good time to also seal the |
The CI pipeline was cancelled due to failure one of the required jobs. |
bot bench substrate-pallet --pallet=pallet_contracts |
@smiasojed https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6662227 was started for your command Comment |
…=dev --target_dir=substrate --pallet=pallet_contracts
@smiasojed Command |
* master: add elastic scaling MVP guide (#4663) Send PeerViewChange with high priority (#4755) [ci] Update forklift in CI image (#5032) Adjust base value for statement-distribution regression tests (#5028) [pallet_contracts] Add support for transient storage in contracts host functions (#4566) [1 / 5] Optimize logic for gossiping assignments (#4848) Remove `pallet-getter` usage from pallet-session (#4972) command-action: added scoped permissions to the github tokens (#5016) net/litep2p: Propagate ValuePut events to the network backend (#5018) rpc: add back rpc logger (#4952) Updated substrate-relay version for tests (#5017) Remove most all usage of `sp-std` (#5010) Use sp_runtime::traits::BadOrigin (#5011)
…t functions (paritytech#4566) Introduce transient storage, which behaves identically to regular storage but is kept only in memory and discarded after every transaction. This functionality is similar to the `TSTORE` and `TLOAD` operations used in Ethereum. The following new host functions have been introduced: `get_transient_storage` `set_transient_storage` `take_transient_storage` `clear_transient_storage` `contains_transient_storage` Note: These functions are declared as `unstable` and thus are not activated. --------- Co-authored-by: command-bot <> Co-authored-by: PG Herveou <pgherveou@gmail.com> Co-authored-by: Alexander Theißen <alex.theissen@me.com>
* master: (125 commits) add elastic scaling MVP guide (#4663) Send PeerViewChange with high priority (#4755) [ci] Update forklift in CI image (#5032) Adjust base value for statement-distribution regression tests (#5028) [pallet_contracts] Add support for transient storage in contracts host functions (#4566) [1 / 5] Optimize logic for gossiping assignments (#4848) Remove `pallet-getter` usage from pallet-session (#4972) command-action: added scoped permissions to the github tokens (#5016) net/litep2p: Propagate ValuePut events to the network backend (#5018) rpc: add back rpc logger (#4952) Updated substrate-relay version for tests (#5017) Remove most all usage of `sp-std` (#5010) Use sp_runtime::traits::BadOrigin (#5011) network/tx: Ban peers with tx that fail to decode (#5002) Try State Hook for Bounties (#4563) [statement-distribution] Add metrics for distributed statements in V2 (#4554) added sync command (#4818) Bridges V2 refactoring backport and `pallet_bridge_messages` simplifications (#4935) xcm-executor: Improve logging (#4996) Remove usage of `sp-std` on templates (#5001) ...
…t functions (paritytech#4566) Introduce transient storage, which behaves identically to regular storage but is kept only in memory and discarded after every transaction. This functionality is similar to the `TSTORE` and `TLOAD` operations used in Ethereum. The following new host functions have been introduced: `get_transient_storage` `set_transient_storage` `take_transient_storage` `clear_transient_storage` `contains_transient_storage` Note: These functions are declared as `unstable` and thus are not activated. --------- Co-authored-by: command-bot <> Co-authored-by: PG Herveou <pgherveou@gmail.com> Co-authored-by: Alexander Theißen <alex.theissen@me.com>
* master: (130 commits) add elastic scaling MVP guide (#4663) Send PeerViewChange with high priority (#4755) [ci] Update forklift in CI image (#5032) Adjust base value for statement-distribution regression tests (#5028) [pallet_contracts] Add support for transient storage in contracts host functions (#4566) [1 / 5] Optimize logic for gossiping assignments (#4848) Remove `pallet-getter` usage from pallet-session (#4972) command-action: added scoped permissions to the github tokens (#5016) net/litep2p: Propagate ValuePut events to the network backend (#5018) rpc: add back rpc logger (#4952) Updated substrate-relay version for tests (#5017) Remove most all usage of `sp-std` (#5010) Use sp_runtime::traits::BadOrigin (#5011) network/tx: Ban peers with tx that fail to decode (#5002) Try State Hook for Bounties (#4563) [statement-distribution] Add metrics for distributed statements in V2 (#4554) added sync command (#4818) Bridges V2 refactoring backport and `pallet_bridge_messages` simplifications (#4935) xcm-executor: Improve logging (#4996) Remove usage of `sp-std` on templates (#5001) ...
Introduce transient storage, which behaves identically to regular storage but is kept only in memory and discarded after every transaction. This functionality is similar to the
TSTORE
andTLOAD
operations used in Ethereum.The following new host functions have been introduced:
get_transient_storage
set_transient_storage
take_transient_storage
clear_transient_storage
contains_transient_storage
Note: These functions are declared as
unstable
and thus are not activated.