core/vm: metropolis precompiles #14959
Conversation
karalabe
commented
Aug 10, 2017
•
karalabe
commented
- EIP 197: Precompiled contracts for pairing function check.
- EIP 198: Precompiled contract for bigint modular exponentiation.
- EIP 213 (196): Precompiled contracts for addition and scalar multiplication on the elliptic curve alt_bn128.
karalabe
force-pushed
the
metropolis-precompiles
branch
2 times, most recently
from
d659d42
to
7b66050
Compare
karalabe
force-pushed
the
metropolis-precompiles
branch
from
7b66050
to
319b9ce
Compare
karalabe
force-pushed
the
metropolis-precompiles
branch
from
319b9ce
to
625767e
Compare
|
@fjl PTAL |
|
@holiman PTAL |
core/vm/contracts.go
Outdated
| ) | ||
| } | ||
| gas.Mul(gas, math.BigMax(adjExpLen, big.NewInt(1))) | ||
| gas.Div(gas, big.NewInt(100)) |
There was a problem hiding this comment.
params.QuadCoeffDiv instead of 100
There was a problem hiding this comment.
That is 512, the spec states 100.
There was a problem hiding this comment.
There was a problem hiding this comment.
Where does it state 100 ?
Consumes
floor(mult_complexity(max(length_of_MODULUS, length_of_BASE)) * max(ADJUSTED_EXPONENT_LENGTH, 1) / GQUADDIVISOR)gas
There was a problem hiding this comment.
Ah, I see now. Then we should add GQUADDIVISOR as a new protocol param, it seems to me
core/vm/contracts.go
Outdated
| gas.Mul(gas, math.BigMax(adjExpLen, big.NewInt(1))) | ||
| gas.Div(gas, big.NewInt(100)) | ||
|
|
||
| if gas.Cmp(new(big.Int).SetUint64(math.MaxUint64)) > 0 { |
There was a problem hiding this comment.
Isn't this a more lightweight way to do the same check:
if x.BitLen() > 64
There was a problem hiding this comment.
Indeed it is, will fix.
karalabe
force-pushed
the
metropolis-precompiles
branch
from
55fd6f0
to
d97b3d9
Compare
|
@holiman PTAL, last commit addresses your brought up issues. |
crypto/bn256/util.go
Outdated
|
|
||
| package bn256 | ||
|
|
||
| import ( |
There was a problem hiding this comment.
I'd prefer to not modify package bn256 even more. It's bad enough we have to change it at all. Please move these two functions to core/vm.
core/vm/contracts.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // Add the two curve points and return the result |
There was a problem hiding this comment.
I know you like to comment things excessively - and usually I just skip over these - but this one triggered me. "add ... and return" followed by x.Add(...); return .... Please remove "XXX and return" comments.
core/vm/contracts.go
Outdated
| } else { | ||
| offset := int(baseLen.Uint64()) | ||
|
|
||
| input = common.RightPadBytes(input, offset+32) |
There was a problem hiding this comment.
This will copy the entire input. Is that really necessary?
Why not just use getData, as in either
expHead := new(big.Int).SetBytes(getData(input, baseLen, expLen))
or
expHead := new(big.Int).SetBytes(getData(input, baseLen, common.Big32))
There was a problem hiding this comment.
We do not have a getData method. Arguably it could be a handy method. If desired, we can add a new method into common.
There was a problem hiding this comment.
Ooooh, my bad, apparently we do have one.
There was a problem hiding this comment.
I'll thumb it it up if you avoid unnecessary copying. I'm wary of the gas calculation being an attack surface
* Update modexp gas calculation to new version * Fix modexp modulo 0 special case to return zero
karalabe
force-pushed
the
metropolis-precompiles
branch
from
409d4f5
to
f8d8b56
Compare
| @@ -34,7 +34,21 @@ func calcMemSize(off, l *big.Int) *big.Int { | |||
|
|
|||
| // getData returns a slice from the data based on the start and size and pads | |||
| // up to size with zero's. This function is overflow safe. | |||
| func getData(data []byte, start, size *big.Int) []byte { | |||
| func getData(data []byte, start uint64, size uint64) []byte { | |||
There was a problem hiding this comment.
Comment says this function is overflow safe. It's not..
There was a problem hiding this comment.
Overflow safe as in you can index outside of the bounds.
| if start > length { | ||
| start = length | ||
| } | ||
| end := start + size |
There was a problem hiding this comment.
Though, I guess it would be overflow safe if you ensured that end >= start ?
There was a problem hiding this comment.
Well, if start + size can overflow a uint64, the big version of this method should be used.
