Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ethereum Core Devs Meeting 135 Agenda #500

Closed
timbeiko opened this issue Mar 23, 2022 · 16 comments
Closed

Ethereum Core Devs Meeting 135 Agenda #500

timbeiko opened this issue Mar 23, 2022 · 16 comments

Comments

@timbeiko
Copy link
Collaborator

timbeiko commented Mar 23, 2022

Meeting Info

  • April 1, 2022, 14:00 UTC
    • Note: Daylight Savings time will start in Europe before the call, but the call is "fixed" at 14:00 UTC - double check your local time!
  • Duration: 90 minutes
  • Youtube Stream: https://youtu.be/1QU8r9-SJDc
  • Zoom: To be shared on #allcoredevs Discord server shortly before the call

Agenda

@q9f
Copy link
Contributor

q9f commented Mar 23, 2022

Before we merge Goerli, I would like to inflate the supply. So I decided not to create an EIP to not distract too many people with that.

Any feedback would be appreciated.

@timbeiko
Copy link
Collaborator Author

Copying Moody's comment from the Shanghai Planning issue here:

I'd like to discuss specifically moving EIP-1153 to CFI in ACD 135 for the reasons:

@yperbasis
Copy link
Member

yperbasis commented Mar 29, 2022

To my mind EIP-4758: Deactivate SELFDESTRUCT is a higher priority than EIP-1153. EIP-4758 is a potentially significant simplification. Also EIP-4758 is a prerequisite for the Verkle tree.

@moodysalem
Copy link

To my mind EIP-4758: Deactivate SELFDESTRUCT is a higher priority than EIP-1153. EIP-4758 is a potentially significant simplification. Also EIP-4758 is a prerequisite for the Verkle tree.

I think deactivating SELFDESTRUCT is more controversial. For example, some people might use self destruct for upgradeability. And EIP-1153 also allows for simplification of gas accounting rules in the future.

I'd love to discuss further on the call. They're obviously not exclusive, but in the case of EIP-1153 we have many protocol developers who want it and are willing to do or fund the work, and the chance of breakage due to inclusion of EIP-1153 is close to zero.

@ralexstokes
Copy link
Member

we decided on the overall approach to validator withdrawals on the last ACD and moved EIP-4895 to CFI.

there was some rough consensus around a few remaining details and I've put those details into writing in an update to 4895 here: ethereum/EIPs#4957

I'd like to run thru the high-level points of this update on the call to make sure everyone is on the same page :)

@gcolvin
Copy link

gcolvin commented Apr 1, 2022

I have an emergency EIP to add to the agenda.

EIP-666

@mkalinin
Copy link
Contributor

mkalinin commented Apr 1, 2022

I would like to touch latestValidHash support in EL clients, here are my rough thoughts on how it could be implemented

@holiman
Copy link

holiman commented Apr 1, 2022

Oops, I meant to post this here, but posted it to the shanghai planning instead

To save some time, here are my thoughts around EIP-1153

  • It's a nice feature, of course we'd want it
  • However, it's been a backburner-nice-to-have since 2018, the reason (I think) being that it doesn't revolutionize anything, just makes certain usecases easier/cheaper/(safer?).
  • And it is quite nontrivial to implement,
  • And also needs a lot of efforts to fully test
  • The implementation must also be tested from a DoS perspective (a quick napkin calculation tells me that with 30M gas, up to 9MB of data can be stored, which is probably fine, but there are journalling to take into consideration too -- this would entail a journal with 300K items to roll back)
  • Unfortunately, we have a lot of things competing for inclusion in Shanghai. In "competition" with those, I would not pick 1153 to be selected.
  • Also, I'd be curious to hear what @AlexeyAkhunov thinks about it. Still want it?

@holiman
Copy link

holiman commented Apr 1, 2022

I think deactivating SELFDESTRUCT is more controversial. For example, some people might use self destruct for upgradeability. And EIP-1153 also allows for simplification of gas accounting rules in the future.

About this, I know @jwasinger has done a pretty thorough investigation. He has a repo about it here: https://github.com/jwasinger/eth-selfdestruct-analysis .

@moodysalem
Copy link

moodysalem commented Apr 1, 2022

The implementation must also be tested from a DoS perspective (a quick napkin calculation tells me that with 30M gas, up to 9MB of data can be stored, which is probably fine, but there are journalling to take into consideration too -- this would entail a journal with 300K items to roll back)

The estimates are covered in security considerations. However the time complexity of the journaling is not deeply considered in the gas costs. Currently in the EthereumJS VM implementation, the time complexity is as follows:

  • on TSTORE O(1) (write into map and list)
  • on TLOAD O(1) (read from map)
  • on call context creation O(1) (pop new changeset into map)
  • on revert is O(N) where N = unique keys written in the call that reverted
  • on return is O(N) where N = unique keys written in the call that reverted

I'd have to check but I think 300k memory accesses in the worst case might be pretty fast still.

And it is quite nontrivial to implement,
And also needs a lot of efforts to fully test
Unfortunately, we have a lot of things competing for inclusion in Shanghai. In "competition" with those, I would not pick 1153 to be selected.

Are they in competition if we do all the work? We are willing to do the work or find people to do the work and fund it. If we do all the work and have working implementations in at least geth and 1-2 other clients, and comprehensive tests by late summer, can we assume this is not an issue? We are just asking for it to be considered for inclusion so we can commit to doing the work.

@holiman
Copy link

holiman commented Apr 1, 2022

In that case, I think there's a different attack that EthereumJS is vulnerable too, namely basically what geth was vulnerable to back in the shanghai days.

We had map-backed "journalling". With map-backing, as opposed to linear log/journal, when one enters a new scope, there are two choices. Either you copy the outer map A into a brand new "inner" map B, and when the scope is done, you either just continue using the B map (and drop A), or if a revert happens, you drop B and continue with A.

With that schema, the attack is to fill up A with a lot of items, and then force a lot of copying: entering into scopes recursively, and/or iteratively.

The second choice is to not copy the outer map, but instead keep it, but have a second B map which contains the scope-local differences. And then you maintain a hierarchy of such maps. The problem with this approach is that to lookup a piece of data, you may have to traverse the entire hierarchy to the outermost scope.

Anyway, in geth, we use a journal, which is just a series of change-events, which can be applied in reverse if needed.

Are they in competition if we do all the work?

Unfortunately, yes

@moodysalem
Copy link

moodysalem commented Apr 1, 2022

In that case, I think there's a different attack that EthereumJS is vulnerable too, namely basically what geth was vulnerable to back in the shanghai days.

In the current implementation I agree. If you create a very deep call stack, write a bunch of keys, and then return from the innermost call, you will copy the map for each call in the callstack. The memory complexity will not be more than O(10MB) because the innermost map is discarded after it's merged and can be garbage collected. However, you will have to copy O(100k) keys per context which could be very bad.

One easy solution is limiting the number of keys in the per-address map to 1024. That is more than enough for our use case.

Anyway, in geth, we use a journal, which is just a series of change-events, which can be applied in reverse if needed.

Do you have to iterate the journal to load a key with this implementation? The stack of maps solution (second choice) seems better than a journal of changes.

Unfortunately, yes

Can you elaborate? Per @gcolvin, this EIP does not interact with other EIPs being considered. We can also implement it in clients without turning it on with Shanghai, if it's CFI.

@holiman
Copy link

holiman commented Apr 1, 2022

Do you have to iterate the journal to load a key with this implementation? The stack of maps solution (second choice) seems better than a journal of changes.

No, the data is in a map. Reverting means applying the change-entries in reverse.

Can you elaborate?

I mean, this is just all my opinion, but this is a large implementation. I don't think any client-implementation will just accept a PR without review. Reviewing a PR (and especially a PR which touches core/consensus) takes more time than writing the code. Reviewing the tests, to ensure they cover all edgecases, testing any DoS-attacks etc etc all takes time.

Anyway, I just wanted to signal my 5c early to save some time on the call.

@gcolvin
Copy link

gcolvin commented Apr 2, 2022

Can you elaborate? Per @gcolvin, this EIP does not interact with other EIPs being considered. We can also implement it in clients without turning it on with Shanghai, if it's CFI.

Not sure where I said that, @moodysalem, but I suspect I spoke only in terms of semantics. Regardless, I'm more likely to be wrong about these things than @holiman.

@moodysalem
Copy link

Can you elaborate? Per @gcolvin, this EIP does not interact with other EIPs being considered. We can also implement it in clients without turning it on with Shanghai, if it's CFI.

Not sure where I said that, @moodysalem, but I suspect I spoke only in terms of semantics. Regardless, I'm more likely to be wrong about these things than @holiman.

#450 (comment)

I don't believe @holiman ever contradicted this statement.

@timbeiko
Copy link
Collaborator Author

timbeiko commented Apr 4, 2022

Closed in favor of #508, we can keep using #438 to discuss 1153

@timbeiko timbeiko closed this as completed Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants