refactor(remove validation of the spending counter): votecast tx

full spending counter removal from code has too many side effects

chain-impl-mockchain/Cargo.toml

@@ -32,6 +32,7 @@ rand_chacha = { version = "0.3", optional = true }
 criterion = { version = "0.3.0", optional = true }
 rand = "0.8"
 cryptoxide = "0.4"
+tracing = "0.1"
 
 [features]
 property-test-api = [

chain-impl-mockchain/src/accounting/account/account_state.rs

@@ -162,18 +162,35 @@ impl<Extra: Clone> AccountState<Extra> {
         Ok(st)
     }
 
-    /// Subtract a value from an account state, and return the new state.
+    /// Spends value from an account state, and returns the new state.
     ///
-    /// Note that this *also* increment the counter, as this function would be usually call
-    /// for spending.
-    pub fn sub(&self, spending: SpendingCounter, v: Value) -> Result<Option<Self>, LedgerError> {
+    /// Note that this *also* increments the counter, as this function is usually
+    /// used for spending.
+    pub fn spend(&self, spending: SpendingCounter, v: Value) -> Result<Option<Self>, LedgerError> {
         let new_value = (self.value - v)?;
         let mut r = self.clone();
         r.spending.next_verify(spending)?;
         r.value = new_value;
         Ok(Some(r))
     }
 
+    /// Spends value from an account state, and returns the new state.
+    ///
+    /// Note that this *also* increments the counter, but does not fail if the
+    /// given counter fails to match the current one. However, it does throw
+    /// a warning.
+    pub(crate) fn spend_unchecked(
+        &self,
+        counter: SpendingCounter,
+        v: Value,
+    ) -> Result<Option<Self>, LedgerError> {
+        let new_value = (self.value - v)?;
+        let mut r = self.clone();
+        r.spending.next_unchecked(counter);
+        r.value = new_value;
+        Ok(Some(r))
+    }
+
     /// Add a value to a token in an account state
     ///
     /// Only error if value is overflowing
@@ -226,7 +243,7 @@ mod tests {
         account_state.spending = SpendingCounterIncreasing::new_from_counter(counter);
         assert_eq!(
             should_sub_fail(account_state.clone(), sub_value),
-            account_state.sub(counter, sub_value).is_err(),
+            account_state.spend(counter, sub_value).is_err(),
         )
     }
 
@@ -385,7 +402,7 @@ mod tests {
                 }
                 ArbitraryAccountStateOp::Sub(value) => {
                     let should_fail = should_sub_fail(account_state.clone(), value);
-                    match (should_fail, account_state.sub(counter, value)) {
+                    match (should_fail, account_state.spend(counter, value)) {
                         (false, Ok(account_state)) => {
                             strategy.next_verify(counter).expect("success");
                             counter = counter.increment();
@@ -399,8 +416,8 @@ mod tests {
                             }
                         }
                         (true, Err(_)) => account_state,
-                        (false,  Err(err)) => panic!("Operation {}: unexpected sub operation failure. Expected success but got: {:?}",op_counter,err),
-                        (true, Ok(account_state)) => panic!("Operation {}: unexpected sub operation success. Expected failure but got: success. AccountState: {:?}",op_counter, &account_state),
+                        (false,  Err(err)) => panic!("Operation {}: unexpected spend operation failure. Expected success but got: {:?}",op_counter,err),
+                        (true, Ok(account_state)) => panic!("Operation {}: unexpected spend operation success. Expected failure but got: success. AccountState: {:?}",op_counter, &account_state),
                     }
                 }
                 ArbitraryAccountStateOp::Delegate(stake_pool_id) => {

chain-impl-mockchain/src/accounting/account/mod.rs

@@ -31,8 +31,11 @@ pub enum LedgerError {
     AlreadyExists,
     #[error("Removed account is not empty")]
     NonZero,
-    #[error("Spending credential invalid")]
-    SpendingCredentialInvalid,
+    #[error("Spending credential invalid, expected {} got {} in lane {}", .expected.unlaned_counter(), .actual.unlaned_counter(), .actual.lane())]
+    SpendingCredentialInvalid {
+        expected: SpendingCounter,
+        actual: SpendingCounter,
+    },
     #[error("Value calculation failed")]
     ValueError(#[from] ValueError),
 }
@@ -173,17 +176,34 @@ impl<ID: Clone + Eq + Hash, Extra: Clone> Ledger<ID, Extra> {
             .map(Ledger)
     }
 
-    /// Subtract value to an existing account.
+    /// Spend value from an existing account.
     ///
-    /// If the account doesn't exist, or that the value would become negative, errors out.
-    pub fn remove_value(
+    /// If the account doesn't exist, or if the value is too much to spend,
+    /// or if the spending counter doesn't match, it throws a `LedgerError`.
+    pub fn spend(
         &self,
         identifier: &ID,
-        spending: SpendingCounter,
+        counter: SpendingCounter,
         value: Value,
     ) -> Result<Self, LedgerError> {
         self.0
-            .update(identifier, |st| st.sub(spending, value))
+            .update(identifier, |st| st.spend(counter, value))
+            .map(Ledger)
+            .map_err(|e| e.into())
+    }
+
+    /// Spend value from an existing account without spending counter check.
+    ///
+    /// If the account doesn't exist, or if the value is too much to spend,
+    /// it throws a `LedgerError`.
+    pub(crate) fn spend_with_no_counter_check(
+        &self,
+        identifier: &ID,
+        counter: SpendingCounter,
+        value: Value,
+    ) -> Result<Self, LedgerError> {
+        self.0
+            .update(identifier, |st| st.spend_unchecked(counter, value))
             .map(Ledger)
             .map_err(|e| e.into())
     }
@@ -593,7 +613,7 @@ mod tests {
         }
 
         // remove value from account
-        ledger = match ledger.remove_value(&account_id, spending_counter, value) {
+        ledger = match ledger.spend(&account_id, spending_counter, value) {
             Ok(ledger) => ledger,
             Err(err) => {
                 return TestResult::error(format!(
@@ -622,7 +642,7 @@ mod tests {
         }
 
         // removes all funds from account
-        ledger = match ledger.remove_value(&account_id, spending_counter, value_before_reward) {
+        ledger = match ledger.spend(&account_id, spending_counter, value_before_reward) {
             Ok(ledger) => ledger,
             Err(err) => {
                 return TestResult::error(format!(
@@ -674,7 +694,7 @@ mod tests {
     }
 
     #[quickcheck]
-    pub fn ledger_total_value_is_correct_after_remove_value(
+    pub fn ledger_total_value_is_correct_after_spend(
         id: Identifier,
         account_state: AccountState<()>,
         value_to_remove: Value,
@@ -683,7 +703,7 @@ mod tests {
         ledger = ledger
             .add_account(id.clone(), account_state.value(), ())
             .unwrap();
-        let result = ledger.remove_value(&id, SpendingCounter::zero(), value_to_remove);
+        let result = ledger.spend(&id, SpendingCounter::zero(), value_to_remove);
         let expected_result = account_state.value() - value_to_remove;
         match (result, expected_result) {
             (Err(_), Err(_)) => verify_total_value(ledger, account_state.value()),

chain-impl-mockchain/src/accounting/account/spending.rs

@@ -53,12 +53,31 @@ impl SpendingCounterIncreasing {
         let actual_counter = self.nexts[counter.lane()];
 
         if actual_counter != counter {
-            Err(LedgerError::SpendingCredentialInvalid)
+            Err(LedgerError::SpendingCredentialInvalid {
+                expected: actual_counter,
+                actual: counter,
+            })
         } else {
-            self.nexts[counter.lane()] = actual_counter.increment();
+            self.next_unchecked(counter);
             Ok(())
         }
     }
+
+    /// Increases the spending counter on the given lane.
+    pub(crate) fn next_unchecked(&mut self, unchecked_counter: SpendingCounter) {
+        let lane = unchecked_counter.lane();
+        let counter_to_update = self.nexts[lane];
+        if counter_to_update != unchecked_counter {
+            tracing::warn!(
+                "Invalid spending counter, {}",
+                LedgerError::SpendingCredentialInvalid {
+                    expected: counter_to_update,
+                    actual: unchecked_counter,
+                }
+            );
+        }
+        self.nexts[lane] = counter_to_update.increment();
+    }
 }
 
 // only used to print the account's ledger

chain-impl-mockchain/src/ledger/check.rs

@@ -78,7 +78,9 @@ pub(super) fn valid_stake_owner_delegation_transaction(
 /// check that the transaction input/outputs/witnesses is valid for the ballot
 ///
 /// * Only 1 input (subsequently 1 witness), no output
-pub(super) fn valid_vote_cast(tx: &TransactionSlice<certificate::VoteCast>) -> LedgerCheck {
+pub(super) fn valid_vote_cast_tx_slice(
+    tx: &TransactionSlice<certificate::VoteCast>,
+) -> LedgerCheck {
     if_cond_fail_with!(
         tx.inputs().nb_inputs() != 1
             || tx.witnesses().nb_witnesses() != 1