Skip to content

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Mar 19, 2014

The idea being that once scripting is its own shared library, one can have a debugger than single-steps through a Script.

{
if (!vfExec.empty())
return false;
return true;
Copy link

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()?

@petertodd
Copy link
Contributor

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.

@sipa
Copy link
Member

sipa commented Mar 24, 2014

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.

@petertodd
Copy link
Contributor

@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.

@sipa
Copy link
Member

sipa commented Jun 1, 2014

Rebase please?

@luke-jr
Copy link
Member Author

luke-jr commented Jun 26, 2014

Rebased, with @Drak 's suggestion.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p3901_704672f36da19535aa81d0dbf950c696809ba0dd/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@jgarzik
Copy link
Contributor

jgarzik commented Jul 29, 2014

I really like the low diff size of this change. It helps reviewing and decreases risk IMO.

@sipa
Copy link
Member

sipa commented Sep 27, 2014

Closing due to inactivity. I'm personally still in favor of this, but should happen after the ongoing script refactorings.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants