From 9a6a08b7fe3b94fcc0d9bce4fec05234ef392531 Mon Sep 17 00:00:00 2001 From: Pino' Surace Date: Wed, 11 Sep 2024 20:08:42 +0200 Subject: [PATCH 1/9] Start work on setup costs for in memory cache contracts --- app/ante.go | 1 + x/wasm/keeper/ante.go | 13 +++++++++++ x/wasm/keeper/keeper.go | 41 +++++++++++++++++++++++++++++------ x/wasm/keeper/recurse_test.go | 26 ++++++++++++---------- x/wasm/types/context.go | 17 +++++++++++++++ x/wasm/types/types.go | 15 +++++++++++++ 6 files changed, 95 insertions(+), 18 deletions(-) diff --git a/app/ante.go b/app/ante.go index 7a8f17e511..aeb139360d 100644 --- a/app/ante.go +++ b/app/ante.go @@ -55,6 +55,7 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { wasmkeeper.NewLimitSimulationGasDecorator(options.WasmConfig.SimulationGasLimit), // after setup context to enforce limits early wasmkeeper.NewCountTXDecorator(options.TXCounterStoreService), wasmkeeper.NewGasRegisterDecorator(options.WasmKeeper.GetGasRegister()), + wasmkeeper.NewTxContractsDecorator(), circuitante.NewCircuitBreakerDecorator(options.CircuitKeeper), ante.NewExtensionOptionsDecorator(options.ExtensionOptionChecker), ante.NewValidateBasicDecorator(), diff --git a/x/wasm/keeper/ante.go b/x/wasm/keeper/ante.go index 96bf474caf..051c651561 100644 --- a/x/wasm/keeper/ante.go +++ b/x/wasm/keeper/ante.go @@ -120,3 +120,16 @@ func NewGasRegisterDecorator(gr types.GasRegister) *GasRegisterDecorator { func (g GasRegisterDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { return next(types.WithGasRegister(ctx, g.gasRegister), tx, simulate) } + +type TxContractsDecorator struct{} + +// NewTxContractsDecorator constructor. +func NewTxContractsDecorator() *TxContractsDecorator { + return &TxContractsDecorator{} +} + +// AnteHandle adds the gas register to the context. +func (d TxContractsDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + txContracts := types.NewTxContracts() + return next(types.WithTxContracts(ctx, txContracts), tx, simulate) +} diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index 1879d432b3..31a89b306b 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -252,13 +252,17 @@ func (k Keeper) instantiate( return nil, nil, types.ErrEmpty.Wrap("creator") } sdkCtx := sdk.UnwrapSDKContext(ctx) - setupCost := k.gasRegister.SetupContractCost(k.IsPinnedCode(sdkCtx, codeID), len(initMsg)) - sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: instantiate") codeInfo := k.GetCodeInfo(ctx, codeID) if codeInfo == nil { return nil, nil, types.ErrNoSuchCodeFn(codeID).Wrapf("code id %d", codeID) } + + sdkCtx, discount := k.shouldGetDiscount(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(sdkCtx, codeID)) + setupCost := k.gasRegister.SetupContractCost(discount, len(initMsg)) + + sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: instantiate") + if !authPolicy.CanInstantiateContract(codeInfo.InstantiateConfig, creator) { return nil, nil, errorsmod.Wrap(sdkerrors.ErrUnauthorized, "can not instantiate") } @@ -395,7 +399,9 @@ func (k Keeper) execute(ctx context.Context, contractAddress, caller sdk.AccAddr return nil, err } - setupCost := k.gasRegister.SetupContractCost(k.IsPinnedCode(ctx, contractInfo.CodeID), len(msg)) + sdkCtx, discount := k.shouldGetDiscount(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) + setupCost := k.gasRegister.SetupContractCost(discount, len(msg)) + sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: execute") // add more funds @@ -555,7 +561,9 @@ func (k Keeper) callMigrateEntrypoint( msg []byte, newCodeID uint64, ) (*wasmvmtypes.Response, error) { - setupCost := k.gasRegister.SetupContractCost(k.IsPinnedCode(sdkCtx, newCodeID), len(msg)) + + sdkCtx, discount := k.shouldGetDiscount(sdkCtx, string(newChecksum), k.IsPinnedCode(sdkCtx, newCodeID)) + setupCost := k.gasRegister.SetupContractCost(discount, len(msg)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: migrate") env := types.NewEnv(sdkCtx, contractAddress) @@ -596,9 +604,10 @@ func (k Keeper) Sudo(ctx context.Context, contractAddress sdk.AccAddress, msg [] if err != nil { return nil, err } - - setupCost := k.gasRegister.SetupContractCost(k.IsPinnedCode(ctx, contractInfo.CodeID), len(msg)) sdkCtx := sdk.UnwrapSDKContext(ctx) + sdkCtx, discount := k.shouldGetDiscount(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) + setupCost := k.gasRegister.SetupContractCost(discount, len(msg)) + sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: sudo") env := types.NewEnv(sdkCtx, contractAddress) @@ -834,7 +843,8 @@ func (k Keeper) QuerySmart(ctx context.Context, contractAddr sdk.AccAddress, req return nil, err } - setupCost := k.gasRegister.SetupContractCost(k.IsPinnedCode(sdkCtx, contractInfo.CodeID), len(req)) + sdkCtx, discount := k.shouldGetDiscount(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) + setupCost := k.gasRegister.SetupContractCost(discount, len(req)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: query") // prepare querier @@ -1158,6 +1168,23 @@ func (k Keeper) IsPinnedCode(ctx context.Context, codeID uint64) bool { return ok } +func (k Keeper) shouldGetDiscount(ctx sdk.Context, checksum string, isPinned bool) (sdk.Context, bool) { + sdkCtx := sdk.UnwrapSDKContext(ctx) + if isPinned { + return sdkCtx, true + } + + txContracts, ok := types.TxContractsFromContext(ctx) + if !ok { + txContracts = types.NewTxContracts() + } else if txContracts.Exists(checksum) { + return sdkCtx, true + } + + txContracts.AddContract(checksum) + return types.WithTxContracts(sdkCtx, txContracts), false +} + // InitializePinnedCodes updates wasmvm to pin to cache all contracts marked as pinned func (k Keeper) InitializePinnedCodes(ctx context.Context) error { store := prefix.NewStore(runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx)), types.PinnedCodeIndexPrefix) diff --git a/x/wasm/keeper/recurse_test.go b/x/wasm/keeper/recurse_test.go index b3af85cb3d..c074c2a2c1 100644 --- a/x/wasm/keeper/recurse_test.go +++ b/x/wasm/keeper/recurse_test.go @@ -56,9 +56,11 @@ func initRecurseContract(t *testing.T) (contract sdk.AccAddress, ctx sdk.Context func TestGasCostOnQuery(t *testing.T) { const ( - GasNoWork uint64 = 63_968 + GasNoWork uint64 = 63_968 + GasNoWorkDiscounted uint64 = 5_968 // Note: about 100 SDK gas (10k CosmWasm gas) for each round of sha256 - GasWork50 uint64 = 64_207 // this is a little shy of 50k gas - to keep an eye on the limit + GasWork50 uint64 = 64_207 // this is a little shy of 50k gas - to keep an eye on the limit + GasWork50Discounted uint64 = 6_207 GasReturnUnhashed uint64 = 55 GasReturnHashed uint64 = 46 @@ -86,7 +88,7 @@ func TestGasCostOnQuery(t *testing.T) { msg: Recurse{ Depth: 1, }, - expectedGas: 2*GasNoWork + GasReturnUnhashed, + expectedGas: GasNoWork + GasNoWorkDiscounted + GasReturnUnhashed, }, "recursion 1, some work": { gasLimit: 400_000, @@ -94,7 +96,7 @@ func TestGasCostOnQuery(t *testing.T) { Depth: 1, Work: 50, }, - expectedGas: 2*GasWork50 + GasReturnHashed, + expectedGas: GasWork50 + GasWork50Discounted + GasReturnHashed, }, "recursion 4, some work": { gasLimit: 400_000, @@ -102,7 +104,7 @@ func TestGasCostOnQuery(t *testing.T) { Depth: 4, Work: 50, }, - expectedGas: 5*GasWork50 + 4*GasReturnHashed, + expectedGas: GasWork50 + 4*(GasWork50Discounted+GasReturnHashed), }, } @@ -172,8 +174,7 @@ func TestGasOnExternalQuery(t *testing.T) { expOutOfGas: true, }, "recursion 4, external gas limit": { - // this uses 244708 gas but give less - gasLimit: 4 * GasWork50, + gasLimit: GasWork50, msg: Recurse{ Depth: 4, Work: 50, @@ -212,6 +213,9 @@ func TestLimitRecursiveQueryGas(t *testing.T) { const ( // Note: about 100 SDK gas (10k CosmWasm gas) for each round of sha256 GasWork2k uint64 = 76_264 // = SetupContractCost + x // we have 6x gas used in cpu than in the instance + + GasWork2kDiscounted uint64 = 18_264 + // This is overhead for calling into a sub-contract GasReturnHashed uint64 = 48 ) @@ -240,13 +244,13 @@ func TestLimitRecursiveQueryGas(t *testing.T) { Work: 2000, }, expectQueriesFromContract: 5, - expectedGas: GasWork2k + 5*(GasWork2k+GasReturnHashed), + expectedGas: GasWork2k + 5*(GasWork2kDiscounted+GasReturnHashed), }, // this is where we expect an error... // it has enough gas to run 5 times and die on the 6th (5th time dispatching to sub-contract) // however, if we don't charge the cpu gas before sub-dispatching, we can recurse over 20 times "deep recursion, should die on 5th level": { - gasLimit: 400_000, + gasLimit: 150_000, msg: Recurse{ Depth: 50, Work: 2000, @@ -262,8 +266,8 @@ func TestLimitRecursiveQueryGas(t *testing.T) { }, expectQueriesFromContract: 10, expectOutOfGas: false, - expectError: "query wasm contract failed", // Error we get from the contract instance doing the failing query, not wasmd - expectedGas: 10*(GasWork2k+GasReturnHashed) + 3279, // lots of additional gas for long error message + expectError: "query wasm contract failed", // Error we get from the contract instance doing the failing query, not wasmd + expectedGas: GasWork2k + GasReturnHashed + 9*(GasWork2kDiscounted+GasReturnHashed) + 3279, // lots of additional gas for long error message }, } diff --git a/x/wasm/types/context.go b/x/wasm/types/context.go index 002cf01aad..92c027ca62 100644 --- a/x/wasm/types/context.go +++ b/x/wasm/types/context.go @@ -20,6 +20,9 @@ const ( contextKeyGasRegister = iota contextKeyCallDepth contextKey = iota + + // contracts in the current tx + contextKeyTxContracts contextKey = iota ) // WithTXCounter stores a transaction counter value in the context @@ -81,3 +84,17 @@ func GasRegisterFromContext(ctx context.Context) (GasRegister, bool) { val, ok := ctx.Value(contextKeyGasRegister).(GasRegister) return val, ok } + +// WithGasRegister stores the gas register into the context returned +func WithTxContracts(ctx sdk.Context, c TxContracts) sdk.Context { + if c == nil { + panic("c must not be nil") + } + return ctx.WithValue(contextKeyTxContracts, c) +} + +// GasRegisterFromContext reads the gas register from the context +func TxContractsFromContext(ctx context.Context) (TxContracts, bool) { + val, ok := ctx.Value(contextKeyTxContracts).(TxContracts) + return val, ok +} diff --git a/x/wasm/types/types.go b/x/wasm/types/types.go index 4b2a83e0d9..4885bd05c6 100644 --- a/x/wasm/types/types.go +++ b/x/wasm/types/types.go @@ -429,3 +429,18 @@ func (a AccessConfig) AllAuthorizedAddresses() []string { } return []string{} } + +type TxContracts map[string]struct{} + +func NewTxContracts() TxContracts { + return make(TxContracts, 0) +} + +func (a TxContracts) AddContract(checksum string) { + a[checksum] = struct{}{} +} + +func (a TxContracts) Exists(checksum string) bool { + _, ok := a[checksum] + return ok +} From 28768e3a36e51ec8692969ed210c49163b860a17 Mon Sep 17 00:00:00 2001 From: Pino' Surace Date: Thu, 12 Sep 2024 14:57:19 +0200 Subject: [PATCH 2/9] Format --- x/wasm/keeper/keeper.go | 1 - 1 file changed, 1 deletion(-) diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index 31a89b306b..07cd45e1e0 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -561,7 +561,6 @@ func (k Keeper) callMigrateEntrypoint( msg []byte, newCodeID uint64, ) (*wasmvmtypes.Response, error) { - sdkCtx, discount := k.shouldGetDiscount(sdkCtx, string(newChecksum), k.IsPinnedCode(sdkCtx, newCodeID)) setupCost := k.gasRegister.SetupContractCost(discount, len(msg)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: migrate") From bcc64d1805bf1efe95cadba3e4ef598b548f8562 Mon Sep 17 00:00:00 2001 From: Pino' Surace Date: Thu, 12 Sep 2024 16:21:24 +0200 Subject: [PATCH 3/9] Clean up --- x/wasm/keeper/ante.go | 2 +- x/wasm/keeper/keeper.go | 12 ++++++------ x/wasm/types/context.go | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/x/wasm/keeper/ante.go b/x/wasm/keeper/ante.go index 051c651561..89b2876147 100644 --- a/x/wasm/keeper/ante.go +++ b/x/wasm/keeper/ante.go @@ -128,7 +128,7 @@ func NewTxContractsDecorator() *TxContractsDecorator { return &TxContractsDecorator{} } -// AnteHandle adds the gas register to the context. +// AnteHandle initializes a new TxContracts object to the context. func (d TxContractsDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { txContracts := types.NewTxContracts() return next(types.WithTxContracts(ctx, txContracts), tx, simulate) diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index 07cd45e1e0..0a8498fb01 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -258,7 +258,7 @@ func (k Keeper) instantiate( return nil, nil, types.ErrNoSuchCodeFn(codeID).Wrapf("code id %d", codeID) } - sdkCtx, discount := k.shouldGetDiscount(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(sdkCtx, codeID)) + sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(sdkCtx, codeID)) setupCost := k.gasRegister.SetupContractCost(discount, len(initMsg)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: instantiate") @@ -399,7 +399,7 @@ func (k Keeper) execute(ctx context.Context, contractAddress, caller sdk.AccAddr return nil, err } - sdkCtx, discount := k.shouldGetDiscount(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) + sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) setupCost := k.gasRegister.SetupContractCost(discount, len(msg)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: execute") @@ -561,7 +561,7 @@ func (k Keeper) callMigrateEntrypoint( msg []byte, newCodeID uint64, ) (*wasmvmtypes.Response, error) { - sdkCtx, discount := k.shouldGetDiscount(sdkCtx, string(newChecksum), k.IsPinnedCode(sdkCtx, newCodeID)) + sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, string(newChecksum), k.IsPinnedCode(sdkCtx, newCodeID)) setupCost := k.gasRegister.SetupContractCost(discount, len(msg)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: migrate") @@ -604,7 +604,7 @@ func (k Keeper) Sudo(ctx context.Context, contractAddress sdk.AccAddress, msg [] return nil, err } sdkCtx := sdk.UnwrapSDKContext(ctx) - sdkCtx, discount := k.shouldGetDiscount(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) + sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) setupCost := k.gasRegister.SetupContractCost(discount, len(msg)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: sudo") @@ -842,7 +842,7 @@ func (k Keeper) QuerySmart(ctx context.Context, contractAddr sdk.AccAddress, req return nil, err } - sdkCtx, discount := k.shouldGetDiscount(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) + sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) setupCost := k.gasRegister.SetupContractCost(discount, len(req)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: query") @@ -1167,7 +1167,7 @@ func (k Keeper) IsPinnedCode(ctx context.Context, codeID uint64) bool { return ok } -func (k Keeper) shouldGetDiscount(ctx sdk.Context, checksum string, isPinned bool) (sdk.Context, bool) { +func (k Keeper) checkDiscountEligibility(ctx sdk.Context, checksum string, isPinned bool) (sdk.Context, bool) { sdkCtx := sdk.UnwrapSDKContext(ctx) if isPinned { return sdkCtx, true diff --git a/x/wasm/types/context.go b/x/wasm/types/context.go index 92c027ca62..1710d7d6bf 100644 --- a/x/wasm/types/context.go +++ b/x/wasm/types/context.go @@ -85,15 +85,15 @@ func GasRegisterFromContext(ctx context.Context) (GasRegister, bool) { return val, ok } -// WithGasRegister stores the gas register into the context returned +// WithTxContracts stores the tx contracts into the context returned func WithTxContracts(ctx sdk.Context, c TxContracts) sdk.Context { if c == nil { - panic("c must not be nil") + panic("tx contracts must not be nil") } return ctx.WithValue(contextKeyTxContracts, c) } -// GasRegisterFromContext reads the gas register from the context +// TxContractsFromContext reads the tx contracts from the context func TxContractsFromContext(ctx context.Context) (TxContracts, bool) { val, ok := ctx.Value(contextKeyTxContracts).(TxContracts) return val, ok From b4f077ab524a23fe34aa005a614cef43765e68f1 Mon Sep 17 00:00:00 2001 From: Pino' Surace Date: Thu, 12 Sep 2024 16:33:48 +0200 Subject: [PATCH 4/9] Handle discount in reply --- x/wasm/keeper/keeper.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index 0a8498fb01..8e4b37f7a7 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -648,8 +648,8 @@ func (k Keeper) reply(ctx sdk.Context, contractAddress sdk.AccAddress, reply was return nil, err } - // always consider this pinned - replyCosts := k.gasRegister.ReplyCosts(true, reply) + ctx, discount := k.checkDiscountEligibility(ctx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) + replyCosts := k.gasRegister.ReplyCosts(discount, reply) ctx.GasMeter().ConsumeGas(replyCosts, "Loading CosmWasm module: reply") env := types.NewEnv(ctx, contractAddress) From 46d721c74e6c6eab43b939c4b1e1904747dc603c Mon Sep 17 00:00:00 2001 From: Pino' Surace Date: Thu, 12 Sep 2024 17:44:03 +0200 Subject: [PATCH 5/9] Fix some tests --- x/wasm/keeper/relay_test.go | 2 +- x/wasm/keeper/submsg_test.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/x/wasm/keeper/relay_test.go b/x/wasm/keeper/relay_test.go index 69177a9538..42746fcf99 100644 --- a/x/wasm/keeper/relay_test.go +++ b/x/wasm/keeper/relay_test.go @@ -427,7 +427,7 @@ func TestOnRecvPacket(t *testing.T) { }, "submessage reply can overwrite ack data": { contractAddr: example.Contract, - expContractGas: types.DefaultInstanceCostDiscount + myContractGas + storageCosts, + expContractGas: types.DefaultInstanceCost + myContractGas + storageCosts + 1_000, contractResp: &wasmvmtypes.IBCReceiveResult{ Ok: &wasmvmtypes.IBCReceiveResponse{ Acknowledgement: []byte("myAck"), diff --git a/x/wasm/keeper/submsg_test.go b/x/wasm/keeper/submsg_test.go index b5e4a879ac..3bbf7fb70e 100644 --- a/x/wasm/keeper/submsg_test.go +++ b/x/wasm/keeper/submsg_test.go @@ -243,14 +243,14 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { "send tokens": { submsgID: 5, msg: validBankSend, - resultAssertions: []assertion{assertReturnedEvents(0), assertGasUsed(110_000, 112_000)}, + resultAssertions: []assertion{assertReturnedEvents(0), assertGasUsed(110_000, 113_000)}, }, "not enough tokens": { submsgID: 6, msg: invalidBankSend, subMsgError: true, // uses less gas than the send tokens (cost of bank transfer) - resultAssertions: []assertion{assertGasUsed(78_000, 81_000), assertErrorString("codespace: sdk, code: 5")}, + resultAssertions: []assertion{assertGasUsed(78_000, 82_000), assertErrorString("codespace: sdk, code: 5")}, }, "out of gas panic with no gas limit": { submsgID: 7, @@ -263,7 +263,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { msg: validBankSend, gasLimit: &subGasLimit, // uses same gas as call without limit (note we do not charge the 40k on reply) - resultAssertions: []assertion{assertReturnedEvents(0), assertGasUsed(110_000, 112_000)}, + resultAssertions: []assertion{assertReturnedEvents(0), assertGasUsed(110_000, 113_000)}, }, "not enough tokens with limit": { submsgID: 16, @@ -271,7 +271,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { subMsgError: true, gasLimit: &subGasLimit, // uses same gas as call without limit (note we do not charge the 40k on reply) - resultAssertions: []assertion{assertGasUsed(78_000, 81_000), assertErrorString("codespace: sdk, code: 5")}, + resultAssertions: []assertion{assertGasUsed(78_000, 82_000), assertErrorString("codespace: sdk, code: 5")}, }, "out of gas caught with gas limit": { submsgID: 17, @@ -279,7 +279,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { subMsgError: true, gasLimit: &subGasLimit, // uses all the subGasLimit, plus the 52k or so for the main contract - resultAssertions: []assertion{assertGasUsed(subGasLimit+75_000, subGasLimit+77_000), assertErrorString("codespace: sdk, code: 11")}, + resultAssertions: []assertion{assertGasUsed(subGasLimit+75_000, subGasLimit+78_000), assertErrorString("codespace: sdk, code: 11")}, }, "instantiate contract gets address in data and events": { submsgID: 21, From 40c766961c125b2a1a979fd5248d501cfa5b03f2 Mon Sep 17 00:00:00 2001 From: Pino' Surace Date: Tue, 17 Sep 2024 14:16:38 +0200 Subject: [PATCH 6/9] Fix comments --- x/wasm/keeper/keeper.go | 15 +++++++-------- x/wasm/keeper/relay_test.go | 2 +- x/wasm/keeper/submsg_test.go | 10 +++++----- x/wasm/types/context.go | 6 +++--- x/wasm/types/types.go | 24 ++++++++++++++++++------ 5 files changed, 34 insertions(+), 23 deletions(-) diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index 8e4b37f7a7..08a48f5840 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -258,7 +258,7 @@ func (k Keeper) instantiate( return nil, nil, types.ErrNoSuchCodeFn(codeID).Wrapf("code id %d", codeID) } - sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(sdkCtx, codeID)) + sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, codeInfo.CodeHash, k.IsPinnedCode(sdkCtx, codeID)) setupCost := k.gasRegister.SetupContractCost(discount, len(initMsg)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: instantiate") @@ -399,7 +399,7 @@ func (k Keeper) execute(ctx context.Context, contractAddress, caller sdk.AccAddr return nil, err } - sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) + sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, codeInfo.CodeHash, k.IsPinnedCode(ctx, contractInfo.CodeID)) setupCost := k.gasRegister.SetupContractCost(discount, len(msg)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: execute") @@ -561,7 +561,7 @@ func (k Keeper) callMigrateEntrypoint( msg []byte, newCodeID uint64, ) (*wasmvmtypes.Response, error) { - sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, string(newChecksum), k.IsPinnedCode(sdkCtx, newCodeID)) + sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, newChecksum, k.IsPinnedCode(sdkCtx, newCodeID)) setupCost := k.gasRegister.SetupContractCost(discount, len(msg)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: migrate") @@ -604,7 +604,7 @@ func (k Keeper) Sudo(ctx context.Context, contractAddress sdk.AccAddress, msg [] return nil, err } sdkCtx := sdk.UnwrapSDKContext(ctx) - sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) + sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, codeInfo.CodeHash, k.IsPinnedCode(ctx, contractInfo.CodeID)) setupCost := k.gasRegister.SetupContractCost(discount, len(msg)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: sudo") @@ -648,8 +648,7 @@ func (k Keeper) reply(ctx sdk.Context, contractAddress sdk.AccAddress, reply was return nil, err } - ctx, discount := k.checkDiscountEligibility(ctx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) - replyCosts := k.gasRegister.ReplyCosts(discount, reply) + replyCosts := k.gasRegister.ReplyCosts(true, reply) ctx.GasMeter().ConsumeGas(replyCosts, "Loading CosmWasm module: reply") env := types.NewEnv(ctx, contractAddress) @@ -842,7 +841,7 @@ func (k Keeper) QuerySmart(ctx context.Context, contractAddr sdk.AccAddress, req return nil, err } - sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, string(codeInfo.CodeHash), k.IsPinnedCode(ctx, contractInfo.CodeID)) + sdkCtx, discount := k.checkDiscountEligibility(sdkCtx, codeInfo.CodeHash, k.IsPinnedCode(ctx, contractInfo.CodeID)) setupCost := k.gasRegister.SetupContractCost(discount, len(req)) sdkCtx.GasMeter().ConsumeGas(setupCost, "Loading CosmWasm module: query") @@ -1167,7 +1166,7 @@ func (k Keeper) IsPinnedCode(ctx context.Context, codeID uint64) bool { return ok } -func (k Keeper) checkDiscountEligibility(ctx sdk.Context, checksum string, isPinned bool) (sdk.Context, bool) { +func (k Keeper) checkDiscountEligibility(ctx sdk.Context, checksum []byte, isPinned bool) (sdk.Context, bool) { sdkCtx := sdk.UnwrapSDKContext(ctx) if isPinned { return sdkCtx, true diff --git a/x/wasm/keeper/relay_test.go b/x/wasm/keeper/relay_test.go index 42746fcf99..69177a9538 100644 --- a/x/wasm/keeper/relay_test.go +++ b/x/wasm/keeper/relay_test.go @@ -427,7 +427,7 @@ func TestOnRecvPacket(t *testing.T) { }, "submessage reply can overwrite ack data": { contractAddr: example.Contract, - expContractGas: types.DefaultInstanceCost + myContractGas + storageCosts + 1_000, + expContractGas: types.DefaultInstanceCostDiscount + myContractGas + storageCosts, contractResp: &wasmvmtypes.IBCReceiveResult{ Ok: &wasmvmtypes.IBCReceiveResponse{ Acknowledgement: []byte("myAck"), diff --git a/x/wasm/keeper/submsg_test.go b/x/wasm/keeper/submsg_test.go index 3bbf7fb70e..b5e4a879ac 100644 --- a/x/wasm/keeper/submsg_test.go +++ b/x/wasm/keeper/submsg_test.go @@ -243,14 +243,14 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { "send tokens": { submsgID: 5, msg: validBankSend, - resultAssertions: []assertion{assertReturnedEvents(0), assertGasUsed(110_000, 113_000)}, + resultAssertions: []assertion{assertReturnedEvents(0), assertGasUsed(110_000, 112_000)}, }, "not enough tokens": { submsgID: 6, msg: invalidBankSend, subMsgError: true, // uses less gas than the send tokens (cost of bank transfer) - resultAssertions: []assertion{assertGasUsed(78_000, 82_000), assertErrorString("codespace: sdk, code: 5")}, + resultAssertions: []assertion{assertGasUsed(78_000, 81_000), assertErrorString("codespace: sdk, code: 5")}, }, "out of gas panic with no gas limit": { submsgID: 7, @@ -263,7 +263,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { msg: validBankSend, gasLimit: &subGasLimit, // uses same gas as call without limit (note we do not charge the 40k on reply) - resultAssertions: []assertion{assertReturnedEvents(0), assertGasUsed(110_000, 113_000)}, + resultAssertions: []assertion{assertReturnedEvents(0), assertGasUsed(110_000, 112_000)}, }, "not enough tokens with limit": { submsgID: 16, @@ -271,7 +271,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { subMsgError: true, gasLimit: &subGasLimit, // uses same gas as call without limit (note we do not charge the 40k on reply) - resultAssertions: []assertion{assertGasUsed(78_000, 82_000), assertErrorString("codespace: sdk, code: 5")}, + resultAssertions: []assertion{assertGasUsed(78_000, 81_000), assertErrorString("codespace: sdk, code: 5")}, }, "out of gas caught with gas limit": { submsgID: 17, @@ -279,7 +279,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { subMsgError: true, gasLimit: &subGasLimit, // uses all the subGasLimit, plus the 52k or so for the main contract - resultAssertions: []assertion{assertGasUsed(subGasLimit+75_000, subGasLimit+78_000), assertErrorString("codespace: sdk, code: 11")}, + resultAssertions: []assertion{assertGasUsed(subGasLimit+75_000, subGasLimit+77_000), assertErrorString("codespace: sdk, code: 11")}, }, "instantiate contract gets address in data and events": { submsgID: 21, diff --git a/x/wasm/types/context.go b/x/wasm/types/context.go index 1710d7d6bf..ca695a126b 100644 --- a/x/wasm/types/context.go +++ b/x/wasm/types/context.go @@ -86,11 +86,11 @@ func GasRegisterFromContext(ctx context.Context) (GasRegister, bool) { } // WithTxContracts stores the tx contracts into the context returned -func WithTxContracts(ctx sdk.Context, c TxContracts) sdk.Context { - if c == nil { +func WithTxContracts(ctx sdk.Context, tc TxContracts) sdk.Context { + if tc.GetContracts() == nil { panic("tx contracts must not be nil") } - return ctx.WithValue(contextKeyTxContracts, c) + return ctx.WithValue(contextKeyTxContracts, tc) } // TxContractsFromContext reads the tx contracts from the context diff --git a/x/wasm/types/types.go b/x/wasm/types/types.go index 4885bd05c6..b6cb2dc916 100644 --- a/x/wasm/types/types.go +++ b/x/wasm/types/types.go @@ -1,6 +1,7 @@ package types import ( + "encoding/hex" "fmt" "reflect" @@ -430,17 +431,28 @@ func (a AccessConfig) AllAuthorizedAddresses() []string { return []string{} } -type TxContracts map[string]struct{} +type txContracts map[string]struct{} + +type TxContracts struct { + contracts txContracts +} func NewTxContracts() TxContracts { - return make(TxContracts, 0) + c := make(txContracts, 0) + return TxContracts{contracts: c} } -func (a TxContracts) AddContract(checksum string) { - a[checksum] = struct{}{} +func (tc TxContracts) AddContract(checksum []byte) { + hexHash := hex.EncodeToString(checksum) + tc.contracts[hexHash] = struct{}{} } -func (a TxContracts) Exists(checksum string) bool { - _, ok := a[checksum] +func (tc TxContracts) Exists(checksum []byte) bool { + hexHash := hex.EncodeToString(checksum) + _, ok := tc.contracts[hexHash] return ok } + +func (tc TxContracts) GetContracts() txContracts { + return tc.contracts +} From b25bd2896973b0bab4c4b3736aa1a73176de4b2f Mon Sep 17 00:00:00 2001 From: Pino' Surace Date: Wed, 18 Sep 2024 18:57:52 +0200 Subject: [PATCH 7/9] Add tests --- x/wasm/keeper/ante_test.go | 79 +++++++++++++++++++++++ x/wasm/keeper/keeper.go | 12 ++-- x/wasm/keeper/keeper_test.go | 119 +++++++++++++++++++++++++++-------- x/wasm/keeper/test_common.go | 1 + x/wasm/types/types.go | 4 ++ x/wasm/types/types_test.go | 71 +++++++++++++++++++++ 6 files changed, 255 insertions(+), 31 deletions(-) diff --git a/x/wasm/keeper/ante_test.go b/x/wasm/keeper/ante_test.go index 5334964615..b1fe6af7b6 100644 --- a/x/wasm/keeper/ante_test.go +++ b/x/wasm/keeper/ante_test.go @@ -232,3 +232,82 @@ func TestGasRegisterDecorator(t *testing.T) { }) } } + +func TestTxContractsDecorator(t *testing.T) { + db := dbm.NewMemDB() + ms := store.NewCommitMultiStore(db, log.NewTestLogger(t), storemetrics.NewNoOpMetrics()) + + specs := map[string]struct { + empty bool + simulate bool + nextAssertAnte func(ctx sdk.Context, tx sdk.Tx, simulate bool) (sdk.Context, error) + }{ + "simulation - empty tx contracts": { + empty: true, + simulate: true, + nextAssertAnte: func(ctx sdk.Context, tx sdk.Tx, simulate bool) (sdk.Context, error) { + txContracts, ok := types.TxContractsFromContext(ctx) + assert.True(t, ok) + require.True(t, simulate) + require.Empty(t, txContracts.GetContracts()) + return ctx, nil + }, + }, + "not simulation - empty tx contracts": { + empty: true, + simulate: false, + nextAssertAnte: func(ctx sdk.Context, tx sdk.Tx, simulate bool) (sdk.Context, error) { + txContracts, ok := types.TxContractsFromContext(ctx) + assert.True(t, ok) + require.False(t, simulate) + require.Empty(t, txContracts.GetContracts()) + return ctx, nil + }, + }, + "simulation - not empty tx contracts": { + empty: false, + simulate: true, + nextAssertAnte: func(ctx sdk.Context, tx sdk.Tx, simulate bool) (sdk.Context, error) { + txContracts, ok := types.TxContractsFromContext(ctx) + assert.True(t, ok) + require.True(t, simulate) + require.Empty(t, txContracts.GetContracts()) + return ctx, nil + }, + }, + "not simulation - not empty tx contracts": { + empty: false, + simulate: false, + nextAssertAnte: func(ctx sdk.Context, tx sdk.Tx, simulate bool) (sdk.Context, error) { + txContracts, ok := types.TxContractsFromContext(ctx) + assert.True(t, ok) + require.False(t, simulate) + require.Empty(t, txContracts.GetContracts()) + return ctx, nil + }, + }, + } + for name, spec := range specs { + t.Run(name, func(t *testing.T) { + ctx := sdk.NewContext(ms, cmtproto.Header{ + Height: 100, + Time: time.Now(), + }, false, log.NewNopLogger()) + + if !spec.empty { + contracts := types.NewTxContracts() + contracts.AddContract([]byte("13a1fc994cc6d1c81b746ee0c0ff6f90043875e0bf1d9be6b7d779fc978dc2a5")) + ctx = types.WithTxContracts(ctx, contracts) + } + + var anyTx sdk.Tx + + // when + ante := keeper.NewTxContractsDecorator() + _, gotErr := ante.AnteHandle(ctx, anyTx, spec.simulate, spec.nextAssertAnte) + + // then + require.NoError(t, gotErr) + }) + } +} diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index 08a48f5840..8e4c11bfa0 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -1167,20 +1167,20 @@ func (k Keeper) IsPinnedCode(ctx context.Context, codeID uint64) bool { } func (k Keeper) checkDiscountEligibility(ctx sdk.Context, checksum []byte, isPinned bool) (sdk.Context, bool) { - sdkCtx := sdk.UnwrapSDKContext(ctx) if isPinned { - return sdkCtx, true + return ctx, true } txContracts, ok := types.TxContractsFromContext(ctx) - if !ok { - txContracts = types.NewTxContracts() + if !ok || txContracts.GetContracts() == nil { + // this should never happen because tx contracts are initialized in ante handler + panic("tx contracts must not be nil") } else if txContracts.Exists(checksum) { - return sdkCtx, true + return ctx, true } txContracts.AddContract(checksum) - return types.WithTxContracts(sdkCtx, txContracts), false + return types.WithTxContracts(ctx, txContracts), false } // InitializePinnedCodes updates wasmvm to pin to cache all contracts marked as pinned diff --git a/x/wasm/keeper/keeper_test.go b/x/wasm/keeper/keeper_test.go index a8f069bf77..e06de56e00 100644 --- a/x/wasm/keeper/keeper_test.go +++ b/x/wasm/keeper/keeper_test.go @@ -15,13 +15,18 @@ import ( wasmvmtypes "github.com/CosmWasm/wasmvm/v2/types" abci "github.com/cometbft/cometbft/abci/types" "github.com/cometbft/cometbft/libs/rand" + cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" tmproto "github.com/cometbft/cometbft/proto/tendermint/types" + dbm "github.com/cosmos/cosmos-db" fuzz "github.com/google/gofuzz" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" errorsmod "cosmossdk.io/errors" + "cosmossdk.io/log" sdkmath "cosmossdk.io/math" + "cosmossdk.io/store" + storemetrics "cosmossdk.io/store/metrics" storetypes "cosmossdk.io/store/types" "github.com/cosmos/cosmos-sdk/baseapp" @@ -335,31 +340,6 @@ func TestCreateWithSimulation(t *testing.T) { require.Equal(t, code, hackatomWasm) } -func TestIsSimulationMode(t *testing.T) { - specs := map[string]struct { - ctx sdk.Context - exp bool - }{ - "genesis block": { - ctx: sdk.Context{}.WithBlockHeader(tmproto.Header{}).WithGasMeter(storetypes.NewInfiniteGasMeter()), - exp: false, - }, - "any regular block": { - ctx: sdk.Context{}.WithBlockHeader(tmproto.Header{Height: 1}).WithGasMeter(storetypes.NewGasMeter(10000000)), - exp: false, - }, - "simulation": { - ctx: sdk.Context{}.WithBlockHeader(tmproto.Header{Height: 1}).WithGasMeter(storetypes.NewInfiniteGasMeter()), - exp: true, - }, - } - for msg := range specs { - t.Run(msg, func(t *testing.T) { - // assert.Equal(t, spec.exp, isSimulationMode(spec.ctx)) - }) - } -} - func TestCreateWithGzippedPayload(t *testing.T) { ctx, keepers := CreateTestInput(t, false, AvailableCapabilities) keeper := keepers.ContractKeeper @@ -2718,3 +2698,92 @@ func must[t any](s t, err error) t { } return s } + +func TestCheckDiscountEligibility(t *testing.T) { + _, keepers := CreateTestInput(t, false, AvailableCapabilities) + k := keepers.WasmKeeper + + db := dbm.NewMemDB() + ms := store.NewCommitMultiStore(db, log.NewTestLogger(t), storemetrics.NewNoOpMetrics()) + + specs := map[string]struct { + isPinned bool + initCtx func() sdk.Context + checksum []byte + expPanic bool + expDiscount bool + expLenTxContracts int + }{ + "checksum pinned": { + isPinned: true, + checksum: []byte("pinned checksum"), + initCtx: func() sdk.Context { + ctx := sdk.NewContext(ms, cmtproto.Header{ + Height: 100, + Time: time.Now(), + }, false, log.NewNopLogger()) + return types.WithTxContracts(ctx, types.NewTxContracts()) + }, + expDiscount: true, + expLenTxContracts: 0, + }, + "checksum unpinned - not in ctx": { + isPinned: false, + checksum: []byte("unpinned checksum"), + initCtx: func() sdk.Context { + ctx := sdk.NewContext(ms, cmtproto.Header{ + Height: 100, + Time: time.Now(), + }, false, log.NewNopLogger()) + return types.WithTxContracts(ctx, types.NewTxContracts()) + }, + expDiscount: false, + expLenTxContracts: 1, + }, + "checksum unpinned - already in ctx": { + isPinned: false, + checksum: []byte("unpinned checksum"), + initCtx: func() sdk.Context { + txContracts := types.NewTxContracts() + txContracts.AddContract([]byte("unpinned checksum")) + ctx := sdk.NewContext(ms, cmtproto.Header{ + Height: 100, + Time: time.Now(), + }, false, log.NewNopLogger()) + return types.WithTxContracts(ctx, txContracts) + }, + expDiscount: true, + expLenTxContracts: 1, + }, + "panic when tx contracts are not initialized": { + isPinned: false, + checksum: []byte("unpinned checksum"), + initCtx: func() sdk.Context { + ctx := sdk.NewContext(ms, cmtproto.Header{ + Height: 100, + Time: time.Now(), + }, false, log.NewNopLogger()) + return ctx + }, + expPanic: true, + }, + } + + for name, spec := range specs { + t.Run(name, func(t *testing.T) { + ctx := spec.initCtx() + if spec.expPanic { + assert.Panics(t, func() { + k.checkDiscountEligibility(ctx, spec.checksum, spec.isPinned) + }) + return + } + ctx, discount := k.checkDiscountEligibility(ctx, spec.checksum, spec.isPinned) + + assert.Equal(t, spec.expDiscount, discount) + txContracts, ok := types.TxContractsFromContext(ctx) + require.True(t, ok) + assert.Equal(t, spec.expLenTxContracts, len(txContracts.GetContracts())) + }) + } +} diff --git a/x/wasm/keeper/test_common.go b/x/wasm/keeper/test_common.go index 0314982612..4c1dd75751 100644 --- a/x/wasm/keeper/test_common.go +++ b/x/wasm/keeper/test_common.go @@ -256,6 +256,7 @@ func createTestInput( Time: time.Date(2020, time.April, 22, 12, 0, 0, 0, time.UTC), }, isCheckTx, log.NewNopLogger()) ctx = types.WithTXCounter(ctx, 0) + ctx = types.WithTxContracts(ctx, types.NewTxContracts()) encodingConfig := MakeEncodingConfig(t) appCodec, legacyAmino := encodingConfig.Codec, encodingConfig.Amino diff --git a/x/wasm/types/types.go b/x/wasm/types/types.go index b6cb2dc916..1aea6ef149 100644 --- a/x/wasm/types/types.go +++ b/x/wasm/types/types.go @@ -434,6 +434,7 @@ func (a AccessConfig) AllAuthorizedAddresses() []string { type txContracts map[string]struct{} type TxContracts struct { + // contracts contains the contracts (identified by checksum) which have already been executed in a transaction contracts txContracts } @@ -443,6 +444,9 @@ func NewTxContracts() TxContracts { } func (tc TxContracts) AddContract(checksum []byte) { + if len(checksum) == 0 { + return + } hexHash := hex.EncodeToString(checksum) tc.contracts[hexHash] = struct{}{} } diff --git a/x/wasm/types/types_test.go b/x/wasm/types/types_test.go index d00cf5ca3a..586c60c604 100644 --- a/x/wasm/types/types_test.go +++ b/x/wasm/types/types_test.go @@ -632,3 +632,74 @@ func TestContractCodeHistoryEntryValidation(t *testing.T) { }) } } + +func TestTxContractsAddContract(t *testing.T) { + specs := map[string]struct { + checksums [][]byte + expLen int + }{ + "single checksum": { + checksums: [][]byte{[]byte("checksum1")}, + expLen: 1, + }, + "duplicate checksums": { + checksums: [][]byte{[]byte("checksum1"), []byte("checksum1")}, + expLen: 1, + }, + "multiple checksums": { + checksums: [][]byte{[]byte("checksum1"), []byte("checksum2")}, + expLen: 2, + }, + "empty checksum": { + checksums: [][]byte{[]byte("")}, + expLen: 0, + }, + } + + for name, spec := range specs { + t.Run(name, func(t *testing.T) { + txContracts := NewTxContracts() + require.Empty(t, txContracts.GetContracts()) + + for _, c := range spec.checksums { + txContracts.AddContract(c) + } + + assert.Equal(t, spec.expLen, len(txContracts.GetContracts())) + }) + } +} + +func TestTxContractsExists(t *testing.T) { + specs := map[string]struct { + checksum []byte + expExists bool + }{ + "checksum exists": { + checksum: []byte("checksum1"), + expExists: true, + }, + "checksum does not exist": { + checksum: []byte("checksum3"), + expExists: false, + }, + "empty checksum": { + checksum: []byte(""), + expExists: false, + }, + } + + for name, spec := range specs { + t.Run(name, func(t *testing.T) { + txContracts := NewTxContracts() + require.Empty(t, txContracts.GetContracts()) + require.False(t, txContracts.Exists(spec.checksum)) + + // add checksums + txContracts.AddContract([]byte("checksum1")) + txContracts.AddContract([]byte("checksum2")) + + assert.Equal(t, spec.expExists, txContracts.Exists(spec.checksum)) + }) + } +} From ea05a69178ed63f3d71981caa0b2444f65b38876 Mon Sep 17 00:00:00 2001 From: Pino' Surace Date: Thu, 19 Sep 2024 14:23:55 +0200 Subject: [PATCH 8/9] Add log warning + fix tests --- x/wasm/keeper/keeper.go | 4 ++-- x/wasm/keeper/keeper_test.go | 19 ++++++++++--------- x/wasm/keeper/recurse_test.go | 4 ++++ x/wasm/keeper/test_common.go | 1 - 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index 8e4c11bfa0..a72ed59040 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -1173,8 +1173,8 @@ func (k Keeper) checkDiscountEligibility(ctx sdk.Context, checksum []byte, isPin txContracts, ok := types.TxContractsFromContext(ctx) if !ok || txContracts.GetContracts() == nil { - // this should never happen because tx contracts are initialized in ante handler - panic("tx contracts must not be nil") + k.Logger(ctx).Warn("cannot get tx contracts from context") + return ctx, false } else if txContracts.Exists(checksum) { return ctx, true } diff --git a/x/wasm/keeper/keeper_test.go b/x/wasm/keeper/keeper_test.go index e06de56e00..81c28517e1 100644 --- a/x/wasm/keeper/keeper_test.go +++ b/x/wasm/keeper/keeper_test.go @@ -2710,9 +2710,9 @@ func TestCheckDiscountEligibility(t *testing.T) { isPinned bool initCtx func() sdk.Context checksum []byte - expPanic bool expDiscount bool expLenTxContracts int + expNilContracts bool }{ "checksum pinned": { isPinned: true, @@ -2755,7 +2755,7 @@ func TestCheckDiscountEligibility(t *testing.T) { expDiscount: true, expLenTxContracts: 1, }, - "panic when tx contracts are not initialized": { + "no discount when tx contracts are not initialized": { isPinned: false, checksum: []byte("unpinned checksum"), initCtx: func() sdk.Context { @@ -2765,24 +2765,25 @@ func TestCheckDiscountEligibility(t *testing.T) { }, false, log.NewNopLogger()) return ctx }, - expPanic: true, + expDiscount: false, + expNilContracts: true, }, } for name, spec := range specs { t.Run(name, func(t *testing.T) { ctx := spec.initCtx() - if spec.expPanic { - assert.Panics(t, func() { - k.checkDiscountEligibility(ctx, spec.checksum, spec.isPinned) - }) - return - } ctx, discount := k.checkDiscountEligibility(ctx, spec.checksum, spec.isPinned) assert.Equal(t, spec.expDiscount, discount) txContracts, ok := types.TxContractsFromContext(ctx) + if spec.expNilContracts { + require.False(t, ok) + assert.Nil(t, txContracts.GetContracts()) + return + } require.True(t, ok) + assert.NotNil(t, txContracts.GetContracts()) assert.Equal(t, spec.expLenTxContracts, len(txContracts.GetContracts())) }) } diff --git a/x/wasm/keeper/recurse_test.go b/x/wasm/keeper/recurse_test.go index c074c2a2c1..de9d2f474b 100644 --- a/x/wasm/keeper/recurse_test.go +++ b/x/wasm/keeper/recurse_test.go @@ -117,6 +117,8 @@ func TestGasCostOnQuery(t *testing.T) { // make sure we set a limit before calling ctx = ctx.WithGasMeter(storetypes.NewGasMeter(tc.gasLimit)) + // init tx contracts in ctx + ctx = types.WithTxContracts(ctx, types.NewTxContracts()) require.Equal(t, uint64(0), ctx.GasMeter().GasConsumed()) // do the query @@ -280,6 +282,8 @@ func TestLimitRecursiveQueryGas(t *testing.T) { // make sure we set a limit before calling ctx = ctx.WithGasMeter(storetypes.NewGasMeter(tc.gasLimit)) + // init tx contracts in ctx + ctx = types.WithTxContracts(ctx, types.NewTxContracts()) require.Equal(t, uint64(0), ctx.GasMeter().GasConsumed()) // prepare the query diff --git a/x/wasm/keeper/test_common.go b/x/wasm/keeper/test_common.go index 4c1dd75751..0314982612 100644 --- a/x/wasm/keeper/test_common.go +++ b/x/wasm/keeper/test_common.go @@ -256,7 +256,6 @@ func createTestInput( Time: time.Date(2020, time.April, 22, 12, 0, 0, 0, time.UTC), }, isCheckTx, log.NewNopLogger()) ctx = types.WithTXCounter(ctx, 0) - ctx = types.WithTxContracts(ctx, types.NewTxContracts()) encodingConfig := MakeEncodingConfig(t) appCodec, legacyAmino := encodingConfig.Codec, encodingConfig.Amino From 308018d4fde8f93a8ad68b43226379a7ce71c9ed Mon Sep 17 00:00:00 2001 From: Pino' Surace <95283998+pinosu@users.noreply.github.com> Date: Wed, 25 Sep 2024 09:33:54 +0200 Subject: [PATCH 9/9] Update doc comment Co-authored-by: Christoph Otter --- x/wasm/keeper/ante.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/wasm/keeper/ante.go b/x/wasm/keeper/ante.go index 89b2876147..cf61c2825b 100644 --- a/x/wasm/keeper/ante.go +++ b/x/wasm/keeper/ante.go @@ -121,6 +121,7 @@ func (g GasRegisterDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bo return next(types.WithGasRegister(ctx, g.gasRegister), tx, simulate) } +// TxContractsDecorator implements an AnteHandler that keeps track of which contracts were already accessed during the current transaction. This allows discounting further calls to those contracts, as they are likely to be in the memory cache of the VM already. type TxContractsDecorator struct{} // NewTxContractsDecorator constructor.