Carry through and return the C++ script verification errors?

[panicfarm]

in C++ interpreter.cpp, there are detailed errors of why a script verification fails, such as:

#grep -nr 'SCRIPT_ERR_' ./depend/bitcoin/src/script/interpreter.cpp | grep -o 'SCRIPT_ERR_[A-Z_]*' | sort | uniq | paste -sd, -

SCRIPT_ERR_BAD_OPCODE,SCRIPT_ERR_CHECKMULTISIGVERIFY,SCRIPT_ERR_CHECKSIGVERIFY,SCRIPT_ERR_CLEANSTACK,SCRIPT_ERR_DISABLED_OPCODE,SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS,SCRIPT_ERR_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM,SCRIPT_ERR_EQUALVERIFY,SCRIPT_ERR_ERROR_COUNT,SCRIPT_ERR_EVAL_FALSE,SCRIPT_ERR_INVALID_ALTSTACK_OPERATION,SCRIPT_ERR_INVALID_STACK_OPERATION,SCRIPT_ERR_MINIMALDATA,SCRIPT_ERR_MINIMALIF,SCRIPT_ERR_NEGATIVE_LOCKTIME,SCRIPT_ERR_NUMEQUALVERIFY,SCRIPT_ERR_OK,SCRIPT_ERR_OP_CODESEPARATOR,SCRIPT_ERR_OP_COUNT,SCRIPT_ERR_OP_RETURN,SCRIPT_ERR_PUBKEY_COUNT,SCRIPT_ERR_PUBKEYTYPE,SCRIPT_ERR_PUSH_SIZE,SCRIPT_ERR_SCRIPT_SIZE,SCRIPT_ERR_SIG_COUNT,SCRIPT_ERR_SIG_DER,SCRIPT_ERR_SIG_FINDANDDELETE,SCRIPT_ERR_SIG_HASHTYPE,SCRIPT_ERR_SIG_HIGH_S,SCRIPT_ERR_SIG_NULLDUMMY,SCRIPT_ERR_SIG_NULLFAIL,SCRIPT_ERR_SIG_PUSHONLY,SCRIPT_ERR_STACK_SIZE,SCRIPT_ERR_UNBALANCED_CONDITIONAL,SCRIPT_ERR_UNKNOWN_ERROR,SCRIPT_ERR_UNSATISFIED_LOCKTIME,SCRIPT_ERR_VERIFY,SCRIPT_ERR_WITNESS_MALLEATED,SCRIPT_ERR_WITNESS_MALLEATED_P,SCRIPT_ERR_WITNESS_PROGRAM_MISMATCH,SCRIPT_ERR_WITNESS_PROGRAM_WITNESS_EMPTY,SCRIPT_ERR_WITNESS_PROGRAM_WRONG_LENGTH,SCRIPT_ERR_WITNESS_PUBKEYTYPE,SCRIPT_ERR_WITNESS_UNEXPECTED

The only error that the Rust lib returns however is error::ERR_SCRIPT for all these cases. Is there a way to return (or at least to print) these detailed errors?

[tcharding]

Most definitely! Thanks for raising the issue.

[tcharding]

It looks to me like VerifyScript is called from verify_script with a null error pointer so we do not get the errors from interpreter.cpp back. From my reading of the code we pass in mutable error pointer accross the ffi layer to bitcoinconsensus_verify_script_with_amount and return it if it was set to one of the error variants.

[powchaser]

For now, in our fork of rust-bitcoinconsensus we pass in a mutable error pointer to VerifyScript, just for the sake of printing out the error if VerifyScript returns false. It would be great if there were rust error variants for all C++ error cases, and these errors are returned. Thank you.

[tcharding]

How do you pass a mutable error pointer to VerifyScript if Bitcoin Core passes nullptr? https://github.com/bitcoin/bitcoin/blob/498994b6f55d04a7940f832e7fbd17e5acdaff15/src/script/bitcoinconsensus.cpp#L113

[tcharding]

The call chain looks to me like:

• rust-bitcoinconessenus calls bitcoinconsensus_verify_script_with_amount
• bitcoinconsensus_verify_script_with_amount calls ::verify_script
• ::verify_script calls VerifyScript with the line linked above

Or am I missing something?

[powchaser]

yes, that's the call chain in our case as well. for now, to get the errors in the log, I just changed the code in VerifyScript in our local fork to this:

...
        // Regardless of the verification result, the tx did not error.
        set_error(err, bitcoinconsensus_ERR_OK);
        ScriptError* serror = nullptr;
        serror = new ScriptError;

        PrecomputedTransactionData txdata(tx);
        bool ret = VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen), &tx.vin[nIn].scriptWitness, flags, TransactionSignatureChecker(&tx, nIn, amount, txdata), serror);
        if (!ret) {
            std::cout << "VerifyScriptError: " << *serror << " txid: " << tx.GetHash().ToString() << " inp: " << nIn << "\n";
        }
        delete serror;
        serror = nullptr;
        return ret;
...

thank you for looking into this issue

[tcharding]

Oh my goodness I'm a wombat, we vendor that code so technically can change it. My bad, the whole time I was confused as to how this would be even possible. This is a totally different thing from what the issue description describes IMO

The issue implies (to me at least) that the problem is in how we call the C++ code from Rust when in fact what you are describing is an issue with how the C++ code calls itself. I'm not convinced we should be patching that. Can you convince me further? Doing this adds a non-trivial maintenance burden on us, we do this in secp256k1.

[powchaser]

we think it's important to have errors passed to the application layer. so, we found where the error info is lost (because nullptr is passed inside cpp) and is not passed on to the rust layer (even if there were all the error variants for the corresponding cpp errors).

[apoelstra]

I think we should move this issue upstream. It's one thing to patch code to completely remove functionality (as we do in libsecp) and another to actually modify internal logic. Even though I agree in principle with this change (if I understand it right).