Merge #16902: O(1) OP_IF/NOTIF/ELSE/ENDIF script implementation

e6e622e5a0e22c2ac1b50b96af818e412d67ac54 Implement O(1) OP_IF/NOTIF/ELSE/ENDIF logic (Pieter Wuille)
d0e8f4d5d8ddaccb37f98b7989fb944081e41ab8 [refactor] interpreter: define interface for vfExec (Anthony Towns)
89fb241c54fc85befacfa3703d8e21bf3b8a76eb Benchmark script verification with 100 nested IFs (Pieter Wuille)

Pull request description:

  While investigating what mechanisms are possible to maximize the per-opcode verification cost of scripts, I noticed that the logic for determining whether a particular opcode is to be executed is O(n) in the nesting depth. This issue was also pointed out by Sergio Demian Lerner in https://bitslog.wordpress.com/2017/04/17/new-quadratic-delays-in-bitcoin-scripts/, and this PR implements a variant of the O(1) algorithm suggested there.

  This is not a problem currently, because even with a nesting depth of 100 (the maximum possible right now due to the 201 ops limit), the slowdown caused by this on my machine is around 70 ns per opcode (or 0.25 s per block) at worst, far lower than what is possible with other opcodes.

  This PR mostly serves as a proof of concept that it's possible to avoid it, which may be relevant in discussions around increasing the opcode limits in future script versions. Without it, the execution time of scripts can grow quadratically with the nesting depth, which very quickly becomes unreasonable.

  This improves upon #14245 by completely removing the `vfExec` vector.

ACKs for top commit:
  jnewbery:
    Code review ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54
  MarcoFalke:
    ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54 🐴
  fjahr:
    ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54
  ajtowns:
    ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54
  laanwj:
    concept and code review ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54
  jonatack:
    ACK e6e622e5a0e22c2ac1b50b96af818e412d67ac54 code review, build, benches, fuzzing

Tree-SHA512: 1dcfac3411ff04773de461959298a177f951cb5f706caa2734073bcec62224d7cd103767cfeef85cd129813e70c14c74fa8f1e38e4da70ec38a0f615aab1f7f7

src/Makefile.bench.include

@@ -47,7 +47,8 @@ bench_bench_dash_SOURCES = \
   bench/lockedpool.cpp \
   bench/poly1305.cpp \
   bench/prevector.cpp \
-  bench/string_cast.cpp
+  bench/string_cast.cpp \
+  bench/verify_script.cpp
 
 nodist_bench_bench_dash_SOURCES = $(GENERATED_BENCH_FILES)
 

src/bench/verify_script.cpp

@@ -0,0 +1,33 @@
+// Copyright (c) 2016-2019 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <bench/bench.h>
+#include <script/script.h>
+#include <script/standard.h>
+
+#include <array>
+
+
+static void VerifyNestedIfScript(benchmark::Bench& bench) {
+    std::vector<std::vector<unsigned char>> stack;
+    CScript script;
+    for (int i = 0; i < 100; ++i) {
+        script << OP_1 << OP_IF;
+    }
+    for (int i = 0; i < 1000; ++i) {
+        script << OP_1;
+    }
+    for (int i = 0; i < 100; ++i) {
+        script << OP_ENDIF;
+    }
+    bench.run([&] {
+        auto stack_copy = stack;
+        ScriptError error;
+        bool ret = EvalScript(stack_copy, script, 0, BaseSignatureChecker(), SigVersion::BASE, &error);
+        assert(ret);
+    });
+}
+
+
+BENCHMARK(VerifyNestedIfScript);

src/script/interpreter.cpp

@@ -274,6 +274,70 @@ int FindAndDelete(CScript& script, const CScript& b)
     return nFound;
 }
 
+namespace {
+/** A data type to abstract out the condition stack during script execution.
+ *
+ * Conceptually it acts like a vector of booleans, one for each level of nested
+ * IF/THEN/ELSE, indicating whether we're in the active or inactive branch of
+ * each.
+ *
+ * The elements on the stack cannot be observed individually; we only need to
+ * expose whether the stack is empty and whether or not any false values are
+ * present at all. To implement OP_ELSE, a toggle_top modifier is added, which
+ * flips the last value without returning it.
+ *
+ * This uses an optimized implementation that does not materialize the
+ * actual stack. Instead, it just stores the size of the would-be stack,
+ * and the position of the first false value in it.
+ */
+class ConditionStack {
+private:
+    //! A constant for m_first_false_pos to indicate there are no falses.
+    static constexpr uint32_t NO_FALSE = std::numeric_limits<uint32_t>::max();
+
+    //! The size of the implied stack.
+    uint32_t m_stack_size = 0;
+    //! The position of the first false value on the implied stack, or NO_FALSE if all true.
+    uint32_t m_first_false_pos = NO_FALSE;
+
+public:
+    bool empty() { return m_stack_size == 0; }
+    bool all_true() { return m_first_false_pos == NO_FALSE; }
+    void push_back(bool f)
+    {
+        if (m_first_false_pos == NO_FALSE && !f) {
+            // The stack consists of all true values, and a false is added.
+            // The first false value will appear at the current size.
+            m_first_false_pos = m_stack_size;
+        }
+        ++m_stack_size;
+    }
+    void pop_back()
+    {
+        assert(m_stack_size > 0);
+        --m_stack_size;
+        if (m_first_false_pos == m_stack_size) {
+            // When popping off the first false value, everything becomes true.
+            m_first_false_pos = NO_FALSE;
+        }
+    }
+    void toggle_top()
+    {
+        assert(m_stack_size > 0);
+        if (m_first_false_pos == NO_FALSE) {
+            // The current stack is all true values; the first false will be the top.
+            m_first_false_pos = m_stack_size - 1;
+        } else if (m_first_false_pos == m_stack_size - 1) {
+            // The top is the first false value; toggling it will make everything true.
+            m_first_false_pos = NO_FALSE;
+        } else {
+            // There is a false value, but not on top. No action is needed as toggling
+            // anything but the first false value is unobservable.
+        }
+    }
+};
+}
+
 /** Helper for OP_CHECKSIG and OP_CHECKSIGVERIFY
  *
  * A return value of false means the script fails entirely. When true is returned, the
@@ -303,6 +367,7 @@ static bool EvalChecksig(const valtype& vchSig, const valtype& vchPubKey, CScrip
     return true;
 }
 
+
 bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* serror)
 {
     static const CScriptNum bnZero(0);
@@ -318,7 +383,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
     CScript::const_iterator pbegincodehash = script.begin();
     opcodetype opcode;
     valtype vchPushValue;
-    std::vector<bool> vfExec;
+    ConditionStack vfExec;
     std::vector<valtype> altstack;
     set_error(serror, SCRIPT_ERR_UNKNOWN_ERROR);
     if (script.size() > MAX_SCRIPT_SIZE)
@@ -330,7 +395,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
     {
         while (pc < pend)
         {
-            bool fExec = !count(vfExec.begin(), vfExec.end(), false);
+            bool fExec = vfExec.all_true();
 
             //
             // Read instruction
@@ -521,7 +586,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
                 {
                     if (vfExec.empty())
                         return set_error(serror, SCRIPT_ERR_UNBALANCED_CONDITIONAL);
-                    vfExec.back() = !vfExec.back();
+                    vfExec.toggle_top();
                 }
                 break;