Merge #723: [HF] Dynafed blocks must have sighash byte

809f4acd3 Dynafed blocks must have sighash byte (Gregory Sanders)

Pull request description:

  It is not included in the signature hash.
  The sighash byte MUST be SIGHASH_ALL to avoid malleability.

Tree-SHA512: d4a658448e3a8b3609b32fc24e8d28fc65ea13021ba0b49109b6c4477253d08183ac6e77e0658f23b2630cbb916c6b19e86db241c5d7456391385a308235d0bd

src/block_proof.cpp

@@ -21,12 +21,15 @@ bool CheckChallenge(const CBlockHeader& block, const CBlockIndex& indexLast, con
 
 static bool CheckProofGeneric(const CBlockHeader& block, const uint32_t max_block_signature_size, const CScript& challenge, const CScript& scriptSig, const CScriptWitness& witness)
 {
-    // scriptSig or witness will be nonempty, but not both, so just compare both limits
-    if (scriptSig.size() > max_block_signature_size) {
-        return false;
-    }
+    // Legacy blocks have empty witness, dynafed blocks have empty scriptSig
+    bool is_dyna = !witness.stack.empty();
 
-    if (witness.GetSerializedSize() > max_block_signature_size) {
+    // Check signature limits for blocks
+    if (scriptSig.size() > max_block_signature_size) {
+        assert(!is_dyna);
+        return false;
+    } else if (witness.GetSerializedSize() > max_block_signature_size) {
+        assert(is_dyna);
         return false;
     }
 
@@ -40,7 +43,7 @@ static bool CheckProofGeneric(const CBlockHeader& block, const uint32_t max_bloc
         | SCRIPT_VERIFY_SIGPUSHONLY // Witness is push-only
         | SCRIPT_VERIFY_LOW_S // Stop easiest signature fiddling
         | SCRIPT_VERIFY_WITNESS // Witness and to enforce cleanstack
-        | SCRIPT_NO_SIGHASH_BYTE; // non-Check(Multi)Sig signatures will not have sighash byte
+        | (is_dyna ? 0 : SCRIPT_NO_SIGHASH_BYTE); // Non-dynafed blocks do not have sighash byte
     return GenericVerifyScript(scriptSig, witness, challenge, proof_flags, block);
 }
 

src/hash.h

@@ -191,7 +191,7 @@ public:
     }
 };
 
-/** Compute the 256-bit hash of an object's serialization. */
+/** Compute the 256-bit hash of an object's serialization, with optional sighash byte. */
 template<typename T>
 uint256 SerializeHash(const T& obj, int nType=SER_GETHASH, int nVersion=PROTOCOL_VERSION)
 {

src/rpc/mining.cpp

@@ -1134,11 +1134,13 @@ UniValue combineblocksigs(const JSONRPCRequest& request)
     if (!DecodeHexBlk(block, request.params[0].get_str()))
         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block decode failed");
 
+    bool is_dynafed = !block.m_dynafed_params.IsNull();
+
     const Consensus::Params& params = Params().GetConsensus();
     const UniValue& sigs = request.params[1].get_array();
     CBasicKeyStore keystore;
     SignatureData sig_data;
-    SimpleSignatureCreator signature_creator(block.GetHash());
+    SimpleSignatureCreator signature_creator(block.GetHash(), is_dynafed ? SIGHASH_ALL : 0);
     for (unsigned int i = 0; i < sigs.size(); i++) {
         UniValue pubkey_sig = sigs[i];
         const std::string& pubkey_str = pubkey_sig["pubkey"].get_str();
@@ -1155,7 +1157,7 @@ UniValue combineblocksigs(const JSONRPCRequest& request)
         sig_data.signatures[pubkey.GetID()] = std::make_pair(pubkey, sig_bytes);
     }
 
-    if (!block.m_dynafed_params.IsNull()) {
+    if (is_dynafed) {
         if (request.params[2].isNull()) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Signing dynamic blocks requires the witnessScript argument");
         }
@@ -1164,7 +1166,7 @@ UniValue combineblocksigs(const JSONRPCRequest& request)
             keystore.AddCScript(CScript(witness_bytes.begin(), witness_bytes.end()));
         }
         // Finalizes the signatures, has no access to keys
-        ProduceSignature(keystore, signature_creator, block.m_dynafed_params.m_current.m_signblockscript, sig_data, SCRIPT_NO_SIGHASH_BYTE);
+        ProduceSignature(keystore, signature_creator, block.m_dynafed_params.m_current.m_signblockscript, sig_data, SCRIPT_VERIFY_NONE);
         block.m_signblock_witness = sig_data.scriptWitness;
     } else {
         // Finalizes the signatures, has no access to keys

src/script/generic.hpp

@@ -15,15 +15,26 @@ class SimpleSignatureChecker : public BaseSignatureChecker
 {
 public:
     uint256 hash;
+    bool sighash_byte;
 
-    SimpleSignatureChecker(const uint256& hashIn) : hash(hashIn) {};
+    SimpleSignatureChecker(const uint256& hashIn, bool sighash_byte_in) : hash(hashIn), sighash_byte(sighash_byte_in) {};
     bool CheckSig(const std::vector<unsigned char>& vchSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const
     {
+        std::vector<unsigned char> vchSigCopy(vchSig);
         CPubKey pubkey(vchPubKey);
         if (!pubkey.IsValid())
             return false;
         if (vchSig.empty())
             return false;
+        // Get rid of sighash byte before verifying hash
+        // Note: We only accept SIGHASH_ALL!
+        if (sighash_byte) {
+            const unsigned char popped_byte = vchSigCopy.back();
+            vchSigCopy.pop_back();
+            if (popped_byte != SIGHASH_ALL) {
+                return false;
+            }
+        }
         return pubkey.Verify(hash, vchSig);
     }
 };
@@ -31,29 +42,41 @@ public:
 class SimpleSignatureCreator : public BaseSignatureCreator
 {
     SimpleSignatureChecker checker;
+    bool sighash_byte;
 
 public:
-    SimpleSignatureCreator(const uint256& hashIn) : checker(hashIn) {};
+    SimpleSignatureCreator(const uint256& hashIn, bool sighash_byte_in) : checker(hashIn, sighash_byte_in), sighash_byte(sighash_byte_in) {};
     const BaseSignatureChecker& Checker() const { return checker; }
     bool CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& keyid, const CScript& scriptCode, SigVersion sigversion) const
     {
         CKey key;
         if (!provider.GetKey(keyid, key))
             return false;
-        return key.Sign(checker.hash, vchSig);
+        if (!key.Sign(checker.hash, vchSig)) {
+            return false;
+        }
+        // We only support sighash all for malleability reasons(it is not committed in sighash)
+        if (sighash_byte) {
+            vchSig.push_back(SIGHASH_ALL);
+        }
+        return true;
     }
 };
 
 template<typename T>
 bool GenericVerifyScript(const CScript& scriptSig, const CScriptWitness& witness, const CScript& scriptPubKey, unsigned int flags, const T& data)
 {
-    return VerifyScript(scriptSig, scriptPubKey, &witness, flags, SimpleSignatureChecker(SerializeHash(data)));
+    bool sighash_byte = (flags & SCRIPT_NO_SIGHASH_BYTE) ? false : true;
+    // Note: Our hash doesn't commit to the sighash byte
+    return VerifyScript(scriptSig, scriptPubKey, &witness, flags, SimpleSignatureChecker(SerializeHash(data), sighash_byte));
 }
 
 template<typename T>
-bool GenericSignScript(const CKeyStore& keystore, const T& data, const CScript& fromPubKey, SignatureData& scriptSig)
+bool GenericSignScript(const CKeyStore& keystore, const T& data, const CScript& fromPubKey, SignatureData& scriptSig, unsigned int additional_flags)
 {
-    return ProduceSignature(keystore, SimpleSignatureCreator(SerializeHash(data)), fromPubKey, scriptSig, SCRIPT_NO_SIGHASH_BYTE);
+    bool sighash_byte = (additional_flags & SCRIPT_NO_SIGHASH_BYTE) ? false : true;
+    // Note: Our hash doesn't commit to the sighash byte
+    return ProduceSignature(keystore, SimpleSignatureCreator(SerializeHash(data), sighash_byte), fromPubKey, scriptSig, additional_flags);
 }
 
 #endif // H_BITCOIN_SCRIPT_GENERIC

src/wallet/rpcwallet.cpp

@@ -4783,7 +4783,7 @@ UniValue signblock(const JSONRPCRequest& request)
     // Expose SignatureData internals in return value in lieu of "Partially Signed Bitcoin Blocks"
     SignatureData block_sigs;
     if (block.m_dynafed_params.IsNull()) {
-        GenericSignScript(*pwallet, block.GetBlockHeader(), block.proof.challenge, block_sigs);
+        GenericSignScript(*pwallet, block.GetBlockHeader(), block.proof.challenge, block_sigs, SCRIPT_NO_SIGHASH_BYTE /* additional_flags */);
     } else {
         if (request.params[1].isNull()) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Signing dynamic blocks requires the witnessScript argument");
@@ -4794,7 +4794,7 @@ UniValue signblock(const JSONRPCRequest& request)
         if (!witness_bytes.empty()) {
             pwallet->AddCScript(CScript(witness_bytes.begin(), witness_bytes.end()));
         }
-        GenericSignScript(*pwallet, block.GetBlockHeader(), block.m_dynafed_params.m_current.m_signblockscript, block_sigs);
+        GenericSignScript(*pwallet, block.GetBlockHeader(), block.m_dynafed_params.m_current.m_signblockscript, block_sigs, SCRIPT_VERIFY_NONE /* additional_flags */);
     }
 
     // Error if sig data didn't "grow"