Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

complete null-signatures removal #11491

Merged
merged 5 commits into from
Feb 15, 2020
Merged

complete null-signatures removal #11491

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be03019
complete null-signatures removal
ordian Dec 13, 2019
eefd451
unsigned transactions are disallowed by the type system
ordian Feb 14, 2020
9dea5a1
"fix" verification test
ordian Feb 14, 2020
dd4bbf3
machine: bring the test back
ordian Feb 15, 2020
2c147db
machine: simplify the test
ordian Feb 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions ethcore/machine/src/executive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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"),
Expand Down
9 changes: 3 additions & 6 deletions ethcore/machine/src/externalities.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to a warning

return Ok(ContractCreateResult::Failed)
}
}

Expand Down
19 changes: 0 additions & 19 deletions ethcore/machine/src/machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,25 +419,6 @@ mod tests {
}
}

#[test]
fn should_disallow_unsigned_transactions() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the test. I guess the failure is going to be at a dfiferent level now, but still.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have this https://github.com/paritytech/parity-ethereum/pull/11335/files#diff-ffdd27c0e6928e872d31ecdc8f8e09eaR643

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or did you mean something else?

Copy link
Collaborator

@tomusdrw tomusdrw Feb 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I just think it's worth to keep the test in it's current form, as it's covering a bit more than constructor of SignedTransaction. I think it was added based on an actual previous bug, so keeping it as a way to prevent regressions should hurt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was added in #8802 and it doesn't make sense anymore. I removed it because it actually fails on this branch, since it creates a null-signed UnverifiedTransaction and calls verify_transaction_basic expecting it to reject it.
But this PR removes this rejection logic, since it's not needed as we don't allow SignedTransactions with null-signatures.
Now if I modify the test to convert a transaction to a SignedTransaction, then it's not different than the test linked above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conversion happens within engine/machine, doesn't it (via verify_transaction). IMHO still worth to check the machine/engine integration with this, but not super strong on this. If you disagree - then fine, let's remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conversion happens in the verification queue when we convert Unverified block to PreverifiedBlock
https://github.com/paritytech/parity-ethereum/blob/856a0755888a30d4dc52a68cb2638a8bfd5786ac/ethcore/verification/src/verification.rs#L100

Added dd4bbf3, let me know what you think.

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,
);
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]
fn ethash_gas_limit_is_multiple_of_determinant() {
use ethereum_types::U256;
Expand Down
20 changes: 0 additions & 20 deletions ethcore/types/src/transaction/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Address> {
match self.unsigned.action {
Expand Down Expand Up @@ -357,7 +352,6 @@ impl UnverifiedTransaction {
/// The chain ID, or `None` if this is a global transaction.
pub fn chain_id(&self) -> Option<u64> {
match self.v {
v if self.is_unsigned() => Some(v),
v if v >= 35 => Some((v - 35) / 2),
_ => None,
}
Expand Down Expand Up @@ -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<u64>) -> Result<(), error::Error> {
if self.is_unsigned() {
return Err(parity_crypto::publickey::Error::InvalidSignature.into());
}
if check_low_s {
self.check_low_s()?;
}
Expand Down Expand Up @@ -439,9 +430,6 @@ impl From<SignedTransaction> for UnverifiedTransaction {
impl SignedTransaction {
/// Try to verify transaction and recover sender.
pub fn new(transaction: UnverifiedTransaction) -> Result<Self, parity_crypto::publickey::Error> {
if transaction.is_unsigned() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the aforementioned type safety is here, SignedTransaction::new will not accept unsigned transactions after #11335

return Err(parity_crypto::publickey::Error::InvalidSignature);
}
let public = transaction.recover_public()?;
let sender = public_to_address(&public);
Ok(SignedTransaction {
Expand All @@ -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<Public>) {
(self.transaction, self.sender, self.public)
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 0 additions & 4 deletions ethcore/vm/src/schedule.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down