Proposal to remove EIP-2315 (simple subroutines) from Berlin
Closed this issue · 14 comments
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 a jumpsub. There were several other pieces of feedback that do not seem to have been addressed, including the note that JUMPs 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.
It is irregular to propose that an EIP is removed from a hardfork after it is scheduled.
Highly irregular, and a very bad precedent.
A thorough analysis was done in April 2020 by several members of the evmone team.
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.
EIP-2315 has been publicly opposed by several members of the Solidity team over the last few days...
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.
... 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.
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.
One critique of this proposal will be that this EIP is a stepping stone to future changes ...
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.
There is a clear debate around this EIP's usefulness, and even arguments that it may encumber 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.
... 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.
Let us not let the (possible, future) best be the enemy of the good.
... I'd like to propose that the EIP is removed from Berlin and these issues be worked out before including it into a fork.
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]
18.10.2019: Alan Li (lead of EVM-LLVM project) receives the proposal positively. He notes that it would great if EVM had multi-byte instructions. [2]
18.10.2019: Alex B responds he is in favor of multi-byte instructions, but this was an issue that EIP-615 ran into. He says EVM versioning would allow for this.
21.1.2020: Greg opens PR 2484 to create EIP-2315. [3]
22.1.2020: lithp posts that he doesn't understand the value of the EIP. [4]
22.1.2020: Greg states that the value of the EIP is lower gas costs for contracts and easier static analysis. [5]
23.1.2020: Martin leaves some comments on the PR. The EIP currently lists no security considerations, but Martin points out that subroutines will "affect program flow analysis frameworks" and those frameworks will need to be updated. [6]
23.1.2020: Alex suggests that a JUMPDEST could follow JUMPSUB to maintain the current program flow. The thread is not resolved. [7]
1.2.2020: Greg merges the draft into EIP repository. [3]
1.2.2020: Greg requests that the EIP be discussed in ACD 80. [8]
7.2.2020: The EIP is discussed in ACD 80. Martin states that the EIP is technically feasible, but that he wants to understand how much it improves things. Danno voices his support, noting that EIP-615 was too much but this is a good first step. [9]
17.2.2020: Greg posts some solidity compiled bytecode and shows that subroutines would reduce the cost by 66%. [10]
21.2.2020: The EIP is discussed during ACD 81. Greg gives a summary of the EIP and notes that it will "bring the EVM up to 1970 standards". Alan agrees. Peter says that on the surface, the EIP looks nice - however, he'd like to hack something into the solidity compiler to gauge how much of an improvement it provides. Alan says that subroutines has nothing to do with the solidity language - just compiler and EVM changes. Peter reiterates that it seems like the whole point of subroutines is to make code more efficient so he'd like to have that confirmed before shipping. Felix agrees that it should be confirmed before inclusion. James asks if the benchmarks are produced, are there any other oppositions to the EIP. Martin responds that there are not. Greg states that in the EIP's appendix there is an example that shows the improved gas costs. He agrees that it should be benchmarked, but expects the gas savings to be substantial. Felix notes that he expects the same, but would prefer to confirm it before shipping the EIP. The EIP is moved to EFI and James states that the next step is to produce the benchmarks. This is not acknowledged as a decision in the notes. [11]
26.2.2020: Alex asks if the EIP is actually better for static analysis since the subroutines destinations are still dynamic. [12]
26.2.2020: Greg says that some kinds are easier, but that he's heard complaints about deciphering subroutines in EVM code. [13]
28.2.2020: Alex responds to Greg's previous claim that subroutines were 66% faster than solidity compiled code stating this was because he compiled the code with functions marked public. [14]
6.3.2020: An update is given in ACD 82 on the EIP, stating that the work is ongoing. [15]
7.3.2020: Chris posts that the only benefit of this EIP is juggling the stack to access return addresses. He notes that a "rotate stack" opcode would be a much simpler change to achieve the same thing. He goes onto say that he'd like to restrict jumping into subroutines, but concedes that is likely too complex. He closes by stating that he cannot state whether the EIP is worth pursuing because Solidity will not benefit from it. He would like "people working on debuggers (remix, truffle, etc), static analysis tools and VM implementations" to review the EIP. [16]
19.3.2020: Martin requests Alex and Chris review the EIP at a high-level to understand their stance on it. [17]
19.3.2020: Chris responds that he would like this EIP to be clear about its goals. He notes that does not help static analysis enough to be included and it is only marginally more efficient than a stack rotation opcode (which would be more general purpose). He reiterates that the EIP author should be getting opinions from debugger and bytecode analyzer developers. [18]
20.3.2020: In ACD 83 Martin states that the EIP was discussed with the Vyper team and they gave it the thumbs up. He notes there is some ongoing discussions in the FEM thread. [19]
3.4.2020: Alex posts that there is a discrepancy between EIP-615 this EIP, because in EIP-615 the JUMPSUB jumps to an immediate destination whereas this EIP's JUMPSUB jumps to a dynamic destination. He notes that he doesn't understand how having dynamic JUMPSUBs will help when other types of dynamic jumps. He would like to see the static version as part of the proposal, although it may not need to be implemented immediately. [20]
3.4.2020: The EIP is discussed in ACD 84. Martin shares some updates made to the spec. Greg states that he is very happy with the EIP. Hudson asks if it should be moved to accepted. Tomasz says that if uses cases for codegend and code verification are there, then its fine. Hudson moves the EIP to accepted. [21]
4.4.2020: Martin disagrees with Alex, stating that he doesn't believe multi-byte opcodes would be major issue. [22]
17.4.2020: The EIP is discussed in ACD 85. Only updates on client implementations are given. [23]
29.4.2020: Paweł publishes an analysis of the EIP. In the analysis, he notes multiple concerns including fallthrough behavior and jumping across subroutines. [24]
29.4.2020: Martin responds that the fallthrough issue can be resolved easily, but the jumping across subroutines is unfortunate. [25]
29.4.2020: Alexey notes his concern with the ability to jump across subroutines and that he thinks if structural restrictions are not introduced for legacy jumps, it will be a messy situation. He concede with Greg that it is a slippery slope back to EIP-615, but hopes that it may cause the former opponents of EIP-615 to reconsider their hypothesis that splitting the EIP into smaller pieces is simpler. [26]
1.5.2020: The EIP is discussed in ACD 86. Martin summarizes the analysis published by Paweł et al. He notes that many people have commented that they prefer to not have the ability to jump into and out of subroutines. Greg says that he does not want to restrict that, because he wants this EIP to be as simple as possible. He notes the discussion is ongoing. James says that he thinks it was premature to schedule the EIP for Berlin and would like to wait until the discussions play out. Martin says that he'd like to go back to the format where EIPs are done when they're done and then they're slated for a hardfork. James agrees and says its not fair to say whether the EIP will or will not be ready for Berlin at this time. [27]
1.5.2020: Greg posts that he is worried about making subroutines syntactic. He says his intent is to provide a pure control flow mechanism with a Forth-style return stack. He acknowledges how, as written, the EIP may cause problems for LLVM compilation. [28]
1.5.2020: Paweł posts that this EIP is good starting place, but that there should be two additional restrictions (stated above) that will maintain the EVM "look and feel" of the EIP. [29]
12.5.2020: Chris reiterates that this EIP does not very beneficial to compilers and that if a subroutine returns a value, it may actually be for efficient to use a regular jump. [30]
13.5.2020: Alan disagrees that the EIP is not beneficial to compilers. [31]
15.5.2020: The EIP is discussed in ACD 87. Péter notes that the specification isn't complete yet. Martin concurs. Greg says it should be complete in the next week. [32]
20.5.2020: Chris opens a PR into the EIP to restrict jumping into a subroutine. Greg opposes the change, while Martin thinks it's good. The PR is never resolved and is eventually closed for inactivity. [33]
29.5.2020: The EIP is discussed in ACD 88. Martin states that he is feeling general consensus on the EIP. He notes the subroutines restrictions that have been proposed and acknowledges that proposal is still being discussed. [34]
4.9.2020: The EIP is discussed in ACD 95. Greg says that there is still a discussion of whether jumping into subroutines should be allowed and he is more amenable to it. [35]
16.9.2020: Alex pings Greg to review Chris' PR regarding the restriction of jumping into subroutines. [36]
17.9.2020: Greg says the PR overall looks good, but is confused by a section. [37]
18.9.2020: The EIP is discussed in ACD 96. Hudson says that there is still an outstanding question as to whether restrict jumping into subroutines. Greg says the discussion can move to FEM in the interest of time, but that he is inclined to accept the restriction. Greg says that believes what exists is ready to go and that everyone agrees with this. Martin agrees. Alex and Paweł say they are still in favor of the restriction. Martin notes that the change will generally make clients more complex, but should be okay. He notes that it may be difficult to get in other clients. James says the current version can go into YoloV2 and the version with the jump restriction can go into YoloV3, if it is finalized. [38]
19.2.2020: Greg resolves his confusion from above, but notes that he believes the proposal break backwards compatibility and is therefore against it. No additional comments are made regarding the EIP in the analysis thread. [39]
Note: ACD 96 is the last time I found 2315 discussed in an ACD. Meeting notes were missing for 98 & 100, so I'm not sure if restricting jumping was discussed in those.
1.3.2021: Chris posts on twitter that he still doesn't believe the EIP is useful. [40]
1.3.2021: Daniel posts that he believes the EIP doesn't achieve what it claims, namely improved efficiency and static analysis, to a degree that warrants its inclusion in a hard fork.
2.3.2021: I read these posts and share them in ACD discord. [41]
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:
- Whether the
JUMPSUBuses jump location from the stack, or from the immediate as a part of the opcode. Eventually the stack solution was used, and I agree with Chris (after my own analysis) that this effectively made this variant of subroutines non beneficial for one of initially stated goals - ease of static analysis due to lack of dynamic jumps. It actually makes that stated goal harder to achieve because to do so,JUMPSUBopcode will need to be deprecated as well asJUMPandJUMPI. - Whether the jumping from the middle of one subroutine into the middle of another subroutine is allowed. Eventually this was allowed, I believe to simplify the implementation and therefore expedite the acceptance of EIP. I believe (after my own analysis) that this decision has unfortunately mostly eroded another stated benefit of the subroutines - strict "first in/first out" behaviour of subroutines calling each other (as we are used as programmers). As a result, simple emulation of the implemented behaviour by using combination of
PUSHandJUMPopcodes seems to suffice.
There were other observations made after my analysis:
- The claim (now apparently retracted) by the EIP author that subroutines are present in every modern virtual machine might be true for the original definition of subroutine from EIP-615 (they would go under the names of virtual and static methods in Java, compiled into
invokevirtualandinvokestatic, under the name of "functions" in Web Assembly). However, in the definition of "subroutine" we ended up in this EIP-2315, the only similar thing I found so far is the concept of "subroutine" in JVM, which is implemented by the pair of instructionsjsrandret. These instructions seem to only be produced by Java Complier to compactly compilefinally {}blocks. Also, sinceretuses a local variable for its return location, this seems to be the only instruction that makes JVM not dynamic-jump free. - The term "subroutine" that was used throughout the process of rejecting EIP-615, then splitting part of it into EIP-2315, and then adapting EIP-2315 for the ease of implementation, have probably led to much confusion and surprise. Because what we got in the end does not look and feel like a "subroutine" (function) that a programmer's intuition usually tells us. I must confess I did not notice that change and assumed until very recently that EIP-2315 still proposed the subroutine in similar meaning to EIP-615 that is why I did not think about objecting earlier.
- The claim that the use of subroutines from EIP-2315 will be beneficial for Vyper compiler as a safely measure for inter-contract calls, needs to be revisited. I have a suspicion (also looking at the timeline posted above) that the Vyper team gave their thumbs up to the proposal while it was still restricting cross subroutine jumps, which does make sense to me. I find the argument for still using subroutines in the current EIP-2315 form (without cross-jump restriction) as a safety feature, much weaker and perhaps non-existent.
- Reference implementation of the currently proposed form of this EIP was done in go-ethererum. After the review of the implementation, I concluded that it does not incur extra performance overhead on other opcodes. However, in evmone implementation, EIP-2315 seems to be introducing overhead to all the execution. After looking at the implementation of jumps (and now of
JUMPSUBin particular), I think the reason for the presence of this overhead in evmone and its absence in go-ethereum is most probably the fact that evmone has ostensibly more optimised implementation of jumps that does more work upfront but less work during the actual jump. Such more optimised implementation suffers extra overhead due to more work upfront to potentially handleJUMPSUB. Since go-ethereum does not have these optimisations and did not experience this overhead, this issue has not been flagged until the evmone implementation was benchmarked.
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.
Surfacing various client team's position about keeping 2315 in Berlin from the discord
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"?
From my humble point of view, the second is the worst option. The first actually shows that people care a lot about what goes in. The second shows that people care more about sunk costs and optics. Sorry if this is too harsh.
Another way to word the latter:
Some people suggested that the EIP may not be worth doing, but there wasn't enough time to thoroughly evaluate it again before Berlin so we pulled it despite the fact that back when we did have time to think about it we decided it was a good idea.
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 was decided that an EIP needs some better review, so to avoid the issues its better to wait with it hardcode implementation for security reasons.
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.