Merge bitcoin/bitcoin#22232: refactor: Pass interpreter flags as uint32_t instead of signed int

fa621ededd refactor: Pass script verify flags as uint32_t (MarcoFalke)

Pull request description:

  The flags are cast to unsigned in the interpreter anyway, so avoid the confusion (and fuzz crashes) by just passing them as unsigned from the beginning.

  Also, the flags are often inverted bit-wise with the `~` operator, which also works on signed integers, but might cause confusion as the sign bit is flipped.

  Fixes #22233

ACKs for top commit:
  theStack:
    Concept and code review ACK fa621ededd
  kristapsk:
    ACK fa621ededd
  jonatack:
    ACK fa621ededd

Tree-SHA512: ea0720f32f823fa7f075309978672aa39773c6019d12b6c1c9d611fc1983a76115b7fe2a28d50814673bb6415c311ccc05b99d6e871575fb6900faf75ed17769
This commit is contained in:
MarcoFalke 2021-07-20 15:35:31 +02:00
commit 9faa4b68db
No known key found for this signature in database
GPG key ID: CE2B75697E69A548
8 changed files with 16 additions and 17 deletions

View file

@ -21,7 +21,7 @@ static void VerifyScriptBench(benchmark::Bench& bench)
const ECCVerifyHandle verify_handle;
ECC_Start();
const int flags = SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_P2SH;
const uint32_t flags{SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_P2SH};
const int witnessversion = 0;
// Key pair.

View file

@ -144,7 +144,7 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& in
return nSigOps;
}
int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& inputs, int flags)
int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& inputs, uint32_t flags)
{
int64_t nSigOps = GetLegacySigOpCount(tx) * WITNESS_SCALE_FACTOR;

View file

@ -49,10 +49,10 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& ma
* Compute total signature operation cost of a transaction.
* @param[in] tx Transaction for which we are computing the cost
* @param[in] inputs Map of previous transactions that have outputs we're spending
* @param[out] flags Script verification flags
* @param[in] flags Script verification flags
* @return Total signature operation cost of tx
*/
int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& inputs, int flags);
int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& inputs, uint32_t flags);
/**
* Check if transaction is final and can be included in a block with the

View file

@ -39,8 +39,7 @@ enum
* All flags are intended to be soft forks: the set of acceptable scripts under
* flags (A | B) is a subset of the acceptable scripts under flag (A).
*/
enum
{
enum : uint32_t {
SCRIPT_VERIFY_NONE = 0,
// Evaluate P2SH subscripts (BIP16).

View file

@ -258,7 +258,7 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view)
// consensus/tx_verify.cpp:130: unsigned int GetP2SHSigOpCount(const CTransaction &, const CCoinsViewCache &): Assertion `!coin.IsSpent()' failed.
return;
}
const int flags = fuzzed_data_provider.ConsumeIntegral<int>();
const auto flags{fuzzed_data_provider.ConsumeIntegral<uint32_t>()};
if (!transaction.vin.empty() && (flags & SCRIPT_VERIFY_WITNESS) != 0 && (flags & SCRIPT_VERIFY_P2SH) == 0) {
// Avoid:
// script/interpreter.cpp:1705: size_t CountWitnessSigOps(const CScript &, const CScript &, const CScriptWitness *, unsigned int): Assertion `(flags & SCRIPT_VERIFY_P2SH) != 0' failed.

View file

@ -123,7 +123,7 @@ static ScriptError_t ParseScriptError(const std::string& name)
BOOST_FIXTURE_TEST_SUITE(script_tests, BasicTestingSetup)
void DoTest(const CScript& scriptPubKey, const CScript& scriptSig, const CScriptWitness& scriptWitness, int flags, const std::string& message, int scriptError, CAmount nValue = 0)
void DoTest(const CScript& scriptPubKey, const CScript& scriptSig, const CScriptWitness& scriptWitness, uint32_t flags, const std::string& message, int scriptError, CAmount nValue = 0)
{
bool expect = (scriptError == SCRIPT_ERR_OK);
if (flags & SCRIPT_VERIFY_CLEANSTACK) {
@ -139,8 +139,8 @@ void DoTest(const CScript& scriptPubKey, const CScript& scriptSig, const CScript
// Verify that removing flags from a passing test or adding flags to a failing test does not change the result.
for (int i = 0; i < 16; ++i) {
int extra_flags = InsecureRandBits(16);
int combined_flags = expect ? (flags & ~extra_flags) : (flags | extra_flags);
uint32_t extra_flags(InsecureRandBits(16));
uint32_t combined_flags{expect ? (flags & ~extra_flags) : (flags | extra_flags)};
// Weed out some invalid flag combinations.
if (combined_flags & SCRIPT_VERIFY_CLEANSTACK && ~combined_flags & (SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS)) continue;
if (combined_flags & SCRIPT_VERIFY_WITNESS && ~combined_flags & SCRIPT_VERIFY_P2SH) continue;
@ -150,7 +150,7 @@ void DoTest(const CScript& scriptPubKey, const CScript& scriptSig, const CScript
#if defined(HAVE_CONSENSUS_LIB)
CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
stream << tx2;
int libconsensus_flags = flags & bitcoinconsensus_SCRIPT_FLAGS_VERIFY_ALL;
uint32_t libconsensus_flags{flags & bitcoinconsensus_SCRIPT_FLAGS_VERIFY_ALL};
if (libconsensus_flags == flags) {
int expectedSuccessCode = expect ? 1 : 0;
if (flags & bitcoinconsensus_SCRIPT_FLAGS_VERIFY_WITNESS) {
@ -258,7 +258,7 @@ private:
bool havePush;
std::vector<unsigned char> push;
std::string comment;
int flags;
uint32_t flags;
int scriptError;
CAmount nValue;
@ -278,7 +278,7 @@ private:
}
public:
TestBuilder(const CScript& script_, const std::string& comment_, int flags_, bool P2SH = false, WitnessMode wm = WitnessMode::NONE, int witnessversion = 0, CAmount nValue_ = 0) : script(script_), havePush(false), comment(comment_), flags(flags_), scriptError(SCRIPT_ERR_OK), nValue(nValue_)
TestBuilder(const CScript& script_, const std::string& comment_, uint32_t flags_, bool P2SH = false, WitnessMode wm = WitnessMode::NONE, int witnessversion = 0, CAmount nValue_ = 0) : script(script_), havePush(false), comment(comment_), flags(flags_), scriptError(SCRIPT_ERR_OK), nValue(nValue_)
{
CScript scriptPubKey = script;
if (wm == WitnessMode::PKH) {
@ -1677,7 +1677,7 @@ static void AssetTest(const UniValue& test)
const std::vector<CTxOut> prevouts = TxOutsFromJSON(test["prevouts"]);
BOOST_CHECK(prevouts.size() == mtx.vin.size());
size_t idx = test["index"].get_int64();
unsigned int test_flags = ParseScriptFlags(test["flags"].get_str());
uint32_t test_flags{ParseScriptFlags(test["flags"].get_str())};
bool fin = test.exists("final") && test["final"].get_bool();
if (test.exists("success")) {

View file

@ -67,7 +67,7 @@ BOOST_AUTO_TEST_CASE(GetSigOpCount)
* Verifies script execution of the zeroth scriptPubKey of tx output and
* zeroth scriptSig and witness of tx input.
*/
static ScriptError VerifyWithFlag(const CTransaction& output, const CMutableTransaction& input, int flags)
static ScriptError VerifyWithFlag(const CTransaction& output, const CMutableTransaction& input, uint32_t flags)
{
ScriptError error;
CTransaction inputi(input);
@ -121,7 +121,7 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost)
key.MakeNewKey(true);
CPubKey pubkey = key.GetPubKey();
// Default flags
int flags = SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_P2SH;
const uint32_t flags{SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_P2SH};
// Multisig script (legacy counting)
{

View file

@ -446,7 +446,7 @@ static void CreateCreditAndSpend(const FillableSigningProvider& keystore, const
assert(input.vin[0].scriptWitness.stack == inputm.vin[0].scriptWitness.stack);
}
static void CheckWithFlag(const CTransactionRef& output, const CMutableTransaction& input, int flags, bool success)
static void CheckWithFlag(const CTransactionRef& output, const CMutableTransaction& input, uint32_t flags, bool success)
{
ScriptError error;
CTransaction inputi(input);