ethereum/AllCoreDevs

Martin Holst Swende

@holiman

Regarding some discussions that has been happening on EIp-1014, Skinny Create2.

I made a problem description
@SergioDemianLerner proposed a solution
@vbuterin suggested a change in eip

I have now performed some benchmarks, and can confirm that the combination of uncapped size of initcode + lack of cost-per-byte is problematic in CREATE2, and can lead to DoS attacks if not fixed. Therefore, I propose that we accept the change to EIp 1014 which uses the folowing phrasing:

Additionally, an extra GSHA3WORD * ceil(len(init_code) / 32) gas is charged.

Furthermore the problem is also present today, with CREATE, but that is due to internal implementation details in most (all?) clients. Short story: it can be fixed without changing CREATE. Client implementors can PM me for details and testcases. I have already spoken to Parity members about this.

An alternative change could be to introduce a cap on initcode size. That would also 'fix' the problem in a less 'correct' manner. cc @/all

Paweł Bylica

@chfast

The gas is charged up front from the caller?

Martin Holst Swende

@holiman

I guess so, yes? It needs to be charged before the address is calculated

Paweł Bylica

@chfast

I think the same. It should be part of the CREATE2 cost.

Martin Holst Swende

@holiman

I consider this 'approved' by @vbuterin (eip author), aleth and geth. Any objections from Parity or Trinity (@pipermerriam) or other client implementors to adding this immediately? Also, we need to determine if we should postpone the Ropsten fork with this change.

Mikhail Kalinin

@mkalinin

@holiman no objections from EthereumJ

Holger Drewes

@holgerd77

I am also seeing the Ropsten fork date as extremly tight, considering that EIP-1014 for CREATE2 is not finalized and tests for EXTCODEHASH ethereum/tests#484, SSTORE ethereum/tests#511 and difficulty bomb delay are not yet production-ready.

One thing is that breaking Ropsten testnet might not be the "end of the world" (Piper), but I think on this path we'll break it for-sure (at least very likely).

Fredrik Harrysson

@folsen

@holiman I talked this over with Tomasz internally as well, so I think whatever he says is good.

Ghost

@ghost~55c3ed250fc9f982beac84b3

In my opinion, it is good to delay Ropsten due to this addition to the EIP-1014. If Ropsten is mostly probably to be broken, it is risk without reward.

Martin Holst Swende

@holiman

I'm of two minds. It's a trivial change, and we haven't released the ropsten release yet, so it would be simple to include it within a few days. More testnet time is better...

Ghost

@ghost~55c3ed250fc9f982beac84b3

If it is trivial, then I would suggest trying to get it implemented. If it works, then keep Ropsten fork on track. If it does not, delay it :)

Martin Holst Swende

@holiman

Oh I made the PR earlier today

ethereum/go-ethereum#17812

But tests won't be regenerated in time :/

Piper Merriam

@pipermerriam

@holiman no need to hold things up for trinity, we're still not quite performant enough to be a viable client.

Adrian Sutton

@ajsutton

For the record, Pantheon’s CREATE2 implementation follows the new gas calculation and other changes from ethereum/go-ethereum#17812. It will be tight given the prep we need to do for initial open sourcing, but I’m hopeful that we’ll have full Constantinople support before it actually kicks in on Ropsten. The biggest concern is that the ethereum reference tests appear to be in a fairly unhappy state at the moment so testing compatibility is challenging.

And that was the wrong link… too many tabs open and all looking like github pull requests….

Martin Holst Swende

@holiman

We've started an initiative to improve on the situation with tests. See this gist for more details: https://gist.github.com/holiman/fdec3547f2b104803abbd2c6e751a8e7 . There is a new hire who will (among other things) work on this

Holger Drewes

@holgerd77

@holiman :thumbsup: :thumbsup: :thumbsup: That's great! @winsvega is doing a great job on the execution side, to complete the picture it would be great if you could also extend on communication with the new hire, atm it is hard to get e.g. the latest state of the Constantinople implementation or generally a picture what is happening related to tests.

Stuff I would value a lot here:

More structured issues or alternatively projects to sum up work e.g. for Constantinople, test reorg,...
More work on the documentation at https://ethereum-tests.readthedocs.io/en/latest/ (still have admin access there and can pass on access if desired)
Regular updates (biweekly or so) what is going on (Reddit, medium, somewhere, can also be minimalistic for not adding too much additional workload)

Just some wishlist. :smile:

Martin Holst Swende

@holiman

@5chdn are parity onboard with cost-per-byte for CREATE2 ? And if so, will it make it in before the ropsten HF release version? Otherwise, we definitely need to postpone

5chdn

@5chdn

tbh the ropsten hardfork date is very unfortunate because we won't be able to backport all constantinople changes to stable in time, but I wont complain because we missed the last dev call

5chdn

@5chdn

re: cost-per-byte, it's not implemented yet, but we could do it. I don't think anyone at parity would veto that

Ghost

@ghost~55c3ed250fc9f982beac84b3

I think it is definitely a good idea to postpone Ropsten hard-fork, and chose another block number when we know geth and parity have CREATE2 gas cost implemented. Also, didn't we overestimate the block time on Ropsten last time (took 15 sec instead of 12 something seconds)?

Martin Holst Swende

@holiman

Then I also think we should postpone Ropsten hardfork, makes no sense to roll it out if it's only geth

Ghost

@ghost~55c3ed250fc9f982beac84b3

@5chdn If you backport all constantinopole changes to stable, does it mean people can use the same version of Parity for Ropsten and mainnet?

Martin Holst Swende

@holiman

I merged my PR to update 1014 now

5chdn

@5chdn

cost per bute pr paritytech/parity-ethereum#9694

to be clear, Parity can support ropsten hf on nightly and beta, but our stable released probably won't make it

since it's only a test network it's probably fine

Lane Rettig

@lrettig

@AlexeyAkhunov yes, we estimated the block time at 14 sec I think, and last I checked it was 12.9

So we were off by about a day

Martin Holst Swende

@holiman

I'm inclined to let it be. @AlexeyAkhunov wants to postpone. @5chdn thinks 'probably fine'. Any other opinions?

By 'let it be' I mean don't postpone

Mikhail Kalinin

@mkalinin

I am currently working on gas calculation update for CREATE2. it should be ready by tomorrow. But it turns that we won't be ready to publish a release before Ropsten fork block happens, so, it'll be available only in a Snapshot version. But it doesn't look like a big problem and we're ok on keeping 4_200_000 for Ropsten

Elichai Turkel

@elichai

In the ABI, if a contract returns a uint. will the evm output it in decimal or in hex? (for e.g. the input to the evm is hex starting with 4b hash of function)

Piper Merriam

@pipermerriam

@elichai this isn't the place for that kind of support, you should ask on https://ethereum.stackexchange.com/

Paweł Bylica

@chfast

Aleth will be fine. But I don't mind postponing

Ghost

@ghost~55c3ed250fc9f982beac84b3

@holiman I don't necessarily want to postpone. I think this is a more prudent from the "process" point of view. If there are tests you can run on the clients before going into the testnet, you should run them first. If not, and you won't, then of course it does not matter

so I will reformulate the question to the client devs: will postponing the Ropsten fork allow you to run more code reviews/checks/tests on the code? If the answer is yes for at least geth or parity, I would recommend postponing. If no, then postponing does not help

Wei Tang

@sorpaas

The only issue we have right now for parity-ethereum is ethereum/tests. All CREATE2 test fails after cost per byte changes.

Martin Holst Swende

@holiman

@AlexeyAkhunov I see it a bit differently. We're understaffed on testing, and a good way to get more test coverage is to put it on a mainnet where everyone can help test it. It wouldn't have to be ropsten, it could have been a new testnet, but I think forcing clients to make real releases with Constantinople as soon as possible is better for everyone. I don't want to bork Ropsten intentionally, but if it uncovers a consensus error then that's a very good thing.

Martin Holst Swende

@holiman

Replace 'on a mainnet' with 'on a network'

Jason Carver

@carver

We know some clients will stay on the non-Constantinople Ropsten fork, like parity stable. Tests aren't complete, so it's reasonable to expect consensus errors too. We might say it's acceptable to break a test network, but plenty of devs think of it as a place to test their contracts rather than for core to test consensus. All of that implies delaying.
On the other hand, I see two key rationales for not delaying: 1) we can use Ropsten to help identify consensus issues quickly and 2) we train developers to deal with chain splits and long reorgs. Those are both very valuable. IMO, they are valuable enough to aggressively launch on Ropsten, with a caveat: we need a loud communications broadcast.
All teams involved should be posting to twitter, blogs, etc saying "we are intentionally testing a contentious fork to Constantinople on Ropsten at 4.2M, we expect clients to split the chain during testing". If parity, EF, etc aren't comfortable broadcasting that message in the next ~24 hours, then I think we should delay the Ropsten fork, and launch a brand new testnet instead, to start consensus tests.

Martin Holst Swende

@holiman

I wouldn't use the word 'contentious fork' though

Jason Carver

@carver

Heh, sure, this is why I'm not in charge of marketing

Martin Holst Swende

@holiman

https://twitter.com/mhswende/status/1047546087698051072?s=19

Greg Colvin

@gcolvin

From Grigore: how to count gas without metering it.
https://gitter.im/runtimeverification/iele-semantics?at=5bb41fa51c100a4f29212814

Paul D.

@poemm

@gcolvin I have proposed something similar for ewasm. I want to build tools for this in PyWebAssembly, and present advantages/disadvantages of various metering methods to everyone. But too busy lately getting ewasm WebAssembly engines ready before devcon.

Greg Colvin

@gcolvin

@poemm I look forward to post-devcon discussions

Where communities thrive

People

Repo info

Activity