Merge #4912

4912: [BUGFIX] misc payment edge cases r=EdHastingsCasperAssociation a=EdHastingsCasperAssociation

Addresses #4782 #4791 and governance-95


Co-authored-by: Ed Hastings <[email protected]>

node/src/components/contract_runtime/operations.rs

@@ -11,6 +11,7 @@ use casper_storage::{
  data_access_layer::{
  balance::BalanceHandling,
  forced_undelegate::{ForcedUndelegateRequest, ForcedUndelegateResult},
+ mint::BalanceIdentifierTransferArgs,
  AuctionMethod, BalanceHoldKind, BalanceHoldRequest, BalanceIdentifier, BalanceRequest,
  BiddingRequest, BlockGlobalRequest, BlockGlobalResult, BlockRewardsRequest,
  BlockRewardsResult, DataAccessLayer, EraValidatorsRequest, EraValidatorsResult, EvictItem,
@@ -96,6 +97,7 @@ pub fn execute_finalized_block(
  let insufficient_balance_handling = InsufficientBalanceHandling::HoldRemaining;
  let refund_handling = chainspec.core_config.refund_handling;
  let fee_handling = chainspec.core_config.fee_handling;
+ let penalty_payment_amount = *casper_execution_engine::engine_state::MAX_PAYMENT;
  let balance_handling = BalanceHandling::Available;
 
  // get scratch state, which must be used for all processing and post processing data
@@ -131,7 +133,7 @@ pub fn execute_finalized_block(
  return Err(BlockExecutionError::RootNotFound(state_root_hash));
  }
  BlockGlobalResult::Failure(err) => {
- return Err(BlockExecutionError::BlockGlobal(format!("{}", err)));
+ return Err(BlockExecutionError::BlockGlobal(format!("{:?}", err)));
  }
  BlockGlobalResult::Success {
  post_state_hash, ..
@@ -196,7 +198,7 @@ pub fn execute_finalized_block(
  let is_standard_payment = transaction.is_standard_payment();
  let refund_purse_active = !is_standard_payment;
  if refund_purse_active {
- // if custom payment  before doing any processing, initialize the initiator's main purse
+ // if custom payment before doing any processing, initialize the initiator's main purse
  // to be the refund purse for this transaction.
  // NOTE: when executed, custom payment logic has the option to call set_refund_purse
  // on the handle payment contract to set up a different refund purse, if desired.
@@ -217,12 +219,37 @@ pub fn execute_finalized_block(
  return Err(BlockExecutionError::RootNotFound(state_root_hash));
  }
  artifacts.push(artifact_builder.build());
- continue; // no reason to commit the effects, move on
+ continue; // don't commit effects, move on
  }
  state_root_hash = scratch_state
  .commit_effects(state_root_hash, handle_refund_result.effects().clone())?;
  }
 
+ {
+ // Ensure the initiator's main purse can cover the penalty payment before proceeding.
+ let initial_balance_result = scratch_state.balance(BalanceRequest::new(
+ state_root_hash,
+ protocol_version,
+ initiator_addr.clone().into(),
+ balance_handling,
+ ProofHandling::NoProofs,
+ ));
+
+ if let Err(root_not_found) = artifact_builder
+ .with_initial_balance_result(initial_balance_result.clone(), penalty_payment_amount)
+ {
+ if root_not_found {
+ return Err(BlockExecutionError::RootNotFound(state_root_hash));
+ }
+ trace!(%transaction_hash, "insufficient initial balance");
+ debug!(%transaction_hash, ?initial_balance_result, %penalty_payment_amount, "insufficient initial balance");
+ artifacts.push(artifact_builder.build());
+ // only reads have happened so far, and we can't charge due
+ // to insufficient balance, so move on with no effects committed
+ continue;
+ }
+ }
+
  let mut balance_identifier = {
  if is_standard_payment {
  // this is the typical scenario; the initiating account pays using its main
@@ -257,7 +284,29 @@ pub fn execute_finalized_block(
  };
  let balance_identifier = {
  if pay_result.error().is_some() {
- BalanceIdentifier::PenalizedAccount(initiator_addr.account_hash())
+ // Charge initiator for the penalty payment amount
+ // the most expedient way to do this that aligns with later code
+ // is to transfer from the initiator's main purse to the payment purse
+ let transfer_result =
+ scratch_state.transfer(TransferRequest::new_indirect(
+ native_runtime_config.clone(),
+ state_root_hash,
+ protocol_version,
+ transaction_hash,
+ initiator_addr.clone(),
+ authorization_keys.clone(),
+ BalanceIdentifierTransferArgs::new(
+ None,
+ BalanceIdentifier::Account(initiator_addr.account_hash()),
+ BalanceIdentifier::Payment,
+ penalty_payment_amount,
+ None,
+ ),
+ ));
+ artifact_builder
+ .with_transfer_result(transfer_result)
+ .map_err(|_| BlockExecutionError::RootNotFound(state_root_hash))?;
+ BalanceIdentifier::PenalizedPayment
  } else {
  BalanceIdentifier::Payment
  }
@@ -272,20 +321,19 @@ pub fn execute_finalized_block(
  }
  };
 
- let initial_balance_result = scratch_state.balance(BalanceRequest::new(
+ let category = transaction.transaction_category();
+ let post_payment_balance_result = scratch_state.balance(BalanceRequest::new(
  state_root_hash,
  protocol_version,
  balance_identifier.clone(),
  balance_handling,
  ProofHandling::NoProofs,
  ));
 
- let category = transaction.transaction_category();
-
  let allow_execution = {
- let is_not_penalized = !balance_identifier.is_penalty();
- let sufficient_balance = initial_balance_result.is_sufficient(cost);
  let is_supported = chainspec.is_supported(category);
+ let is_not_penalized = !balance_identifier.is_penalty();
+ let sufficient_balance = post_payment_balance_result.is_sufficient(cost);
  trace!(%transaction_hash, ?sufficient_balance, ?is_not_penalized, ?is_supported, "payment preprocessing");
  is_not_penalized && sufficient_balance && is_supported
  };
@@ -575,6 +623,7 @@ pub fn execute_finalized_block(
  };
  state_root_hash =
  scratch_state.commit_effects(state_root_hash, handle_fee_result.effects().clone())?;
+
  artifact_builder
  .with_handle_fee_result(&handle_fee_result)
  .map_err(|_| BlockExecutionError::RootNotFound(state_root_hash))?;

node/src/components/contract_runtime/types.rs

@@ -10,8 +10,8 @@ use casper_execution_engine::engine_state::{
 use casper_storage::{
  block_store::types::ApprovalsHashes,
  data_access_layer::{
- auction::AuctionMethodError, BalanceHoldResult, BiddingResult, EraValidatorsRequest,
- HandleFeeResult, HandleRefundResult, TransferResult,
+ auction::AuctionMethodError, BalanceHoldResult, BalanceResult, BiddingResult,
+ EraValidatorsRequest, HandleFeeResult, HandleRefundResult, TransferResult,
  },
 };
 use casper_types::{
@@ -119,16 +119,43 @@ impl ExecutionArtifactBuilder {
  self
  }
 
+ pub fn with_initial_balance_result(
+ &mut self,
+ balance_result: BalanceResult,
+ minimum_amount: U512,
+ ) -> Result<&mut Self, bool> {
+ if let BalanceResult::RootNotFound = balance_result {
+ return Err(true);
+ }
+ if let (None, Some(err)) = (&self.error_message, balance_result.error()) {
+ self.error_message = Some(format!("{}", err));
+ return Err(false);
+ }
+ if let Some(purse) = balance_result.purse_addr() {
+ let is_sufficient = balance_result.is_sufficient(minimum_amount);
+ if !is_sufficient {
+ self.error_message = Some(format!(
+ "Purse {} has less than {}",
+ base16::encode_lower(&purse),
+ minimum_amount
+ ));
+ return Ok(self);
+ }
+ }
+ Ok(self)
+ }
+
  pub fn with_wasm_v1_result(&mut self, wasm_v1_result: WasmV1Result) -> Result<&mut Self, ()> {
  if let Some(Error::RootNotFound(_)) = wasm_v1_result.error() {
  return Err(());
  }
+ self.with_added_consumed(wasm_v1_result.consumed());
  if let (None, Some(err)) = (&self.error_message, wasm_v1_result.error()) {
  self.error_message = Some(format!("{}", err));
+ return Ok(self);
  }
- self.with_added_consumed(wasm_v1_result.consumed())
+ self.with_appended_transfers(&mut wasm_v1_result.transfers().clone())
  .with_appended_messages(&mut wasm_v1_result.messages().clone())
- .with_appended_transfers(&mut wasm_v1_result.transfers().clone())
  .with_appended_effects(wasm_v1_result.effects().clone());
  Ok(self)
  }

node/src/components/transaction_acceptor/tests.rs

@@ -804,7 +804,7 @@ impl reactor::Reactor for Reactor {
  .ignore::<Self::Event>();
  return Effects::new();
  }
- BalanceIdentifier::Payment => {
+ BalanceIdentifier::Payment | BalanceIdentifier::PenalizedPayment => {
  responder
  .respond(BalanceResult::Failure(
  TrackingCopyError::NamedKeyNotFound("payment".to_string()),

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

@@ -1709,8 +1709,7 @@ async fn only_refunds_are_burnt_no_fee_custom_payment() {
  test.get_balances(None);
  let initial_total_supply = test.get_total_supply(None);
  let (_txn_hash, block_height, exec_result) = test.send_transaction(txn).await;
- assert!(!exec_result_is_success(&exec_result)); // transaction should not succeed because we didn't request enough gas for this transaction
- // to succeed.
+
  match exec_result {
  ExecutionResult::V2(exec_result_v2) => {
  assert_eq!(exec_result_v2.cost, expected_transaction_cost.into());
@@ -1807,8 +1806,7 @@ async fn no_refund_no_fee_custom_payment() {
  test.get_balances(None);
  let initial_total_supply = test.get_total_supply(None);
  let (_txn_hash, block_height, exec_result) = test.send_transaction(txn).await;
- // expected to fail due to insufficient funding
- assert!(!exec_result_is_success(&exec_result), "should have failed");
+
  match exec_result {
  ExecutionResult::V2(exec_result_v2) => {
  assert_eq!(exec_result_v2.cost, expected_transaction_cost.into());
@@ -3246,7 +3244,7 @@ async fn charge_when_session_code_fails_with_user_error() {
  ExecutionResult::V2(res) if res.error_message.as_deref() == Some("User error: 100")
  ),
  "{:?}",
- exec_result
+ exec_result.error_message()
  );
 
  let (alice_current_balance, bob_current_balance, _) = test.get_balances(Some(block_height));
@@ -3258,11 +3256,11 @@ async fn charge_when_session_code_fails_with_user_error() {
  "fee is {}, expected to be greater than 0",
  fee
  );
- assert_eq!(
-  bob_current_balance.total,
-  bob_initial_balance.total - fee,
-  "bob should pay the fee"
- );
+ let init = bob_initial_balance.total;
+ let curr = bob_current_balance.total;
+ let actual = curr;
+ let expected = init - fee;
+ assert_eq!(actual, expected, "init {} curr {} fee {}", init, curr, fee,);
 }
 
 #[tokio::test]

storage/src/data_access_layer/balance.rs

@@ -66,6 +66,8 @@ pub enum BalanceIdentifier {
  Internal(URefAddr),
  /// Penalized account identifier.
  PenalizedAccount(AccountHash),
+ /// Penalized payment identifier.
+ PenalizedPayment,
 }
 
 impl BalanceIdentifier {
@@ -77,6 +79,7 @@ impl BalanceIdentifier {
  BalanceIdentifier::Public(_)
  | BalanceIdentifier::Account(_)
  | BalanceIdentifier::PenalizedAccount(_)
+ | BalanceIdentifier::PenalizedPayment
  | BalanceIdentifier::Entity(_)
  | BalanceIdentifier::Refund
  | BalanceIdentifier::Payment
@@ -119,7 +122,7 @@ impl BalanceIdentifier {
  BalanceIdentifier::Refund => {
  self.get_system_purse(tc, HANDLE_PAYMENT, REFUND_PURSE_KEY)?
  }
- BalanceIdentifier::Payment => {
+ BalanceIdentifier::Payment | BalanceIdentifier::PenalizedPayment => {
  self.get_system_purse(tc, HANDLE_PAYMENT, PAYMENT_PURSE_KEY)?
  }
  BalanceIdentifier::Accumulate => {
@@ -162,9 +165,10 @@ impl BalanceIdentifier {
 
  /// Is this balance identifier for penalty?
  pub fn is_penalty(&self) -> bool {
- // currently there is one variant of this kind, but more may be added later to
- // support more use cases.
- matches!(self, BalanceIdentifier::PenalizedAccount(_))
+ matches!(
+ self,
+ BalanceIdentifier::Payment | BalanceIdentifier::PenalizedPayment
+ )
  }
 }
 
@@ -762,6 +766,14 @@ impl BalanceResult {
  BalanceResult::Success { .. } => true,
  }
  }
+
+ /// Tracking copy error, if any.
+ pub fn error(&self) -> Option<&TrackingCopyError> {
+ match self {
+ BalanceResult::RootNotFound | BalanceResult::Success { .. } => None,
+ BalanceResult::Failure(err) => Some(err),
+ }
+ }
 }
 
 impl From<TrackingCopyError> for BalanceResult {