consensus lib work: add status codes to the script interpreter and remove logging

theuni commented at 1:14 am on November 5, 2014: member

Cleaned up version of #5188

Attempt to codify the possible error statuses associated with script validation. The second commit (move-only) moves the other flags into the newly created script/types.h as part of the upcoming external interface.

Logging has also been removed in order to drop the dependency on util.h. It can be re-added to bitcoind as-needed. This makes script verification finally free of application state and boost!

sipa commented at 6:38 am on November 5, 2014: member

@theuni I was actually thinking not to expose the script verification flags externally, as they are implementation and policy specific, but instead have the interface define its own flags, which get translated into internal ones.

theuni commented at 6:42 am on November 5, 2014: member

I’ll defer to you on that. I can nuke the top commit and save that for a separate discussion.

in src/script/types.h: in 4e6dd2fb2c outdated

75+    SCRIPT_ERR_BAD_OPCODE,
76+    SCRIPT_ERR_DISABLED_OPCODE,
77+    SCRIPT_ERR_INVALID_STACK_OPERATION,
78+    SCRIPT_ERR_INVALID_ALTSTACK_OPERATION,
79+    SCRIPT_ERR_UNBALANCED_CONDITIONAL,
80+    SCRIPT_ERR_SIG_HASHTYPE,

sipa commented at 6:53 am on November 5, 2014:

These two are also under BIP62 (they’re also used for standardness checks outside of BIP62, but that shouldn’t matter for a consensus lib).

gmaxwell commented at 10:44 am on November 5, 2014: contributor

With respect to the concern that callers might use the error code to decide pass/fail instead of the actual return. Why not set the error value to the unknown error at entry, so long as an error pointer is provided at all?

theuni force-pushed on Nov 6, 2014

theuni commented at 7:15 am on November 6, 2014: member

Updated for @sipa’s and @gmaxwell’s comments.

in src/script/interpreter.cpp: in f9787fec41 outdated

72@@ -56,15 +73,15 @@ static inline void popstack(vector<valtype>& stack)
73 
74 bool static IsCompressedOrUncompressedPubKey(const valtype &vchPubKey) {
75     if (vchPubKey.size() < 33)
76-        return error("Non-canonical public key: too short");
77+        return false;

laanwj commented at 9:37 am on November 7, 2014:

Nit: can you add the error messages that you remove here as comment? ie

0- if (vchPubKey.size() < 33)
1+ if (vchPubKey.size() < 33) // Non-canonical public key: too short
2- return error("Non-canonical public key: too short");
3+ return false;

We do lose some troubleshooting information here, as the error mechanism provides no way to signal warnings/details of script interpretation. But I don’t see an alternative either. Maybe in the future it would be useful to add a way to get a detailed trace of execution.

theuni commented at 4:50 pm on November 7, 2014:

I started to add a bitmask for more detailed info in the case of ERR_EVAL_VALSE or ERR_VERIFY_*, but that ended up looking a little over-engineered and error-prone. If you think it’s worth it, i can re-add that.

laanwj commented at 6:14 pm on November 7, 2014:

That’s not necessary for this pull

laanwj commented at 8:50 am on November 11, 2014:

But please do add the comments as suggested, so that the cases are documented and if later on we have a way to return details execution status, we can return it.

laanwj commented at 9:54 am on November 7, 2014: member

Looks good to me, utACK (will test)

in src/script/script_error.cpp: in f9787fec41 outdated

0@@ -0,0 +1,67 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2014 The Bitcoin developers
3+// Distributed under the MIT software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+#include "script_error.h"
7+
8+const char* ScriptErrorString(const ScriptError serror)

laanwj commented at 9:57 am on November 7, 2014:

Currently this function is unused! Shouldn’t we call this function where we use EvalScript/VerifyScript, and log the message?

theuni commented at 4:46 pm on November 7, 2014:

@laanwj I didn’t want to hook it up in each place, since it could be potentially much noisier than the old logging. To be close to on-par with current behavior, we could check for something like SCRIPT_ERR_SIG_DER || SCRIPT_ERR_SIG_HASHTYPE || SCRIPT_ERR_SIG_HIGH_S. Those are pretty arbitrary though.

laanwj commented at 11:51 am on November 13, 2014:

Yes, it’s pretty arbitrary. But it’s also strange to have a function that is called from nowhere, not even from the unit tests. But I’m fine with doing that in a later pull, and regarding this as preparation.

theuni commented at 7:31 pm on November 13, 2014:

@laanwj Yes, I’d prefer to address the logging in a later pull, as I assumed that would lead to some bikeshedding. Though I agree that it wouldn’t make sense to push this without using it anywhere, so I’ve updated the tests to check the values.

theuni force-pushed on Nov 11, 2014

theuni commented at 11:42 pm on November 11, 2014: member

@laanwj Sorry, I missed your comment (about comments) the first time around. re-added those and rebased on master.

in src/script/interpreter.cpp: in 4eefa373ae outdated

895                 }
896                 break;
897 
898                 default:
899-                    return false;
900+                    return set_error(serror, SCRIPT_ERR_UNKNOWN_ERROR);

TheBlueMatt commented at 2:34 am on November 13, 2014:

Isnt this unknown opcode?

theuni commented at 2:48 am on November 13, 2014:

yes, thanks for catching that!

TheBlueMatt commented at 3:00 am on November 13, 2014: member

ACK commithash 4eefa373ae4866a1ad25cea3615270f58693e6a7 (+/- the incorrect error message on unknown opcode): http://bitcoin.ninja/TheBlueMatt-5212.txt

theuni force-pushed on Nov 13, 2014

in src/script/interpreter.cpp: in ddadf2d1ec outdated

87+        }
88     } else if (vchPubKey[0] == 0x02 || vchPubKey[0] == 0x03) {
89-        if (vchPubKey.size() != 33)
90-            return error("Non-canonical public key: invalid length for compressed key");
91+        if (vchPubKey.size() != 33) {
92+        //  Non-canonical public key: invalid length for compressed key

laanwj commented at 11:17 am on November 14, 2014:

Sorry to nit, but indentation is wrong here (and in some other places where comments are added)

sipa commented at 11:20 am on November 14, 2014: member

@theuni to get back to an earlier comment I made: if #5247 would be merged, non-standard public key encoding do result in script failure immediately, so IsCompressedOrUncompressedPublicKey could set a status code etc, like you did earlier.

laanwj commented at 11:24 am on November 14, 2014: member

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK ddadf2d1ecc0d646ae83b06f275c15a5a7704573 after squash
 4-----BEGIN PGP SIGNATURE-----
 5Version: GnuPG v1
 6
 7iQEcBAEBCgAGBQJUZeZIAAoJEHSBCwEjRsmmDvEH/1Ugi0b3iy70QKEw3OBs+yc1
 8uOvlkBuNVX+UM6alY90jJp5WZRNLnoOKv/1CZW8kD6jPZsb4fHJG9jE3xhvNY5sY
 96M0T4LTZzhqFHqC2ob1sXpHkztfQ/+klmJh1rrPCgDb2KtrIcHJT2ZNHl59WZQ1S
10tCHAQz23COk7O9uACRhqKB3oCmmxxnI/IzmEOncaMHR2at0DL9/Z90FOWD65OoLM
11bl4SIx3MEBXkgvgAB/41eQ1oWTVuxTl0FexhgUsNQWgHzpNxFOoHSraDBfED4DVy
12ZOOVmyS6o0GaCbVy/xlrdbUaM5Gm/MsYPZV65BZesIGa2lJLgTM/Jha4uD3ECyw=
13=+m+q
14-----END PGP SIGNATURE-----

theuni commented at 9:14 pm on November 14, 2014: member

@sipa Ok, thanks. If/when that happens, I’ll follow-up with the more detailed values.

theuni force-pushed on Nov 14, 2014

theuni commented at 9:22 pm on November 14, 2014: member

Squashed. This is code-identical to the last push except for the comments white-space nit fix.

script: create sane error return codes for script validation and remove logging

Attempt to codify the possible error statuses associated with script
validation. script/types.h has been created with the expectation that it will
be part of the public lib interface. The other flag enums will be moved here in
a future commit.

Logging has also been removed in order to drop the dependency on core.h. It can
be re-added to bitcoind as-needed. This makes script verification finally free
of application state and boost!

ab9edbd6b6

script: check ScriptError values in script tests 219a1470c4

sipa commented at 3:40 pm on November 17, 2014: member

utACK

laanwj merged this on Nov 17, 2014

laanwj closed this on Nov 17, 2014

laanwj referenced this in commit 8adf457047 on Nov 17, 2014

MarcoFalke locked this on Sep 8, 2021

consensus lib work: add status codes to the script interpreter and remove logging #5212