refactor: Extract ParseOpCode from ParseScript #19851

Seems more natural to have mapOpNames "hidden" in ParseOpCode than in ParseScript.

A second lookup in mapOpNames is also removed.

When touching ParseScript: what also about removing the uses of boost::algorithm::replace_first and boost::algorithm::split in it?

@practicalswift that's fine but I guess not here?

ACK 7b3a0150a5e7d0cd8f31ff7bbd5a421d9020cfd1 📃

When touching ParseScript: what also about removing the uses of boost::algorithm::replace_first and boost::algorithm::split in it? @practicalswift: Could you elaborate on why boost::algorithm::split should be avoided? I can totally see that boost::algorithm::replace_first is overkill here and can be easily done via STL, for splitting though there doesn't seem to be a non-ugly solution. (E.g. https://www.fluentcpp.com/2017/04/21/how-to-split-a-string-in-c/ also recommends boost for this task).

@theStack The long term plan is to remove all usages of Boost, so I thought it would be nice to take one small step in that direction when creating ParseOpCode (and thus touching this code). I agree that removing the trivial boost::algorithm::replace_first before the less trivial boost::algorithm::split makes sense. Also, only replace_first is used in the new ParseOpCode :)

The unnecessary Boost usage in ParseOpCode:

https://github.com/bitcoin/bitcoin/blob/7b3a0150a5e7d0cd8f31ff7bbd5a421d9020cfd1/src/core_read.cpp#L42-L43

Ok, let's do it here.

@promag: Feel free to add my commit: https://github.com/theStack/bitcoin/commit/5dfc039e89107a261c7f8eda70bae1fe63c581c7 (branch https://github.com/theStack/bitcoin/tree/20200903-refactor-get-rid-of-boost-replace_first):

diff --git a/src/core_read.cpp b/src/core_read.cpp
index 1829f18..155f443 100644
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -15,7 +15,6 @@
 #include <version.h>

 #include <boost/algorithm/string/classification.hpp>
-#include <boost/algorithm/string/replace.hpp>
 #include <boost/algorithm/string/split.hpp>

 #include <algorithm>
@@ -40,8 +39,8 @@ opcodetype ParseOpCode(const std::string& s)
                 continue;
             mapOpNames[strName] = static_cast<opcodetype>(op);
             // Convenience: OP_ADD and just ADD are both recognized:
-            boost::algorithm::replace_first(strName, "OP_", "");
-            mapOpNames[strName] = static_cast<opcodetype>(op);
+            if (strName.rfind("OP_", 0) == 0)
+                mapOpNames[strName.substr(3)] = static_cast<opcodetype>(op);
         }
     }

Another solution would be to use if (strName.substr(0,3) == "OP_") ....

Code review ACK https://github.com/bitcoin/bitcoin/pull/19851/commits/7b3a0150a5e7d0cd8f31ff7bbd5a421d9020cfd1

Code review ACK 7b3a0150a5e7d0cd8f31ff7bbd5a421d9020cfd1

@theStack I don't think your code is equivalent. In the orignal case, both OP_X and X are accepted. In the new case, only OP_X is. (concept ACK on getting rid of boost::algorithm::replace_first in favor of a direct implementation though)

@theStack I don't think your code is equivalent. In the orignal case, both OP_X and X are accepted. In the new case, only OP_X is.

Hm, I guess the .rfind() call is confusing here, leading you to think that the condition is true if the string doesn't start with "OP_"? Note that the condition foo.rfind(bar, 0) == 0 simply checks if foo starts with bar, e.g. it would be equal to a (fictional) foo.startswith(bar). (See https://stackoverflow.com/a/40441240)

(concept ACK on getting rid of boost::algorithm::replace_first in favor of a direct implementation though)

Will come up with a follow-up PR then.

Decided to keep boost in this PR, don't want to drag this because of that unrelated change. Applied @laanwj suggestion.

I'd suggest to adapt the expected result string for the util test case "Create a new transaction with an invalid output script":

diff --git a/test/util/data/bitcoin-util-test.json b/test/util/data/bitcoin-util-test.json
index 99cd4ab..0a9846b 100644
--- a/test/util/data/bitcoin-util-test.json
+++ b/test/util/data/bitcoin-util-test.json
@@ -221,7 +221,7 @@
   { "exec": "./bitcoin-tx",
     "args": ["-create", "outscript=0:123badscript"],
     "return_code": 1,
-    "error_txt": "error: script parse error",
+    "error_txt": "error: script parse error: unknown opcode",
     "description": "Create a new transaction with an invalid output script"
   },
   { "exec": "./bitcoin-tx",

(The test still passes without this patch though, as matching a substring is sufficient).

re-ACK c92387232f750397da7d131f262c150a608408c2

The long term plan is to remove all usages of Boost, @practicalswift No... rather C++ standards are preferred over Boost when appropriate, and Boost isn't allowed in libconsensus. Boost still provides many useful things the standards don't, and I doubt we will ever remove its use entirely.

ACK c92387232f750397da7d131f262c150a608408c2