From c0505ce55c3fa3cea37ebfc37a8654234c03d271 Mon Sep 17 00:00:00 2001 From: ptrus Date: Mon, 29 May 2023 19:50:20 +0200 Subject: [PATCH 1/3] accounts: emit Transfer events for fee payments/disbursements --- client-sdk/go/modules/accounts/accounts.go | 7 +++ runtime-sdk/modules/evm/src/lib.rs | 2 +- runtime-sdk/src/dispatcher.rs | 40 ++++++++---- runtime-sdk/src/module.rs | 8 +-- runtime-sdk/src/modules/accounts/mod.rs | 71 ++++++++++++++++------ runtime-sdk/src/modules/accounts/test.rs | 31 ++++++---- runtime-sdk/src/modules/core/mod.rs | 4 +- runtime-sdk/src/modules/core/test.rs | 5 +- tests/e2e/simplekvtest.go | 43 ++++++++----- 9 files changed, 144 insertions(+), 67 deletions(-) diff --git a/client-sdk/go/modules/accounts/accounts.go b/client-sdk/go/modules/accounts/accounts.go index 677c367d5a..0f7f66741b 100644 --- a/client-sdk/go/modules/accounts/accounts.go +++ b/client-sdk/go/modules/accounts/accounts.go @@ -10,6 +10,13 @@ import ( "github.com/oasisprotocol/oasis-sdk/client-sdk/go/types" ) +var ( + // CommonPoolAddress is the address of the internal common pool account in the accounts module. + CommonPoolAddress = types.NewAddressForModule("accounts", []byte("common-pool")) + // FeeAccumulatorAddress is the address of the internal fee accumulator account in the accounts module. + FeeAccumulatorAddress = types.NewAddressForModule("accounts", []byte("fee-accumulator")) +) + var ( // Callable methods. methodTransfer = types.NewMethodName("accounts.Transfer", Transfer{}) diff --git a/runtime-sdk/modules/evm/src/lib.rs b/runtime-sdk/modules/evm/src/lib.rs index 7cdc4b8aee..a8eb3e3b4b 100644 --- a/runtime-sdk/modules/evm/src/lib.rs +++ b/runtime-sdk/modules/evm/src/lib.rs @@ -591,7 +591,7 @@ impl Module { // Move the difference from the fee accumulator back to the caller. let caller_address = Cfg::map_address(source.into()); - Cfg::Accounts::move_from_fee_accumulator( + Cfg::Accounts::return_tx_fee( ctx, caller_address, &token::BaseUnits::new(return_fee.as_u128(), fee_denomination), diff --git a/runtime-sdk/src/dispatcher.rs b/runtime-sdk/src/dispatcher.rs index 47366448e4..ac285068be 100644 --- a/runtime-sdk/src/dispatcher.rs +++ b/runtime-sdk/src/dispatcher.rs @@ -187,6 +187,33 @@ impl Dispatcher { call: types::transaction::Call, opts: &DispatchOptions<'_>, ) -> (module::CallResult, callformat::Metadata) + where + C::Store: NestedStore, + { + let read_only = call.read_only; + + // Dispatch the call. + let (result, metadata) = Self::_dispatch_tx_call(ctx, call, opts); + + // Unconditionally call after handle call hook. + R::Modules::after_handle_call(ctx, &result); + + // Make sure that a read-only call did not result in any modifications. + if read_only && ctx.runtime_state().has_pending_updates() { + return ( + modules::core::Error::ReadOnlyTransaction.into_call_result(), + metadata, + ); + } + + (result, metadata) + } + + fn _dispatch_tx_call( + ctx: &mut C, + call: types::transaction::Call, + opts: &DispatchOptions<'_>, + ) -> (module::CallResult, callformat::Metadata) where C::Store: NestedStore, { @@ -224,19 +251,6 @@ impl Dispatcher { } }; - // Call after hook. - if let Err(e) = R::Modules::after_handle_call(ctx) { - return (e.into_call_result(), call_format_metadata); - } - - // Make sure that a read-only call did not result in any modifications. - if call.read_only && ctx.runtime_state().has_pending_updates() { - return ( - modules::core::Error::ReadOnlyTransaction.into_call_result(), - call_format_metadata, - ); - } - (result, call_format_metadata) } diff --git a/runtime-sdk/src/module.rs b/runtime-sdk/src/module.rs index 6686824d2f..ea67ece989 100644 --- a/runtime-sdk/src/module.rs +++ b/runtime-sdk/src/module.rs @@ -382,9 +382,8 @@ pub trait TransactionHandler { /// Perform any action after call, within the transaction context. /// /// If an error is returned the transaction call fails and updates are rolled back. - fn after_handle_call(_ctx: &mut C) -> Result<(), modules::core::Error> { + fn after_handle_call(_ctx: &mut C, _result: &CallResult) { // Default implementation doesn't do anything. - Ok(()) } /// Perform any action after dispatching the transaction, in batch context. @@ -438,9 +437,8 @@ impl TransactionHandler for Tuple { Ok(()) } - fn after_handle_call(ctx: &mut C) -> Result<(), modules::core::Error> { - for_tuples!( #( Tuple::after_handle_call(ctx)?; )* ); - Ok(()) + fn after_handle_call(ctx: &mut C, result: &CallResult) { + for_tuples!( #( Tuple::after_handle_call(ctx, result); )* ); } fn after_dispatch_tx(ctx: &mut C, tx_auth_info: &AuthInfo, result: &CallResult) { diff --git a/runtime-sdk/src/modules/accounts/mod.rs b/runtime-sdk/src/modules/accounts/mod.rs index 5193091a23..97645bcb14 100644 --- a/runtime-sdk/src/modules/accounts/mod.rs +++ b/runtime-sdk/src/modules/accounts/mod.rs @@ -218,15 +218,15 @@ pub trait API { denomination: &token::Denomination, ) -> Result; - /// Move amount from address into fee accumulator. - fn move_into_fee_accumulator( + /// Moves the amount into the per-transaction fee accumulator. + fn charge_tx_fee( ctx: &mut C, from: Address, amount: &token::BaseUnits, ) -> Result<(), modules::core::Error>; - /// Move amount from fee accumulator into address. - fn move_from_fee_accumulator( + /// Returns the amount from the per-transaction fee accumulator to the account. + fn return_tx_fee( ctx: &mut C, to: Address, amount: &token::BaseUnits, @@ -429,7 +429,9 @@ impl FeeAccumulator { } } -/// Context key for the fee accumulator. +/// Context key for the per-transaction fee accumulator. +const CONTEXT_KEY_TX_FEE_ACCUMULATOR: &str = "accounts.TxFeeAccumulator"; +/// Context key for the per block fee accumulator. const CONTEXT_KEY_FEE_ACCUMULATOR: &str = "accounts.FeeAccumulator"; impl API for Module { @@ -596,7 +598,7 @@ impl API for Module { .ok_or(Error::NotFound) } - fn move_into_fee_accumulator( + fn charge_tx_fee( ctx: &mut C, from: Address, amount: &token::BaseUnits, @@ -608,14 +610,14 @@ impl API for Module { Self::sub_amount(ctx.runtime_state(), from, amount) .map_err(|_| modules::core::Error::InsufficientFeeBalance)?; - ctx.value::(CONTEXT_KEY_FEE_ACCUMULATOR) + ctx.value::(CONTEXT_KEY_TX_FEE_ACCUMULATOR) .or_default() .add(amount); Ok(()) } - fn move_from_fee_accumulator( + fn return_tx_fee( ctx: &mut C, to: Address, amount: &token::BaseUnits, @@ -624,7 +626,7 @@ impl API for Module { return Ok(()); } - ctx.value::(CONTEXT_KEY_FEE_ACCUMULATOR) + ctx.value::(CONTEXT_KEY_TX_FEE_ACCUMULATOR) .or_default() .sub(amount) .map_err(|_| modules::core::Error::InsufficientFeeBalance)?; @@ -929,11 +931,9 @@ impl module::TransactionHandler for Module { .map_err(|_| modules::core::Error::InsufficientFeeBalance)?; } else { // Actually perform the move. - Self::move_into_fee_accumulator(ctx, payer, &tx.auth_info.fee.amount)?; + Self::charge_tx_fee(ctx, payer, &tx.auth_info.fee.amount)?; } - // TODO: Emit event that fee has been paid. - let gas_price = tx.auth_info.fee.gas_price(); // Set transaction priority. ::Core::set_priority( @@ -952,6 +952,30 @@ impl module::TransactionHandler for Module { Ok(()) } + fn after_handle_call(ctx: &mut C, _result: &module::CallResult) { + let acc = ctx + .value::(CONTEXT_KEY_TX_FEE_ACCUMULATOR) + .take() + .unwrap_or_default(); + + // Emit events for paid fees. + for (denom, amount) in acc.total_fees.iter() { + ctx.emit_unconditional_event(Event::Transfer { + from: ctx.tx_caller_address(), + to: *ADDRESS_FEE_ACCUMULATOR, + amount: token::BaseUnits::new(*amount, denom.clone()), + }); + } + + // Move transaction fees into the block fee accumulator. + let fee_acc = ctx + .value::(CONTEXT_KEY_FEE_ACCUMULATOR) + .or_default(); + for (denom, amount) in acc.total_fees.into_iter() { + fee_acc.add(&token::BaseUnits(amount, denom)); + } + } + fn after_dispatch_tx( ctx: &mut C, tx_auth_info: &AuthInfo, @@ -1029,18 +1053,29 @@ impl module::BlockHandler for Module { Self::add_amount(ctx.runtime_state(), address, amount) .expect("add_amount must succeed for fee disbursement"); + + // Emit transfer event for fee disbursement. + ctx.emit_event(Event::Transfer { + from: *ADDRESS_FEE_ACCUMULATOR, + to: address, + amount: amount.clone(), + }); } } } // Transfer remainder to a common pool account. for (denom, remainder) in previous_fees.into_iter() { - Self::add_amount( - ctx.runtime_state(), - *ADDRESS_COMMON_POOL, - &token::BaseUnits::new(remainder, denom), - ) - .expect("add_amount must succeed for transfer to common pool") + let amount = token::BaseUnits::new(remainder, denom); + Self::add_amount(ctx.runtime_state(), *ADDRESS_COMMON_POOL, &amount) + .expect("add_amount must succeed for transfer to common pool"); + + // Emit transfer event for fee disbursement. + ctx.emit_event(Event::Transfer { + from: *ADDRESS_FEE_ACCUMULATOR, + to: *ADDRESS_COMMON_POOL, + amount, + }) } // Fees for the active block should be transferred to the fee accumulator address. diff --git a/runtime-sdk/src/modules/accounts/test.rs b/runtime-sdk/src/modules/accounts/test.rs index 7e72123689..e105c42697 100644 --- a/runtime-sdk/src/modules/accounts/test.rs +++ b/runtime-sdk/src/modules/accounts/test.rs @@ -8,7 +8,7 @@ use anyhow::anyhow; use crate::{ context::{BatchContext, Context}, - module::{BlockHandler, InvariantHandler, MethodHandler, TransactionHandler}, + module::{self, BlockHandler, InvariantHandler, MethodHandler, TransactionHandler}, modules::{core, core::API as _}, testing::{keys, mock}, types::{ @@ -571,6 +571,15 @@ fn test_fee_disbursement() { // Authenticate transaction, fees should be moved to accumulator. Accounts::authenticate_tx(&mut ctx, &tx).expect("transaction authentication should succeed"); + ctx.with_tx(0, 0, tx, |mut tx_ctx, _call| { + // Run after call tx handler. + Accounts::after_handle_call( + &mut tx_ctx, + &module::CallResult::Ok(cbor::Value::Simple(cbor::SimpleValue::NullValue)), + ); + tx_ctx.commit() + }); + // Run end block handler. Accounts::end_block(&mut ctx); @@ -1019,14 +1028,14 @@ fn test_fee_acc() { init_accounts(&mut ctx); - // Check that Accounts::move_{into,from}_fee_accumulator work. + // Check that Accounts::{charge,return}_tx_fee work. ctx.with_tx(0, 0, mock::transaction(), |mut tx_ctx, _call| { - Accounts::move_into_fee_accumulator( + Accounts::charge_tx_fee( &mut tx_ctx, keys::alice::address(), &BaseUnits::new(1_000, Denomination::NATIVE), ) - .expect("move into should succeed"); + .expect("charge tx fee should succeed"); let ab = Accounts::get_balance( tx_ctx.runtime_state(), @@ -1036,12 +1045,12 @@ fn test_fee_acc() { .expect("get_balance should succeed"); assert_eq!(ab, 999_000, "balance in source account should be correct"); - Accounts::move_from_fee_accumulator( + Accounts::return_tx_fee( &mut tx_ctx, keys::alice::address(), &BaseUnits::new(1_000, Denomination::NATIVE), ) - .expect("move from should succeed"); + .expect("return tx fee should succeed"); let ab = Accounts::get_balance( tx_ctx.runtime_state(), @@ -1060,16 +1069,16 @@ fn test_fee_acc_sim() { init_accounts(&mut ctx); - // Check that Accounts::move_{into,from}_fee_accumulator don't do + // Check that Accounts::{charge,return}_tx_fee don't do // anything in simulation mode. ctx.with_simulation(|mut sctx| { sctx.with_tx(0, 0, mock::transaction(), |mut tx_ctx, _call| { - Accounts::move_into_fee_accumulator( + Accounts::charge_tx_fee( &mut tx_ctx, keys::alice::address(), &BaseUnits::new(1_000, Denomination::NATIVE), ) - .expect("move into should succeed"); + .expect("charge tx fee should succeed"); let ab = Accounts::get_balance( tx_ctx.runtime_state(), @@ -1079,12 +1088,12 @@ fn test_fee_acc_sim() { .expect("get_balance should succeed"); assert_eq!(ab, 1_000_000, "balance in source account should be correct"); - Accounts::move_from_fee_accumulator( + Accounts::return_tx_fee( &mut tx_ctx, keys::alice::address(), &BaseUnits::new(1_000, Denomination::NATIVE), ) - .expect("move from should succeed"); + .expect("return tx fee should succeed"); let ab = Accounts::get_balance( tx_ctx.runtime_state(), diff --git a/runtime-sdk/src/modules/core/mod.rs b/runtime-sdk/src/modules/core/mod.rs index 27ab6ecf03..e2decba4d6 100644 --- a/runtime-sdk/src/modules/core/mod.rs +++ b/runtime-sdk/src/modules/core/mod.rs @@ -999,14 +999,12 @@ impl module::TransactionHandler for Module { Ok(()) } - fn after_handle_call(ctx: &mut C) -> Result<(), Error> { + fn after_handle_call(ctx: &mut C, _result: &module::CallResult) { // Emit gas used event. if Cfg::EMIT_GAS_USED_EVENTS { let used_gas = Self::used_tx_gas(ctx); ctx.emit_unconditional_event(Event::GasUsed { amount: used_gas }); } - - Ok(()) } } diff --git a/runtime-sdk/src/modules/core/test.rs b/runtime-sdk/src/modules/core/test.rs index a0680d1dc9..20885f5ea6 100644 --- a/runtime-sdk/src/modules/core/test.rs +++ b/runtime-sdk/src/modules/core/test.rs @@ -1086,7 +1086,10 @@ fn test_gas_used_events() { let etags = ctx.with_tx(0, 0, tx, |mut tx_ctx, _call| { Core::use_tx_gas(&mut tx_ctx, 10).expect("using gas under limit should succeed"); assert_eq!(Core::used_tx_gas(&mut tx_ctx), 10); - Core::after_handle_call(&mut tx_ctx).unwrap(); + Core::after_handle_call( + &mut tx_ctx, + &module::CallResult::Ok(cbor::Value::Simple(cbor::SimpleValue::NullValue)), + ); let (etags, _) = tx_ctx.commit(); let tags = etags.clone().into_tags(); diff --git a/tests/e2e/simplekvtest.go b/tests/e2e/simplekvtest.go index fec5662bcc..ec8e75b99e 100644 --- a/tests/e2e/simplekvtest.go +++ b/tests/e2e/simplekvtest.go @@ -37,7 +37,7 @@ const EventWaitTimeout = 20 * time.Second const defaultGasAmount = 400 // expectedKVTransferGasUsed is the expected gas used by the kv transfer transaction. -const expectedKVTransferGasUsed = 373 +const expectedKVTransferGasUsed = 374 // expectedKVTransferFailGasUsed is the expected gas used by the failing kv transfer transaction. const expectedKVTransferFailGasUsed = 376 @@ -723,6 +723,7 @@ func KVTransferTest(sc *RuntimeScenario, log *logging.Logger, conn *grpc.ClientC log.Info("transferring 100 units from Alice to Bob") tb := ac.Transfer(testing.Bob.Address, types.NewBaseUnits(*quantity.NewFromUint64(100), types.NativeDenomination)). SetFeeGas(defaultGasAmount). + SetFeeAmount(types.NewBaseUnits(*quantity.NewFromUint64(1), types.NativeDenomination)). AppendAuthSignature(testing.Alice.SigSpec, nonce) _ = tb.AppendSign(ctx, testing.Alice.Signer) var meta *client.TransactionMeta @@ -749,29 +750,41 @@ func KVTransferTest(sc *RuntimeScenario, log *logging.Logger, conn *grpc.ClientC if err != nil { return fmt.Errorf("failed to fetch events: %w", err) } - expected := accounts.TransferEvent{ - From: testing.Alice.Address, - To: testing.Bob.Address, - Amount: types.NewBaseUnits(*quantity.NewFromUint64(100), types.NativeDenomination), - } - var gotTransfer bool + var gotTransfer, gotFee bool for _, ev := range evs { if ev.Transfer == nil { continue } transfer := ev.Transfer - if transfer.From != expected.From { + if transfer.From != testing.Alice.Address { // There can also be reward disbursements. continue } - if transfer.To != expected.To || transfer.Amount.Amount.Cmp(&expected.Amount.Amount) != 0 { - return fmt.Errorf("unexpected event, expected: %v, got: %v", expected, transfer) + switch transfer.To { + case testing.Bob.Address: + // Expected transfer event for the Alice->Bob transfer. + expected := types.NewBaseUnits(*quantity.NewFromUint64(100), types.NativeDenomination) + if transfer.Amount.Amount.Cmp(&expected.Amount) != 0 { + return fmt.Errorf("unexpected transfer event amount, expected: %v, got: %v", expected, transfer) + } + gotTransfer = true + case accounts.FeeAccumulatorAddress: + // Expected transfer event for the fee payment. + expected := types.NewBaseUnits(*quantity.NewFromUint64(1), types.NativeDenomination) + if transfer.Amount.Amount.Cmp(&expected.Amount) != 0 { + return fmt.Errorf("unexpected transfer event amount for fee payment, expected: %v, got: %v", expected, transfer) + } + gotFee = true + default: + return fmt.Errorf("unexpected transfer event for Alice to address: %v", transfer.To) } - gotTransfer = true } if !gotTransfer { return fmt.Errorf("did not receive the expected transfer event") } + if !gotFee { + return fmt.Errorf("did not receive the expected transfer event for fee payment") + } log.Info("checking Alice's account balance") ab, err := ac.Balances(ctx, client.RoundLatest, testing.Alice.Address) @@ -779,8 +792,8 @@ func KVTransferTest(sc *RuntimeScenario, log *logging.Logger, conn *grpc.ClientC return err } if q, ok := ab.Balances[types.NativeDenomination]; ok { - if q.Cmp(quantity.NewFromUint64(100_002_900)) != 0 { - return fmt.Errorf("Alice's account balance is wrong (expected 100002900, got %s)", q.String()) //nolint: stylecheck + if q.Cmp(quantity.NewFromUint64(100_002_899)) != 0 { + return fmt.Errorf("Alice's account balance is wrong (expected 100002899, got %s)", q.String()) //nolint: stylecheck } } else { return fmt.Errorf("Alice's account is missing native denomination balance") //nolint: stylecheck @@ -895,8 +908,8 @@ func KVDaveTest(sc *RuntimeScenario, log *logging.Logger, conn *grpc.ClientConn, return err } if q, ok := ab.Balances[types.NativeDenomination]; ok { - if q.Cmp(quantity.NewFromUint64(100_002_910)) != 0 { - return fmt.Errorf("Alice's account balance is wrong (expected 100002910, got %s)", q.String()) //nolint: stylecheck + if q.Cmp(quantity.NewFromUint64(100_002_909)) != 0 { + return fmt.Errorf("Alice's account balance is wrong (expected 100002909, got %s)", q.String()) //nolint: stylecheck } } else { return fmt.Errorf("Alice's account is missing native denomination balance") //nolint: stylecheck From 3b3e6adfa7ac1a0458a91484a64bd83b92a4fde1 Mon Sep 17 00:00:00 2001 From: ptrus Date: Mon, 5 Jun 2023 17:40:22 +0200 Subject: [PATCH 2/3] e2e/ConfidentialTest: test tx failure in `before_handle_call` --- tests/e2e/simplekvtest.go | 100 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 97 insertions(+), 3 deletions(-) diff --git a/tests/e2e/simplekvtest.go b/tests/e2e/simplekvtest.go index ec8e75b99e..47669800c6 100644 --- a/tests/e2e/simplekvtest.go +++ b/tests/e2e/simplekvtest.go @@ -4,16 +4,20 @@ import ( "bytes" "context" "crypto" + "crypto/rand" "fmt" - "math/rand" + mathRand "math/rand" "sort" "time" + "github.com/oasisprotocol/deoxysii" "google.golang.org/grpc" "github.com/oasisprotocol/oasis-core/go/common/cbor" "github.com/oasisprotocol/oasis-core/go/common/crypto/drbg" "github.com/oasisprotocol/oasis-core/go/common/crypto/mathrand" + mrae "github.com/oasisprotocol/oasis-core/go/common/crypto/mrae/api" + mraeDeoxysii "github.com/oasisprotocol/oasis-core/go/common/crypto/mrae/deoxysii" coreSignature "github.com/oasisprotocol/oasis-core/go/common/crypto/signature" "github.com/oasisprotocol/oasis-core/go/common/logging" "github.com/oasisprotocol/oasis-core/go/common/quantity" @@ -26,7 +30,6 @@ import ( "github.com/oasisprotocol/oasis-sdk/client-sdk/go/modules/rewards" "github.com/oasisprotocol/oasis-sdk/client-sdk/go/testing" "github.com/oasisprotocol/oasis-sdk/client-sdk/go/types" - "github.com/oasisprotocol/oasis-sdk/tests/e2e/txgen" ) @@ -36,6 +39,9 @@ const EventWaitTimeout = 20 * time.Second // defaultGasAmount is the default amount of gas to specify. const defaultGasAmount = 400 +// expectedConfidentialInvalidInsertGasUsed is the expected gas used by the invalid confidential insert transaction. +const expectedConfidentialInvalidInsertGasUsed = 417 + // expectedKVTransferGasUsed is the expected gas used by the kv transfer transaction. const expectedKVTransferGasUsed = 374 @@ -437,6 +443,94 @@ func ConfidentialTest(sc *RuntimeScenario, log *logging.Logger, conn *grpc.Clien return fmt.Errorf("fetching removed key should fail") } + // Following tests a specific runtime SDK case where transaction fails in the `before_handle_call` hook. + // This test ensures that `after_handle_call` hooks are called, even when the call fails in the `before_handle_call` hook. + // This tests a bugfix fixed in: #1380, where gas used events were not emitted in such scenarios. + log.Info("test transaction execution failure") + + // Makes the call invalid by negating the encoded read-only flag. This makes the call pass + // check transaction, but fail in execution after decryption, with: "invalid call format: read-only flag mismatch". + invalidCall := func(tx *types.Transaction) (*types.Call, error) { + var rsp *core.CallDataPublicKeyResponse + rsp, err = core.NewV1(rtc).CallDataPublicKey(ctx) + if err != nil { + return nil, fmt.Errorf("failed to query calldata X25519 public key: %w", err) + } + + // Generate ephemeral X25519 key pair. + pk, sk, kerr := mrae.GenerateKeyPair(rand.Reader) + if kerr != nil { + return nil, fmt.Errorf("failed to generate ephemeral X25519 key pair: %w", kerr) + } + // Generate random nonce. + var nonce [deoxysii.NonceSize]byte + if _, err = rand.Read(nonce[:]); err != nil { + return nil, fmt.Errorf("failed to generate random nonce: %w", err) + } + // Seal serialized plain call. + rawCall := cbor.Marshal(tx.Call) + sealedCall := mraeDeoxysii.Box.Seal(nil, nonce[:], rawCall, nil, &rsp.PublicKey.PublicKey, sk) + + encoded := &types.Call{ + Format: types.CallFormatEncryptedX25519DeoxysII, + Method: "", + Body: cbor.Marshal(&types.CallEnvelopeX25519DeoxysII{ + Pk: *pk, + Nonce: nonce, + Data: sealedCall, + }), + ReadOnly: !tx.Call.ReadOnly, // Use inverted read-only flag to cause an error during transaction execution. + } + return encoded, nil + } + + nonce, err = ac.Nonce(ctx, client.RoundLatest, types.NewAddress(sigspecForSigner(signer))) + if err != nil { + return fmt.Errorf("failed to query nonce: %w", err) + } + tx := types.NewTransaction(nil, "keyvalue.Insert", kvKeyValue{ + Key: testKey, + Value: testValue, + }) + tx.AuthInfo.Fee.Gas = 2 * defaultGasAmount + + call, err := invalidCall(tx) + if err != nil { + return err + } + tx.Call = *call + tx.AppendAuthSignature(sigspecForSigner(signer), nonce) + ts := tx.PrepareForSigning() + rtInfo, err := rtc.GetInfo(ctx) + if err != nil { + return fmt.Errorf("failed to retrieve runtime info: %w", err) + } + if err = ts.AppendSign(rtInfo.ChainContext, signer); err != nil { + return err + } + // Ensure that the transaction failed during execution. + meta, err := rtc.SubmitTxMeta(ctx, ts.UnverifiedTransaction()) + if err == nil { + return fmt.Errorf("invalid transaction should have failed") + } + if meta == nil { + return fmt.Errorf("meta should not be nil") + } + if meta.CheckTxError != nil { + return fmt.Errorf("transaction should not fail during check transaction: %v", meta.CheckTxError) + } + // Ensure that the expected gas used event was emitted. + cevs, err := core.NewV1(rtc).GetEvents(ctx, meta.Round) + if err != nil { + return err + } + if len(cevs) != 1 { + return fmt.Errorf("expected 1 core event, got %d", len(cevs)) + } + if cevs[0].GasUsed.Amount != expectedConfidentialInvalidInsertGasUsed { + return fmt.Errorf("expected gas used %d, got %d", expectedConfidentialInvalidInsertGasUsed, cevs[0].GasUsed.Amount) + } + return nil } @@ -1057,7 +1151,7 @@ func KVTxGenTest(sc *RuntimeScenario, log *logging.Logger, conn *grpc.ClientConn if err != nil { return err } - rng := rand.New(mathrand.New(rngSrc)) //nolint: gosec + rng := mathRand.New(mathrand.New(rngSrc)) //nolint:gosec // Generate accounts. log.Info("generating accounts", "num_accounts", numAccounts, "coins_per_account", coinsPerAccount, "rng_seed", seed) From 859d5b7bc5a700fa77e46bac600d018185ff2b46 Mon Sep 17 00:00:00 2001 From: ptrus Date: Tue, 6 Jun 2023 15:18:16 +0200 Subject: [PATCH 3/3] runtime-sdk: support aborting in `after_handle_call` --- runtime-sdk/src/dispatcher.rs | 8 +++++++- runtime-sdk/src/module.rs | 16 +++++++++++++--- runtime-sdk/src/modules/accounts/mod.rs | 7 ++++++- runtime-sdk/src/modules/accounts/test.rs | 5 +++-- runtime-sdk/src/modules/core/mod.rs | 7 ++++++- runtime-sdk/src/modules/core/test.rs | 5 +++-- 6 files changed, 38 insertions(+), 10 deletions(-) diff --git a/runtime-sdk/src/dispatcher.rs b/runtime-sdk/src/dispatcher.rs index ac285068be..80099c1093 100644 --- a/runtime-sdk/src/dispatcher.rs +++ b/runtime-sdk/src/dispatcher.rs @@ -196,7 +196,13 @@ impl Dispatcher { let (result, metadata) = Self::_dispatch_tx_call(ctx, call, opts); // Unconditionally call after handle call hook. - R::Modules::after_handle_call(ctx, &result); + let result = match R::Modules::after_handle_call(ctx, result) { + Ok(result) => result, + Err(e) => { + // If the call failed, return the error. + return (e.into_call_result(), metadata); + } + }; // Make sure that a read-only call did not result in any modifications. if read_only && ctx.runtime_state().has_pending_updates() { diff --git a/runtime-sdk/src/module.rs b/runtime-sdk/src/module.rs index ea67ece989..a6dc25f5a4 100644 --- a/runtime-sdk/src/module.rs +++ b/runtime-sdk/src/module.rs @@ -382,8 +382,12 @@ pub trait TransactionHandler { /// Perform any action after call, within the transaction context. /// /// If an error is returned the transaction call fails and updates are rolled back. - fn after_handle_call(_ctx: &mut C, _result: &CallResult) { + fn after_handle_call( + _ctx: &mut C, + result: CallResult, + ) -> Result { // Default implementation doesn't do anything. + Ok(result) } /// Perform any action after dispatching the transaction, in batch context. @@ -437,8 +441,14 @@ impl TransactionHandler for Tuple { Ok(()) } - fn after_handle_call(ctx: &mut C, result: &CallResult) { - for_tuples!( #( Tuple::after_handle_call(ctx, result); )* ); + fn after_handle_call( + ctx: &mut C, + mut result: CallResult, + ) -> Result { + for_tuples!( #( + result = Tuple::after_handle_call(ctx, result)?; + )* ); + Ok(result) } fn after_dispatch_tx(ctx: &mut C, tx_auth_info: &AuthInfo, result: &CallResult) { diff --git a/runtime-sdk/src/modules/accounts/mod.rs b/runtime-sdk/src/modules/accounts/mod.rs index 97645bcb14..aa9ca52280 100644 --- a/runtime-sdk/src/modules/accounts/mod.rs +++ b/runtime-sdk/src/modules/accounts/mod.rs @@ -952,7 +952,10 @@ impl module::TransactionHandler for Module { Ok(()) } - fn after_handle_call(ctx: &mut C, _result: &module::CallResult) { + fn after_handle_call( + ctx: &mut C, + result: module::CallResult, + ) -> Result { let acc = ctx .value::(CONTEXT_KEY_TX_FEE_ACCUMULATOR) .take() @@ -974,6 +977,8 @@ impl module::TransactionHandler for Module { for (denom, amount) in acc.total_fees.into_iter() { fee_acc.add(&token::BaseUnits(amount, denom)); } + + Ok(result) } fn after_dispatch_tx( diff --git a/runtime-sdk/src/modules/accounts/test.rs b/runtime-sdk/src/modules/accounts/test.rs index e105c42697..5bf576d14f 100644 --- a/runtime-sdk/src/modules/accounts/test.rs +++ b/runtime-sdk/src/modules/accounts/test.rs @@ -575,8 +575,9 @@ fn test_fee_disbursement() { // Run after call tx handler. Accounts::after_handle_call( &mut tx_ctx, - &module::CallResult::Ok(cbor::Value::Simple(cbor::SimpleValue::NullValue)), - ); + module::CallResult::Ok(cbor::Value::Simple(cbor::SimpleValue::NullValue)), + ) + .expect("after_handle_call should succeed"); tx_ctx.commit() }); diff --git a/runtime-sdk/src/modules/core/mod.rs b/runtime-sdk/src/modules/core/mod.rs index e2decba4d6..a6519e09c0 100644 --- a/runtime-sdk/src/modules/core/mod.rs +++ b/runtime-sdk/src/modules/core/mod.rs @@ -999,12 +999,17 @@ impl module::TransactionHandler for Module { Ok(()) } - fn after_handle_call(ctx: &mut C, _result: &module::CallResult) { + fn after_handle_call( + ctx: &mut C, + result: module::CallResult, + ) -> Result { // Emit gas used event. if Cfg::EMIT_GAS_USED_EVENTS { let used_gas = Self::used_tx_gas(ctx); ctx.emit_unconditional_event(Event::GasUsed { amount: used_gas }); } + + Ok(result) } } diff --git a/runtime-sdk/src/modules/core/test.rs b/runtime-sdk/src/modules/core/test.rs index 20885f5ea6..de9f66e1de 100644 --- a/runtime-sdk/src/modules/core/test.rs +++ b/runtime-sdk/src/modules/core/test.rs @@ -1088,8 +1088,9 @@ fn test_gas_used_events() { assert_eq!(Core::used_tx_gas(&mut tx_ctx), 10); Core::after_handle_call( &mut tx_ctx, - &module::CallResult::Ok(cbor::Value::Simple(cbor::SimpleValue::NullValue)), - ); + module::CallResult::Ok(cbor::Value::Simple(cbor::SimpleValue::NullValue)), + ) + .expect("after_handle_call should succeed"); let (etags, _) = tx_ctx.commit(); let tags = etags.clone().into_tags();