Merge #350: Make block solutions more strictly validated

1348313 add basic functional tests for signed block rpcs and validation (Gregory Sanders)
c07b0af Add signature checking flag for when no sighash byte is expected (Gregory Sanders)
8f92b21 remove block (multi)signature malleability vectors (Gregory Sanders)

Author: instagibbs

qa/pull-tester/rpc-tests.py

@@ -164,6 +164,7 @@
     'rpcnamedargs.py',
     'listsinceblock.py',
     'p2p-leaktests.py',
+    'signed_blockchain.py',
 ]
 if ENABLE_ZMQ:
     testScripts.append('zmq_test.py')

qa/rpc-tests/signed_blockchain.py

@@ -0,0 +1,120 @@
+#!/usr/bin/env python3
+# Copyright (c) 2014-2016 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#
+# Test RPC calls related to blockchain state. Tests correspond to code in
+# rpc/blockchain.cpp.
+#
+
+from decimal import Decimal
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.authproxy import JSONRPCException
+from test_framework.util import (
+    assert_equal,
+    start_nodes,
+    assert_raises_jsonrpc,
+)
+
+
+class SignedBlockchainTest(BitcoinTestFramework):
+    """
+    Test signed-blockchain-related RPC calls:
+
+        - getnewblockhex
+        - signblock
+        - combineblocksigs
+        - submitblock
+
+    """
+
+    def __init__(self):
+        super().__init__()
+        self.setup_clean_chain = True
+        self.num_nodes = 3
+
+    def setup_network(self, split=False):
+        # Keys for signing 2-of-3
+        pubkeys = ["039560e48d4336e40db447fc136ce24ae1dfdefa5701e4d4e57aa1a1a9f47f3faa", "025a66517c1d85adcd909f9f675bf656708edc4da1f614693e347be6baf0fef4ae", "02ca238faeb3b01d26ae8a39869220dbd84cc8516398afa8958fe613fe4fdf1c04"]
+        # Normal multisig scriptPubKey: 1 <33 byte pubkey> <33 byte pubkey> ... 2 OP_CMS
+        sign_script = "-signblockscript=5221"+pubkeys[0]+"21"+pubkeys[1]+"21"+pubkeys[2]+"53ae"
+        self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, [[sign_script],[sign_script], [sign_script]])
+        # nodes are disconnected for this test
+        self.is_network_split = True
+
+    def run_test(self):
+        keys = ["cRANrxPMceu8jKAA76xzpA9PtTEhBknyZyaaQZ3Z5FnFTGkCAqmT", "cTJRjDBWo1JXdie31B5wv5eXNrPGCAjQKn48umubhmNLjsnj951V", "cVjTXVgKE8PhvCgDowRJVQW68q7j5kSbDGfAT5CUwt9D8dn2cAwf"]
+        assert_equal(self.nodes[0].getblockcount(), 0)
+        assert_equal(self.nodes[1].getblockcount(), 0)
+
+        block_hex = self.nodes[0].getnewblockhex()
+
+        # Block needs signatures, but valid and extends chaintip otherwise
+        assert_equal(self.nodes[0].testproposedblock(block_hex), None)
+        assert_equal(self.nodes[1].testproposedblock(block_hex), None)
+
+        assert_equal(self.nodes[0].submitblock(block_hex), "block-proof-invalid")
+        assert_equal(self.nodes[1].submitblock(block_hex), "block-proof-invalid")
+
+        assert_equal(self.nodes[0].getblockcount(), 0)
+        assert_equal(self.nodes[1].getblockcount(), 0)
+
+        # combineblocksigs only returns true when signatures are appended and enough
+        # are included to pass validation
+        assert_equal(self.nodes[0].combineblocksigs(block_hex, [])["complete"], False)
+        assert_equal(self.nodes[1].combineblocksigs(block_hex, [])["complete"], False)
+
+        # Now we can try to sign, without key
+        assert_equal(self.nodes[0].signblock(block_hex), "00")
+        assert_equal(self.nodes[1].signblock(block_hex), "00")
+
+        # Import keys
+        self.nodes[0].importprivkey(keys[0])
+        self.nodes[1].importprivkey(keys[1])
+        self.nodes[2].importprivkey(keys[2])
+
+        sig0 = self.nodes[0].signblock(block_hex)
+        sig1 = self.nodes[1].signblock(block_hex)
+
+        combined0 = self.nodes[0].combineblocksigs(block_hex, [sig0])
+        assert(not combined0["complete"])
+        combined1 = self.nodes[0].combineblocksigs(combined0["hex"], [sig1])
+        assert(combined1["complete"])
+
+        # Still haven't moved forward
+        assert_equal(self.nodes[0].getblockcount(), 0)
+        assert_equal(self.nodes[1].getblockcount(), 0)
+
+        self.nodes[0].submitblock(combined1["hex"])
+        # Move his chain along for later
+        self.nodes[2].submitblock(combined1["hex"])
+
+        assert_equal(self.nodes[0].getblockcount(), 1)
+        assert_equal(self.nodes[1].getblockcount(), 0)
+        assert_equal(self.nodes[2].getblockcount(), 1)
+
+        block_too_far = self.nodes[0].getnewblockhex()
+
+        assert_raises_jsonrpc,(JSONRPCException, -25, "proposal was not based on our best chain", self.nodes[1].testproposedblock, block_too_far)
+
+        # Finally, submit block
+        self.nodes[1].submitblock(combined1["hex"])
+        assert_equal(self.nodes[1].getblockcount(), 1)
+
+        # Now proposal is fine, aside from sigs
+        assert_equal(self.nodes[1].testproposedblock(block_too_far), None)
+        sig0 = self.nodes[0].signblock(block_too_far)
+        sig1 = self.nodes[1].signblock(block_too_far)
+        sig2 = self.nodes[2].signblock(block_too_far)
+
+        combined0 = self.nodes[0].combineblocksigs(block_too_far, [sig0])
+        assert(not combined0["complete"])
+        # combining signature from third node this time
+        combined1 = self.nodes[0].combineblocksigs(combined0["hex"], [sig2])
+        assert(combined1["complete"])
+
+        # TODO stuff with too many signatures or junk data manually
+
+if __name__ == '__main__':
+    SignedBlockchainTest().main()

src/pow.cpp

@@ -61,7 +61,28 @@ bool CheckProof(const CBlockHeader& block, const Consensus::Params& params)
 {
     if (block.GetHash() == params.hashGenesisBlock)
        return true;
-    return GenericVerifyScript(block.proof.solution, block.proof.challenge, SCRIPT_VERIFY_P2SH, block);
+
+    // Some important anti-DoS flags.
+    // Note: Blockhashes do not commit to the proof.
+    // Therefore we may have a signature be mealleated
+    // to stay valid, but cause the block to fail
+    // validation, in this case, block weight.
+    // In that case, the block will be marked as permanently
+    // invalid and not processed.
+    // NOTE: These have only been deemed sufficient for OP_CMS
+    // ANY OTHER SCRIPT TYPE MAY REQUIRE DIFFERENT FLAGS/CONSIDERATIONS
+    // TODO: Better design to not have to worry about script specifics
+    // i.e. exempt block header solution from weight limit
+    unsigned int proof_flags = SCRIPT_VERIFY_P2SH // Just allows P2SH evaluation
+        | SCRIPT_VERIFY_STRICTENC // Minimally-sized DER sigs
+        | SCRIPT_VERIFY_NULLDUMMY // No extra data stuffed into OP_CMS witness
+        | SCRIPT_VERIFY_CLEANSTACK // No extra pushes leftover in witness
+        | SCRIPT_VERIFY_MINIMALDATA // Pushes are minimally-sized
+        | SCRIPT_VERIFY_SIGPUSHONLY // Witness is push-only
+        | SCRIPT_VERIFY_LOW_S // Stop easiest signature fiddling
+        | SCRIPT_VERIFY_WITNESS // Required for cleanstack eval in VerifyScript
+        | SCRIPT_NO_SIGHASH_BYTE; // non-Check(Multi)Sig signatures will not have sighash byte
+    return GenericVerifyScript(block.proof.solution, block.proof.challenge, proof_flags, block);
 }
 
 bool MaybeGenerateProof(CBlockHeader *pblock, CWallet *pwallet)

src/script/interpreter.cpp

@@ -223,12 +223,19 @@ bool CheckSignatureEncoding(const vector<unsigned char> &vchSig, unsigned int fl
     if (vchSig.size() == 0) {
         return true;
     }
-    if ((flags & (SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_LOW_S | SCRIPT_VERIFY_STRICTENC)) != 0 && !IsValidSignatureEncoding(vchSig)) {
+    bool no_hash_byte = (flags & SCRIPT_NO_SIGHASH_BYTE) == SCRIPT_NO_SIGHASH_BYTE;
+    std::vector<unsigned char> vchSigCopy(vchSig.begin(), vchSig.begin() + vchSig.size());
+    // Push a dummy sighash byte to pass checks
+    if (no_hash_byte) {
+        vchSigCopy.push_back(SIGHASH_ALL);
+    }
+
+    if ((flags & (SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_LOW_S | SCRIPT_VERIFY_STRICTENC)) != 0 && !IsValidSignatureEncoding(vchSigCopy)) {
         return set_error(serror, SCRIPT_ERR_SIG_DER);
-    } else if ((flags & SCRIPT_VERIFY_LOW_S) != 0 && !IsLowDERSignature(vchSig, serror)) {
+    } else if ((flags & SCRIPT_VERIFY_LOW_S) != 0 && !IsLowDERSignature(vchSigCopy, serror)) {
         // serror is set
         return false;
-    } else if ((flags & SCRIPT_VERIFY_STRICTENC) != 0 && !IsDefinedHashtypeSignature(vchSig)) {
+    } else if ((flags & SCRIPT_VERIFY_STRICTENC) != 0 && !IsDefinedHashtypeSignature(vchSigCopy)) {
         return set_error(serror, SCRIPT_ERR_SIG_HASHTYPE);
     }
     return true;
@@ -1313,8 +1320,8 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
                     valtype& vchData   = stacktop(-2);
                     valtype& vchPubKey = stacktop(-1);
 
-                    // Sigs from stack have no hash type, so we disable strictenc check
-                    if (!CheckSignatureEncoding(vchSig, (flags & ~SCRIPT_VERIFY_STRICTENC), serror) || !CheckPubKeyEncoding(vchPubKey, flags, sigversion, serror)) {
+                    // Sigs from stack have no hash byte ever
+                    if (!CheckSignatureEncoding(vchSig, (flags | SCRIPT_NO_SIGHASH_BYTE), serror) || !CheckPubKeyEncoding(vchPubKey, flags, sigversion, serror)) {
                         //serror is set
                         return false;
                     }

src/script/interpreter.h

@@ -107,6 +107,10 @@ enum
     // Public keys in segregated witness scripts must be compressed
     //
     SCRIPT_VERIFY_WITNESS_PUBKEYTYPE = (1U << 15),
+
+    // Signature checking assumes no sighash byte after the DER signature
+    //
+    SCRIPT_NO_SIGHASH_BYTE = (1U << 16),
 
 };