remove null signatures (#11335)

* tx: clean up legacy eip-86 based null signature

* tx: add a test for null signature rejection

* tx: revert json txn changes

* fix evmbin bench build

* tx: put UNSIGNED_SENDER behind 'test-helpers' feature

* Revert "tx: put UNSIGNED_SENDER behind 'test-helpers' feature"

This reverts commit 1dde47831e4dc5098d064e6022ead43512c31efb.

* tx: add comment for null_sign

* even more cleanup

* Revert "even more cleanup"

This reverts commit be29f032415c53d4d40842f93f1fd1fb6f9cc6bd.
This commit is contained in:
Andronik Ordian 2019-12-13 18:12:22 +01:00 committed by Tomasz Drwięga
parent 6e76be7fad
commit 63535860bc
6 changed files with 62 additions and 29 deletions

View File

@ -343,7 +343,7 @@ impl Machine {
} else {
None
};
t.verify_basic(check_low_s, chain_id, false)?;
t.verify_basic(check_low_s, chain_id)?;
Ok(())
}

View File

@ -260,7 +260,7 @@ impl<'a> EvmTestClient<'a> {
) -> std::result::Result<TransactSuccess<T::Output, V::Output>, TransactErr> {
let initial_gas = transaction.gas;
// Verify transaction
let is_ok = transaction.verify_basic(true, None, false);
let is_ok = transaction.verify_basic(true, None);
if let Err(error) = is_ok {
return Err(
TransactErr{

View File

@ -138,6 +138,7 @@ impl Transaction {
}
}
#[cfg(any(test, feature = "test-helpers"))]
impl From<ethjson::transaction::Transaction> for SignedTransaction {
fn from(t: ethjson::transaction::Transaction) -> Self {
let to: Option<ethjson::hash::Address> = t.to.into();
@ -237,7 +238,10 @@ impl Transaction {
}
}
/// Add EIP-86 compatible empty signature.
/// Legacy EIP-86 compatible empty signature.
/// This method is used in json tests as well as
/// signature verification tests.
#[cfg(any(test, feature = "test-helpers"))]
pub fn null_sign(self, chain_id: u64) -> SignedTransaction {
SignedTransaction {
transaction: UnverifiedTransaction {
@ -295,7 +299,7 @@ impl rlp::Decodable for UnverifiedTransaction {
v: d.val_at(6)?,
r: d.val_at(7)?,
s: d.val_at(8)?,
hash: hash,
hash,
})
}
}
@ -369,7 +373,7 @@ impl UnverifiedTransaction {
/// Checks whether the signature has a low 's' value.
pub fn check_low_s(&self) -> Result<(), parity_crypto::publickey::Error> {
if !self.signature().is_low_s() {
Err(parity_crypto::publickey::Error::InvalidSignature.into())
Err(parity_crypto::publickey::Error::InvalidSignature)
} else {
Ok(())
}
@ -386,17 +390,12 @@ impl UnverifiedTransaction {
}
/// Verify basic signature params. Does not attempt sender recovery.
pub fn verify_basic(&self, check_low_s: bool, chain_id: Option<u64>, allow_empty_signature: bool) -> Result<(), error::Error> {
if check_low_s && !(allow_empty_signature && self.is_unsigned()) {
self.check_low_s()?;
}
// Disallow unsigned transactions in case EIP-86 is disabled.
if !allow_empty_signature && self.is_unsigned() {
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());
}
// EIP-86: Transactions of this form MUST have gasprice = 0, nonce = 0, value = 0, and do NOT increment the nonce of account 0.
if allow_empty_signature && self.is_unsigned() && !(self.gas_price.is_zero() && self.value.is_zero() && self.nonce.is_zero()) {
return Err(parity_crypto::publickey::Error::InvalidSignature.into())
if check_low_s {
self.check_low_s()?;
}
match (self.chain_id(), chain_id) {
(None, _) => {},
@ -441,21 +440,16 @@ impl SignedTransaction {
/// Try to verify transaction and recover sender.
pub fn new(transaction: UnverifiedTransaction) -> Result<Self, parity_crypto::publickey::Error> {
if transaction.is_unsigned() {
Ok(SignedTransaction {
transaction: transaction,
sender: UNSIGNED_SENDER,
public: None,
})
} else {
return Err(parity_crypto::publickey::Error::InvalidSignature);
}
let public = transaction.recover_public()?;
let sender = public_to_address(&public);
Ok(SignedTransaction {
transaction: transaction,
sender: sender,
transaction,
sender,
public: Some(public),
})
}
}
/// Returns transaction sender.
pub fn sender(&self) -> Address {
@ -645,6 +639,24 @@ mod tests {
assert_eq!(t.chain_id(), None);
}
#[test]
fn should_reject_null_signature() {
let t = Transaction {
nonce: U256::zero(),
gas_price: U256::from(10000000000u64),
gas: U256::from(21000),
action: Action::Call(Address::from_str("d46e8dd67c5d32be8058bb8eb970870f07244567").unwrap()),
value: U256::from(1),
data: vec![]
}.null_sign(1);
let res = SignedTransaction::new(t.transaction);
match res {
Err(parity_crypto::publickey::Error::InvalidSignature) => {}
_ => panic!("null signature should be rejected"),
}
}
#[test]
fn should_recover_from_chain_specific_signing() {
use parity_crypto::publickey::{Random, Generator};

View File

@ -11,7 +11,7 @@ path = "./src/main.rs"
[dependencies]
account-state = { path = "../ethcore/account-state" }
common-types = { path = "../ethcore/types" }
common-types = { path = "../ethcore/types", features = ["test-helpers"] }
docopt = "1.0"
env_logger = "0.5"
ethcore = { path = "../ethcore", features = ["test-helpers", "json-tests"] }

View File

@ -203,6 +203,28 @@ mod tests {
]);
}
#[test]
fn deserialization_alt_bn128_const_operations() {
let s = r#"{
"name": "alt_bn128_mul",
"pricing": {
"100500": {
"price": { "alt_bn128_const_operations": { "price": 123 }}
}
}
}"#;
let builtin: Builtin = serde_json::from_str::<BuiltinCompat>(s).unwrap().into();
assert_eq!(builtin.name, "alt_bn128_mul");
assert_eq!(builtin.pricing, map![
100500 => PricingAt {
info: None,
price: Pricing::AltBn128ConstOperations(AltBn128ConstOperations {
price: 123,
}),
}
]);
}
#[test]
fn activate_at() {
let s = r#"{

View File

@ -125,7 +125,6 @@ pub fn validate_optional_non_zero<'de, D>(d: D) -> Result<Option<Uint>, D::Error
mod test {
use super::Uint;
use ethereum_types::U256;
use serde_json::error::Category;
#[test]
fn uint_deserialization() {