Merge #5063

5063: GH-5058: Handle payment fix r=EdHastingsCasperAssociation a=mpapierski

This change fixes custom payment under `Transaction::Deploy` variant which incorrectly took session bytes, rather than payment field. Additionally, it does properly remove the named key when payment is finalized.

Closes #5058 

Co-authored-by: Michał Papierski <michal@casper.network>

node/src/components/contract_runtime/operations.rs

@@ -308,11 +308,13 @@ pub fn execute_finalized_block(
                 // in a custom way. If anything goes wrong, penalize the sender, do not execute
                 let custom_payment_gas_limit =
                     Gas::new(chainspec.transaction_config.native_transfer_minimum_motes * 5);
-                let session_input_data = transaction.to_session_input_data();
+
+                let payment_input_data = transaction.to_payment_input_data();
+
                 let pay_result = match WasmV1Request::new_custom_payment(
                     BlockInfo::new(state_root_hash, block_time, parent_block_hash, block_height),
                     custom_payment_gas_limit,
-                    &session_input_data,
+                    &payment_input_data,
                 ) {
                     Ok(mut pay_request) => {
                         // We'll send a hint to the custom payment logic on the amount

node/src/reactor/main_reactor/tests/transactions.rs

@@ -9,7 +9,8 @@ use casper_types::{
     addressable_entity::NamedKeyAddr,
     runtime_args,
     system::mint::{ARG_AMOUNT, ARG_TARGET},
-    AddressableEntity, Digest, EntityAddr, ExecutionInfo, TransactionRuntimeParams,
+    AddressableEntity, Digest, EntityAddr, ExecutableDeployItem, ExecutionInfo,
+    TransactionRuntimeParams,
 };
 use once_cell::sync::Lazy;
 
@@ -98,7 +99,7 @@ async fn send_wasm_transaction(
         TransactionV1Builder::new_session(
             false,
             module_bytes,
-            casper_types::TransactionRuntimeParams::VmCasperV1,
+            TransactionRuntimeParams::VmCasperV1,
         )
         .with_chain_name(chain_name)
         .with_pricing_mode(pricing)
@@ -1769,7 +1770,7 @@ async fn only_refunds_are_burnt_no_fee_custom_payment() {
         TransactionV1Builder::new_session(
             false,
             module_bytes,
-            casper_types::TransactionRuntimeParams::VmCasperV1,
+            TransactionRuntimeParams::VmCasperV1,
         )
         .with_chain_name(CHAIN_NAME)
         .with_pricing_mode(PricingMode::PaymentLimited {
@@ -2434,7 +2435,7 @@ fn invalid_wasm_txn(initiator: Arc<SecretKey>, pricing_mode: PricingMode) -> Tra
         TransactionV1Builder::new_session(
             false,
             module_bytes,
-            casper_types::TransactionRuntimeParams::VmCasperV1,
+            TransactionRuntimeParams::VmCasperV1,
         )
         .with_chain_name(CHAIN_NAME)
         .with_pricing_mode(pricing_mode)
@@ -3141,7 +3142,7 @@ async fn insufficient_funds_transfer_from_purse() {
         TransactionV1Builder::new_session(
             false,
             module_bytes,
-            casper_types::TransactionRuntimeParams::VmCasperV1,
+            TransactionRuntimeParams::VmCasperV1,
         )
         .with_runtime_args(runtime_args! { "destination" => purse_name, "amount" => U512::zero() })
         .with_chain_name(CHAIN_NAME)
@@ -3267,7 +3268,7 @@ async fn charge_when_session_code_succeeds() {
         TransactionV1Builder::new_session(
             false,
             module_bytes,
-            casper_types::TransactionRuntimeParams::VmCasperV1,
+            TransactionRuntimeParams::VmCasperV1,
         )
         .with_runtime_args(runtime_args! {
             ARG_TARGET => CHARLIE_PUBLIC_KEY.to_account_hash(),
@@ -3338,7 +3339,7 @@ async fn charge_when_session_code_fails_with_user_error() {
         TransactionV1Builder::new_session(
             false,
             module_bytes,
-            casper_types::TransactionRuntimeParams::VmCasperV1,
+            TransactionRuntimeParams::VmCasperV1,
         )
         .with_chain_name(CHAIN_NAME)
         .with_initiator_addr(BOB_PUBLIC_KEY.clone())
@@ -3406,7 +3407,7 @@ async fn charge_when_session_code_runs_out_of_gas() {
         TransactionV1Builder::new_session(
             false,
             module_bytes,
-            casper_types::TransactionRuntimeParams::VmCasperV1,
+            TransactionRuntimeParams::VmCasperV1,
         )
         .with_chain_name(CHAIN_NAME)
         .with_initiator_addr(BOB_PUBLIC_KEY.clone())
@@ -3732,7 +3733,7 @@ async fn out_of_gas_txn_does_not_produce_effects() {
         TransactionV1Builder::new_session(
             false,
             module_bytes,
-            casper_types::TransactionRuntimeParams::VmCasperV1,
+            TransactionRuntimeParams::VmCasperV1,
         )
         .with_chain_name(CHAIN_NAME)
         .with_initiator_addr(BOB_PUBLIC_KEY.clone())
@@ -3951,3 +3952,79 @@ async fn gas_holds_accumulate_for_multiple_transactions_in_the_same_block() {
         "Holds should have expired."
     );
 }
+
+#[tokio::test]
+async fn gh_5058_regression_custom_payment_with_deploy_variant_works() {
+    let config = SingleTransactionTestCase::default_test_config()
+        .with_pricing_handling(PricingHandling::Classic)
+        .with_refund_handling(RefundHandling::NoRefund)
+        .with_fee_handling(FeeHandling::NoFee);
+
+    let mut test = SingleTransactionTestCase::new(
+        ALICE_SECRET_KEY.clone(),
+        BOB_SECRET_KEY.clone(),
+        CHARLIE_SECRET_KEY.clone(),
+        Some(config),
+    )
+    .await;
+
+    test.fixture
+        .run_until_consensus_in_era(ERA_ONE, ONE_MIN)
+        .await;
+
+    // This WASM creates named key called "new_key". Then it would loop endlessly trying to write a
+    // value to storage. Eventually it will run out of gas and it should exit causing a revert.
+    let base_path = RESOURCES_PATH
+        .parent()
+        .unwrap()
+        .join("target")
+        .join("wasm32-unknown-unknown")
+        .join("release");
+
+    let payment_amount = U512::from(1_000_000u64);
+
+    let txn = {
+        let timestamp = Timestamp::now();
+        let ttl = TimeDiff::from_seconds(100);
+        let gas_price = 1;
+        let chain_name = test.chainspec().network_config.name.clone();
+
+        let payment = ExecutableDeployItem::ModuleBytes {
+            module_bytes: std::fs::read(base_path.join("gh_5058_regression.wasm"))
+                .unwrap()
+                .into(),
+            args: runtime_args! {
+                "amount" => payment_amount,
+                "this_is_payment" => true,
+            },
+        };
+
+        let session = ExecutableDeployItem::ModuleBytes {
+            module_bytes: std::fs::read(base_path.join("do_nothing.wasm"))
+                .unwrap()
+                .into(),
+            args: runtime_args! {
+                "this_is_session" => true,
+            },
+        };
+
+        Transaction::Deploy(Deploy::new_signed(
+            timestamp,
+            ttl,
+            gas_price,
+            vec![],
+            chain_name.clone(),
+            payment,
+            session,
+            &ALICE_SECRET_KEY,
+            Some(ALICE_PUBLIC_KEY.clone()),
+        ))
+    };
+
+    let acct = get_balance(&mut test.fixture, &ALICE_PUBLIC_KEY, None, true);
+    assert!(acct.total_balance().cloned().unwrap() >= payment_amount);
+
+    let (_txn_hash, _block_height, exec_result) = test.send_transaction(txn).await;
+
+    assert_eq!(exec_result.error_message(), None);
+}

node/src/types/transaction/meta_transaction.rs

@@ -284,6 +284,54 @@ impl MetaTransaction {
         }
     }
 
+    /// Returns the `SessionInputData` for a payment code if present.
+    pub fn to_payment_input_data(&self) -> SessionInputData {
+        match self {
+            MetaTransaction::Deploy(meta_deploy) => {
+                let initiator_addr =
+                    InitiatorAddr::PublicKey(meta_deploy.deploy().account().clone());
+                let is_standard_payment = matches!(meta_deploy.deploy().payment(), ExecutableDeployItem::ModuleBytes { module_bytes, .. } if module_bytes.is_empty());
+                let deploy = meta_deploy.deploy();
+                let data = SessionDataDeploy::new(
+                    deploy.hash(),
+                    deploy.payment(),
+                    initiator_addr,
+                    self.signers().clone(),
+                    is_standard_payment,
+                );
+                SessionInputData::DeploySessionData { data }
+            }
+            MetaTransaction::V1(v1) => {
+                let initiator_addr = v1.initiator_addr().clone();
+
+                let is_standard_payment = if let PricingMode::PaymentLimited {
+                    standard_payment,
+                    ..
+                } = v1.pricing_mode()
+                {
+                    *standard_payment
+                } else {
+                    true
+                };
+
+                // Under V1 transaction we don't have a separate payment code, and custom payment is
+                // executed as session code with a phase set to Payment.
+                let data = SessionDataV1::new(
+                    v1.args().as_named().expect("V1 wasm args should be named and validated at the transaction acceptor level"),
+                    v1.target(),
+                    v1.entry_point(),
+                    v1.lane_id() == INSTALL_UPGRADE_LANE_ID,
+                    v1.hash(),
+                    v1.pricing_mode(),
+                    initiator_addr,
+                    self.signers().clone(),
+                    is_standard_payment,
+                );
+                SessionInputData::SessionDataV1 { data }
+            }
+        }
+    }
+
     /// Size estimate.
     pub fn size_estimate(&self) -> usize {
         match self {

rust-toolchain.toml

@@ -1,2 +1,2 @@
 [toolchain]
-channel = "1.77.2"
+channel = "1.78.0"

smart_contracts/contracts/test/gh-5058-regression/Cargo.toml

@@ -0,0 +1,15 @@
+[package]
+name = "gh-5058-regression"
+version = "0.1.0"
+edition = "2021"
+
+[[bin]]
+name = "gh_5058_regression"
+path = "src/main.rs"
+bench = false
+doctest = false
+test = false
+
+[dependencies]
+casper-contract = { path = "../../../contract" }
+casper-types = { path = "../../../../types" }

smart_contracts/contracts/test/gh-5058-regression/src/main.rs

@@ -0,0 +1,61 @@
+#![no_main]
+#![no_std]
+
+extern crate alloc;
+
+use casper_contract::{
+    contract_api::{account, runtime, system},
+    unwrap_or_revert::UnwrapOrRevert,
+};
+
+use casper_types::{
+    runtime_args, system::handle_payment, ApiError, Phase, RuntimeArgs, URef, U512,
+};
+
+const ARG_AMOUNT: &str = "amount";
+
+#[repr(u16)]
+enum Error {
+    InvalidPhase,
+}
+
+impl From<Error> for ApiError {
+    fn from(e: Error) -> Self {
+        ApiError::User(e as u16)
+    }
+}
+
+fn get_payment_purse() -> URef {
+    runtime::call_contract(
+        system::get_handle_payment(),
+        handle_payment::METHOD_GET_PAYMENT_PURSE,
+        RuntimeArgs::default(),
+    )
+}
+
+fn set_refund_purse(new_refund_purse: URef) {
+    let args = runtime_args! {
+        handle_payment::ARG_PURSE => new_refund_purse,
+    };
+
+    runtime::call_contract(
+        system::get_handle_payment(),
+        handle_payment::METHOD_SET_REFUND_PURSE,
+        args,
+    )
+}
+
+#[no_mangle]
+pub extern "C" fn call() {
+    if runtime::get_phase() != Phase::Payment {
+        runtime::revert(Error::InvalidPhase);
+    }
+
+    let amount: U512 = runtime::get_named_arg(ARG_AMOUNT);
+    let payment_purse = get_payment_purse();
+    set_refund_purse(account::get_main_purse());
+
+    // transfer amount from named purse to payment purse, which will be used to pay for execution
+    system::transfer_from_purse_to_purse(account::get_main_purse(), payment_purse, amount, None)
+        .unwrap_or_revert();
+}

smart_contracts/sdk/src/lib.rs

@@ -342,16 +342,6 @@ impl<'a, T: ContractRef> ContractBuilder<'a, T> {
 
 #[cfg(test)]
 mod tests {
-    use super::*;
-
-    #[allow(dead_code)]
-    struct MyContract;
-
-    #[derive(BorshSerialize)]
-    struct DoSomethingArg {
-        foo: u64,
-    }
-
     // impl ToCallData for DoSomethingArg {
     //     const SELECTOR: Selector = Selector::new(1);
     //     type Return<'a> = ();

storage/src/data_access_layer/handle_refund.rs

@@ -83,12 +83,14 @@ impl HandleRefundMode {
     /// Returns the appropriate phase for the mode.
     pub fn phase(&self) -> Phase {
         match self {
-            HandleRefundMode::ClearRefundPurse
-            | HandleRefundMode::Burn { .. }
+            HandleRefundMode::Burn { .. }
             | HandleRefundMode::Refund { .. }
             | HandleRefundMode::CustomHold { .. }
             | HandleRefundMode::RefundAmount { .. } => Phase::FinalizePayment,
-            HandleRefundMode::SetRefundPurse { .. } => Phase::Payment,
+
+            HandleRefundMode::ClearRefundPurse | HandleRefundMode::SetRefundPurse { .. } => {
+                Phase::Payment
+            }
         }
     }
 }