Merge #12803: Make BaseSignatureCreator a pure interface

Summary:
be67831 Make DummySignatureCreator a singleton (Pieter Wuille)
190b8d2 Make BaseSignatureCreator a pure interface (Pieter Wuille)

Pull request description:

  * Removes the `m_provider` field from `BaseSignatureCreator`. Instead both a `SigningProvider` (which provides keys and scripts) and a `BaseSignatureCreator` (which implements the transaction-specific (or other) signing logic) are passed into and down in `ProduceSignature`, making the two concepts orthogonal.
  * Makes `BaseSignatureCreator` a pure interface without constructor, making it easier to implement new derivations of it (for example for message signing).
  * As `DummySignatureCreator` now becomes a stateless object, turn it into a singleton `DUMMY_SIGNATURE_CREATOR`.

Tree-SHA512: 5f1f4512e4ea7d02a31df7b9ede55008efa716c5b74a2630ca1c2fc6599584d8bf5f5641487266127f4b3788033803539fbd22b03ef1219c83c10da2d3da3dcd

Backport of Core PR12803
bitcoin/bitcoin#12803

Depends on D3888

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3889

src/bitcoin-tx.cpp

@@ -669,8 +669,9 @@ static void MutateTxSign(CMutableTransaction &tx, const std::string &flagStr) {
         // Only sign SIGHASH_SINGLE if there's a corresponding output:
         if ((sigHashType.getBaseType() != BaseSigHashType::SINGLE) ||
             (i < mergedTx.vout.size())) {
-            ProduceSignature(MutableTransactionSignatureCreator(
-                                 &keystore, &mergedTx, i, amount, sigHashType),
+            ProduceSignature(keystore,
+                             MutableTransactionSignatureCreator(
+                                 &mergedTx, i, amount, sigHashType),
                              prevPubKey, sigdata);
         }
 

src/rpc/rawtransaction.cpp

@@ -983,8 +983,9 @@ UniValue SignTransaction(CMutableTransaction &mtx,
         // Only sign SIGHASH_SINGLE if there's a corresponding output:
         if ((sigHashType.getBaseType() != BaseSigHashType::SINGLE) ||
             (i < mtx.vout.size())) {
-            ProduceSignature(MutableTransactionSignatureCreator(
-                                 keystore, &mtx, i, amount, sigHashType),
+            ProduceSignature(*keystore,
+                             MutableTransactionSignatureCreator(&mtx, i, amount,
+                                                                sigHashType),
                              prevPubKey, sigdata);
         }
         sigdata = CombineSignatures(

src/script/ismine.cpp

@@ -96,7 +96,7 @@ isminetype IsMine(const CKeyStore &keystore, const CScript &scriptPubKey,
         // TODO: This could be optimized some by doing some work after the above
         // solver
         SignatureData sigs;
-        return ProduceSignature(DummySignatureCreator(&keystore), scriptPubKey,
+        return ProduceSignature(keystore, DUMMY_SIGNATURE_CREATOR, scriptPubKey,
                                 sigs)
                    ? ISMINE_WATCH_SOLVABLE
                    : ISMINE_WATCH_UNSOLVABLE;

src/script/sign.cpp

@@ -14,17 +14,17 @@
 typedef std::vector<uint8_t> valtype;
 
 TransactionSignatureCreator::TransactionSignatureCreator(
-    const SigningProvider *provider, const CTransaction *txToIn,
-    unsigned int nInIn, const Amount amountIn, SigHashType sigHashTypeIn)
-    : BaseSignatureCreator(provider), txTo(txToIn), nIn(nInIn),
-      amount(amountIn), sigHashType(sigHashTypeIn),
+    const CTransaction *txToIn, unsigned int nInIn, const Amount amountIn,
+    SigHashType sigHashTypeIn)
+    : txTo(txToIn), nIn(nInIn), amount(amountIn), sigHashType(sigHashTypeIn),
       checker(txTo, nIn, amountIn) {}
 
-bool TransactionSignatureCreator::CreateSig(std::vector<uint8_t> &vchSig,
+bool TransactionSignatureCreator::CreateSig(const SigningProvider &provider,
+                                            std::vector<uint8_t> &vchSig,
                                             const CKeyID &address,
                                             const CScript &scriptCode) const {
     CKey key;
-    if (!m_provider->GetKey(address, key)) {
+    if (!provider.GetKey(address, key)) {
         return false;
     }
 
@@ -37,18 +37,20 @@ bool TransactionSignatureCreator::CreateSig(std::vector<uint8_t> &vchSig,
     return true;
 }
 
-static bool Sign1(const CKeyID &address, const BaseSignatureCreator &creator,
+static bool Sign1(const SigningProvider &provider, const CKeyID &address,
+                  const BaseSignatureCreator &creator,
                   const CScript &scriptCode, std::vector<valtype> &ret) {
     std::vector<uint8_t> vchSig;
-    if (!creator.CreateSig(vchSig, address, scriptCode)) {
+    if (!creator.CreateSig(provider, vchSig, address, scriptCode)) {
         return false;
     }
 
     ret.push_back(vchSig);
     return true;
 }
 
-static bool SignN(const std::vector<valtype> &multisigdata,
+static bool SignN(const SigningProvider &provider,
+                  const std::vector<valtype> &multisigdata,
                   const BaseSignatureCreator &creator,
                   const CScript &scriptCode, std::vector<valtype> &ret) {
     int nSigned = 0;
@@ -57,7 +59,7 @@ static bool SignN(const std::vector<valtype> &multisigdata,
          i++) {
         const valtype &pubkey = multisigdata[i];
         CKeyID keyID = CPubKey(pubkey).GetID();
-        if (Sign1(keyID, creator, scriptCode, ret)) {
+        if (Sign1(provider, keyID, creator, scriptCode, ret)) {
             ++nSigned;
         }
     }
@@ -72,7 +74,8 @@ static bool SignN(const std::vector<valtype> &multisigdata,
  * scriptSigRet is the redemption script.
  * Returns false if scriptPubKey could not be completely satisfied.
  */
-static bool SignStep(const BaseSignatureCreator &creator,
+static bool SignStep(const SigningProvider &provider,
+                     const BaseSignatureCreator &creator,
                      const CScript &scriptPubKey, std::vector<valtype> &ret,
                      txnouttype &whichTypeRet) {
     CScript scriptRet;
@@ -91,21 +94,20 @@ static bool SignStep(const BaseSignatureCreator &creator,
             return false;
         case TX_PUBKEY:
             keyID = CPubKey(vSolutions[0]).GetID();
-            return Sign1(keyID, creator, scriptPubKey, ret);
+            return Sign1(provider, keyID, creator, scriptPubKey, ret);
         case TX_PUBKEYHASH: {
             keyID = CKeyID(uint160(vSolutions[0]));
-            if (!Sign1(keyID, creator, scriptPubKey, ret)) {
+            if (!Sign1(provider, keyID, creator, scriptPubKey, ret)) {
                 return false;
             }
 
             CPubKey vch;
-            creator.Provider().GetPubKey(keyID, vch);
+            provider.GetPubKey(keyID, vch);
             ret.push_back(ToByteVector(vch));
             return true;
         }
         case TX_SCRIPTHASH:
-            if (creator.Provider().GetCScript(uint160(vSolutions[0]),
-                                              scriptRet)) {
+            if (provider.GetCScript(uint160(vSolutions[0]), scriptRet)) {
                 ret.push_back(
                     std::vector<uint8_t>(scriptRet.begin(), scriptRet.end()));
                 return true;
@@ -114,7 +116,7 @@ static bool SignStep(const BaseSignatureCreator &creator,
         case TX_MULTISIG:
             // workaround CHECKMULTISIG bug
             ret.push_back(valtype());
-            return (SignN(vSolutions, creator, scriptPubKey, ret));
+            return (SignN(provider, vSolutions, creator, scriptPubKey, ret));
         default:
             return false;
     }
@@ -135,19 +137,21 @@ static CScript PushAll(const std::vector<valtype> &values) {
     return result;
 }
 
-bool ProduceSignature(const BaseSignatureCreator &creator,
+bool ProduceSignature(const SigningProvider &provider,
+                      const BaseSignatureCreator &creator,
                       const CScript &fromPubKey, SignatureData &sigdata) {
     std::vector<valtype> result;
     txnouttype whichType;
-    bool solved = SignStep(creator, fromPubKey, result, whichType);
+    bool solved = SignStep(provider, creator, fromPubKey, result, whichType);
     CScript subscript;
 
     if (solved && whichType == TX_SCRIPTHASH) {
         // Solver returns the subscript that needs to be evaluated; the final
         // scriptSig is the signatures from that and then the serialized
         // subscript:
         subscript = CScript(result[0].begin(), result[0].end());
-        solved = solved && SignStep(creator, subscript, result, whichType) &&
+        solved = solved &&
+                 SignStep(provider, creator, subscript, result, whichType) &&
                  whichType != TX_SCRIPTHASH;
         result.push_back(
             std::vector<uint8_t>(subscript.begin(), subscript.end()));
@@ -185,11 +189,10 @@ bool SignSignature(const SigningProvider &provider, const CScript &fromPubKey,
     assert(nIn < txTo.vin.size());
 
     CTransaction txToConst(txTo);
-    TransactionSignatureCreator creator(&provider, &txToConst, nIn, amount,
-                                        sigHashType);
+    TransactionSignatureCreator creator(&txToConst, nIn, amount, sigHashType);
 
     SignatureData sigdata;
-    bool ret = ProduceSignature(creator, fromPubKey, sigdata);
+    bool ret = ProduceSignature(provider, creator, fromPubKey, sigdata);
     UpdateTransaction(txTo, nIn, sigdata);
     return ret;
 }
@@ -352,36 +355,40 @@ SignatureData CombineSignatures(const CScript &scriptPubKey,
 
 namespace {
 /** Dummy signature checker which accepts all signatures. */
-class DummySignatureChecker : public BaseSignatureChecker {
+class DummySignatureChecker final : public BaseSignatureChecker {
 public:
     DummySignatureChecker() {}
-
     bool CheckSig(const std::vector<uint8_t> &scriptSig,
                   const std::vector<uint8_t> &vchPubKey,
                   const CScript &scriptCode, uint32_t flags) const override {
         return true;
     }
 };
-const DummySignatureChecker dummyChecker;
-} // namespace
+const DummySignatureChecker DUMMY_CHECKER;
 
-const BaseSignatureChecker &DummySignatureCreator::Checker() const {
-    return dummyChecker;
-}
+class DummySignatureCreator final : public BaseSignatureCreator {
+public:
+    DummySignatureCreator() {}
+    const BaseSignatureChecker &Checker() const override {
+        return DUMMY_CHECKER;
+    }
+    bool CreateSig(const SigningProvider &provider,
+                   std::vector<uint8_t> &vchSig, const CKeyID &keyid,
+                   const CScript &scriptCode) const {
+        // Create a dummy signature that is a valid DER-encoding
+        vchSig.assign(72, '\000');
+        vchSig[0] = 0x30;
+        vchSig[1] = 69;
+        vchSig[2] = 0x02;
+        vchSig[3] = 33;
+        vchSig[4] = 0x01;
+        vchSig[4 + 33] = 0x02;
+        vchSig[5 + 33] = 32;
+        vchSig[6 + 33] = 0x01;
+        vchSig[6 + 33 + 32] = SIGHASH_ALL | SIGHASH_FORKID;
+        return true;
+    }
+};
+} // namespace
 
-bool DummySignatureCreator::CreateSig(std::vector<uint8_t> &vchSig,
-                                      const CKeyID &keyid,
-                                      const CScript &scriptCode) const {
-    // Create a dummy signature that is a valid DER-encoding
-    vchSig.assign(72, '\000');
-    vchSig[0] = 0x30;
-    vchSig[1] = 69;
-    vchSig[2] = 0x02;
-    vchSig[3] = 33;
-    vchSig[4] = 0x01;
-    vchSig[4 + 33] = 0x02;
-    vchSig[5 + 33] = 32;
-    vchSig[6 + 33] = 0x01;
-    vchSig[6 + 33 + 32] = SIGHASH_ALL | SIGHASH_FORKID;
-    return true;
-}
+const BaseSignatureCreator &DUMMY_SIGNATURE_CREATOR = DummySignatureCreator();

src/script/sign.h

@@ -26,20 +26,15 @@ class SigningProvider {
     virtual bool GetKey(const CKeyID &address, CKey &key) const = 0;
 };
 
-/** Virtual base class for signature creators. */
+/** Interface for signature creators. */
 class BaseSignatureCreator {
-protected:
-    const SigningProvider *m_provider;
-
 public:
-    explicit BaseSignatureCreator(const SigningProvider *provider)
-        : m_provider(provider) {}
-    const SigningProvider &Provider() const { return *m_provider; }
     virtual ~BaseSignatureCreator() {}
     virtual const BaseSignatureChecker &Checker() const = 0;
 
     /** Create a singular (non-script) signature. */
-    virtual bool CreateSig(std::vector<uint8_t> &vchSig, const CKeyID &keyid,
+    virtual bool CreateSig(const SigningProvider &provider,
+                           std::vector<uint8_t> &vchSig, const CKeyID &keyid,
                            const CScript &scriptCode) const = 0;
 };
 
@@ -52,38 +47,29 @@ class TransactionSignatureCreator : public BaseSignatureCreator {
     const TransactionSignatureChecker checker;
 
 public:
-    TransactionSignatureCreator(const SigningProvider *provider,
-                                const CTransaction *txToIn, unsigned int nInIn,
+    TransactionSignatureCreator(const CTransaction *txToIn, unsigned int nInIn,
                                 const Amount amountIn,
                                 SigHashType sigHashTypeIn = SigHashType());
     const BaseSignatureChecker &Checker() const override { return checker; }
-    bool CreateSig(std::vector<uint8_t> &vchSig, const CKeyID &keyid,
+    bool CreateSig(const SigningProvider &provider,
+                   std::vector<uint8_t> &vchSig, const CKeyID &keyid,
                    const CScript &scriptCode) const override;
 };
 
 class MutableTransactionSignatureCreator : public TransactionSignatureCreator {
     CTransaction tx;
 
 public:
-    MutableTransactionSignatureCreator(const SigningProvider *provider,
-                                       const CMutableTransaction *txToIn,
+    MutableTransactionSignatureCreator(const CMutableTransaction *txToIn,
                                        unsigned int nInIn,
                                        const Amount amountIn,
                                        SigHashType sigHashTypeIn)
-        : TransactionSignatureCreator(provider, &tx, nInIn, amountIn,
-                                      sigHashTypeIn),
+        : TransactionSignatureCreator(&tx, nInIn, amountIn, sigHashTypeIn),
           tx(*txToIn) {}
 };
 
 /** A signature creator that just produces 72-byte empty signatures. */
-class DummySignatureCreator : public BaseSignatureCreator {
-public:
-    explicit DummySignatureCreator(const SigningProvider *provider)
-        : BaseSignatureCreator(provider) {}
-    const BaseSignatureChecker &Checker() const override;
-    bool CreateSig(std::vector<uint8_t> &vchSig, const CKeyID &keyid,
-                   const CScript &scriptCode) const override;
-};
+extern const BaseSignatureCreator &DUMMY_SIGNATURE_CREATOR;
 
 struct SignatureData {
     CScript scriptSig;
@@ -93,7 +79,8 @@ struct SignatureData {
 };
 
 /** Produce a script signature using a generic signature creator. */
-bool ProduceSignature(const BaseSignatureCreator &creator,
+bool ProduceSignature(const SigningProvider &provider,
+                      const BaseSignatureCreator &creator,
                       const CScript &scriptPubKey, SignatureData &sigdata);
 
 /** Produce a script signature for a transaction. */

src/test/txvalidationcache_tests.cpp

@@ -394,15 +394,15 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) {
 
         // Sign
         SignatureData sigdata;
-        ProduceSignature(
-            MutableTransactionSignatureCreator(&keystore, &tx, 0, 11 * CENT,
-                                               SigHashType().withForkId()),
-            spend_tx.vout[0].scriptPubKey, sigdata);
+        ProduceSignature(keystore,
+                         MutableTransactionSignatureCreator(
+                             &tx, 0, 11 * CENT, SigHashType().withForkId()),
+                         spend_tx.vout[0].scriptPubKey, sigdata);
         UpdateTransaction(tx, 0, sigdata);
-        ProduceSignature(
-            MutableTransactionSignatureCreator(&keystore, &tx, 1, 11 * CENT,
-                                               SigHashType().withForkId()),
-            spend_tx.vout[3].scriptPubKey, sigdata);
+        ProduceSignature(keystore,
+                         MutableTransactionSignatureCreator(
+                             &tx, 1, 11 * CENT, SigHashType().withForkId()),
+                         spend_tx.vout[3].scriptPubKey, sigdata);
         UpdateTransaction(tx, 1, sigdata);
 
         // This should be valid under all script flags

src/wallet/wallet.cpp

@@ -1618,7 +1618,8 @@ bool CWallet::DummySignInput(CTxIn &tx_in, const CTxOut &txout) const {
     const CScript &scriptPubKey = txout.scriptPubKey;
     SignatureData sigdata;
 
-    if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata)) {
+    if (!ProduceSignature(*this, DUMMY_SIGNATURE_CREATOR, scriptPubKey,
+                          sigdata)) {
         return false;
     }
 
@@ -2807,8 +2808,9 @@ bool CWallet::SignTransaction(CMutableTransaction &tx) {
         const Amount amount = mi->second.tx->vout[input.prevout.GetN()].nValue;
         SignatureData sigdata;
         SigHashType sigHashType = SigHashType().withForkId();
-        if (!ProduceSignature(TransactionSignatureCreator(
-                                  this, &txNewConst, nIn, amount, sigHashType),
+        if (!ProduceSignature(*this,
+                              TransactionSignatureCreator(&txNewConst, nIn,
+                                                          amount, sigHashType),
                               scriptPubKey, sigdata)) {
             return false;
         }
@@ -3276,10 +3278,11 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient> &vecSend,
                 const CScript &scriptPubKey = coin.txout.scriptPubKey;
                 SignatureData sigdata;
 
-                if (!ProduceSignature(TransactionSignatureCreator(
-                                          this, &txNewConst, nIn,
-                                          coin.txout.nValue, sigHashType),
-                                      scriptPubKey, sigdata)) {
+                if (!ProduceSignature(
+                        *this,
+                        TransactionSignatureCreator(
+                            &txNewConst, nIn, coin.txout.nValue, sigHashType),
+                        scriptPubKey, sigdata)) {
                     strFailReason = _("Signing transaction failed");
                     return false;
                 }