-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
Why not just charge a fixed deposit per stored byte? Charge storage by gas makes no sense nor does weights. |
If you guys decide to remove rent, I advise to recover put_code |
The "just" is doing a lot of heavy lifting here, though. Making the caller pay a deposit is the obvious solution but it has some open questions associated with it. It is certainly something that we consider and it might be implemented. However, the code that is removed here can't be reused for that because it is all about the contract being responsible for paying the rent/deposit which is fundamentally different. Why this solution wasn't explored before was the stance that we want to give smart contract authors maximum flexibility with their financing model. However, It turned out that the current approach provides that but didn't pass the reality check when it comes to ease of development.
I agree. But that is not the point. I stated in my write up that those calculations are just to assess how feasible a DoS attack is right now. It is not a long term solution. |
/benchmark runtime pallet pallet_contracts |
Benchmark Runtime Pallet for branch "at-remove-rent" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
Less complex sounds good. Smart contract developers already have the hurdle of getting up to speed with rust. It's one less thing for builders to worry about if they can assume things will always be where they put them. Maybe we should make old state more costly to retrieve than newer state (if it isn't already)? |
Yes it can be re-added after this is merged. It is not a priority right now, though.
We should definitely discuss all the options we have. But this PR is not the right place. FWIW nothing like this is implemented as of right now.
For the solidity folks that is true. My hope is that we can get some rustaceans into smart contracts with ink!. |
ad24743
to
12df701
Compare
return weight_limit | ||
return 0 |
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.
This is a drive by fix. I think this was a refactoring artifact.
#[derive(Encode, Decode)] | ||
pub struct DeletedContract { | ||
pair_count: u32, |
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 host API was improved. We no longer need to store how many keys we are deleting because the host API tells us how many where removed.
/benchmark runtime pallet pallet_contracts |
Benchmark Runtime Pallet for branch "at-remove-rent" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs
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.
Looks good to me.
Would be great if you could create a follow-up issue for polkadot-js
. AFAIK that's the only UI which displays state rent info.
I propose setting the maximum value size per key to 512 bytes as an initial value (ethereum uses 32 bytes for various reasons).
We should think about warning developers at compile time when using bigger values in ink!. I'll create a follow-up issue once this one is merged.
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.
Simplifying and deleting code FTW
frame/contracts/src/weights.rs
Outdated
.saturating_add((39_799_000 as Weight).saturating_mul(q as Weight)) | ||
(79_790_000 as Weight) | ||
// Standard Error: 2_000 | ||
.saturating_add((391_000 as Weight).saturating_mul(q as Weight)) |
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.
Reducing the item size by 4 byte really decreased the decoding time by a lot.
substrate have removed the rent in pallet-contracts in newest substrate after this pr paritytech/substrate#9669. but we do not wanna update current substrate to new version, for we are waiting substrate to release 4.0.0. thus, we just change the rent parameters to zero, it will cause the same result compared with removed rent pallet-contracts.
substrate have removed the rent in pallet-contracts in newest substrate after this pr paritytech/substrate#9669. but we do not wanna update current substrate to new version, for we are waiting substrate to release 4.0.0. thus, we just change the rent parameters to zero, it will cause the same result compared with removed rent pallet-contracts.
paritytech/substrate#9669 The concept of paying rent to keep a contract active has been eliminated and the deployer of the Smart Contract just needs to make an extra deposit on trop of the existential deposit.
* Remove ref to Tombstone paritytech/substrate#9669 The concept of paying rent to keep a contract active has been eliminated and the deployer of the Smart Contract just needs to make an extra deposit on trop of the existential deposit. * minor fixes
This PR removes state rent (data, code) from the pallet_contracts code base. It greatly reduces complexity and allows coming changes to be much simpler to implement. It includes a storage migration and is backwards compatible for existing contracts (for the ones not using the
unstable-interface
feature). Tombstones are no longer a thing and existing ones are removed by the migration.Motivation
The pallet_contracts was designed from the beginning to combat unbounded state growth by using state rent (charging contracts for the state they consume). The main reason for the introduction appears to be that this is what Ethereum was exploring at the time. The idea is elegant because it pushes the policy decisions around contract financing to the contract authors and allows them to experiment with different ideas rather than enforcing one at the runtime level.
In practice however, it turns out that state rent is really difficult to deal with and the smart contract audience is not equipped to deal with this issue and simply uses any other solution where this is not a thing. Please keep in mind that in contrast to runtime development where flexibility is key we are going for a crowd where ease of use is paramount. Additionally, Ethereum itself abandoned the idea and is on track on using what they call state expiry. Something that could be added to substrate later as a runtime opt-in for smart contract hosting chains.
The conclusion is that rolling out state rent will put us as a severe disadvantage with regard to adoption when compared to other smart contract technologies with regard to adaption without any short to medium time gain. Even on Ethereum with years of heavy usage on its back running a mining node is still something totally feasible. With the luxury of runtime upgrades and storage migrations we should solve the problem in a less intrusive way later.
Implementation
The PR mainly deletes code and adjusts tests/benchmarks. The biggest change is that the concept of a tombstone is gone. The
TombstoneDeposit
is renamed toContractDeposit
and is a deposit a deployer needs to deposit into the new contract on top of theExistentialDeposit
(no change to the previous behavior). Please note thatseal_terminate
can still be used to remove a contract and is the only way to remove a contract and regain the deposit.One side effect of this change is that the
ContractInfo
data structure no longer contains any mutable data. Meaning that all the data there is fixed at the point of contract instantiation. This makes theContractInfo
caching we do obsolete. Instead of deleting the code I hid it behind theCONTRACT_INFO_CAN_CHANGE
constant which can be switched totrue
once we change the structure to contain mutable data again.Security Discussion
With the removal of state rent we open the door for attackers that try to DoS the chain by creating as much storage as possible. This is because you pay once (in weight) to create the storage but it needs to be kept around forever by collators (in absence of a future solution like state expiry). The means of making this attack economically infeasible is to adjust the
Limits::payload_len
field which determines the maximum amount of storage per key. Lower sizes make creating storage more expensive because creating or updating storage items is expensive in weight. However, for maximum flexibility and efficiency we want to set the value as high as possible.Please keep in mind that this attack would not affect the relay chain because validators do not need to keep the state of a potentially smart contract hosting parachain.
I propose setting the maximum value size per key to 512 bytes as an initial value (ethereum uses 32 bytes for various reasons). We can always increase the value but lowering it would be a breaking change. This would put the price of 1 GB of storage on Kusama to 271 KSM. Calculation here. Try to play around with different values. These are the weights obtained through benchmarking I used for the calculation (1ps = 1 weight = 1 unit of balance):
Those values seem rather low but note that:
Please keep in mind that we are not trying to find a long term solution due to these calculation but merely try to avoid a DoS attack.
Porting Guide
Smart Contract Languages
All existing stable functions are retained and are replaced by doing nothing or returning sane default values. The unstable
seal_rent_params
andseal_rent_status
are removed.Nodes
All rent related configuration options are removed from the
Config
trait. Remove them without replacement as your compiler requests. The publicrent_projection
function is removed andrent_projection
field is removed from the return types ofbare_instantiate
andbare_call
.Offchain Tools
@jacogr
polkadot-js/api#3925
The
rent_projection
RPC is removed and therent_projection
field is removed from theinstantiate
RPC result type. The in-storageContractInfo
type changed and need to be synced with typescript.