set_code_hash with contract reconstruction #6985
Conversation
…nto zebedeusz/constracts_code_hash
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
There was a problem hiding this comment.
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_datawill make sure it is only run inside of a constructor. However, we are executing the constructor in the current contractscallcontext (Frame). This code needs some adoption. Maybe a flag insideFrameto signal when we are insideset_code_hash. This also needs to be checked inget_immutable_datato 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_lento0refund the deposit usingstorage_meter.charge()before calling the constructor. You also need to delete the current imutables from storage before calling the constuctor. Callingset_code_hashwill 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 + 1codes in memory. This also means that we need to take into account if we are inset_code_hashwhen doing a sub call. We basically need to adapt this check to add1if we are insideset_code_hash.
| let result = executable | ||
| .execute(self, ExportedFunction::Constructor, current_immutable.to_vec()) | ||
| .map_err(|e| e.error)?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
This should to be done before calling the constructor. Otherwise it will see the old code hash when querying it.
|
@athei |
|
@athei |
…_base_deposit` (#7230) This PR is centered around a main fix regarding the base deposit and a bunch of drive by or related fixtures that make sense to resolve in one go. It could be broken down more but I am constantly rebasing this PR and would appreciate getting those fixes in as-one. **This adds a multi block migration to Westend AssetHub that wipes the pallet state clean. This is necessary because of the changes to the `ContractInfo` storage item. It will not delete the child storage though. This will leave a tiny bit of garbage behind but won't cause any problems. They will just be orphaned.** ## Record the deposit for immutable data into the `storage_base_deposit` The `storage_base_deposit` are all the deposit a contract has to pay for existing. It included the deposit for its own metadata and a deposit proportional (< 1.0x) to the size of its code. However, the immutable code size was not recorded there. This would lead to the situation where on terminate this portion wouldn't be refunded staying locked into the contract. It would also make the calculation of the deposit changes on `set_code_hash` more complicated when it updates the immutable data (to be done in #6985). Reason is because it didn't know how much was payed before since the storage prices could have changed in the mean time. In order for this solution to work I needed to delay the deposit calculation for a new contract for after the contract is done executing is constructor as only then we know the immutable data size. Before, we just charged this eagerly in `charge_instantiate` before we execute the constructor. Now, we merely send the ED as free balance before the constructor in order to create the account. After the constructor is done we calculate the contract base deposit and charge it. This will make `set_code_hash` much easier to implement. As a side effect it is now legal to call `set_immutable_data` multiple times per constructor (even though I see no reason to do so). It simply overrides the immutable data with the new value. The deposit accounting will be done after the constructor returns (as mentioned above) instead of when setting the immutable data. ## Don't pre-charge for reading immutable data I noticed that we were pre-charging weight for the max allowable immutable data when reading those values and then refunding after read. This is not necessary as we know its length without reading the storage as we store it out of band in contract metadata. This makes reading it free. Less pre-charging less problems. ## Remove delegate locking Fixes #7092 This is also in the spirit of making #6985 easier to implement. The locking complicates `set_code_hash` as we might need to block settings the code hash when locks exist. Check #7092 for further rationale. ## Enforce "no terminate in constructor" eagerly We used to enforce this rule after the contract execution returned. Now we error out early in the host call. This makes it easier to be sure to argue that a contract info still exists (wasn't terminated) when a constructor successfully returns. All around this his just much simpler than dealing this check. ## Moved refcount functions to `CodeInfo` They never really made sense to exist on `Stack`. But now with the locking gone this makes even less sense. The refcount is stored inside `CodeInfo` to lets just move them there. ## Set `CodeHashLockupDepositPercent` for test runtime The test runtime was setting `CodeHashLockupDepositPercent` to zero. This was trivializing many code paths and excluded them from testing. I set it to `30%` which is our default value and fixed up all the tests that broke. This should give us confidence that the lockup doeposit collections properly works. ## Reworked the `MockExecutable` to have both a `deploy` and a `call` entry point This type used for testing could only have either entry points but not both. In order to fix the `immutable_data_set_overrides` I needed to a new function `add_both` to `MockExecutable` that allows to have both entry points. Make sure to make use of it in the future :) --------- Co-authored-by: command-bot <> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: PG Herveou <pgherveou@gmail.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
|
With #7230 merged this PR becomes much more clear. It removes the delegate locking and delays the deposit calculation for immutable storage. This is now unblocked. Let me know when you want to pick it up again and I can coach you on it. |
Fixes #6717