Implement contract state rent #32

codchen · 2023-06-26T05:10:25Z

Summary

Implement duration-based fee (rent) based on https://docs.google.com/document/d/1FbbV3vQZnyM1xgZF-ug7zNCVlt_3Z9aD7Hng2OIveX0/edit

Specifically, each contract will have a counter tracking the number of bytes it owns as well as the amount of usei people have deposited for it as rent. Each time a write operation happens to a contract (i.e. execute/sudo/migrate/reply), it will settle the rent charges since the last write. For example, if a contract was last written at block height 15 with a post-write state size of 500 bytes, and it's now written again at block height 20, it would need to pay (20 - 15) * 500 * rent price out of its rent balance. If the balance is insufficient, the write would fail. rent price is a wasm module param that can be updated via gov proposal. Anyone can deposit rent for any contract via the DepositRent transaction type. Note that deposited rent cannot be withdrawn.

One thing to note is that in order to track the size of a contract, we need to do an additional query before each Set and Delete to update the size properly.

Future steps:

Add exemption mechanism (so that the first X bytes of a contract is exempted from rent charge)
Rent withdrawal mechanism (tbd who is entitled to the unused rent)

Testing

tested on local Sei [Do Not Merge] testing wasm rent sei-chain#928
unit test WIP

x/wasm/keeper/keeper.go

udpatil · 2023-06-30T14:10:59Z

proto/cosmwasm/wasm/v1/types.proto

+  string unit_rent_price = 3
+    [
+      (gogoproto.moretags)   = "yaml:\"unit_gas_price\"",
+        (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",


i imagine this would be such a small value, would it be more worth it to make this param per 10kb per block or something and then have the unit price represented as an int (similar to how the wasm vm gas multiplier is represented)

we can do price per 10kb or more but I'd prefer keeping the type as dec for flexibility just in case in the future we want to decrease price by a lot

udpatil · 2023-06-30T14:13:30Z

store/sized/store.go

+	}
+	s.parent.Set(key, value)
+	if key != nil {
+		s.sizeChange += int64(len(key))


do we care about key size? i thought the limiting factor on performance for keys was key cardinality due to tree traversal operational time.

we care about key size for the storage space it consumes

udpatil · 2023-06-30T14:16:16Z

store/sized/store.go

+	// has to perform a read here to know the size change
+	value := s.Get(key)
+	s.parent.Delete(key)
+	if value != nil {


is it possible for a key to be set with a nil value (such that the key existed previously). in this case, we would still need to decrement for keyLen bytes, right?

it's not possible and validated here https://github.com/sei-protocol/sei-cosmos/blob/main/store/cachekv/store.go#L132

udpatil · 2023-06-30T14:16:57Z

store/sized/store.go

+// CacheWrap implements the KVStore interface. It panics as a Store
+// cannot be cache wrapped.
+func (s *Store) CacheWrap(_ types.StoreKey) types.CacheWrap {
+	panic("cannot CacheWrap a ListenKVStore")


just curious, what is the normal behavior of cachewrap, and why do we want it to panic here?

It's used for store types that need the ability the "branch out" on that particular inheritance level. Unfortunately a lot of functions' parameter are using the interface that includes these methods. Panicing for wrapper store types that don't need branching at their inheritance levels seem to be common practice for types like gaskv/listenkv/tracekv

udpatil · 2023-06-30T15:08:22Z

x/wasm/keeper/keeper.go

 	gas := k.runtimeGasForContract(ctx)
-	res, gasUsed, err := k.wasmVM.Migrate(newCodeInfo.CodeHash, env, msg, &prefixStore, cosmwasmAPI, &querier, k.gasMeter(ctx), gas, costJSONDeserialization)
+	res, gasUsed, err := k.wasmVM.Migrate(newCodeInfo.CodeHash, env, msg, sizedPrefixStore, cosmwasmAPI, &querier, k.gasMeter(ctx), gas, costJSONDeserialization)


is it ok that we're no longer passing in the pointer (see &prefixStore) (instead passing sized store)?

sizedPrefixStore is a pointer

udpatil · 2023-06-30T15:43:00Z

x/wasm/keeper/keeper.go

+		rentInfo.Balance -= rentToBeCharged
+	}
+	rentInfo.LastChargedBlock = uint64(ctx.BlockHeight())
+	k.setRentInfo(ctx, contractAddress, rentInfo)


should we put this after the error check for slight optimization (then the write doesnt have to occur to just rollback if the balance is negative)

hm good point i'm doing it wrong here now that you mentioned rollback. My intention was to persist the negative balance so that the contract owner would still be responsible for the accrued rent for his state since the last time rent was charged even if the most recent charge attempt results in out of rent, but forgot it wouldn't persist due to the rollback. We might need to do something like:

DeliverTx -> branch cachekv WASM execution -> branch of branch of cachekv Charge rent -> if success, commits branch of branch -> if out of rent, revert branch of branch DeliverTX -> branch cachekv commits/aborts regardless of out of rent or not

actually a better way would be to place the rent deduction logic into ante handler and the check in msgServer, so that we leverage the existing rollback infra instead of throwing in yet another layer

udpatil · 2023-06-30T15:44:25Z

x/wasm/keeper/keeper.go

+	}
+	rentInfo.LastChargedBlock = uint64(ctx.BlockHeight())
+	k.setRentInfo(ctx, contractAddress, rentInfo)
+	if rentInfo.Balance < 0 {


just to confirm, the expected behavior here is that if a contract has insufficient duration rent, then the contract is borked until the deposit rent tx is done, right? and this would result in any executes, migrates, etc to fail

yeah that's correct

udpatil · 2023-06-30T15:46:28Z

x/wasm/keeper/keeper.go

@@ -1081,6 +1215,18 @@ func (c BankCoinTransferrer) TransferCoins(parentCtx sdk.Context, fromAddr sdk.A
 	return nil
 }

+func (c BankCoinTransferrer) TransferCoinsToModule(ctx sdk.Context, fromAddr sdk.AccAddress, recipientModule string, amount sdk.Coins) error {


nit, we may want to use the deferred send to module for this if we expect a lot of these action to occur (assuming this only happens on deposit rent, so maybe its infrequent enough as to not degrade concurrency, but if it becomes problematic we can make this a concurrent safe process)

udpatil · 2023-06-30T15:49:14Z

x/wasm/keeper/keeper_test.go

+	assert.Equal(t, uint64(ctx.BlockHeight()), rentInfo.LastChargedBlock)
+
+	// bump block height
+	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1)


can we add a test case for two txs in the same block that modify state size each?

yeah good call

udpatil · 2023-06-30T15:51:27Z

x/wasm/types/rent.go

+	"errors"
+)
+
+// custom struct instead of protobuf to minimize state needed


why not just use protobuf for consistency with other store objects?

i was thinking that protobuf might be too heavy for such a simple struct but yeah i guess it probably won't be that bad. Better keep it consistent. Will change

github-advanced-security bot found potential problems Jun 26, 2023

View reviewed changes

x/wasm/keeper/keeper.go Fixed Show fixed Hide fixed

codchen force-pushed the tony-chen-rent-1 branch 2 times, most recently from 58c85e8 to 3d5783c Compare June 26, 2023 16:11

Implement contract state rent

b71a4e0

codchen force-pushed the tony-chen-rent-1 branch from 3d5783c to b71a4e0 Compare June 26, 2023 16:13

add test

cd21ee3

udpatil requested changes Jun 30, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement contract state rent #32

Implement contract state rent #32

codchen commented Jun 26, 2023

udpatil Jun 30, 2023

codchen Jul 3, 2023

udpatil Jun 30, 2023

codchen Jul 3, 2023

udpatil Jun 30, 2023

codchen Jul 3, 2023

udpatil Jun 30, 2023

codchen Jul 3, 2023

udpatil Jun 30, 2023

codchen Jul 3, 2023

udpatil Jun 30, 2023

codchen Jul 3, 2023 •

edited

Loading

codchen Jul 5, 2023

udpatil Jun 30, 2023

codchen Jul 3, 2023

udpatil Jun 30, 2023

udpatil Jun 30, 2023

codchen Jul 3, 2023

udpatil Jun 30, 2023

codchen Jul 3, 2023

Implement contract state rent #32

Are you sure you want to change the base?

Implement contract state rent #32

Conversation

codchen commented Jun 26, 2023

Summary

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codchen Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codchen Jul 3, 2023 •

edited

Loading