-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Split up EvalScript into EvalScriptState::Start and ::Step, so single-stepping can be done #3901
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
Conversation
{ | ||
if (!vfExec.empty()) | ||
return false; | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming vfExec.empty()
returns boolean, so could you simplify with return vfExec.empty()
?
ACK on the idea of making some debugging tools, but a very, very strong NACK on actually changing the consensus-critical code that gets executed by default. It's just unnecessarily risky for little gain when you can just as easily make a copy of the code. Sure, the latter is a annoying to maintain, but I'll take "annoying to maintain" over "accidentally cause a fork" any day, particularly since the code being copied rarely ever changes. |
I fully agree that changing consensus-critical code is dangerous, but not that copying is the right solution. The purpose of the reference client code shouldn't just be being "right" (even though that has very high priority). It should also be clear, transparent and reusable. If it's not reusable, people will either reimplement it (and make mistakes), or copy & modify & make mistakes. I'm very much in favor of creating a low-dependency (ideally: none) library that separates out the script evaluation functionality (and later on, one that implements the validation code without actual disk storage, peer communication, ... ). Design ACK here, but I haven't reviewed the code changes yet. |
@sipa Agreed on the library, but that doesn't exist yet, and this is a very, very high-risk case. If anything, maybe what that is saying is that we should just hold off on this code until we've got that library and can actually make use of it. |
Rebase please? |
…-stepping can be done
Rebased, with @Drak 's suggestion. |
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p3901_704672f36da19535aa81d0dbf950c696809ba0dd/ for binaries and test log. |
I really like the low diff size of this change. It helps reviewing and decreases risk IMO. |
Closing due to inactivity. I'm personally still in favor of this, but should happen after the ongoing script refactorings. |
The idea being that once scripting is its own shared library, one can have a debugger than single-steps through a Script.