[RPC] Add utility getsignaturehash #11653

NicolasDorier commented at 10:01 AM on November 10, 2017: contributor

Add an utility to get the signature hash in a generic way. signrawtransaction can't create a signature for an arbitrary scriptCode, as it is using ProduceSignature internally. This make it impossible to create complex scripts without a library.

fanquake added the label RPC/REST/ZMQ on Nov 10, 2017

in src/rpc/misc.cpp:386 in 8875b95bc4 outdated

 381 | +    int inputIndex = request.params[4].get_int();
 382 | +    int nHashType = SIGHASH_ALL;
 383 | +    if (!request.params[5].isNull()) {
 384 | +        RPCTypeCheckArgument(request.params[5], UniValue::VSTR);
 385 | +        static std::map<std::string, int> mapSigHashValues = {
 386 | +            { std::string("ALL"), int(SIGHASH_ALL) },

laanwj commented at 10:03 AM on November 10, 2017:

This table is duplicated with signrawtransaction, which makes it harder to change later. Please factor it out.

NicolasDorier force-pushed on Nov 10, 2017

NicolasDorier commented at 10:14 AM on November 10, 2017: contributor

Just refactored.

in test/functional/getsignaturehash.py:22 in 3f98c3c554 outdated

  17 | +    def set_test_params(self):
  18 | +        self.num_nodes = 1
  19 | +        self.setup_clean_chain = True
  20 | +
  21 | +    def run_test(self):
  22 | +        assert_equal("64e049981a10cf597406a175d7443c8499d308168915377637e3f706ce186137", self.nodes[0].getsignaturehash(

MarcoFalke commented at 3:48 PM on November 10, 2017:

No trailing white space, please.

in src/rpc/server.cpp:135 in 3f98c3c554 outdated

 130 | @@ -131,6 +131,25 @@ uint256 ParseHashV(const UniValue& v, std::string strName)
 131 |      result.SetHex(strHex);
 132 |      return result;
 133 |  }
 134 | +
 135 | +int ParseSigHash(std::string strHashType)

promag commented at 4:26 PM on November 10, 2017:

const std::string& hash_type

in src/rpc/server.cpp:147 in 3f98c3c554 outdated

 142 | +        { std::string("NONE|ANYONECANPAY"), int(SIGHASH_NONE | SIGHASH_ANYONECANPAY) },
 143 | +        { std::string("SINGLE"), int(SIGHASH_SINGLE) },
 144 | +        { std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE | SIGHASH_ANYONECANPAY) },
 145 | +    };
 146 | +    if (mapSigHashValues.count(strHashType))
 147 | +        nHashType = mapSigHashValues[strHashType];

promag commented at 4:27 PM on November 10, 2017:

Avoid 2nd lookup by using map::find.

in src/rpc/server.cpp:149 in 3f98c3c554 outdated

 144 | +        { std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE | SIGHASH_ANYONECANPAY) },
 145 | +    };
 146 | +    if (mapSigHashValues.count(strHashType))
 147 | +        nHashType = mapSigHashValues[strHashType];
 148 | +    else
 149 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid sighash param");

promag commented at 4:28 PM on November 10, 2017:

For getsignaturehash the parameter name is sighashtype.

in test/functional/getsignaturehash.py:15 in 3f98c3c554 outdated

  10 | +import time
  11 | +
  12 | +from test_framework.test_framework import BitcoinTestFramework
  13 | +from test_framework.util import *
  14 | +
  15 | +

promag commented at 4:30 PM on November 10, 2017:

Nit, remove 2nd empty line.

in test/functional/getsignaturehash.py:40 in 3f98c3c554 outdated

  35 | +        ))
  36 | +
  37 | +        # optional
  38 | +        assert_equal("64e049981a10cf597406a175d7443c8499d308168915377637e3f706ce186137", self.nodes[0].getsignaturehash( 
  39 | +            # tx
  40 | +            "010000000243ec7a579f5561a42a7e9637ad4156672735a658be2752181801f723ba3316d200000000844730440220449a203d0062ea01022f565c94c4b62bf2cc9a05de20519bc18cded9e99aa5f702201a2b8361e2af179eb93e697d9fedd0bf1e036f0a5be39af4b1f791df803bdb6501210363e38e2e0e55cdebfb7e27d0cb53ded150c320564a4614c18738feb124c8efd21976a9141a5fdcb6201f7e4fd160f9dca81075bd8537526088acffffffff43ec7a579f5561a42a7e9637ad4156672735a658be2752181801f723ba3316d20100000085483045022100ff6e7edffa5e0758244af6af77edd14f1d80226d66f81ed58dba2def34778236022057d95177497758e467c194f0d897f175645d30e7ac2f9490247e13227ac4d2fc01210363e38e2e0e55cdebfb7e27d0cb53ded150c320564a4614c18738feb124c8efd21976a9141a5fdcb6201f7e4fd160f9dca81075bd8537526088acffffffff0100752b7d000000001976a9141a5fdcb6201f7e4fd160f9dca81075bd8537526088ac00000000",

promag commented at 4:31 PM on November 10, 2017:

Declare tx only once, and at the same time these 2 asserts will be more readable.

in test/functional/getsignaturehash.py:46 in 3f98c3c554 outdated

  46 | +            "BASE",
  47 | +            # input
  48 | +            0
  49 | +        ))
  50 | +
  51 | +

promag commented at 4:32 PM on November 10, 2017:

Remove 2nd empty line.

in src/rpc/server.cpp:138 in 3f98c3c554 outdated

 130 | @@ -131,6 +131,25 @@ uint256 ParseHashV(const UniValue& v, std::string strName)
 131 |      result.SetHex(strHex);
 132 |      return result;
 133 |  }
 134 | +
 135 | +int ParseSigHash(std::string strHashType)
 136 | +{
 137 | +    int nHashType = 0;
 138 | +    static std::map<std::string, int> mapSigHashValues = {

promag commented at 4:35 PM on November 10, 2017:

Should ANYONECANPAY|SINGLE be possible?

NicolasDorier commented at 1:59 AM on November 11, 2017:

why not?

promag commented at 2:05 AM on November 11, 2017:

Currently it's not.

in src/rpc/misc.cpp:359 in 3f98c3c554 outdated

 354 | +        UniValue::VSTR, // tx
 355 | +        UniValue::VSTR, // scriptCode
 356 | +        UniValue::VSTR, // amount
 357 | +        UniValue::VSTR, // sigVersion
 358 | +        UniValue::VNUM // inputIndex
 359 | +    },

promag commented at 4:36 PM on November 10, 2017:

Wrong indentation?

in src/rpc/misc.cpp:363 in 3f98c3c554 outdated

 358 | +        UniValue::VNUM // inputIndex
 359 | +    },
 360 | +        false);
 361 | +
 362 | +    CMutableTransaction mtx;
 363 | +    if (!DecodeHexTx(mtx, request.params[0].get_str(), true))

promag commented at 4:38 PM on November 10, 2017:

Nit, there are a couple of {} missing, as per developer notes.

in src/rpc/misc.cpp:331 in 3f98c3c554 outdated

 326 | +        throw std::runtime_error(
 327 | +            "getsignaturehash \"tx\" \"scriptcode\" \"amount\" \"sigversion\" inputindex (\"sighashtype\")\n"
 328 | +            "\nCreate the hash to sign for the given input\n"
 329 | +
 330 | +            "\nArguments:\n"
 331 | +            "1. \"tx\"     (string, required) The transaction hex string\n"

promag commented at 4:41 PM on November 10, 2017:

Please align the descriptions.

promag commented at 4:41 PM on November 10, 2017: member

Some comments.

Missing tests for all sighashtype values and missing test for sigversion = WITNESS_V0?

in src/rpc/misc.cpp:371 in 3f98c3c554 outdated

 364 | +        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
 365 | +
 366 | +    std::vector<unsigned char> scriptCodeData(ParseHexV(request.params[1], "scriptCode"));
 367 | +    CScript scriptCode(scriptCodeData.begin(), scriptCodeData.end());
 368 | +
 369 | +    auto amount = AmountFromValue(request.params[2].get_str());

luke-jr commented at 4:43 PM on November 10, 2017:

New RPCs should use satoshis, especially for low-level stuff.

in src/rpc/misc.cpp:332 in 3f98c3c554 outdated

 327 | +            "getsignaturehash \"tx\" \"scriptcode\" \"amount\" \"sigversion\" inputindex (\"sighashtype\")\n"
 328 | +            "\nCreate the hash to sign for the given input\n"
 329 | +
 330 | +            "\nArguments:\n"
 331 | +            "1. \"tx\"     (string, required) The transaction hex string\n"
 332 | +            "2. \"scriptcode\"     (string, required) The scriptCode to sign\n"

luke-jr commented at 4:43 PM on November 10, 2017:

This doesn't explain what it is.

NicolasDorier commented at 2:16 AM on November 11, 2017:

the scriptCode is difficult to explain, it depends on OP_CODESEPARATOR and whether you are using Segwit/P2SH/Segwit-P2SH/Normal script. It will never fit in one line.

luke-jr commented at 4:47 PM on November 10, 2017: member

(Light concept NACK because utilities should be in libraries, not RPC.)

promag commented at 9:20 PM on November 10, 2017: member

@luke-jr I understand that but there is no harm in having these utilities:

they don't block, just spend a thread handling the request;
make the source stronger by using core code and having test coverage;
always updated response, libraries can have bugs or be outdated.

That said, most *rawtransactions are also utilities, and they are supported.

NicolasDorier commented at 1:43 AM on November 11, 2017: contributor

@luke-jr When you start doing services which span multiple blockchain (cross chain swap), then library can't possibly know how to sign each and every of them. While if each expose getsignaturehash it becomes possible. I bumped into the need of that as I was trying to do a cross chain swap between bitcoin and elements.

sipa commented at 1:45 AM on November 11, 2017: member

@NicolasDorier I don't see how that is related. You need some code specific for each chain anyway, no?

NicolasDorier commented at 1:48 AM on November 11, 2017: contributor

@sipa, not really. By using using decoderawtransaction, decodescript, createrawtransaction and all of those utilities, I can create transaction on two chains even if their binary representation of a transaction is completely different. Basically, this is one of the piece missing to have a generic code for all crypto forked from Bitcoin, which does not rely on a library knowing the details.

sipa commented at 2:00 AM on November 11, 2017: member

@NicolasDorier I don't understand, you can't just paste signature hashes into any other RPC, so it can't be a 'missing piece'. You still need to know what the chain's signatures look like (ECDSA, sighash flags, scripting system, ...) to use that information for anything. I expect that if not already, between any two interesting chains those things will differ significantly.

A more useful thing would be an RPC which you give an unsigned transaction, a key and/or an output being spent, possibly a scriptCode, and a sighash flag, and it gives you a piece of scriptSig/witness containing a valid signature for it. You'd still be responsible for combining it with other script elements. Even that isn't a missing piece IMHO, but it wouldn't need intricate knowledge of the digital signature system in your application.

NicolasDorier commented at 2:09 AM on November 11, 2017: contributor

@sipa there is indeed the creation of script that the library need. But the scripts are most likely similar accross all chains. Good point for the ECDSA. I thought they were not different, but they are: Bitcoin Cash is using another sighash. I agree, having a RPC method gettransactionsignature where you pass the private key be more generic. Will work on another PR for this as well.

NicolasDorier force-pushed on Nov 11, 2017

[RPC] Add utility getsignaturehash 0a688c4f61

NicolasDorier force-pushed on Nov 11, 2017

NicolasDorier commented at 3:14 AM on November 11, 2017: contributor

I addressed the nits, I keep this open while working on an alternative gettransactionsignature which accept a private key as well and returns the signature.

luke-jr referenced this in commit 4ac2c48b20 on Nov 11, 2017

NicolasDorier commented at 5:10 AM on November 12, 2017: contributor

Alternative signinput: #11666

NicolasDorier commented at 5:04 PM on March 6, 2018: contributor

supersede by #11666

NicolasDorier closed this on Mar 6, 2018

DrahtBot locked this on Sep 8, 2021