Wrap EvalScript in a ScriptExecution class

luke-jr commented at 8:29 am on July 3, 2017: member

When we last discussed making scripts debuggable (sometime after #3901), the plan was to instead trace execution rather than single-step through it.

This is the first step toward that goal. The full implementation can be found on my script_debugger branch.

fanquake added the label Validation on Jul 3, 2017

in src/script/interpreter.h:193 in 0fa35e3c80 outdated

178+class ScriptExecution {
179+public:
180+    typedef std::vector<unsigned char> StackElementType;
181+    typedef std::vector<StackElementType> StackType;
182+
183+    const CScript& script;

promag commented at 9:57 am on July 4, 2017:

Make these private?

luke-jr commented at 10:45 am on July 4, 2017:

They are meant for external inspection.

promag commented at 11:00 am on July 4, 2017:

When that happens either make it public, create a getter or an Inspect method?

in src/script/interpreter.h:213 in 0fa35e3c80 outdated

198+    bool Eval(ScriptError* error = NULL);
199+};
200+
201+inline bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* error = NULL)
202+{
203+    return ScriptExecution(stack, script, flags, checker, sigversion).Eval(error);

promag commented at 10:35 am on July 4, 2017:

Move stack to ::Eval() for the time being as this is an output (also remove stack member from class)?

luke-jr commented at 10:46 am on July 4, 2017:

It’s not only an output.

in src/script/interpreter.h:188 in 0fa35e3c80 outdated

173@@ -174,7 +174,34 @@ class MutableTransactionSignatureChecker : public TransactionSignatureChecker
174     MutableTransactionSignatureChecker(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn) : TransactionSignatureChecker(&txTo, nInIn, amountIn), txTo(*txToIn) {}
175 };
176 
177-bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* error = NULL);
178+class ScriptExecution {

promag commented at 10:36 am on July 4, 2017:

Nit,rename to ScriptExecuter?

luke-jr commented at 10:45 am on July 4, 2017:

It can only execute a single script.

in src/script/interpreter.h:197 in 0fa35e3c80 outdated

193+    StackType altstack;
194+    int nOpCount;
195+
196+    ScriptExecution(StackType& stack, const CScript&, unsigned int flags, const BaseSignatureChecker&, SigVersion);
197+
198+    bool Eval(ScriptError* error = NULL);

promag commented at 10:37 am on July 4, 2017:

Rename to Execute?

luke-jr force-pushed on Sep 2, 2017

luke-jr commented at 5:48 am on September 2, 2017: member

Rebased

jtimon commented at 0:58 am on September 6, 2017: contributor

Concept ACK, will review

MarcoFalke added the label Needs rebase on Jun 6, 2018

script/interpreter: Wrap EvalScript in a ScriptExecution class 33e2c48ec4

script/interpreter: Move useful evaluation variables to ScriptExecution class 1887155ef2

luke-jr commented at 9:25 am on November 3, 2018: member

Rebased

luke-jr force-pushed on Nov 3, 2018

DrahtBot removed the label Needs rebase on Nov 3, 2018

in src/script/interpreter.h:206 in 1887155ef2

202+
203+    std::vector<bool> vfExec;
204+    StackType altstack;
205+    int nOpCount;
206+
207+    ScriptExecution(StackType& stack, const CScript&, unsigned int flags, const BaseSignatureChecker&, SigVersion);

practicalswift commented at 9:39 pm on November 4, 2018:

Match parameter names between declaration and definition :-)

in src/script/interpreter.h:208 in 1887155ef2

204+    StackType altstack;
205+    int nOpCount;
206+
207+    ScriptExecution(StackType& stack, const CScript&, unsigned int flags, const BaseSignatureChecker&, SigVersion);
208+
209+    bool Eval(ScriptError* error = nullptr);

practicalswift commented at 9:39 pm on November 4, 2018:

Same here :-)

in src/script/interpreter.cpp:282 in 1887155ef2

277@@ -278,7 +278,12 @@ int FindAndDelete(CScript& script, const CScript& b)
278     return nFound;
279 }
280 
281-bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* serror)
282+ScriptExecution::ScriptExecution(StackType& stack_in, const CScript& script_in, unsigned int flags_in, const BaseSignatureChecker& checker_in, SigVersion sigversion_in) :
283+    script(script_in), stack(stack_in), flags(flags_in), checker(checker_in), sigversion(sigversion_in), pc(script.begin()), pbegincodehash(script.begin()), nOpCount(0)

practicalswift commented at 9:41 pm on November 4, 2018:

Should vfExec and altstack be initialized too?

DrahtBot commented at 3:49 am on November 9, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17977 ([WIP] Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa)
#16902 (O(1) OP_IF/NOTIF/ELSE/ENDIF script implementation by sipa)
#13062 (Make script interpreter independent from storage type CScript by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jtimon commented at 9:13 pm on April 3, 2019: contributor

utACK, unless I’m missing something, this should not change behavior at all.

DrahtBot closed this on Aug 16, 2019

DrahtBot commented at 1:49 pm on August 16, 2019: member

DrahtBot reopened this on Aug 16, 2019

luke-jr commented at 8:59 pm on February 26, 2020: member

Abandoning this until there’s some indication anyone cares

luke-jr closed this on Feb 26, 2020

DrahtBot locked this on Feb 15, 2022

Wrap EvalScript in a ScriptExecution class #10729

Conflicts