Merge pull request #168 from casper-network/release-2.0.4-patch

[BUG] Insufficient Funds for Fee (PaymentImputed + Refund + PayToProposer)

Author: EdHastingsCasperAssociation

node/src/components/contract_runtime/operations.rs

@@ -85,7 +85,13 @@ pub(crate) struct ExecutionArtifactBuilder {
 }
 
 impl ExecutionArtifactBuilder {
-    pub fn new(transaction: &Transaction, min_cost: U512, current_price: u8) -> Self {
+    pub fn new(
+        transaction: &Transaction,
+        limit: Gas,
+        current_price: u8,
+        initial_cost: U512,
+        min_cost: U512,
+    ) -> Self {
         ExecutionArtifactBuilder {
             effects: Effects::new(),
             hash: transaction.hash(),
@@ -95,19 +101,50 @@ impl ExecutionArtifactBuilder {
             messages: Default::default(),
             initiator: transaction.initiator_addr(),
             current_price,
+            cost: initial_cost,
+            limit,
+            consumed: Gas::zero(),
+            refund: U512::zero(),
+            size_estimate: transaction.size_estimate() as u64,
+            min_cost,
+        }
+    }
+
+    pub fn pre_condition_failure(
+        transaction: &Transaction,
+        current_price: u8,
+        invalid_transaction: InvalidTransaction,
+    ) -> Self {
+        ExecutionArtifactBuilder {
+            effects: Effects::new(),
+            hash: transaction.hash(),
+            header: transaction.into(),
+            error_message: Some(format!("{}", invalid_transaction)),
+            transfers: vec![],
+            messages: Default::default(),
+            initiator: transaction.initiator_addr(),
+            current_price,
             cost: U512::zero(),
             limit: Gas::zero(),
             consumed: Gas::zero(),
             refund: U512::zero(),
             size_estimate: transaction.size_estimate() as u64,
-            min_cost,
+            min_cost: U512::zero(),
         }
     }
 
     pub fn error_message(&self) -> Option<String> {
         self.error_message.clone()
     }
 
+    pub fn gas_limit(&self) -> Gas {
+        self.limit
+    }
+
+    pub fn limit(&self) -> U512 {
+        self.limit.value()
+    }
+
     pub fn consumed(&self) -> U512 {
         self.consumed.value()
     }
@@ -123,6 +160,11 @@ impl ExecutionArtifactBuilder {
         }
     }
 
+    pub fn consume_limit(&mut self) -> &mut Self {
+        self.consumed = self.consumed.saturating_add(self.limit);
+        self
+    }
+
     pub fn with_added_consumed(&mut self, consumed: Gas) -> &mut Self {
         self.consumed = self.consumed.saturating_add(consumed);
         self
@@ -279,11 +321,17 @@ impl ExecutionArtifactBuilder {
         if let HandleFeeResult::RootNotFound = handle_fee_result {
             return Err(());
         }
-        if let (None, HandleFeeResult::Failure(err)) = (&self.error_message, handle_fee_result) {
-            self.error_message = Some(format!("{}", err));
+        if let HandleFeeResult::Success {
+            effects, transfers, ..
+        } = handle_fee_result
+        {
+            self.with_appended_transfers(&mut transfers.clone())
+                .with_appended_effects(effects.clone());
+        }
+        if let (None, HandleFeeResult::Failure(_)) = (&self.error_message, handle_fee_result) {
+            self.error_message = handle_fee_result.error_message();
             return Ok(self);
         }
-        self.with_appended_effects(handle_fee_result.effects());
         Ok(self)
     }
 
@@ -294,26 +342,18 @@ impl ExecutionArtifactBuilder {
         if let BalanceHoldResult::RootNotFound = hold_result {
             return Err(());
         }
-        if let (None, BalanceHoldResult::Failure(err)) = (&self.error_message, hold_result) {
-            self.error_message = Some(format!("{}", err));
+        if let BalanceHoldResult::Success { effects, .. } = hold_result {
+            self.with_appended_effects(*effects.clone());
+        }
+        if let (None, BalanceHoldResult::Failure(_)) = (&self.error_message, hold_result) {
+            self.error_message = hold_result.error_message();
             return Ok(self);
         }
-        self.with_appended_effects(hold_result.effects());
         Ok(self)
     }
 
-    pub fn with_added_cost(&mut self, cost: U512) -> &mut Self {
-        self.cost = self.cost.saturating_add(cost);
-        self
-    }
-
-    pub fn with_min_cost(&mut self, min_cost: U512) -> &mut Self {
-        self.min_cost = min_cost;
-        self
-    }
-
-    pub fn with_gas_limit(&mut self, limit: Gas) -> &mut Self {
-        self.limit = limit;
+    pub fn with_cost(&mut self, new_cost: U512) -> &mut Self {
+        self.cost = new_cost;
         self
     }
 
@@ -322,16 +362,6 @@ impl ExecutionArtifactBuilder {
         self
     }
 
-    pub fn with_invalid_transaction(
-        &mut self,
-        invalid_transaction: &InvalidTransaction,
-    ) -> &mut Self {
-        if self.error_message.is_none() {
-            self.error_message = Some(format!("{}", invalid_transaction));
-        }
-        self
-    }
-
     pub fn with_invalid_wasm_v1_request(
         &mut self,
         invalid_request: &InvalidWasmV1Request,

node/src/components/contract_runtime/types.rs

@@ -34,9 +34,18 @@ async fn run_equivocator_network() {
     let charlie_public_key = PublicKey::from(&*charlie_secret_key);
 
     let mut stakes = BTreeMap::new();
-    stakes.insert(alice_public_key.clone(), U512::from(1));
-    stakes.insert(bob_public_key.clone(), U512::from(1));
-    stakes.insert(charlie_public_key, U512::from(u64::MAX));
+    stakes.insert(
+        alice_public_key.clone(),
+        (U512::from(u64::MAX), U512::from(1)),
+    );
+    stakes.insert(
+        bob_public_key.clone(),
+        (U512::from(u64::MAX), U512::from(1)),
+    );
+    stakes.insert(
+        charlie_public_key,
+        (U512::from(u64::MAX), U512::from(u64::MAX)),
+    );
 
     // Here's where things go wrong: Bob doesn't run a node at all, and Alice runs two!
     let secret_keys = vec![
@@ -205,12 +214,12 @@ async fn run_equivocator_network() {
                 .expect("should have bid for public key {public_key} in era {era}");
             let staked_amount = bid.staked_amount();
             assert!(
-                staked_amount >= *stake,
+                staked_amount >= stake.1,
                 "expected stake {} for public key {} in era {}, found {}",
                 staked_amount,
                 public_key,
                 era,
-                stake
+                stake.1
             );
         }
     }

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

@@ -100,15 +100,21 @@ impl TestFixture {
         let stakes = secret_keys
             .iter()
             .zip(stake_values)
-            .map(|(secret_key, stake)| (PublicKey::from(secret_key.as_ref()), stake))
+            .map(|(secret_key, stake)| {
+                (
+                    PublicKey::from(secret_key.as_ref()),
+                    (U512::from(100_000_000_000_000_000u64), stake),
+                )
+            })
             .collect();
+
         Self::new_with_keys(rng, secret_keys, stakes, spec_override).await
     }
 
     pub(crate) async fn new_with_keys(
-        mut rng: TestRng,
+        rng: TestRng,
         secret_keys: Vec<Arc<SecretKey>>,
-        stakes: BTreeMap<PublicKey, U512>,
+        stakes: BTreeMap<PublicKey, (U512, U512)>,
         spec_override: Option<ConfigsOverride>,
     ) -> Self {
         testing::init_logging();
@@ -117,14 +123,10 @@ impl TestFixture {
         let (mut chainspec, chainspec_raw_bytes) =
             <(Chainspec, ChainspecRawBytes)>::from_resources("local");
 
-        let min_motes = 100_000_000_000_000_000u64;
-        let max_motes = min_motes * 100;
-        let balance = U512::from(rng.gen_range(min_motes..max_motes));
-
         // Override accounts with those generated from the keys.
         let accounts = stakes
             .into_iter()
-            .map(|(public_key, bonded_amount)| {
+            .map(|(public_key, (balance, bonded_amount))| {
                 let validator_config =
                     ValidatorConfig::new(Motes::new(bonded_amount), DelegationRate::zero());
                 AccountConfig::new(public_key, Motes::new(balance), Some(validator_config))
@@ -952,3 +954,27 @@ impl TestFixture {
         })
     }
 }
+
+pub(crate) fn standard_stakes(
+    alice_public_key: PublicKey,
+    bob_public_key: PublicKey,
+    charlie_public_key: Option<PublicKey>,
+) -> BTreeMap<PublicKey, (U512, U512)> {
+    let mut ret = BTreeMap::new();
+    ret.insert(
+        alice_public_key.clone(),
+        (
+            U512::from(100_000_000_000_000_000u64),
+            U512::from(u128::MAX),
+        ),
+    );
+    ret.insert(
+        bob_public_key.clone(),
+        (U512::from(100_000_000_000_000_000u64), U512::from(1)),
+    );
+
+    if let Some(pub_k) = charlie_public_key {
+        ret.insert(pub_k, (U512::from(u32::MAX - 1), U512::from(1)));
+    }
+    ret
+}

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

@@ -24,8 +24,11 @@ async fn run_gas_price_scenario(gas_price_scenario: GasPriceScenario) {
     let alice_stake = 200_000_000_000_u64;
     let bob_stake = 300_000_000_000_u64;
     let charlie_stake = 300_000_000_000_u64;
-    let initial_stakes: Vec<U512> =
-        vec![alice_stake.into(), bob_stake.into(), charlie_stake.into()];
+    let initial_stakes: Vec<(U512, U512)> = vec![
+        (U512::from(u64::MAX), alice_stake.into()),
+        (U512::from(u64::MAX), bob_stake.into()),
+        (U512::from(u64::MAX), charlie_stake.into()),
+    ];
 
     let mut secret_keys: Vec<Arc<SecretKey>> = (0..3)
         .map(|_| Arc::new(SecretKey::random(&mut rng)))
@@ -34,7 +37,7 @@ async fn run_gas_price_scenario(gas_price_scenario: GasPriceScenario) {
     let stakes = secret_keys
         .iter()
         .zip(initial_stakes)
-        .map(|(secret_key, stake)| (PublicKey::from(secret_key.as_ref()), stake))
+        .map(|(secret_key, (bal, stake))| (PublicKey::from(secret_key.as_ref()), (bal, stake)))
         .collect();
 
     let non_validating_secret_key = SecretKey::random(&mut rng);

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

@@ -13,6 +13,7 @@ use utils::{build_wasm_transction, RunUntilCondition, TestScenarioBuilder};
 
 use crate::{
     reactor::main_reactor::tests::{
+        transaction_scenario::asertions::BalanceChange,
         transactions::{
             invalid_wasm_txn, ALICE_PUBLIC_KEY, ALICE_SECRET_KEY, BOB_PUBLIC_KEY, BOB_SECRET_KEY,
             CHARLIE_PUBLIC_KEY, MIN_GAS_PRICE,
@@ -98,19 +99,25 @@ async fn should_native_transfer_nofee_norefund_fixed() {
             Gas::new(expected_transfer_gas),
         ))
         .await;
+
+    let transfer_amount = U512::from(TRANSFER_AMOUNT);
+    let transfer_amount_and_gas: U512 = transfer_amount
+        .checked_add(expected_transfer_gas)
+        .expect("should math");
+
     test_scenario
         .assert(PublicKeyBalanceChange::new(
             ALICE_PUBLIC_KEY.clone(),
-            -(TRANSFER_AMOUNT as i64),
-            -((TRANSFER_AMOUNT + expected_transfer_gas.as_u64()) as i64),
+            BalanceChange::Down(transfer_amount),
+            BalanceChange::Down(transfer_amount_and_gas),
         ))
         .await;
     //Charlie should have the transfer amount at his disposal
     test_scenario
         .assert(PublicKeyBalanceChange::new(
             CHARLIE_PUBLIC_KEY.clone(),
-            TRANSFER_AMOUNT as i64,
-            TRANSFER_AMOUNT as i64,
+            BalanceChange::Up(transfer_amount),
+            BalanceChange::Up(transfer_amount),
         ))
         .await;
     // Check if the hold is released.
@@ -190,11 +197,17 @@ async fn erroneous_native_transfer_nofee_norefund_fixed() {
         .await;
     // Even though the transaction failed, a hold must still be in place for the transfer cost.
     // The hold will show up in "available" being smaller than "total"
+
+    let transfer_amount_x = U512::from(transfer_amount);
+    let transfer_amount_y = transfer_amount_x
+        .checked_sub(U512::from(expected_transfer_cost))
+        .expect("should sub transfer from transfer amount");
+
     test_scenario
         .assert(PublicKeyBalanceChange::new(
             CHARLIE_PUBLIC_KEY.clone(),
-            transfer_amount as i64,
-            (transfer_amount - expected_transfer_cost) as i64,
+            BalanceChange::Up(transfer_amount_x),
+            BalanceChange::Up(transfer_amount_y),
         ))
         .await;
 }
@@ -233,24 +246,20 @@ async fn should_cancel_refund_for_erroneous_wasm() {
         ))
         .await;
 
-    test_scenario.assert(TransactionFailure::new(hash)).await; // transaction should have failed.
-                                                               // Bob gets no refund because the wasm errored
+    // transaction should have failed.
+    test_scenario.assert(TransactionFailure::new(hash)).await;
+
+    let x = BalanceChange::Down(U512::from(expected_transaction_cost));
+    // Bob gets no refund because the wasm errored
     test_scenario
-        .assert(PublicKeyBalanceChange::new(
-            BOB_PUBLIC_KEY.clone(),
-            -(expected_transaction_cost as i64),
-            -(expected_transaction_cost as i64),
-        ))
+        .assert(PublicKeyBalanceChange::new(BOB_PUBLIC_KEY.clone(), x, x))
         .await;
 
+    let y = BalanceChange::Up(U512::from(expected_transaction_cost));
     // Alice should get the all the fee since it's set to pay to proposer
     // AND Bob didn't get a refund
     test_scenario
-        .assert(PublicKeyBalanceChange::new(
-            ALICE_PUBLIC_KEY.clone(),
-            expected_transaction_cost as i64,
-            expected_transaction_cost as i64,
-        ))
+        .assert(PublicKeyBalanceChange::new(ALICE_PUBLIC_KEY.clone(), y, y))
         .await;
 }
 
@@ -291,19 +300,21 @@ async fn should_not_refund_erroneous_wasm_burn_fixed() {
     // Bobs transaction was invalid. He should get NO refund. But also -
     // since no refund is calculated nothing will be burned (despite
     // RefundHandling::Burn - we don't calculate refunds for erroneous wasms)
+    let gas_limit_x = BalanceChange::Down(U512::from(gas_limit));
     test_scenario
         .assert(PublicKeyBalanceChange::new(
             BOB_PUBLIC_KEY.clone(),
-            -(gas_limit as i64),
-            -(gas_limit as i64),
+            gas_limit_x,
+            gas_limit_x,
         ))
         .await;
+    let gas_limit_y = BalanceChange::Up(U512::from(gas_limit));
     // Alice gets payed for executing the transaction since it's set to pay to proposer
     test_scenario
         .assert(PublicKeyBalanceChange::new(
             ALICE_PUBLIC_KEY.clone(),
-            gas_limit as i64,
-            gas_limit as i64,
+            gas_limit_y,
+            gas_limit_y,
         ))
         .await;
 }