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:
0--- a/src/consensus/tx_verify.cpp
1+++ b/src/consensus/tx_verify.cpp
2@@ -113,10 +113,10 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx)
3 {
4 unsigned int nSigOps = 0;
5 for (const auto& txin : tx.vin) {
6- nSigOps += txin.scriptSig.GetLegacySigOpCount(/*fAccurate=*/false);
7+ nSigOps += txin.scriptSig.CountSigOps(/*fAccurate=*/false);
8 }
9 for (const auto& txout : tx.vout) {
10- nSigOps += txout.scriptPubKey.GetLegacySigOpCount(/*fAccurate=*/false);
11+ nSigOps += txout.scriptPubKey.CountSigOps(/*fAccurate=*/false);
12 }
13 return nSigOps;
14 }
15@@ -133,7 +133,7 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& in
16 assert(!coin.IsSpent());
17 const CTxOut &prevout = coin.out;
18 if (prevout.scriptPubKey.IsPayToScriptHash())
19- nSigOps += prevout.scriptPubKey.GetSigOpCount(tx.vin[i].scriptSig);
20+ nSigOps += CountP2SHSigOps(tx.vin[i].scriptSig);
21 }
22 return nSigOps;
23 }
24--- a/src/policy/policy.cpp
25+++ b/src/policy/policy.cpp
26@@ -181,8 +181,11 @@ static bool CheckSigopsBIP54(const CTransaction& tx, const CCoinsViewCache& inpu
27 // `fAccurate` means correctly accounting sigops for CHECKMULTISIGs(VERIFY) with 16 pubkeys
28 // or fewer. This method of accounting was introduced by BIP16, and BIP54 reuses it.
29 // The GetSigOpCount call on the previous scriptPubKey counts both bare and P2SH sigops.
30- sigops += txin.scriptSig.GetLegacySigOpCount(/*fAccurate=*/true);
31- sigops += prev_txo.scriptPubKey.GetSigOpCount(txin.scriptSig);
32+ sigops += txin.scriptSig.CountSigOps(/*fAccurate=*/true);
33+ sigops += prev_txo.scriptPubKey.CountSigOps(/*fAccurate=*/true);
34+ if (prev_txo.scriptPubKey.IsPayToScriptHash()) {
35+ sigops += CountP2SHSigOps(txin.scriptSig);
36+ }
37
38 if (sigops > MAX_TX_LEGACY_SIGOPS) {
39 return false;
40@@ -239,7 +242,7 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
41 if (stack.empty())
42 return false;
43 CScript subscript(stack.back().begin(), stack.back().end());
44- if (subscript.GetLegacySigOpCount(/*fAccurate=*/true) > MAX_P2SH_SIGOPS) {
45+ if (subscript.CountSigOps(/*fAccurate=*/true) > MAX_P2SH_SIGOPS) {
46 return false;
47 }
48 }
49--- a/src/script/interpreter.cpp
50+++ b/src/script/interpreter.cpp
51@@ -2084,7 +2084,7 @@ size_t static WitnessSigOps(int witversion, const std::vector<unsigned char>& wi
52
53 if (witprogram.size() == WITNESS_V0_SCRIPTHASH_SIZE && witness.stack.size() > 0) {
54 CScript subscript(witness.stack.back().begin(), witness.stack.back().end());
55- return subscript.GetLegacySigOpCount(/*fAccurate=*/true);
56+ return subscript.CountSigOps(/*fAccurate=*/true);
57 }
58 }
59
60--- a/src/script/script.cpp
61+++ b/src/script/script.cpp
62@@ -156,7 +156,7 @@ std::string GetOpName(opcodetype opcode)
63 }
64 }
65
66-unsigned int CScript::GetLegacySigOpCount(bool fAccurate) const
67+unsigned int CScript::CountSigOps(bool fAccurate) const
68 {
69 unsigned int n = 0;
70 const_iterator pc = begin();
71@@ -180,15 +180,12 @@ unsigned int CScript::GetLegacySigOpCount(bool fAccurate) const
72 return n;
73 }
74
75-unsigned int CScript::GetSigOpCount(const CScript& scriptSig) const
76+unsigned int CountP2SHSigOps(const CScript& scriptSig)
77 {
78- if (!IsPayToScriptHash())
79- return GetLegacySigOpCount(/*fAccurate=*/true);
80-
81 // This is a pay-to-script-hash scriptPubKey;
82 // get the last item that the scriptSig
83 // pushes onto the stack:
84- const_iterator pc = scriptSig.begin();
85+ CScript::const_iterator pc = scriptSig.begin();
86 std::vector<unsigned char> vData;
87 while (pc < scriptSig.end())
88 {
89@@ -201,7 +198,7 @@ unsigned int CScript::GetSigOpCount(const CScript& scriptSig) const
90
91 /// ... and return its opcount:
92 CScript subscript(vData.begin(), vData.end());
93- return subscript.GetLegacySigOpCount(/*fAccurate=*/true);
94+ return subscript.CountSigOps(/*fAccurate=*/true);
95 }
96
97 bool CScript::IsPayToAnchor() const
98--- a/src/script/script.h
99+++ b/src/script/script.h
100@@ -528,19 +528,20 @@ public:
101 }
102
103 /**
104- * Pre-version-0.6, Bitcoin always counted CHECKMULTISIGs
105- * as 20 sigops. With pay-to-script-hash, that changed:
106- * CHECKMULTISIGs serialized in scriptSigs are
107- * counted more accurately, assuming they are of the form
108- * ... OP_N CHECKMULTISIG ...
109- */
110- unsigned int GetLegacySigOpCount(bool fAccurate) const;
111-
112- /**
113- * Accurately count sigOps, including sigOps in
114- * pay-to-script-hash transactions:
115- */
116- unsigned int GetSigOpCount(const CScript& scriptSig) const;
117+ * Count the number of signature operations (sigops) in this script.
118+ *
119+ * The fAccurate parameter controls how sigops are counted for CHECKMULTISIG
120+ * operations. Set fAccurate = true when analyzing P2SH redeem scripts or
121+ * SegWit witness scripts, and false when analyzing scriptPubKeys or
122+ * scriptSigs.
123+ *
124+ * Historically, Bitcoin always counted each CHECKMULTISIG as 20 sigops
125+ * (MAX_PUBKEYS_PER_MULTISIG), regardless of the number of pubkeys. Starting
126+ * with P2SH in version 0.6, CHECKMULTISIG operations inside wrapped scripts
127+ * began to be counted more precisely, using the preceding OP_N opcode to
128+ * determine the number of pubkeys and thus the number of sigops.
129+ */
130+ unsigned int CountSigOps(bool fAccurate) const;
131
132 /*
133 * OP_1 <0x4e73>
134@@ -606,6 +607,15 @@ public:
135 explicit CScriptID(const uint160& in) : BaseHash(in) {}
136 };
137
138+/**
139+ * Count the number of signature operations (sigops) in a P2SH scriptSig.
140+ *
141+ * If the scriptSig contains a SegWit redeem script (i.e., a P2SH-P2WPKH or
142+ * P2SH-P2WSH script), this function counts only the non-SegWit sigops.
143+ * To count SegWit sigops in such cases, use CountWitnessSigOps.
144+ */
145+unsigned int CountP2SHSigOps(const CScript& scriptSig);unsigned int CountP2SHSigOps(const CScript& scriptSig);
146+
147 /** Test for OP_SUCCESSx opcodes as defined by BIP342. */
148 bool IsOpSuccess(const opcodetype& opcode);
149
