From ace69ecb11ab363dd5f196ff9c6f5931c6f3c6ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joaqu=C3=ADn=20Rosales?= Date: Mon, 24 Jul 2023 18:10:37 -0600 Subject: [PATCH] breaking: remove validation of spending counter for votecast --- chain-impl-mockchain/Cargo.toml | 1 + .../src/accounting/account/account_state.rs | 33 ++- .../src/accounting/account/mod.rs | 35 ++- .../src/accounting/account/spending.rs | 18 +- chain-impl-mockchain/src/ledger/check.rs | 4 +- chain-impl-mockchain/src/ledger/ledger.rs | 248 +++++++++++++++--- chain-impl-mockchain/src/multisig/ledger.rs | 53 +++- 7 files changed, 316 insertions(+), 76 deletions(-) diff --git a/chain-impl-mockchain/Cargo.toml b/chain-impl-mockchain/Cargo.toml index 5a335e137..c3db964e3 100644 --- a/chain-impl-mockchain/Cargo.toml +++ b/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 = [ diff --git a/chain-impl-mockchain/src/accounting/account/account_state.rs b/chain-impl-mockchain/src/accounting/account/account_state.rs index da3e20baa..b547abd58 100644 --- a/chain-impl-mockchain/src/accounting/account/account_state.rs +++ b/chain-impl-mockchain/src/accounting/account/account_state.rs @@ -162,11 +162,11 @@ impl AccountState { 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, 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, LedgerError> { let new_value = (self.value - v)?; let mut r = self.clone(); r.spending.next_verify(spending)?; @@ -174,6 +174,23 @@ impl AccountState { 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, 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) => { diff --git a/chain-impl-mockchain/src/accounting/account/mod.rs b/chain-impl-mockchain/src/accounting/account/mod.rs index 002918198..7ad04ef52 100644 --- a/chain-impl-mockchain/src/accounting/account/mod.rs +++ b/chain-impl-mockchain/src/accounting/account/mod.rs @@ -176,17 +176,34 @@ impl Ledger { .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.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.0 + .update(identifier, |st| st.spend_unchecked(counter, value)) .map(Ledger) .map_err(|e| e.into()) } @@ -596,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!( @@ -625,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!( @@ -677,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, @@ -686,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()), diff --git a/chain-impl-mockchain/src/accounting/account/spending.rs b/chain-impl-mockchain/src/accounting/account/spending.rs index a200a3e63..2cf585809 100644 --- a/chain-impl-mockchain/src/accounting/account/spending.rs +++ b/chain-impl-mockchain/src/accounting/account/spending.rs @@ -58,10 +58,26 @@ impl SpendingCounterIncreasing { 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 diff --git a/chain-impl-mockchain/src/ledger/check.rs b/chain-impl-mockchain/src/ledger/check.rs index acc960af1..0ac1e5ae8 100644 --- a/chain-impl-mockchain/src/ledger/check.rs +++ b/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) -> LedgerCheck { +pub(super) fn valid_vote_cast_tx_slice( + tx: &TransactionSlice, +) -> LedgerCheck { if_cond_fail_with!( tx.inputs().nb_inputs() != 1 || tx.witnesses().nb_witnesses() != 1 diff --git a/chain-impl-mockchain/src/ledger/ledger.rs b/chain-impl-mockchain/src/ledger/ledger.rs index a630f057a..695cbf9f2 100644 --- a/chain-impl-mockchain/src/ledger/ledger.rs +++ b/chain-impl-mockchain/src/ledger/ledger.rs @@ -180,6 +180,15 @@ pub enum Block0Error { pub type OutputOldAddress = Output; pub type OutputAddress = Output
; +// Indicates whether account verification deduces the fee value from the account. +// This is used, for example, for VoteCast transactions. +enum FeeDeductionMode { + // Checks the spending counter when spending. + Default, + // Disables checking the spending counter when spending. + NoSpendingCounterCheck, +} + #[allow(clippy::large_enum_variant)] #[derive(Debug, Error, Clone, PartialEq, Eq)] pub enum Error { @@ -1039,9 +1048,13 @@ impl Ledger { Fragment::VoteCast(tx) => { let tx = tx.as_slice(); // this is a lightweight check, do this early to avoid doing any unnecessary computation - check::valid_vote_cast(&tx)?; - let (new_ledger_, _fee) = - new_ledger.apply_transaction(&fragment_id, &tx, block_date, ledger_params)?; + check::valid_vote_cast_tx_slice(&tx)?; + let (new_ledger_, _fee) = new_ledger.apply_transaction_for_vote_cast( + &fragment_id, + &tx, + block_date, + ledger_params, + )?; // we've just verified that this is a valid transaction (i.e. contains 1 input and 1 witness) let account_id = match tx @@ -1125,12 +1138,13 @@ impl Ledger { Ok(new_ledger) } - pub fn apply_transaction<'a, Extra>( + fn execute_apply_transaction<'a, Extra>( mut self, fragment_id: &FragmentId, tx: &TransactionSlice<'a, Extra>, cur_date: BlockDate, dyn_params: &LedgerParameters, + fee_deduction: FeeDeductionMode, ) -> Result<(Self, Value), Error> where Extra: Payload, @@ -1140,12 +1154,60 @@ impl Ledger { check::valid_transaction_date(&self.settings, tx.valid_until(), cur_date)?; let fee = calculate_fee(tx, dyn_params); tx.verify_strictly_balanced(fee)?; - self = self.apply_tx_inputs(tx)?; + self = self.apply_tx_inputs(tx, fee_deduction)?; self = self.apply_tx_outputs(*fragment_id, tx.outputs())?; self = self.apply_tx_fee(fee)?; Ok((self, fee)) } + /// Applies transaction to the account ledger by validating and verifying + /// inputs and outputs, as well as spending the fee value from the related + /// account id. Returns an Error if the account cannot afford the fee or + /// if the spending counter doesn't match. + pub fn apply_transaction<'a, Extra>( + self, + fragment_id: &FragmentId, + tx: &TransactionSlice<'a, Extra>, + cur_date: BlockDate, + dyn_params: &LedgerParameters, + ) -> Result<(Self, Value), Error> + where + Extra: Payload, + LinearFee: FeeAlgorithm, + { + let result = self.execute_apply_transaction( + fragment_id, + tx, + cur_date, + dyn_params, + FeeDeductionMode::Default, + )?; + Ok(result) + } + + // Applies vote cast transaction to the account ledger by validating and verifying + // inputs and outputs, deducing the fee, but ignoring the spending counter checks. + fn apply_transaction_for_vote_cast<'a, Extra>( + self, + fragment_id: &FragmentId, + tx: &TransactionSlice<'a, Extra>, + cur_date: BlockDate, + dyn_params: &LedgerParameters, + ) -> Result<(Self, Value), Error> + where + Extra: Payload, + LinearFee: FeeAlgorithm, + { + let result = self.execute_apply_transaction( + fragment_id, + tx, + cur_date, + dyn_params, + FeeDeductionMode::NoSpendingCounterCheck, + )?; + Ok(result) + } + pub fn apply_update(mut self, update: &UpdateProposal) -> Result { self.settings = self.settings.try_apply(update.changes())?; Ok(self) @@ -1566,9 +1628,11 @@ impl Ledger { Value::sum(all_utxo_values).map_err(|_| Error::Block0(Block0Error::UtxoTotalValueTooBig)) } + // Verifies witness data and spends the transaction fee from the account. fn apply_tx_inputs( mut self, tx: &TransactionSlice, + fee_deduction: FeeDeductionMode, ) -> Result { let sign_data_hash = tx.transaction_sign_data_hash(); for (input, witness) in tx.inputs_and_witnesses().iter() { @@ -1583,26 +1647,48 @@ impl Ledger { witness, spending_counter, ) => { - self.accounts = input_single_account_verify( - self.accounts, - &self.static_params.block0_initial_hash, - &sign_data_hash, - &account_id, - witness, - spending_counter, - value, - )? + self.accounts = match fee_deduction { + FeeDeductionMode::Default => input_single_account_verify( + self.accounts, + &self.static_params.block0_initial_hash, + &sign_data_hash, + &account_id, + witness, + spending_counter, + value, + )?, + FeeDeductionMode::NoSpendingCounterCheck => input_single_account_witness_verify_with_no_spending_counter_check( + self.accounts, + &self.static_params.block0_initial_hash, + &sign_data_hash, + &account_id, + witness, + spending_counter, + value, + )?, + }; } MatchingIdentifierWitness::Multi(account_id, witness, spending_counter) => { - self.multisig = input_multi_account_verify( - self.multisig, - &self.static_params.block0_initial_hash, - &sign_data_hash, - &account_id, - witness, - spending_counter, - value, - )? + self.multisig = match fee_deduction { + FeeDeductionMode::Default => input_multi_account_verify( + self.multisig, + &self.static_params.block0_initial_hash, + &sign_data_hash, + &account_id, + witness, + spending_counter, + value, + )?, + FeeDeductionMode::NoSpendingCounterCheck => input_multi_account_witness_verify_with_no_spending_counter_check( + self.multisig, + &self.static_params.block0_initial_hash, + &sign_data_hash, + &account_id, + witness, + spending_counter, + value, + )?, + }; } } } @@ -1875,50 +1961,126 @@ fn match_identifier_witness<'a>( } } -fn input_single_account_verify<'a>( - mut ledger: account::Ledger, +fn single_account_witness_verify<'a>( + ledger: account::Ledger, block0_hash: &HeaderId, sign_data_hash: &TransactionSignDataHash, - account: &account::Identifier, + account_id: &account::Identifier, witness: &'a account::Witness, spending_counter: account::SpendingCounter, - value: Value, ) -> Result { - // .remove_value() check if there's enough value and if not, returns a Err. - let new_ledger = ledger.remove_value(account, spending_counter, value)?; - ledger = new_ledger; - let tidsc = WitnessAccountData::new(block0_hash, sign_data_hash, spending_counter); - let verified = witness.verify(account.as_ref(), &tidsc); + let verified = witness.verify(account_id.as_ref(), &tidsc); if verified == chain_crypto::Verification::Failed { return Err(Error::AccountInvalidSignature { - account: account.clone(), + account: account_id.clone(), witness: Witness::Account(spending_counter, witness.clone()), }); }; Ok(ledger) } -fn input_multi_account_verify<'a>( - mut ledger: multisig::Ledger, +fn input_single_account_verify<'a>( + ledger: account::Ledger, block0_hash: &HeaderId, sign_data_hash: &TransactionSignDataHash, - account: &multisig::Identifier, - witness: &'a multisig::Witness, + account_id: &account::Identifier, + witness: &'a account::Witness, spending_counter: account::SpendingCounter, value: Value, -) -> Result { - // .remove_value() check if there's enough value and if not, returns a Err. - let (new_ledger, declaration) = ledger.remove_value(account, spending_counter, value)?; +) -> Result { + // Remove value from the account before proceeding + let updated_ledger = ledger.spend(account_id, spending_counter, value)?; + let ledger = single_account_witness_verify( + updated_ledger, + block0_hash, + sign_data_hash, + account_id, + witness, + spending_counter, + )?; + Ok(ledger) +} + +fn input_single_account_witness_verify_with_no_spending_counter_check<'a>( + ledger: account::Ledger, + block0_hash: &HeaderId, + sign_data_hash: &TransactionSignDataHash, + account_id: &account::Identifier, + witness: &'a account::Witness, + spending_counter: account::SpendingCounter, + value: Value, +) -> Result { + let updated_ledger = ledger.spend_with_no_counter_check(account_id, spending_counter, value)?; + let ledger = single_account_witness_verify( + updated_ledger, + block0_hash, + sign_data_hash, + account_id, + witness, + spending_counter, + )?; + Ok(ledger) +} +fn multi_account_witness_verify<'a>( + ledger: multisig::Ledger, + block0_hash: &HeaderId, + sign_data_hash: &TransactionSignDataHash, + account_id: &multisig::Identifier, + witness: &'a multisig::Witness, + spending_counter: account::SpendingCounter, +) -> Result { + let declaration = ledger.get_declaration_by_id(account_id)?; let data_to_verify = WitnessMultisigData::new(block0_hash, sign_data_hash, spending_counter); if !witness.verify(declaration, &data_to_verify) { return Err(Error::MultisigInvalidSignature { - multisig: account.clone(), + multisig: account_id.clone(), witness: Witness::Multisig(spending_counter, witness.clone()), }); } - ledger = new_ledger; + Ok(ledger) +} + +fn input_multi_account_verify<'a>( + ledger: multisig::Ledger, + block0_hash: &HeaderId, + sign_data_hash: &TransactionSignDataHash, + account_id: &multisig::Identifier, + witness: &'a multisig::Witness, + spending_counter: account::SpendingCounter, + value: Value, +) -> Result { + let updated_ledger = ledger.spend(account_id, spending_counter, value)?; + let ledger = multi_account_witness_verify( + updated_ledger, + block0_hash, + sign_data_hash, + account_id, + witness, + spending_counter, + )?; + Ok(ledger) +} + +fn input_multi_account_witness_verify_with_no_spending_counter_check<'a>( + ledger: multisig::Ledger, + block0_hash: &HeaderId, + sign_data_hash: &TransactionSignDataHash, + account_id: &multisig::Identifier, + witness: &'a multisig::Witness, + spending_counter: account::SpendingCounter, + value: Value, +) -> Result { + let updated_ledger = ledger.spend_with_no_counter_check(account_id, spending_counter, value)?; + let ledger = multi_account_witness_verify( + updated_ledger, + block0_hash, + sign_data_hash, + account_id, + witness, + spending_counter, + )?; Ok(ledger) } @@ -2195,7 +2357,7 @@ mod tests { } #[test] - fn test_input_account_wrong_value() { + fn test_input_single_account_verify_with_value_greater_than_initial_value_is_err() { let account = AddressData::account(Discrimination::Test); let initial_value = Value(100); let value_to_sub = Value(110); diff --git a/chain-impl-mockchain/src/multisig/ledger.rs b/chain-impl-mockchain/src/multisig/ledger.rs index 38551c5e8..0ddc0b7bc 100644 --- a/chain-impl-mockchain/src/multisig/ledger.rs +++ b/chain-impl-mockchain/src/multisig/ledger.rs @@ -107,27 +107,52 @@ impl Ledger { self.declarations.iter() } - /// If the account doesn't exist, or that the value would become negative, errors out. - pub fn remove_value( + /// Spend value from an existing account. + /// + /// 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: &Identifier, - spending_counter: SpendingCounter, + counter: SpendingCounter, value: Value, - ) -> Result<(Self, &Declaration), LedgerError> { + ) -> Result { + let new_accts = self.accounts.spend(identifier, counter, value)?; + Ok(Self { + accounts: new_accts, + declarations: self.declarations.clone(), + }) + } + + /// 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: &Identifier, + counter: SpendingCounter, + value: Value, + ) -> Result { + let new_accts = self + .accounts + .spend_with_no_counter_check(identifier, counter, value)?; + Ok(Self { + accounts: new_accts, + declarations: self.declarations.clone(), + }) + } + + /// Gets the `&Declaration` for the given `&Identifier`. + pub(crate) fn get_declaration_by_id( + &self, + identifier: &Identifier, + ) -> Result<&Declaration, LedgerError> { let decl = self .declarations .lookup(identifier) .ok_or(LedgerError::DoesntExist)?; - let new_accts = self - .accounts - .remove_value(identifier, spending_counter, value)?; - Ok(( - Self { - accounts: new_accts, - declarations: self.declarations.clone(), - }, - decl, - )) + Ok(decl) } /// Set the delegation of an account in this ledger