RFC: Remove logging from script interpreter

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

I believe this is on-par feature-wise with what’s in master, but it’s incomplete as far as error codes go.

As suggested by @gmaxwell and @sipa, remove logging in favor of well-defined error codes for the script interpreter. This is necessary for the consensus lib, in order to remove the last bits of application state (along with #5162).

Every possible exit path of EvalScript() sets a ScriptError to indicate if/why it failed. I quickly enumerated the cases that had existing log messages, but the rest still need to be filled in. For every undefined failure, SCRIPT_UNKNOWN_ERROR is returned. It’s important to keep in mind that these values will be part of the lib’s stable external API, so it’s necessary to put some thought into how they’re defined.

Before continuing, I’d like to know that others are comfortable with this approach.

script: work towards creating sane error return codes for script validation

This is far from complete, but it's enough to get started. The eventual goal is
to remove application logging from this code, and let bitcoind log based on
the return code instead. For now, only the errors with existing error messages
have been enumerated, the rest are defined as UNKNOWN_ERROR.

b0cb5bda71

script: remove logging from the script interpreter

Handle logging based on the ScriptError that's returned.

69931df631

in src/script/interpreter.cpp: in 69931df631

85         if (vchPubKey.size() != 33)
86-            return error("Non-canonical public key: invalid length for compressed key");
87+            return set_error(serror, PUBKEY_BAD_COMPRESSED_LENGTH);
88     } else {
89-        return error("Non-canonical public key: neither compressed nor uncompressed");
90+        return set_error(serror, PUBKEY_UNKNOWN_LENGTH);

sipa commented at 8:44 am on November 1, 2014:

It’s not unknown length here, but unknown type.

theuni commented at 5:30 pm on November 1, 2014:

@sipa Ok. Admittedly I didn’t spend much effort on the names, as I assumed they’d be reworked to something more uniform by the time this gets merged. Are you OK with the concept?

in src/main.cpp: in 69931df631

1341@@ -1338,8 +1342,9 @@ void UpdateCoins(const CTransaction& tx, CValidationState &state, CCoinsViewCach
1342 
1343 bool CScriptCheck::operator()() const {
1344     const CScript &scriptSig = ptxTo->vin[nIn].scriptSig;
1345-    if (!VerifyScript(scriptSig, scriptPubKey, nFlags, CachingSignatureChecker(*ptxTo, nIn, cacheStore)))
1346-        return error("CScriptCheck() : %s:%d VerifySignature failed", ptxTo->GetHash().ToString(), nIn);
1347+    ScriptError serror;
1348+    if (!VerifyScript(scriptSig, scriptPubKey, nFlags, CachingSignatureChecker(*ptxTo, nIn, cacheStore), &serror))
1349+        return error("CScriptCheck() : %s:%d VerifySignature failed. Error: %s", ptxTo->GetHash().ToString(), nIn, ScriptErrorString(serror));

Diapolo commented at 4:46 pm on November 1, 2014:

Could you use func instead of a hard-coded function name?

in src/script/interpreter.cpp: in 69931df631

68@@ -52,68 +69,69 @@ static inline void popstack(vector<valtype>& stack)
69         throw runtime_error("popstack() : stack empty");
70     stack.pop_back();
71 }
72-
73-bool static IsCompressedOrUncompressedPubKey(const valtype &vchPubKey) {
74-    if (vchPubKey.size() < 33)
75-        return error("Non-canonical public key: too short");
76+bool static IsCompressedOrUncompressedPubKey(const valtype &vchPubKey, ScriptError* serror) {

sipa commented at 5:00 pm on November 1, 2014:

I don’t think it makes sense to report a ScriptError from this function, as the result never gets propagated up: an incorrectly encoded public key causes that public key to be skipped (which causes the OP_CHECKSIG operation to fail, not the script entirely). In the usual case, that means the script just fails due to non-true final stack element, and not due to error.

gmaxwell commented at 1:02 am on November 2, 2014: contributor

I like the approach (surprise).

sipa commented at 7:38 am on November 2, 2014: member

Yes, I’d even ACK it, if not for the IsCompressedPubKey nit :)

theuni commented at 5:57 pm on November 3, 2014: member

Mmm, but in the case of an OP_CHECKSIGVERIFY with a bad pubkey the script would fail, no? In that case, wouldn’t we want that error set?

That raises another question: If EvalScript returns true but contains a false/empty stack, should something like SCRIPT_EVAL_NOT_TRUE be returned from EvalScript() as the error? Or should that be considered to be SCRIPT_NO_ERROR, and VerifyScript() sets the ultimate result?

sipa commented at 6:06 pm on November 3, 2014: member

Yes, for OP_CHECKSIGVERIFY you’re right. But generality…

The point is that, as currently implemented, a bad public key is not something that results in (direct) script failure, so you can’t consider it to be an error condition that causes script failure. I don’t care enough about the actual error messages produced because of this that I don’t want to lose them. They’re pretty verbose anyway.

Regarding no-error-failure: it’s pretty important that for actual consensus behaviour you do not distinguish between error-failure and normal-failure. The distinction between the ScriptError object and the return value is perfect for that. if it helps for better messages: sure, make evaluation failure an error condition too.

theuni commented at 6:44 pm on November 3, 2014: member

I really don’t care about error messages, I’m just considering the inevitability (since these error codes will be an exposed public api) that people will erroneously use the ScriptError to determine pass/fail, since it looks like a typical C api to do so. I’d prefer to always have it set to something other than NO_ERROR on failure to help protect from that. Admittedly I’m thinking much more in terms of lib usage than Bitcoind’s.

As for the condition that causes failure, how about something like this at the end of EvalScript:

 0diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
 1index 8802806..a913f1c 100644
 2--- a/src/script/interpreter.cpp
 3+++ b/src/script/interpreter.cpp
 4@@ -882,6 +882,13 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
 5     if (!vfExec.empty())
 6         return set_error(serror);
 7
 8+    if (stack.empty() || CastToBool(stack.back()) == false)
 9+    {
10+        // If we get here with an error set, that error is ignored because the
11+        // ultimate reason for failure is a non-true top stack element.
12+        set_error(SCRIPT_STACK_TOP_NOT_TRUE);
13+        return true;
14+    }
15     return set_success(serror);
16 }

Fwiw, I don’t disagree with you at all, and I’m not trying to be pedantic about this case, I’d just like to avoid special-casing so that all error-handling is done uniformly.

sipa commented at 7:00 pm on November 3, 2014: member

LGTM

theuni commented at 3:35 am on November 5, 2014: member

cleaned up and removed the RFC tag in #5212. Ended up removing logging and handling IsCompressedOrUncompressedPubKey() as you suggested.

theuni closed this on Nov 5, 2014

MarcoFalke locked this on Sep 8, 2021

RFC: Remove logging from script interpreter #5188