Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[RPC] Add utility getsignaturehash #11653

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
Member

NicolasDorier commented Nov 10, 2017

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.

src/rpc/misc.cpp
+ if (!request.params[5].isNull()) {
+ RPCTypeCheckArgument(request.params[5], UniValue::VSTR);
+ static std::map<std::string, int> mapSigHashValues = {
+ { std::string("ALL"), int(SIGHASH_ALL) },
@laanwj

laanwj Nov 10, 2017

Owner

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

Member

NicolasDorier commented Nov 10, 2017

Just refactored.

test/functional/getsignaturehash.py
+ self.setup_clean_chain = True
+
+ def run_test(self):
+ assert_equal("64e049981a10cf597406a175d7443c8499d308168915377637e3f706ce186137", self.nodes[0].getsignaturehash(
@MarcoFalke

MarcoFalke Nov 10, 2017

Member

No trailing white space, please.

Some comments.

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

src/rpc/misc.cpp
+ "\nCreate the hash to sign for the given input\n"
+
+ "\nArguments:\n"
+ "1. \"tx\" (string, required) The transaction hex string\n"
@promag

promag Nov 10, 2017

Contributor

Please align the descriptions.

src/rpc/misc.cpp
+ UniValue::VSTR, // amount
+ UniValue::VSTR, // sigVersion
+ UniValue::VNUM // inputIndex
+ },
@promag

promag Nov 10, 2017

Contributor

Wrong indentation?

src/rpc/misc.cpp
+ false);
+
+ CMutableTransaction mtx;
+ if (!DecodeHexTx(mtx, request.params[0].get_str(), true))
@promag

promag Nov 10, 2017

Contributor

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

src/rpc/server.cpp
@@ -131,6 +131,25 @@ uint256 ParseHashV(const UniValue& v, std::string strName)
result.SetHex(strHex);
return result;
}
+
+int ParseSigHash(std::string strHashType)
@promag

promag Nov 10, 2017

Contributor

const std::string& hash_type

src/rpc/server.cpp
+int ParseSigHash(std::string strHashType)
+{
+ int nHashType = 0;
+ static std::map<std::string, int> mapSigHashValues = {
@promag

promag Nov 10, 2017

Contributor

Should ANYONECANPAY|SINGLE be possible?

@promag

promag Nov 11, 2017

Contributor

Currently it's not.

src/rpc/server.cpp
+ { std::string("SINGLE|ANYONECANPAY"), int(SIGHASH_SINGLE | SIGHASH_ANYONECANPAY) },
+ };
+ if (mapSigHashValues.count(strHashType))
+ nHashType = mapSigHashValues[strHashType];
@promag

promag Nov 10, 2017

Contributor

Avoid 2nd lookup by using map::find.

src/rpc/server.cpp
+ if (mapSigHashValues.count(strHashType))
+ nHashType = mapSigHashValues[strHashType];
+ else
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid sighash param");
@promag

promag Nov 10, 2017

Contributor

For getsignaturehash the parameter name is sighashtype.

test/functional/getsignaturehash.py
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import *
+
+
@promag

promag Nov 10, 2017

Contributor

Nit, remove 2nd empty line.

test/functional/getsignaturehash.py
+ # optional
+ assert_equal("64e049981a10cf597406a175d7443c8499d308168915377637e3f706ce186137", self.nodes[0].getsignaturehash(
+ # tx
+ "010000000243ec7a579f5561a42a7e9637ad4156672735a658be2752181801f723ba3316d200000000844730440220449a203d0062ea01022f565c94c4b62bf2cc9a05de20519bc18cded9e99aa5f702201a2b8361e2af179eb93e697d9fedd0bf1e036f0a5be39af4b1f791df803bdb6501210363e38e2e0e55cdebfb7e27d0cb53ded150c320564a4614c18738feb124c8efd21976a9141a5fdcb6201f7e4fd160f9dca81075bd8537526088acffffffff43ec7a579f5561a42a7e9637ad4156672735a658be2752181801f723ba3316d20100000085483045022100ff6e7edffa5e0758244af6af77edd14f1d80226d66f81ed58dba2def34778236022057d95177497758e467c194f0d897f175645d30e7ac2f9490247e13227ac4d2fc01210363e38e2e0e55cdebfb7e27d0cb53ded150c320564a4614c18738feb124c8efd21976a9141a5fdcb6201f7e4fd160f9dca81075bd8537526088acffffffff0100752b7d000000001976a9141a5fdcb6201f7e4fd160f9dca81075bd8537526088ac00000000",
@promag

promag Nov 10, 2017

Contributor

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

test/functional/getsignaturehash.py
+ 0
+ ))
+
+
@promag

promag Nov 10, 2017

Contributor

Remove 2nd empty line.

src/rpc/misc.cpp
+
+ "\nArguments:\n"
+ "1. \"tx\" (string, required) The transaction hex string\n"
+ "2. \"scriptcode\" (string, required) The scriptCode to sign\n"
@luke-jr

luke-jr Nov 10, 2017

Member

This doesn't explain what it is.

@NicolasDorier

NicolasDorier Nov 11, 2017

Member

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.

src/rpc/misc.cpp
+ std::vector<unsigned char> scriptCodeData(ParseHexV(request.params[1], "scriptCode"));
+ CScript scriptCode(scriptCodeData.begin(), scriptCodeData.end());
+
+ auto amount = AmountFromValue(request.params[2].get_str());
@luke-jr

luke-jr Nov 10, 2017

Member

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

Member

luke-jr commented Nov 10, 2017

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

Contributor

promag commented Nov 10, 2017

@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.

Member

NicolasDorier commented Nov 11, 2017

@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.

Owner

sipa commented Nov 11, 2017

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

Member

NicolasDorier commented Nov 11, 2017

@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.

Owner

sipa commented Nov 11, 2017

@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.

Member

NicolasDorier commented Nov 11, 2017

@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.

Member

NicolasDorier commented Nov 11, 2017

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 added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

Member

NicolasDorier commented Nov 12, 2017

Alternative signinput: #11666

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment