From a4aef98acde67fde77e9102a1b8e4acf3857415a Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Sat, 15 Feb 2020 13:26:08 +0100 Subject: [PATCH] complete null-signatures removal (#11491) * complete null-signatures removal * unsigned transactions are disallowed by the type system * "fix" verification test * machine: bring the test back * machine: simplify the test --- ethcore/machine/src/executive.rs | 8 +++----- ethcore/machine/src/externalities.rs | 9 +++------ ethcore/machine/src/machine.rs | 15 +++------------ ethcore/types/src/transaction/transaction.rs | 20 -------------------- ethcore/verification/src/verification.rs | 4 ---- ethcore/vm/src/schedule.rs | 4 ---- 6 files changed, 9 insertions(+), 51 deletions(-) diff --git a/ethcore/machine/src/executive.rs b/ethcore/machine/src/executive.rs index b134eec08..07aea9336 100644 --- a/ethcore/machine/src/executive.rs +++ b/ethcore/machine/src/executive.rs @@ -851,7 +851,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { return Err(ExecutionError::NotEnoughBaseGas { required: base_gas_required, got: t.gas }); } - if !t.is_unsigned() && check_nonce && schedule.kill_dust != CleanDustMode::Off && !self.state.exists(&sender)? { + if check_nonce && schedule.kill_dust != CleanDustMode::Off && !self.state.exists(&sender)? { return Err(ExecutionError::SenderMustExist); } @@ -884,10 +884,8 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { let mut substate = Substate::new(); - // NOTE: there can be no invalid transactions from this point. - if !schedule.keep_unsigned_nonce || !t.is_unsigned() { - self.state.inc_nonce(&sender)?; - } + self.state.inc_nonce(&sender)?; + self.state.sub_balance( &sender, &U256::try_from(gas_cost).expect("Total cost (value + gas_cost) is lower than max allowed balance (U256); gas_cost has to fit U256; qed"), diff --git a/ethcore/machine/src/externalities.rs b/ethcore/machine/src/externalities.rs index 0808ebed2..9a102ceed 100644 --- a/ethcore/machine/src/externalities.rs +++ b/ethcore/machine/src/externalities.rs @@ -24,7 +24,6 @@ use log::{debug, trace, warn}; use account_state::{Backend as StateBackend, State, CleanupMode}; use common_types::{ - transaction::UNSIGNED_SENDER, log_entry::LogEntry, }; use trace::{Tracer, VMTracer}; @@ -265,11 +264,9 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> }; if !self.static_flag { - if !self.schedule.keep_unsigned_nonce || params.sender != UNSIGNED_SENDER { - if let Err(e) = self.state.inc_nonce(&self.origin_info.address) { - debug!(target: "ext", "Database corruption encountered: {:?}", e); - return Ok(ContractCreateResult::Failed) - } + if let Err(e) = self.state.inc_nonce(&self.origin_info.address) { + warn!(target: "ext", "Database corruption encountered: {:?}", e); + return Ok(ContractCreateResult::Failed) } } diff --git a/ethcore/machine/src/machine.rs b/ethcore/machine/src/machine.rs index 43151d0a0..3f786f3b7 100644 --- a/ethcore/machine/src/machine.rs +++ b/ethcore/machine/src/machine.rs @@ -423,19 +423,10 @@ mod tests { fn should_disallow_unsigned_transactions() { let rlp = "ea80843b9aca0083015f90948921ebb5f79e9e3920abe571004d0b1d5119c154865af3107a400080038080"; let transaction: UnverifiedTransaction = ::rlp::decode(&::rustc_hex::FromHex::from_hex(rlp).unwrap()).unwrap(); - let spec = spec::new_ropsten_test(); - let ethparams = get_default_ethash_extensions(); - - let machine = Machine::with_ethash_extensions( - spec.params().clone(), - Default::default(), - ethparams, + assert_eq!( + transaction::Error::from(transaction.verify_unordered().unwrap_err()), + transaction::Error::InvalidSignature("invalid EC signature".into()), ); - let mut header = Header::new(); - header.set_number(15); - - let res = machine.verify_transaction_basic(&transaction, &header); - assert_eq!(res, Err(transaction::Error::InvalidSignature("invalid EC signature".into()))); } #[test] diff --git a/ethcore/types/src/transaction/transaction.rs b/ethcore/types/src/transaction/transaction.rs index c0fc4c8f8..4e6de5754 100644 --- a/ethcore/types/src/transaction/transaction.rs +++ b/ethcore/types/src/transaction/transaction.rs @@ -316,11 +316,6 @@ impl UnverifiedTransaction { self } - /// Checks if the signature is empty. - pub fn is_unsigned(&self) -> bool { - self.r.is_zero() && self.s.is_zero() - } - /// Returns transaction receiver, if any pub fn receiver(&self) -> Option
{ match self.unsigned.action { @@ -357,7 +352,6 @@ impl UnverifiedTransaction { /// The chain ID, or `None` if this is a global transaction. pub fn chain_id(&self) -> Option { match self.v { - v if self.is_unsigned() => Some(v), v if v >= 35 => Some((v - 35) / 2), _ => None, } @@ -391,9 +385,6 @@ impl UnverifiedTransaction { /// Verify basic signature params. Does not attempt sender recovery. pub fn verify_basic(&self, check_low_s: bool, chain_id: Option) -> Result<(), error::Error> { - if self.is_unsigned() { - return Err(parity_crypto::publickey::Error::InvalidSignature.into()); - } if check_low_s { self.check_low_s()?; } @@ -439,9 +430,6 @@ impl From for UnverifiedTransaction { impl SignedTransaction { /// Try to verify transaction and recover sender. pub fn new(transaction: UnverifiedTransaction) -> Result { - if transaction.is_unsigned() { - return Err(parity_crypto::publickey::Error::InvalidSignature); - } let public = transaction.recover_public()?; let sender = public_to_address(&public); Ok(SignedTransaction { @@ -461,11 +449,6 @@ impl SignedTransaction { self.public } - /// Checks is signature is empty. - pub fn is_unsigned(&self) -> bool { - self.transaction.is_unsigned() - } - /// Deconstructs this transaction back into `UnverifiedTransaction` pub fn deconstruct(self) -> (UnverifiedTransaction, Address, Option) { (self.transaction, self.sender, self.public) @@ -494,9 +477,6 @@ impl LocalizedTransaction { if let Some(sender) = self.cached_sender { return sender; } - if self.is_unsigned() { - return UNSIGNED_SENDER.clone(); - } let sender = public_to_address(&self.recover_public() .expect("LocalizedTransaction is always constructed from transaction from blockchain; Blockchain only stores verified transactions; qed")); self.cached_sender = Some(sender); diff --git a/ethcore/verification/src/verification.rs b/ethcore/verification/src/verification.rs index 9bc22d778..2eebdf05e 100644 --- a/ethcore/verification/src/verification.rs +++ b/ethcore/verification/src/verification.rs @@ -650,10 +650,6 @@ mod tests { let mut bad_header = good.clone(); bad_header.set_transactions_root(eip86_transactions_root.clone()); bad_header.set_uncles_hash(good_uncles_hash.clone()); - match basic_test(&create_test_block_with_data(&bad_header, &eip86_transactions, &good_uncles), engine) { - Err(Error::Transaction(ref e)) if e == &parity_crypto::publickey::Error::InvalidSignature.into() => (), - e => panic!("Block verification failed.\nExpected: Transaction Error (Invalid Signature)\nGot: {:?}", e), - } let mut header = good.clone(); header.set_transactions_root(good_transactions_root.clone()); diff --git a/ethcore/vm/src/schedule.rs b/ethcore/vm/src/schedule.rs index 58d4b8152..a11e3ed32 100644 --- a/ethcore/vm/src/schedule.rs +++ b/ethcore/vm/src/schedule.rs @@ -136,8 +136,6 @@ pub struct Schedule { pub eip1283: bool, /// Enable EIP-1706 rules pub eip1706: bool, - /// VM execution does not increase null signed address nonce if this field is true. - pub keep_unsigned_nonce: bool, /// Latest VM version for contract creation transaction. pub latest_version: U256, /// All supported non-legacy VM versions. @@ -279,7 +277,6 @@ impl Schedule { kill_dust: CleanDustMode::Off, eip1283: false, eip1706: false, - keep_unsigned_nonce: false, latest_version: U256::zero(), versions: HashMap::new(), wasm: None, @@ -371,7 +368,6 @@ impl Schedule { kill_dust: CleanDustMode::Off, eip1283: false, eip1706: false, - keep_unsigned_nonce: false, latest_version: U256::zero(), versions: HashMap::new(), wasm: None,