EIP198 and built-in activation #4926
Conversation
rphmeier
added
A0-pleasereview 🤓
| @@ -189,6 +189,7 @@ | |||
| "0000000000000000000000000000000000000002": { "builtin": { "name": "sha256", "pricing": { "linear": { "base": 60, "word": 12 } } } }, | |||
| "0000000000000000000000000000000000000003": { "builtin": { "name": "ripemd160", "pricing": { "linear": { "base": 600, "word": 120 } } } }, | |||
| "0000000000000000000000000000000000000004": { "builtin": { "name": "identity", "pricing": { "linear": { "base": 15, "word": 3 } } } }, | |||
| "0000000000000000000000000000000000000005": { "builtin": { "name": "modexp", "activate_at": "0x7fffffffffffff", "pricing": { "modexp": { "divisor": 20 } } } }, | |||
There was a problem hiding this comment.
Unfortunately, the pricing scheme for modexp isn't a simple function of the size like the others, so we have to create a special pricer which can't really be reused.
ethcore/src/executive.rs
Outdated
| // prefer to fail rather than silently break consensus. | ||
| if !builtin.is_active(self.info.number) { | ||
| let msg = format!("Engine returned unactivated built-in at {}", params.code_address); | ||
| return Err(evm::Error::Internal(msg)); |
There was a problem hiding this comment.
State checkpoint is not discarded nor reverted here.
There was a problem hiding this comment.
Nice catch. It should be done with a Drop guard to prevent mistakes like that.
There was a problem hiding this comment.
Although, looking closer, this branch already indicates a consensus failure of some kind.
|
|
||
| // read lengths as usize. | ||
| // ignoring the first 24 bytes might technically lead us to fall out of consensus, | ||
| // but so would running out of addressable memory! |
There was a problem hiding this comment.
Why impose such artificial limitation? Is it part of the spec?
There was a problem hiding this comment.
Yeah, the spec favors 32 bytes over 8 to encode the lengths because that's the native EVM word size and would require much less fiddling with bits to pass arguments, although really nobody will be able to pass lengths anywhere near that size.
I'd argue that this is a physical rather than an artificial limitation: I don't think a machine will exist for a long time which can handle 2^256-bit numbers.
There was a problem hiding this comment.
Sure, we can't handle such memory now as well that's why I see no point in this limit at all. Why 8 and not 7?
There was a problem hiding this comment.
two reasons: it makes decoding the size much easier with the byteorder library, and it's an amount of memory actually addressable on 64-bit machines.
There was a problem hiding this comment.
Right, sorry. Makes sense!
ethcore/src/builtin.rs
Outdated
|
|
||
| // calculate modexp: exponentiation by squaring. | ||
| fn modexp(mut base: BigUint, mut exp: BigUint, modulus: BigUint) -> BigUint { | ||
| if base == BigUint::zero() || modulus <= BigUint::one() { return BigUint::zero() } |
There was a problem hiding this comment.
0**0 will return 0, there is a comment in original EIP suggesting that it should be 1 for consistency with existing evm EXP opcode.
ethcore/src/builtin.rs
Outdated
| if base == BigUint::zero() || modulus <= BigUint::one() { return BigUint::zero() } | ||
|
|
||
| let mut result = BigUint::one(); | ||
| base = base % &modulus; |
There was a problem hiding this comment.
This may lead to base == 0 and we sould short circuit this case as well.
|
@tomusdrw Grumbles addressed; I changed the detected consensus failure in executive (which should never happen) to panic instead of issue an error which would be silently swallowed. |
ethcore/src/builtin.rs
Outdated
| (true, false) => return BigUint::zero(), // 0^n % m, n>0 | ||
| (false, false) if modulus <= BigUint::one() => return BigUint::zero(), // a^b % 1 = 0. | ||
| _ => {} | ||
| } | ||
|
|
||
| let mut result = BigUint::one(); | ||
| base = base % &modulus; |
There was a problem hiding this comment.
You could do this line at the very beginning.
There was a problem hiding this comment.
At the very least you have to check if the modulus is zero beforehand or the code will panic.
|
looks reasonable in design. haven't reviewed algo yet, but tests should cover that. |
|
merged with master to trigger the CI build |
arkpar
added
A8-looksgood 🦄
Implements EIP198 with exponentiation by squaring.
Changes to built-in declaration: optional
activate_atfield, defaults to 0. Built-ins won't be available from theEngineuntil activated. Replaces thebuiltin_cost,execute_builtin, andis_builtinof engine with a singlebuiltin(&Address, block_number) -> Option<&Builtin>, reducing the amount of lookups done.Could use a few more test cases, although I expect the consensus tests will cover this in detail.
num::BigUintis known to be much slower than GMP.rampis a faster pure-Rust alternative, but requires nightly. If this calculation starts to become a bottleneck, we can move to GMP.