Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Contracts Add deposit for dependencies #14079

Merged
merged 77 commits into from
Jul 26, 2023
Merged

Conversation

pgherveou
Copy link
Contributor

@pgherveou pgherveou commented May 4, 2023

cumulus companion: paritytech/cumulus#2913

To prevent a contract to be removed when it's invoked via delegate_call, this PR introduces two new runtime functions add_delegate_dependency and remove_delegate_dependency.

add_delegate_dependency will increase the refcount for the code hash, and add it to the new delegate_dependencies field of the Contract info. In order to prevent abuse adding a dependency will be protected by a percentage of the storage deposit of the original code.

Likewise, since instantiating a contract, increment the refcount, the same amount will be added to the base deposit for using this code hash.

To make these updates possible we introduce the following new configuration fields:

/// The maximum number of delegate dependencies that a contract can lock with `add_delegate_dependency`.
#[pallet::constant]
type MaxDelegateDependencies: Get<u32>;

/// The percentage of the storage deposit that should be held for using a code hash.
/// Instantiating a contract, or calling `add_delegate_dependency` (for contract delegation) protects
/// the code from being removed. In order to prevent abuse these actions are protected with
/// a percentage of the code deposit.
#[pallet::constant]
type CodeHashLockupDepositPercent: Get<Perbill>;

@pgherveou pgherveou changed the title pg/delegate deposit Contracts Add deposit for dependencies May 4, 2023
@pgherveou pgherveou force-pushed the pg/delegate_deposit branch 4 times, most recently from c0069e8 to 51000ec Compare May 5, 2023 12:59
@pgherveou pgherveou added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels May 5, 2023
@pgherveou pgherveou marked this pull request as ready for review May 5, 2023 19:53
@pgherveou pgherveou requested a review from athei as a code owner May 5, 2023 19:53
@pgherveou
Copy link
Contributor Author

bot bench $ pallet dev pallet_contracts

@command-bot
Copy link

command-bot bot commented May 5, 2023

@pgherveou https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2791408 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 13-69bf6ef0-c7b8-4642-929f-baff9a20b46a to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented May 6, 2023

@pgherveou Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2791408 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2791408/artifacts/download.

frame/contracts/src/exec.rs Outdated Show resolved Hide resolved
frame/contracts/src/exec.rs Outdated Show resolved Hide resolved
@pgherveou
Copy link
Contributor Author

This is going to require a migration as well for adding a property to ContractInfo

@pgherveou
Copy link
Contributor Author

Also since the length of the encoded ContractInfo can vary based on the size of the dependencies, we might need to alter the benchmark to take the worst situation into account (aka fill the map with max dependencies)?

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The terminate benchmark needs to be done with max dependencies in order to account for the worst case. We need to check how bad this is. It is 32 transfers. Could be prohibitive.

frame/contracts/src/storage.rs Outdated Show resolved Hide resolved
frame/contracts/src/exec.rs Outdated Show resolved Hide resolved
frame/contracts/src/exec.rs Outdated Show resolved Hide resolved
frame/contracts/src/storage/meter.rs Outdated Show resolved Hide resolved
@athei
Copy link
Member

athei commented May 10, 2023

Post-merge PR

Add v12 migration step that adds the dependencies to the contract info

I think we should leave that PR open until the new migration system is merged. So we avoid the situation of a master with missing migrations.

@pgherveou pgherveou requested review from a team May 30, 2023 09:59
@pgherveou pgherveou changed the base branch from master to pg/frame-add-translate_next June 1, 2023 09:39
Base automatically changed from pg/frame-add-translate_next to master June 2, 2023 14:11
@pgherveou
Copy link
Contributor Author

merging master after #14084 , might need to self review a few things before it's ready for review again

@pgherveou pgherveou force-pushed the pg/delegate_deposit branch from c4c4e7b to 3cafdb6 Compare July 4, 2023 11:10
@pgherveou pgherveou changed the base branch from master to pg/simplify_instantiate_with_code July 4, 2023 11:11
@pgherveou
Copy link
Contributor Author

bot bench $ pallet dev pallet_contracts

@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@pgherveou Positional arguments are not supported anymore. I guess you meant bot bench substrate-pallet --pallet=pallet_contracts, but I could be wrong.
Read docs to find out how to run your command.

@pgherveou
Copy link
Contributor Author

bot bench substrate-pallet --pallet=pallet_contracts

@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@pgherveou https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3261305 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 19-9b158607-97de-4c77-a93b-5b8efd12b4c4 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@pgherveou Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3261305 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3261305/artifacts/download.

@pgherveou pgherveou requested a review from agryaznov July 26, 2023 07:27
Co-authored-by: Juan <juangirini@gmail.com>
// Measured: `709`
// Estimated: `9124`
// Minimum execution time: 42_457_000 picoseconds.
Weight::from_parts(44_556_000, 9124)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one increased markedly, on both dimensions. Any ideas why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did that when you were off #14523 but did not update the bench

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm but that one changed v12 migration, while the weights change in question is for the v10 one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right 🤔

@pgherveou
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/cumulus#2913

@pgherveou
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 9780579 into master Jul 26, 2023
@paritytech-processbot paritytech-processbot bot deleted the pg/delegate_deposit branch July 26, 2023 12:09
paritytech-processbot bot pushed a commit to paritytech/cumulus that referenced this pull request Jul 26, 2023
* Companion PR for paritytech/substrate#14079

* update lockfile for {"substrate", "polkadot"}

---------

Co-authored-by: parity-processbot <>
@simonsso simonsso mentioned this pull request Feb 22, 2024
19 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants