In commit "refactor: rename GetSigOpCount to GetLegacySigOpCount" (06cb252c8285746737e3385886da2efd1ab41f5a)
I like getting rid of this overload, but I found the code here very confusing before this change, and I think renaming this to GetLegacySigOpCount makes the confusion worse because this method is not acting like a legacy function when it's called with fAccurate = true. At first I though you might have been using legacy here to mean "pre-segwit" which is different than the way the GetLegacySigOpCount(CTransaction) uses legacy to mean "pre-p2sh". But after noticing this is also called by the segwit WitnessSigOps function, I don't see how the term "legacy" even applies here at all.
I think I would suggest renaming this to CountSigOps to be consistent with the CountWitnessSigOps function, and giving it better documentation. I also think the other overload should be a standalone function not a CScript method because it is not really using the CScript.
I think a change like the one below could be a lot clearer:
<details><summary>diff</summary>
<p>
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -113,10 +113,10 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx)
{
unsigned int nSigOps = 0;
for (const auto& txin : tx.vin) {
- nSigOps += txin.scriptSig.GetLegacySigOpCount(/*fAccurate=*/false);
+ nSigOps += txin.scriptSig.CountSigOps(/*fAccurate=*/false);
}
for (const auto& txout : tx.vout) {
- nSigOps += txout.scriptPubKey.GetLegacySigOpCount(/*fAccurate=*/false);
+ nSigOps += txout.scriptPubKey.CountSigOps(/*fAccurate=*/false);
}
return nSigOps;
}
@@ -133,7 +133,7 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& in
assert(!coin.IsSpent());
const CTxOut &prevout = coin.out;
if (prevout.scriptPubKey.IsPayToScriptHash())
- nSigOps += prevout.scriptPubKey.GetSigOpCount(tx.vin[i].scriptSig);
+ nSigOps += CountP2SHSigOps(tx.vin[i].scriptSig);
}
return nSigOps;
}
--- a/src/policy/policy.cpp
+++ b/src/policy/policy.cpp
@@ -181,8 +181,11 @@ static bool CheckSigopsBIP54(const CTransaction& tx, const CCoinsViewCache& inpu
// `fAccurate` means correctly accounting sigops for CHECKMULTISIGs(VERIFY) with 16 pubkeys
// or fewer. This method of accounting was introduced by BIP16, and BIP54 reuses it.
// The GetSigOpCount call on the previous scriptPubKey counts both bare and P2SH sigops.
- sigops += txin.scriptSig.GetLegacySigOpCount(/*fAccurate=*/true);
- sigops += prev_txo.scriptPubKey.GetSigOpCount(txin.scriptSig);
+ sigops += txin.scriptSig.CountSigOps(/*fAccurate=*/true);
+ sigops += prev_txo.scriptPubKey.CountSigOps(/*fAccurate=*/true);
+ if (prev_txo.scriptPubKey.IsPayToScriptHash()) {
+ sigops += CountP2SHSigOps(txin.scriptSig);
+ }
if (sigops > MAX_TX_LEGACY_SIGOPS) {
return false;
@@ -239,7 +242,7 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
if (stack.empty())
return false;
CScript subscript(stack.back().begin(), stack.back().end());
- if (subscript.GetLegacySigOpCount(/*fAccurate=*/true) > MAX_P2SH_SIGOPS) {
+ if (subscript.CountSigOps(/*fAccurate=*/true) > MAX_P2SH_SIGOPS) {
return false;
}
}
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -2084,7 +2084,7 @@ size_t static WitnessSigOps(int witversion, const std::vector<unsigned char>& wi
if (witprogram.size() == WITNESS_V0_SCRIPTHASH_SIZE && witness.stack.size() > 0) {
CScript subscript(witness.stack.back().begin(), witness.stack.back().end());
- return subscript.GetLegacySigOpCount(/*fAccurate=*/true);
+ return subscript.CountSigOps(/*fAccurate=*/true);
}
}
--- a/src/script/script.cpp
+++ b/src/script/script.cpp
@@ -156,7 +156,7 @@ std::string GetOpName(opcodetype opcode)
}
}
-unsigned int CScript::GetLegacySigOpCount(bool fAccurate) const
+unsigned int CScript::CountSigOps(bool fAccurate) const
{
unsigned int n = 0;
const_iterator pc = begin();
@@ -180,15 +180,12 @@ unsigned int CScript::GetLegacySigOpCount(bool fAccurate) const
return n;
}
-unsigned int CScript::GetSigOpCount(const CScript& scriptSig) const
+unsigned int CountP2SHSigOps(const CScript& scriptSig)
{
- if (!IsPayToScriptHash())
- return GetLegacySigOpCount(/*fAccurate=*/true);
-
// This is a pay-to-script-hash scriptPubKey;
// get the last item that the scriptSig
// pushes onto the stack:
- const_iterator pc = scriptSig.begin();
+ CScript::const_iterator pc = scriptSig.begin();
std::vector<unsigned char> vData;
while (pc < scriptSig.end())
{
@@ -201,7 +198,7 @@ unsigned int CScript::GetSigOpCount(const CScript& scriptSig) const
/// ... and return its opcount:
CScript subscript(vData.begin(), vData.end());
- return subscript.GetLegacySigOpCount(/*fAccurate=*/true);
+ return subscript.CountSigOps(/*fAccurate=*/true);
}
bool CScript::IsPayToAnchor() const
--- a/src/script/script.h
+++ b/src/script/script.h
@@ -528,19 +528,20 @@ public:
}
/**
- * Pre-version-0.6, Bitcoin always counted CHECKMULTISIGs
- * as 20 sigops. With pay-to-script-hash, that changed:
- * CHECKMULTISIGs serialized in scriptSigs are
- * counted more accurately, assuming they are of the form
- * ... OP_N CHECKMULTISIG ...
- */
- unsigned int GetLegacySigOpCount(bool fAccurate) const;
-
- /**
- * Accurately count sigOps, including sigOps in
- * pay-to-script-hash transactions:
- */
- unsigned int GetSigOpCount(const CScript& scriptSig) const;
+ * Count the number of signature operations (sigops) in this script.
+ *
+ * The fAccurate parameter controls how sigops are counted for CHECKMULTISIG
+ * operations. Set fAccurate = true when analyzing P2SH redeem scripts or
+ * SegWit witness scripts, and false when analyzing scriptPubKeys or
+ * scriptSigs.
+ *
+ * Historically, Bitcoin always counted each CHECKMULTISIG as 20 sigops
+ * (MAX_PUBKEYS_PER_MULTISIG), regardless of the number of pubkeys. Starting
+ * with P2SH in version 0.6, CHECKMULTISIG operations inside wrapped scripts
+ * began to be counted more precisely, using the preceding OP_N opcode to
+ * determine the number of pubkeys and thus the number of sigops.
+ */
+ unsigned int CountSigOps(bool fAccurate) const;
/*
* OP_1 <0x4e73>
@@ -606,6 +607,15 @@ public:
explicit CScriptID(const uint160& in) : BaseHash(in) {}
};
+/**
+ * Count the number of signature operations (sigops) in a P2SH scriptSig.
+ *
+ * If the scriptSig contains a SegWit redeem script (i.e., a P2SH-P2WPKH or
+ * P2SH-P2WSH script), this function counts only the non-SegWit sigops.
+ * To count SegWit sigops in such cases, use CountWitnessSigOps.
+ */
+unsigned int CountP2SHSigOps(const CScript& scriptSig);unsigned int CountP2SHSigOps(const CScript& scriptSig);
+
/** Test for OP_SUCCESSx opcodes as defined by BIP342. */
bool IsOpSuccess(const opcodetype& opcode);
</p>
</details>
