Skip to content
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

set_code_hash with contract reconstruction #6985

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Zebedeusz
Copy link
Contributor

@Zebedeusz Zebedeusz commented Dec 23, 2024

Fixes #6717

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12466255018
Failed job name: test-linux-stable-no-try-runtime

@athei athei self-requested a review December 26, 2024 12:39
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.

This is going in the right direction. A few additional remarks:

  • When the constructor is trying to set the immutables it will error out because set_immutable_data will make sure it is only run inside of a constructor. However, we are executing the constructor in the current contracts call context (Frame). This code needs some adoption. Maybe a flag inside Frame to signal when we are inside set_code_hash. This also needs to be checked in get_immutable_data to prevent reading immutables inside the constructor.
  • The current code will also run into problems when the contract already has some immutable data since we disallow replacing it. Solution: Set contract_info.immutable_data_len to 0 refund the deposit using storage_meter.charge() before calling the constructor. You also need to delete the current imutables from storage before calling the constuctor. Calling set_code_hash will always delete the current immutables which is fine and expected. A constructor doesn't have access to them anyways and is expected to write them if there are any.
  • We need to prevent recursive calls into set_code_hash. Otherwise you can trivially consume a lot of memory. Since we are not creating a new frame the stack depth limit is not enforced. I would just generally disallow the usage of this API inside a constructor to address this issue. It seems nonsensical to do that.
  • We also need to error out if the current stack depth is at its maximum when calling this API. This is because we are loading a potentially big chunk of code into memory. We are assuming that there are never more than CALL_STACK_DEPTH + 1 codes in memory. This also means that we need to take into account if we are in set_code_hash when doing a sub call. We basically need to adapt this check to add 1 if we are inside set_code_hash.

Comment on lines +1855 to +1857
let result = executable
.execute(self, ExportedFunction::Constructor, current_immutable.to_vec())
.map_err(|e| e.error)?;
Copy link
Member

Choose a reason for hiding this comment

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

Constructors don't have access to immutables. They set the immutables. Usually by deriving them from the constructor arguments or environment properties (like the caller of the constructor).

So what should be passed here are not immutables but thet arguments to the constructor. In order to do that you need to add a new argument to this function (Vec<u8>) so that the contract calling set_code_hash can provide the chosen constructor with its arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Those new error codes don't make sense since we don't check any compat with the old immutables (see other comment).

I think we should add a single new error: SetCodeHashConstructorFailed to signal the error happened within the constructor. All other errors are just bubbled up.


let old_base_deposit = info.storage_base_deposit();
let new_base_deposit = info.update_base_deposit(&code_info);
let deposit = StorageDeposit::Charge(new_base_deposit)
.saturating_sub(&StorageDeposit::Charge(old_base_deposit));

ExecStack::<T, WasmBlob<T>>::increment_refcount(hash)?;
ExecStack::<T, WasmBlob<T>>::decrement_refcount(prev_hash);
info.code_hash = hash;
Copy link
Member

Choose a reason for hiding this comment

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

This should to be done before calling the constructor. Otherwise it will see the old code hash when querying it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

set_code_hash must collect immutables
2 participants