Wrap EvalScript in a ScriptExecution class #10729

pull luke-jr wants to merge 2 commits into bitcoin:master from luke-jr:scriptex changing 2 files +35 −8

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 12: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

The last travis run for this pull request was 286 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
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

Contributors

Labels

Linked (view graph)

#3901 Split up EvalScript into EvalScriptState::Start and ::Step, so single-stepping can be done #10730 Move script flag to/from-string logic from tests to script/interpreter

Wrap EvalScript in a ScriptExecution class #10729

Conflicts