Skip to content

Commit

Permalink
Cherrypick precompiles fixes (#148)
Browse files Browse the repository at this point in the history 
* v can only be 27 or 28 prefixed only with zeros (polkadot-evm#964)

* check input size is a multiple of 192 (like in geth) (polkadot-evm#963)

* pad input if too short

* pad input in modexp

* pad all input fields + fix wrong test

* fix modexp gas cost

* remove dbg!

* remove redundant arg + import vec!

* use copy_from_slice

* clippy

* clippy

* fix template compilation

* more ecrecover tests

Co-authored-by: librelois <c@elo.tf>
  • Loading branch information
@nanocryk @librelois
nanocryk and librelois authored Jan 16, 2023
1 parent c378c7a commit 238d002
Show file tree
Hide file tree
Showing 4 changed files with 139 additions and 83 deletions.
44 changes: 25 additions & 19 deletions frame/evm/precompile/bn128/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,37 +26,43 @@ use fp_evm::{
};
use sp_core::U256;

fn read_fr(input: &[u8], start_inx: usize) -> Result<bn::Fr, PrecompileFailure> {
if input.len() < start_inx + 32 {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("Input not long enough".into()),
});
/// Copy bytes from input to target.
fn read_input(source: &[u8], target: &mut [u8], offset: usize) {
// Out of bounds, nothing to copy.
if source.len() <= offset {
return;
}

bn::Fr::from_slice(&input[start_inx..(start_inx + 32)]).map_err(|_| PrecompileFailure::Error {
// Find len to copy up to target len, but not out of bounds.
let len = core::cmp::min(target.len(), source.len() - offset);
target[..len].copy_from_slice(&source[offset..][..len]);
}

fn read_fr(input: &[u8], start_inx: usize) -> Result<bn::Fr, PrecompileFailure> {
let mut buf = [0u8; 32];
read_input(input, &mut buf, start_inx);

bn::Fr::from_slice(&buf).map_err(|_| PrecompileFailure::Error {
exit_status: ExitError::Other("Invalid field element".into()),
})
}

fn read_point(input: &[u8], start_inx: usize) -> Result<bn::G1, PrecompileFailure> {
use bn::{AffineG1, Fq, Group, G1};

if input.len() < start_inx + 64 {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("Input not long enough".into()),
});
}
let mut px_buf = [0u8; 32];
let mut py_buf = [0u8; 32];
read_input(input, &mut px_buf, start_inx);
read_input(input, &mut py_buf, start_inx + 32);

let px = Fq::from_slice(&input[start_inx..(start_inx + 32)]).map_err(|_| {
PrecompileFailure::Error {
exit_status: ExitError::Other("Invalid point x coordinate".into()),
}
let px = Fq::from_slice(&px_buf).map_err(|_| PrecompileFailure::Error {
exit_status: ExitError::Other("Invalid point x coordinate".into()),
})?;
let py = Fq::from_slice(&input[(start_inx + 32)..(start_inx + 64)]).map_err(|_| {
PrecompileFailure::Error {
exit_status: ExitError::Other("Invalid point y coordinate".into()),
}

let py = Fq::from_slice(&py_buf).map_err(|_| PrecompileFailure::Error {
exit_status: ExitError::Other("Invalid point y coordinate".into()),
})?;

Ok(if px == Fq::zero() && py == Fq::zero() {
G1::zero()
} else {
Expand Down
162 changes: 100 additions & 62 deletions frame/evm/precompile/modexp/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@

extern crate alloc;

use alloc::vec::Vec;
use core::{cmp::max, ops::BitAnd};
use alloc::{vec, vec::Vec};
use core::cmp::max;

use num::{BigUint, FromPrimitive, One, ToPrimitive, Zero};

Expand All @@ -38,9 +38,9 @@ const MIN_GAS_COST: u64 = 200;
// https://eips.ethereum.org/EIPS/eip-2565
fn calculate_gas_cost(
base_length: u64,
exp_length: u64,
mod_length: u64,
exponent: &BigUint,
exponent_bytes: &[u8],
) -> u64 {
fn calculate_multiplication_complexity(base_length: u64, mod_length: u64) -> u64 {
let max_length = max(base_length, mod_length);
Expand All @@ -56,18 +56,15 @@ fn calculate_gas_cost(
words * words
}

fn calculate_iteration_count(exp_length: u64, exponent: &BigUint) -> u64 {
fn calculate_iteration_count(exponent: &BigUint, exponent_bytes: &[u8]) -> u64 {
let mut iteration_count: u64 = 0;
let exp_length = exponent_bytes.len() as u64;

if exp_length <= 32 && exponent.is_zero() {
iteration_count = 0;
} else if exp_length <= 32 {
iteration_count = exponent.bits() - 1;
} else if exp_length > 32 {
// construct BigUint to represent (2^256) - 1
let bytes: [u8; 32] = [0xFF; 32];
let max_256_bit_uint = BigUint::from_bytes_be(&bytes);

// from the EIP spec:
// (8 * (exp_length - 32)) + ((exponent & (2**256 - 1)).bit_length() - 1)
//
Expand All @@ -77,21 +74,42 @@ fn calculate_gas_cost(
// must be > 0)
// * the addition can't overflow because the terms are both capped at roughly
// 8 * max size of exp_length (1024)
iteration_count =
(8 * (exp_length - 32)) + exponent.bitand(max_256_bit_uint).bits() - 1;
// * the EIP spec is written in python, in which (exponent & (2**256 - 1)) takes the
// FIRST 32 bytes. However this `BigUint` `&` operator takes the LAST 32 bytes.
// We thus instead take the bytes manually.
let exponent_head = BigUint::from_bytes_be(&exponent_bytes[..32]);

iteration_count = (8 * (exp_length - 32)) + exponent_head.bits() - 1;
}

max(iteration_count, 1)
}

let multiplication_complexity = calculate_multiplication_complexity(base_length, mod_length);
let iteration_count = calculate_iteration_count(exp_length, exponent);
let iteration_count = calculate_iteration_count(exponent, exponent_bytes);
max(
MIN_GAS_COST,
multiplication_complexity * iteration_count / 3,
)
}

/// Copy bytes from input to target.
fn read_input(source: &[u8], target: &mut [u8], source_offset: &mut usize) {
// We move the offset by the len of the target, regardless of what we
// actually copy.
let offset = *source_offset;
*source_offset += target.len();

// Out of bounds, nothing to copy.
if source.len() <= offset {
return;
}

// Find len to copy up to target len, but not out of bounds.
let len = core::cmp::min(target.len(), source.len() - offset);
target[..len].copy_from_slice(&source[offset..][..len]);
}

// ModExp expects the following as inputs:
// 1) 32 bytes expressing the length of base
// 2) 32 bytes expressing the length of exponent
Expand All @@ -111,35 +129,35 @@ fn calculate_gas_cost(
impl Precompile for Modexp {
fn execute(handle: &mut impl PrecompileHandle) -> PrecompileResult {
let input = handle.input();
let mut input_offset = 0;

if input.len() < 96 {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("input must contain at least 96 bytes".into()),
});
};
// Yellowpaper: whenever the input is too short, the missing bytes are
// considered to be zero.
let mut base_len_buf = [0u8; 32];
read_input(input, &mut base_len_buf, &mut input_offset);
let mut exp_len_buf = [0u8; 32];
read_input(input, &mut exp_len_buf, &mut input_offset);
let mut mod_len_buf = [0u8; 32];
read_input(input, &mut mod_len_buf, &mut input_offset);

// reasonable assumption: this must fit within the Ethereum EVM's max stack size
let max_size_big = BigUint::from_u32(1024).expect("can't create BigUint");

let mut buf = [0; 32];
buf.copy_from_slice(&input[0..32]);
let base_len_big = BigUint::from_bytes_be(&buf);
let base_len_big = BigUint::from_bytes_be(&base_len_buf);
if base_len_big > max_size_big {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("unreasonably large base length".into()),
});
}

buf.copy_from_slice(&input[32..64]);
let exp_len_big = BigUint::from_bytes_be(&buf);
let exp_len_big = BigUint::from_bytes_be(&exp_len_buf);
if exp_len_big > max_size_big {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("unreasonably large exponent length".into()),
});
}

buf.copy_from_slice(&input[64..96]);
let mod_len_big = BigUint::from_bytes_be(&buf);
let mod_len_big = BigUint::from_bytes_be(&mod_len_buf);
if mod_len_big > max_size_big {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("unreasonably large modulus length".into()),
Expand All @@ -151,11 +169,11 @@ impl Precompile for Modexp {
let exp_len = exp_len_big.to_usize().expect("exp_len out of bounds");
let mod_len = mod_len_big.to_usize().expect("mod_len out of bounds");

// input length should be at least 96 + user-specified length of base + exp + mod
let total_len = base_len + exp_len + mod_len + 96;
if input.len() < total_len {
return Err(PrecompileFailure::Error {
exit_status: ExitError::Other("insufficient input size".into()),
// if mod_len is 0 output must be empty
if mod_len == 0 {
return Ok(PrecompileOutput {
exit_status: ExitSucceed::Returned,
output: vec![],
});
}

Expand All @@ -165,21 +183,22 @@ impl Precompile for Modexp {
BigUint::zero()
} else {
// read the numbers themselves.
let base_start = 96; // previous 3 32-byte fields
let base = BigUint::from_bytes_be(&input[base_start..base_start + base_len]);
let mut base_buf = vec![0u8; base_len];
read_input(input, &mut base_buf, &mut input_offset);
let base = BigUint::from_bytes_be(&base_buf);

let exp_start = base_start + base_len;
let exponent = BigUint::from_bytes_be(&input[exp_start..exp_start + exp_len]);
let mut exp_buf = vec![0u8; exp_len];
read_input(input, &mut exp_buf, &mut input_offset);
let exponent = BigUint::from_bytes_be(&exp_buf);

let mut mod_buf = vec![0u8; mod_len];
read_input(input, &mut mod_buf, &mut input_offset);
let modulus = BigUint::from_bytes_be(&mod_buf);

// do our gas accounting
let gas_cost =
calculate_gas_cost(base_len as u64, exp_len as u64, mod_len as u64, &exponent);
let gas_cost = calculate_gas_cost(base_len as u64, mod_len as u64, &exponent, &exp_buf);

handle.record_cost(gas_cost)?;
let input = handle.input();

let mod_start = exp_start + exp_len;
let modulus = BigUint::from_bytes_be(&input[mod_start..mod_start + mod_len]);

if modulus.is_zero() || modulus.is_one() {
BigUint::zero()
Expand Down Expand Up @@ -228,7 +247,7 @@ mod tests {
}

#[test]
fn test_empty_input() -> Result<(), PrecompileFailure> {
fn test_empty_input() {
let input = Vec::new();

let cost: u64 = 1;
Expand All @@ -242,25 +261,17 @@ mod tests {
let mut handle = MockHandle::new(input, Some(cost), context);

match Modexp::execute(&mut handle) {
Ok(_) => {
panic!("Test not expected to pass");
Ok(precompile_result) => {
assert_eq!(precompile_result.output.len(), 0);
}
Err(e) => {
assert_eq!(
e,
PrecompileFailure::Error {
exit_status: ExitError::Other(
"input must contain at least 96 bytes".into()
)
}
);
Ok(())
Err(_) => {
panic!("Modexp::execute() returned error"); // TODO: how to pass error on?
}
}
}

#[test]
fn test_insufficient_input() -> Result<(), PrecompileFailure> {
fn test_insufficient_input() {
let input = hex::decode(
"0000000000000000000000000000000000000000000000000000000000000001\
0000000000000000000000000000000000000000000000000000000000000001\
Expand All @@ -279,17 +290,12 @@ mod tests {
let mut handle = MockHandle::new(input, Some(cost), context);

match Modexp::execute(&mut handle) {
Ok(_) => {
panic!("Test not expected to pass");
Ok(precompile_result) => {
assert_eq!(precompile_result.output.len(), 1);
assert_eq!(precompile_result.output, vec![0x00]);
}
Err(e) => {
assert_eq!(
e,
PrecompileFailure::Error {
exit_status: ExitError::Other("insufficient input size".into())
}
);
Ok(())
Err(_) => {
panic!("Modexp::execute() returned error"); // TODO: how to pass error on?
}
}
}
Expand Down Expand Up @@ -474,4 +480,36 @@ mod tests {
let expected = BigUint::parse_bytes(b"0", 10).unwrap();
assert_eq!(result, expected);
}

#[test]
fn test_long_exp_gas_cost_matches_specs() {
let input = vec![
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 38, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 96, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
16, 0, 0, 0, 255, 255, 255, 2, 0, 0, 179, 0, 0, 2, 0, 0, 122, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 255, 251, 0, 0, 0, 0, 4, 38, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 96, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 16, 0, 0, 0, 255, 255, 255, 2, 0, 0, 179, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255,
255, 255, 255, 249,
];

let context: Context = Context {
address: Default::default(),
caller: Default::default(),
apparent_value: From::from(0),
};

let mut handle = MockHandle::new(input, Some(100_000), context);

let _ = Modexp::execute(&mut handle).expect("Modexp::execute() returned error");

assert_eq!(handle.gas_used, 7104); // gas used when ran in geth
}
}
14 changes: 14 additions & 0 deletions frame/evm/precompile/testdata/ecRecover.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,20 @@
"Name": "ValidKey",
"NoBenchmark": false
},
{
"Input": "18c547e4f7b0f325ad1e56f57e26c745b09a3e503d86e00e5255ff7f715d3d1c000000000000000000000000000000000000000000000000000000000000000173b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75feeb940b1d03b21e36b0e47e79769f095fe2ab855bd91e3a38756b7d75a9c4549",
"Expected": "",
"Gas": 3000,
"Name": "Non supported V value (1)",
"NoBenchmark": false
},
{
"Input": "18c547e4f7b0f325ad1e56f57e26c745b09a3e503d86e00e5255ff7f715d3d1c000000000000000000000000000001000000000000000000000000000000001c73b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75feeb940b1d03b21e36b0e47e79769f095fe2ab855bd91e3a38756b7d75a9c4549",
"Expected": "",
"Gas": 3000,
"Name": "Non supported V padding",
"NoBenchmark": false
},
{
"Input": "38d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e000000000000000000000000000000000000000000000000000000000000001b38d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e789d1dd423d25f0772d2748d60f7e4b81bb14d086eba8e8e8efb6dcff8a4ae02",
"Expected": "000000000000000000000000ceaccac640adf55b2028469bd36ba501f28b699d",
Expand Down
2 changes: 0 additions & 2 deletions template/node/src/service.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,6 @@ pub fn new_partial(
registry: config.prometheus_registry(),
check_for_equivocation: Default::default(),
telemetry: telemetry.as_ref().map(|x| x.handle()),
compatibility_mode: sc_consensus_aura::CompatibilityMode::None,
},
)?;

Expand Down Expand Up @@ -451,7 +450,6 @@ pub fn new_full(mut config: Configuration, cli: &Cli) -> Result<TaskManager, Ser
block_proposal_slot_portion: sc_consensus_aura::SlotProportion::new(2f32 / 3f32),
max_block_proposal_slot_portion: None,
telemetry: telemetry.as_ref().map(|x| x.handle()),
compatibility_mode: sc_consensus_aura::CompatibilityMode::None,
},
)?;
// the AURA authoring task is considered essential, i.e. if it
Expand Down

0 comments on commit 238d002

Please sign in to comment.