-
Notifications
You must be signed in to change notification settings - Fork 326
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
Proposal to remove EIP-2315 (simple subroutines) from Berlin #263
Comments
Highly irregular, and a very bad precedent.
Yes, and we discussed it thoroughly at the time. One change - not walking into subroutines - we agreed was useful and straightforward to implement - a semantic change to make BEGINSUB throw if executed. Another - disallowing jumps into subroutines - was more difficult to implement, as it required syntactic analysis of EVM bytecode. It was not just a semantics change. EIP-615 attempted to impose syntactic restrictions, and that was part of why it failed. The suggestion for a BEGINDATA opcode I judged to be independent of this proposal, and better proposed separately, which it has been -- https://eips.ethereum.org/EIPS/eip-2327. The authors of that analysis - including members of the Solidity team - have not objected to moving this proposal forward after our discussion. Until now.
I was told today that they found ways to get the same gas costs for subroutine calls and returns without using dedicated opcodes, and that using them would impede optimizations. I've yet to see a written analysis to engage with, and find the claims to be surprising.
The Vyper team wants to use them. Wei Tang was looking forward to these opcodes to improve his LLVM-to-EVM translator. For other possible users I'd have to wrack my brain and dig though old business cards for conversations at conferences over the years. Maybe chase down lots of "likes" on Twitter. But EVM programming does not begin and end with Solidity. Further, the changes are done and the increase in complexity - IMHO - is small.
Yes, it is a first step in that direction, but useful in its own right. And yes EIP-615 gives a general direction for future changes.
Not all that clear, as it arose only today in one chat channel. Opcodes for subroutine calls and returns are useful and used in almost every real and virtual machine I know of. I don't expect these will impede future progress, given they haven't slowed down progress anywhere else.
Let us not let the (possible, future) best be the enemy of the good.
My time and resources as a champion are limited. I've already spent years working out these issues. And there is little reason for me believe that having been worked out, new issues won't be raised prior to its next scheduled upgrade. I am not hearing any objections that couldn't be or haven't been raised before, and certainly none worth removing the EIP over. |
@fubuloubu - the current maintainer of Vyper - has publicly expressed a desire for this EIP and an indication that they will use it. https://twitter.com/fubuloubu/status/1366949333854277632 |
The entire Vyper team has been looking forward to moving to subroutines and abandoning usage of dynamic jumps entirely for over 3 years now. I've also heard many folks from the security community express support for subroutines, if not EIP-2315 specifically. For the Solidity team, it might be difficult to make the transition or even use this feature in the first place, but I don't think that should block it's introduction if at least one other compiler team would make use of it. It sounds like there are no specific technical concerns to this moving forwards, simply some questioning usefulness to their specific projects. I would argue that it being useful to several users outside of the Solidity team should alleviate that concern. I believe this proposal pushes us in a better direction, having not made any progress on EIP-615 due to concerns of complexity. |
I will make a more proper response tomorrow, but for those interested currently: I have created a timeline of events to understand how we ended up in this situation with EIP-2315. I have done my best to provide an unbiased commentary of events. Please feel free to verify my commentary against the provided links. -- 17.10.2019: Greg publishes first draft as issue #2315. [1] |
I think from what @lightclient posted above (great work, BTW), it is clear that there were 2 points of contention with regards to the properties of the subroutines:
There were other observations made after my analysis:
And finally, as frustrating as it may be for @gcolvin and other authors, we are not trying to belittle your efforts on this. I am offering my bit of skill and expertise here, without claiming any expert authority, but just having done some analysis myself. I think trusting expert authority a bit too much was one of the reason we got into this situation. Experts do make mistakes and everyone has blindspots. Hope this was useful for anybody |
Thank you very much for this interesting timeline! If it is correct, it confirms my impression that the EIP was never properly accepted. It was moved to accepted once, but that was never reflected on the proposal or the discussion thread. Also it was further discussed and modified after this point in time. Please also note that my request to clarify the goals of the EIP was never reacted upon. Also during 2020 I was trying to make the EIP better for static analysis because I thought this was the goal. Since no static analysis exerts participated in the discussion, we don't know if the modifications improved the situation. Recently it turned out that at least on the initial example in the EIP, the subroutine version is more expensive, so the gas savings goal is also questionable. I have said that elsewhere, but wanted to repeat it here: I'm totally fine with this being part of Berlin if that is the wish of the ACD, I just want to advise that it is a change that is very difficult if not impossible to undo or modify, it's not just adding a new opcode, it's a much more profound change and it's a change whose short and long-term benefits are unclear. |
With respect to the process, we always only accepted proposals without any outstanding technical concerns. I haven't followed the discussions last year but from what I read in the summary, there are yet some questions and concerns to be addressed. We can find that out on Friday. I'm not confident to make a technical comment on the proposal, but would like to emphasize that we should rather risk breaking testnets by pulling 2315 just to be on the safe side than activating a feature on mainnet prematurely. Just from looking at the time left for Berlin, the only realistic question to ask right now is not "shall we keep 2315 in Berlin" but "will pulling 2315 from Berlin have any unintended side-effects?" Once everything is resolved, we can reconsider it for London, but given the current debate we should buy some time for 2315. |
I do not actually agree that this sets a "very bad precedent". What is worse: "They found out very late in the process the EIP does not meet initial requirements and does not seem to be useful, and decided to take it out while it was still in their hands" or "They found out very late in the process the EIP does not meet initial requirements and does not seem to be useful, and decided to leave it as it is, because it has been accepted, and they did not want to make EIP authors too upset, and they were worried about setting a bad precedent"? |
Another way to word the latter:
While it is certainly possible that the people suggesting it isn't worth doing are right, the problem is that we don't have enough time to dive deep and thus we are allowing a well timed suggestion of "not worth it" to block an EIP. |
I would rather put it like this,
|
It appears things are leaning towards removing EIP-2315, so I'll keep this post short. Those who support EIP-2315 in Berlin continue to base their argument for inclusion on the past decision by ACD to accept the EIP (1, 2, 3, 4, 5, 6, 7). We have processes to avoid these types of situations and make our lives easier. Those processes are only as airtight as the humans who design and implement them. Humans make mistakes and those mistakes can manifest anywhere at anytime. There is no need to be victims of our own creation. A mistake was made in the process, and EIP-2315 should not have been accepted. As far back as ACD 81, the Geth team requested benchmarks be done to prove gains claimed by the EIP. These benchmarks did not materialize. In ACD 84, @Souptacular motioned to move the EIP to accepted. @tkstanczak reiterated that if the use cases were there (improved codegen + static analysis) then he was onboard. Neither had been proved, yet the EIP was scheduled for Berlin. In ACD 86, @MadeofTin concedes that moving the EIP to accepted was premature given the continued debate regarding the spec. Even months later, in the last reference I can find to the EIP in an ACD call, @Souptacular notes that there are still outstanding questions around the spec. @gcolvin stated that it would be resolved offline in the Magicians thread, but it was not resolved. Throughout this, at almost every step of the way @axic, @chfast, and @chriseth were voicing concerns with the proposal. An analysis was written and a PR was opened to the EIP to avoid jumping into and out of subroutines - which is probably the strongest complaint against the EIP. Unfortunately for some reason, they became less involved with the EIP last fall and it managed to spend several more months on the Berlin docket. This gave the EIP an undue sense of authority for those less involved in the discussion of its viability. The process should've ensured their complaints with the EIP were resolved, but it didn't. It would have been nice if they were there to continue fighting it, but they weren't. They had already spent months fighting it - the process should've blocked the EIP until the discussion was resolved. In terms of technical complaints of the EIP, I'm not correct person to speak on the issue. Hopefully @AlexeyAkhunov's thoughts above in combination with @chfast's analysis is enough to concede that the usefulness of this EIP is still being questioned. Although this is a highly irregular proposal, it is not personal. I sincerely apologize for disruption it has caused. I plan to do my part moving forward to avoid this happening again in the future. I hope that we can have further constructive conversations as a group to improve the EIP process. |
This proposal was accepted in ACD 107. EIP-2315 will be removed from Berlin. |
While waiting for my girlfriend to come celebrate my birthday I took time to listen in on the meeting and review the notes above. I'd like to thank Peter for his comment on the "nastiness" of this decision and its follow-on effects. For myself, I am left to seriously reconsider my future involvement with the Ethereum project. And to seriously question whether I could in good conscience advise others to participate. Unless they enjoy chaos and disappointment. It is true that a single technical concern (whether to bear the cost to prevent branching into subroutines) remained active for some time after initial acceptance, as I attempted to give it the best consideration I could. In the end, as author, I made the choice not to accept the change, for reasons I stated at the time -- from the very start I meant this proposal to be a very small change to provide subroutines with Forth semantics, and not to impose syntactic restrictions. The issue was not raised again until Christian made a comment - not on the ACD channel, but on Twitter - that for him it remained a blocking objection. That objection left me - unprepared and at the last minute - trying to field objections and explain decisions long since made. I should not have needed to do that. I put some degree of blame on the client teams themselves. Before implementing, testing, and scheduling an EIP for an upgrade I believe it falls on them to do their own due diligence. To be convinced that the EIP is really an improvement to the protocol, and be able to justify their decision. And Hudson, thank you for keeping the discussion respectful and on track, as disappointed as I am in the outcome. I hope this sad turn of affairs leads to some serious soul-searching -- we have tasked ourselves with the research, development, and management of a network with a current market capitalization of $173 billion USD. I have no clue how many businesses are building on that network, or how many livelihoods it supports. We must learn to operate like professionals. |
EIP-2315 has a long history. It descends from EIP-615, a much more heavy-handed approach to improving the EVM semantics to produce more efficient code from compilers and be more amenable to static analysis. That EIP was abandoned due it's complexity and lack of concrete improvements to code efficiency.
EIP-2315 was proposed in late 2019 as a backwards-compatible mechanism for improving calling conventions within the EVM. A thorough analysis was done in April 2020 by several members of the evmone team. It appears that the "fallthrough subroutine" case was addressed with a modification to 2315 which would revert if a
beginsub
was called directly instead of as the result of ajumpsub
. There were several other pieces of feedback that do not seem to have been addressed, including the note thatJUMP
s across subroutines would i) complicate static analysis ii) inhibit the translation of EVM bytecode to LLVM IR.It is irregular to propose that an EIP is removed from a hardfork after it is scheduled. However, I think we should focus on the issue at hand rather than argue about past failures of the process. EIP-2315 has been publicly opposed by several members of the Solidity team over the last few days, including @chriseth (link) and @ekpyron (link).
If they don't believe the EIP will be useful for the Solidity compiler, then it is unlikely that the EIP will be used by many contracts on mainnet. Therefore, it will increase the complexity of the EVM with little to no benefit. For this reason, I'd like to propose that the EIP is removed from Berlin and these issues be worked out before including it into a fork.
One critique of this proposal will be that this EIP is a stepping stone to future changes, possibly even full EIP-615-like support. Generally, splitting large EIPs into smaller EIPs make sense. However, I think each step should be useful on its own. There is a clear debate around this EIP's usefulness, and even arguments that it may encumber future changes. By introducing changes piecewise, we run the risk that something better comes along or worse or it turns out that the future changes do not make it into the protocol.
--
On a personal note, I want to apologize for bringing this up so late in the process. As I said above, such a proposal is irregular and I feel partially to blame for not spending more time reviewing this EIP and making sure that it met the needs of its users. I will make a much more dedicated effort for future forks to ensure that this situation doesn't happen again.
The text was updated successfully, but these errors were encountered: