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

pallet-contracts: Migrate to weights mechanism #4147

Closed
wants to merge 44 commits into from

Conversation

pepyakin
Copy link
Contributor

@pepyakin pepyakin commented Nov 19, 2019

This PR changes srml-contracts to tap on the weight system instead of relying on a separate gas mechanism.

In this change we treat 1 gas unit as 1 weight unit. I wasn't sure about getting rid of the notion of gas althogether since we might want to scale it relative to weight later on and because gas is familiar lingo for the existing smart-contract developers. Apart from that, it would also introduce some backward compatibility fall out.

This change also makes an assumption that the weight to price is a linear function.

The transition is not complete though, because there are basically two problems.

Dynamic Weights

It is not yet clear how to implement ext_dispatch_call, a contract-to-runtime call, within the weights system. The problem here is that ext_dispatch_call(T::Call) has no information regarding how much weight the given T::Call would consume and thus there is no way to ensure that executing this call won't be out of given gas/weight limit budget. Related issues were filled and partially implemented (#3730, #3921), but it is still not enough. We will work this out with @kianenigma.

Block weight limit

The current values for the weight budget are a bit off. To understand the issue let's take a simple example where a contract transfers some funds to another account. The situation right now is that the fees for transfers are charged with gas, trying to emulate the behavior of the balances module. The transfer fee in balances module is defined in terms of balance units. We use approximation of the gas cost (we can do that since we know the price of one unit of gas in terms of balance units) for the fees and charge that. At the moment of writing, the fee is set to 1 "cent".

The problem is that with current values set in node-runtime the whole weight limit costs 1 cent, so it is not possible to perform even one transfer.

There are different cures to that to name a few:

  1. We don't need to have fees for transfers and it is a remnant of pre-weights system. We just need to remove it.
  2. We can just tune the fees themselves. This might allow a weird loophole where it makes sense to transfer balances via contracts rather than specialized balances.

I opted-in to the last one, since merging of #4590 will allow us to basically use T::Currency::transfer directly without involving any emulation!

@pepyakin pepyakin added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 19, 2019
# Conflicts:
#	bin/node/runtime/src/lib.rs
# Conflicts:
#	bin/node/runtime/src/lib.rs
#	frame/contracts/src/gas.rs
#	frame/contracts/src/lib.rs
#	frame/contracts/src/tests.rs
#	frame/contracts/src/wasm/code_cache.rs
@pepyakin pepyakin force-pushed the ser-contract-weights-2 branch from 3dae604 to dc85fa5 Compare December 10, 2019 12:47
frame/contracts/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/lib.rs Show resolved Hide resolved
frame/contracts/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/lib.rs Show resolved Hide resolved
frame/contracts/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/lib.rs Show resolved Hide resolved
# Conflicts:
#	bin/node/executor/src/lib.rs
#	bin/node/runtime/src/lib.rs
#	frame/contracts/src/gas.rs
#	frame/contracts/src/lib.rs
#	frame/contracts/src/wasm/code_cache.rs
@pepyakin
Copy link
Contributor Author

@kianenigma yeah, as you say it yourself, it turns out get_dispatch_info is not enough since register_extra_weight_unchecked will add additional weight after the execution. Or, put in a different perspective, there is no way for the caller of register_extra_weight_unchecked to satisfy it requirements, namely that it doesn't overrun the max block limit.

It seems to me that we have to know the upper bound of the call at hand before execution. Otherwise, there is no way to fulfil register_extra_weight_unchecked requirements.

# Conflicts:
#	bin/node/executor/src/lib.rs
@kianenigma
Copy link
Contributor

It seems to me that we have to know the upper bound of the call at hand before execution. Otherwise, there is no way to fulfil register_extra_weight_unchecked requirements.

Weight are surely from day 0 defined to be the upper bound. At the moment we don't have good estimates but this is an empirical issue from the substrate core, not your side. You can still assume

  • All weights are upper bound.
  • Except for calls that use register_extra_weight_unchecked, which is not used by anyone atm and will stay limited.

For calls that do use register_extra_weight_unchecked, IFAIK there's really no way to know how long will a nested call take and how much weight it will consume. You can know it ahead of time, but that will take as long as just executing it in the first place (related). You can't do it just by examining it in a cheap way.

How about banning certain calls, or providing a different, more complicated ways to call them? for example if you want to call sudo from your contract, you need to put some deposit down etc.

What we can do at substrate level to help a bit with this is to add a is_dynamic flag or similar to DispatchInfo so you'd know for each call, if it is safe to assume the weight is actually the upper bound, or not.

@pepyakin
Copy link
Contributor Author

For posterity, we moved this discussion to the other channels. We should do a write up after we come to a conclusion though. My current thinking is that we won't be able to fix it in this PR.

@pepyakin pepyakin added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Feb 18, 2020
# Conflicts:
#	bin/node/runtime/src/lib.rs
#	frame/contracts/src/gas.rs
#	frame/contracts/src/lib.rs
#	frame/contracts/src/tests.rs
@pepyakin pepyakin added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 18, 2020
//
<GasPrice<T>>::put(gas_price);

let imbalance = match T::Currency::withdraw(
Copy link
Member

Choose a reason for hiding this comment

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

You call this from SignedExtension::validate and SignedExtension::pre_dispatch? Aren't you charging the who double by doing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Whatever we do in validate() is not persistent. The state is dropped thereafter.

Copy link
Member

Choose a reason for hiding this comment

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

But changes in pre_dispatch are persisted, right?

@gnunicorn gnunicorn added A1-onice and removed A0-please_review Pull request needs code review. labels Apr 14, 2020
@gnunicorn
Copy link
Contributor

gnunicorn commented Apr 14, 2020

maybe re-opened at a later time.

@gnunicorn gnunicorn closed this Apr 14, 2020
@pepyakin
Copy link
Contributor Author

To clarify: this, of course, doesn't imply that we drop this feature. We close it for clearing the PR queue. In future, we can re-open this PR or perhaps open another one.

@athei
Copy link
Member

athei commented Apr 14, 2020

I am currently working on this by building on the currently merged weight refund mechanism and the unmerged weight rework (#5446). I will start a new PR for this when it is ready for review.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants