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

  2. luke-jr cross-referenced this on Jul 3, 2017 from issue Move script flag to/from-string logic from tests to script/interpreter by luke-jr
  3. fanquake added the label Validation on Jul 3, 2017
  4. 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?

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

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

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

  8. luke-jr force-pushed on Sep 2, 2017
  9. luke-jr commented at 5:48 AM on September 2, 2017: member

    Rebased

  10. jtimon commented at 12:58 AM on September 6, 2017: contributor

    Concept ACK, will review

  11. MarcoFalke added the label Needs rebase on Jun 6, 2018
  12. script/interpreter: Wrap EvalScript in a ScriptExecution class 33e2c48ec4
  13. script/interpreter: Move useful evaluation variables to ScriptExecution class 1887155ef2
  14. luke-jr commented at 9:25 AM on November 3, 2018: member

    Rebased

  15. luke-jr force-pushed on Nov 3, 2018
  16. DrahtBot removed the label Needs rebase on Nov 3, 2018
  17. 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 :-)

  18. 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 :-)

  19. 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?

  20. DrahtBot commented at 3:49 AM on November 9, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  21. jtimon commented at 9:13 PM on April 3, 2019: contributor

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

  22. DrahtBot closed this on Aug 16, 2019
  23. DrahtBot commented at 1:49 PM on August 16, 2019: contributor

    <!--5d09a71f8925f3f132321140b44b946d-->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.

  24. DrahtBot reopened this on Aug 16, 2019
  25. DrahtBot cross-referenced this on Feb 11, 2020 from issue Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa
  26. DrahtBot cross-referenced this on Feb 11, 2020 from issue O(1) OP_IF/NOTIF/ELSE/ENDIF script implementation by sipa
  27. DrahtBot cross-referenced this on Feb 12, 2020 from issue Make script interpreter independent from storage type CScript by sipa
  28. luke-jr commented at 8:59 PM on February 26, 2020: member

    Abandoning this until there's some indication anyone cares

  29. luke-jr closed this on Feb 26, 2020
  30. bitcoin locked this on Feb 15, 2022
Contributors
luke-jrpromagjtimonpracticalswiftDrahtBot
Labels
Validation
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
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC