Auto merge of #3520 - Eirik0:3327-sign-offline, r=bitcartel

Fix signing raw transactions with unsynced offline nodes

This PR address the issue in two different ways:

- In `signrawtransaction` we determine the consensus branch ID (which we then later use to construct the transaction) using the chain height. We now also consider the `APPROX_RELEASE_HEIGHT` as this is a better estimation than 0 for the height of the chain if we are unsynced. (This in and of itself solves the Overwinter signing issue).
- We have added an additional parameter to `signrawtransaction` to allow manually overriding the consensus branch ID that zcashd determines we are on. This allows users to work around corner cases where the first strategy is still insufficient.

Closes #3327.

qa/pull-tester/rpc-tests.sh

@@ -44,6 +44,7 @@ testScripts=(
     'merkle_blocks.py'
 #    'fundrawtransaction.py'
     'signrawtransactions.py'
+    'signrawtransaction_offline.py'
     'walletbackup.py'
     'key_import_export.py'
     'nodehandling.py'

qa/rpc-tests/signrawtransaction_offline.py

@@ -0,0 +1,46 @@
+#!/usr/bin/env python2
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal, assert_true, initialize_chain_clean, start_node
+
+class SignOfflineTest (BitcoinTestFramework):
+    # Setup Methods
+    def setup_chain(self):
+        print "Initializing test directory " + self.options.tmpdir
+        initialize_chain_clean(self.options.tmpdir, 2)
+
+    def setup_network(self):
+        self.nodes = [ start_node(0, self.options.tmpdir, ["-nuparams=5ba81b19:10"]) ]
+        self.is_network_split = False
+        self.sync_all()
+
+    # Tests
+    def run_test(self):
+        print "Mining blocks..."
+        self.nodes[0].generate(101)
+
+        offline_node = start_node(1, self.options.tmpdir, ["-maxconnections=0", "-nuparams=5ba81b19:10"])
+        self.nodes.append(offline_node)
+
+        assert_equal(0, len(offline_node.getpeerinfo())) # make sure node 1 has no peers
+
+        privkeys = [self.nodes[0].dumpprivkey(self.nodes[0].getnewaddress())]
+        taddr = self.nodes[0].getnewaddress()
+
+        tx = self.nodes[0].listunspent()[0]
+        txid = tx['txid']
+        scriptpubkey = tx['scriptPubKey']
+
+        create_inputs = [{'txid': txid, 'vout': 0}]
+        sign_inputs = [{'txid': txid, 'vout': 0, 'scriptPubKey': scriptpubkey, 'amount': 10}]
+
+        create_hex = self.nodes[0].createrawtransaction(create_inputs, {taddr: 9.9999})
+
+        signed_tx = offline_node.signrawtransaction(create_hex, sign_inputs, privkeys)
+
+        # If we return the transaction hash, then we have have not thrown an error (success)
+        online_tx_hash = self.nodes[0].sendrawtransaction(signed_tx['hex'])
+        assert_true(len(online_tx_hash) > 0)
+
+if __name__ == '__main__':
+    SignOfflineTest().main()

src/consensus/upgrades.cpp

@@ -82,6 +82,15 @@ uint32_t CurrentEpochBranchId(int nHeight, const Consensus::Params& params) {
     return NetworkUpgradeInfo[CurrentEpoch(nHeight, params)].nBranchId;
 }
 
+bool IsConsensusBranchId(int branchId) {
+    for (int idx = Consensus::BASE_SPROUT; idx < Consensus::MAX_NETWORK_UPGRADES; idx++) {
+        if (branchId == NetworkUpgradeInfo[idx].nBranchId) {
+            return true;
+        }
+    }
+    return false;
+}
+
 bool IsActivationHeight(
     int nHeight,
     const Consensus::Params& params,

src/consensus/upgrades.h

@@ -63,6 +63,12 @@ int CurrentEpoch(int nHeight, const Consensus::Params& params);
  */
 uint32_t CurrentEpochBranchId(int nHeight, const Consensus::Params& params);
 
+/**
+ * Returns true if a given branch id is a valid nBranchId for one of the network
+ * upgrades contained in NetworkUpgradeInfo.
+ */
+bool IsConsensusBranchId(int branchId);
+
 /**
  * Returns true if the given block height is the activation height for the given
  * upgrade.

src/rpc/rawtransaction.cpp

@@ -7,6 +7,7 @@
 #include "consensus/validation.h"
 #include "core_io.h"
 #include "init.h"
+#include "deprecation.h"
 #include "key_io.h"
 #include "keystore.h"
 #include "main.h"
@@ -713,7 +714,7 @@ static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std::
 
 UniValue signrawtransaction(const UniValue& params, bool fHelp)
 {
-    if (fHelp || params.size() < 1 || params.size() > 4)
+    if (fHelp || params.size() < 1 || params.size() > 5)
         throw runtime_error(
             "signrawtransaction \"hexstring\" ( [{\"txid\":\"id\",\"vout\":n,\"scriptPubKey\":\"hex\",\"redeemScript\":\"hex\"},...] [\"privatekey1\",...] sighashtype )\n"
             "\nSign inputs for raw transaction (serialized, hex-encoded).\n"
@@ -750,6 +751,8 @@ UniValue signrawtransaction(const UniValue& params, bool fHelp)
             "       \"ALL|ANYONECANPAY\"\n"
             "       \"NONE|ANYONECANPAY\"\n"
             "       \"SINGLE|ANYONECANPAY\"\n"
+            "5.  \"branchid\"       (string, optional) The hex representation of the consensus branch id to sign with."
+            " This can be used to force signing with consensus rules that are ahead of the node's current height.\n"
 
             "\nResult:\n"
             "{\n"
@@ -777,7 +780,7 @@ UniValue signrawtransaction(const UniValue& params, bool fHelp)
 #else
     LOCK(cs_main);
 #endif
-    RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VARR)(UniValue::VARR)(UniValue::VSTR), true);
+    RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VARR)(UniValue::VARR)(UniValue::VSTR)(UniValue::VSTR), true);
 
     vector<unsigned char> txData(ParseHexV(params[0], "argument 1"));
     CDataStream ssData(txData, SER_NETWORK, PROTOCOL_VERSION);
@@ -913,9 +916,19 @@ UniValue signrawtransaction(const UniValue& params, bool fHelp)
     }
 
     bool fHashSingle = ((nHashType & ~SIGHASH_ANYONECANPAY) == SIGHASH_SINGLE);
-
+    // Use the approximate release height if it is greater so offline nodes 
+    // have a better estimation of the current height and will be more likely to
+    // determine the correct consensus branch ID.
+    int chainHeight = std::max(chainActive.Height() + 1, APPROX_RELEASE_HEIGHT);
     // Grab the current consensus branch ID
-    auto consensusBranchId = CurrentEpochBranchId(chainActive.Height() + 1, Params().GetConsensus());
+    auto consensusBranchId = CurrentEpochBranchId(chainHeight, Params().GetConsensus());
+
+    if (params.size() > 4 && !params[4].isNull()) {
+        consensusBranchId = ParseHexToUInt32(params[4].get_str());
+        if (!IsConsensusBranchId(consensusBranchId)) {
+            throw runtime_error(params[4].get_str() + " is not a valid consensus branch id");
+        }
+    } 
     
     // Script verification errors
     UniValue vErrors(UniValue::VARR);

src/test/rpc_tests.cpp

@@ -92,6 +92,8 @@ BOOST_AUTO_TEST_CASE(rpc_rawparams)
     BOOST_CHECK_NO_THROW(CallRPC(string("signrawtransaction ")+rawtx+" null null NONE|ANYONECANPAY"));
     BOOST_CHECK_NO_THROW(CallRPC(string("signrawtransaction ")+rawtx+" [] [] NONE|ANYONECANPAY"));
     BOOST_CHECK_THROW(CallRPC(string("signrawtransaction ")+rawtx+" null null badenum"), runtime_error);
+    BOOST_CHECK_NO_THROW(CallRPC(string("signrawtransaction ")+rawtx+" [] [] NONE|ANYONECANPAY 5ba81b19"));
+    BOOST_CHECK_THROW(CallRPC(string("signrawtransaction ")+rawtx+" [] [] ALL NONE|ANYONECANPAY 123abc"), runtime_error);
 
     // Only check failure cases for sendrawtransaction, there's no network to send to...
     BOOST_CHECK_THROW(CallRPC("sendrawtransaction"), runtime_error);