Metropolis aggregate PR #14726
Metropolis aggregate PR #14726
Conversation
This PR polishes the EIP 100 difficulty adjustment algorithm to match the same mechanisms as the Homestead was implemented to keep the code uniform. It also avoids a few memory allocs by reusing big1 and big2, pulling it out of the common package and into ethash. The commit also fixes chain maker to forward the uncle hash when creating a simulated chain (it wasn't needed until now so we just skipped a copy there).
|
Not a concern pre-metro, but the receipt address here seems to use old nonce-based algo: https://github.com/ethereum/go-ethereum/pull/14726/files#diff-5b5942d1b7feac47d38b879796555098R121 |
|
Yeah, I'm waiting for today's meeting about the address scheme before I go through the code and update everything. |
|
Also not a concern pre-metro: in order for a tx to be a valid eip-86 tx, these must also be |
| return nil | ||
| var hr homesteadReceiptRLP | ||
| var mr metropolisReceiptRLP | ||
| if err = rlp.DecodeBytes(raw, &mr); err == nil { |
There was a problem hiding this comment.
This will be active immediately. Is there a possibility that an attacker can insert a metro-receipt before metro is active?
core/vm/contracts.go
Outdated
| // required for anything significant is so high it's impossible to pay for. | ||
| func (c *pairing) RequiredGas(input []byte) uint64 { | ||
| //return 0 // TODO | ||
| return uint64(60000*len(input) + 40000) |
There was a problem hiding this comment.
This is wrong (and it's my fault). Should be like this: obscuren@90010b2 . @pirapira please verify?
| // homestead we must check for CodeStoreOutOfGasError (homestead only | ||
| // rule) and treat as an error, if the ruleset is frontier we must | ||
| // ignore this error and pretend the operation was successful. | ||
| if evm.ChainConfig().IsHomestead(evm.BlockNumber) && suberr == ErrCodeStoreOutOfGas { |
There was a problem hiding this comment.
(Not a concern pre-metro)
Does it make sense to have special handling for Homestead, for an op only on Metro ?
| writes: true, | ||
| clearsReturndata: true, | ||
| }, | ||
| CREATE2: { |
There was a problem hiding this comment.
Whoa - why is this in homestead ??
|
Benchmarks : |
|
Important thing to note for the benchmarks, the bn256 Go implementation from the Google crypto libs is incomplete. It doesn't handle the case where the points you're trying to add are equal to each other. See: https://github.com/golang/crypto/blob/master/bn256/bn256.go#L76 The Go equivalent of the following needs to be implemented: https://github.com/scipr-lab/libsnark/blob/master/src/algebra/curves/alt_bn128/alt_bn128_g1.cpp#L325 Edit: Never mind! I see it's been added in curve.go - the comments in bn256.go are just misleading. |
| var ( | ||
| baseLen = new(big.Int).SetBytes(input[:31]) | ||
| expLen = math.BigMax(new(big.Int).SetBytes(input[32:64]), big.NewInt(1)) | ||
| modLen = new(big.Int).SetBytes(input[65:97]) |
There was a problem hiding this comment.
These three are wrong, should be the same as the calculations in Run below (but without constraining them to Uint64() as the Run does:
baseLen = new(big.Int).SetBytes(input[:32])
expLen = new(big.Int).SetBytes(input[32:64])
modLen = new(big.Int).SetBytes(input[64:96])
|
All code pulled out from this PR, and superseded by the individual PRs linked in the description. Closing this one. |
Todos:
EIP 96: Blockhash refactoringDone:
EIP 208 (86): Abstraction of transaction origin and signature.Separated out: