[RPC] Split signrawtransaction into wallet and non-wallet RPC command

achow101 commented at 10:28 pm on June 12, 2017: member

This PR is part of #10570. It also builds on top of #10571.

This PR splits signrawtransaction into two commands, signrawtransactionwithkey and signrawtransactionwithwallet. signrawtransactionwithkey requires private keys to be passed in and does not use the wallet for any signing. signrawtransactionwithwallet uses the wallet to sign a raw transaction and does not have any parameters to take private keys.

The signrawtransaction RPC has been marked as deprecated and will call the appropriate RPC command based upon the parameters given. A test was added to check this behavior is still consistent with the original behavior.

All tests that used signrawtransaction have been updated to use one of the two new RPCs. Most uses were changed to signrawtransactionwithwallet. These were changed via a scripted diff.

achow101 force-pushed on Jun 12, 2017

fanquake added the label RPC/REST/ZMQ on Jun 13, 2017

sipa commented at 1:15 am on June 13, 2017: member

Incorrect scripted diff.

achow101 force-pushed on Jun 13, 2017

jonasschnelli commented at 8:50 am on June 13, 2017: contributor

I’m not entirely sure if this is a good long term strategy. signrawtransactionwithwallet seems okay(ish) but I don’t see a reason to pass around a private key (though RPC, TCP into the same process that runs the p2p/validation/node).

Where are the differences betweenbitcoin-tx’s sign command and the here proposed signrawtransactionwithkey? Wouldn’t it make more sense to focus on bitcoin-tx for (offline) rawtx signing?

achow101 commented at 6:23 pm on June 13, 2017: member

@jonasschnelli signrawtransactionwithkey will lookup the UTXOs in order to sign whereas bitcoin-tx’s `sign’ command requires you to supply them. This is much easier to use.

achow101 force-pushed on Jun 13, 2017

sipa added the label Needs release notes on Jun 17, 2017

achow101 force-pushed on Jun 19, 2017

laanwj added this to the milestone 0.15.0 on Jul 6, 2017

achow101 force-pushed on Jul 7, 2017

achow101 force-pushed on Jul 8, 2017

achow101 force-pushed on Jul 17, 2017

laanwj commented at 3:21 pm on July 18, 2017: member

This has missed the 0.15 feature freeze, moving to 0.16.

laanwj added this to the milestone 0.16.0 on Jul 18, 2017

laanwj removed this from the milestone 0.15.0 on Jul 18, 2017

jnewbery commented at 2:33 pm on August 15, 2017: member

needs rebase

achow101 force-pushed on Aug 15, 2017

achow101 commented at 9:33 pm on August 15, 2017: member

rebased

achow101 force-pushed on Aug 15, 2017

in src/rpc/client.cpp:97 in 744c9a6a14 outdated

 94@@ -95,6 +95,9 @@ static const CRPCConvertParam vRPCConvertParams[] =
 95     { "createrawtransaction", 3, "replaceable" },
 96     { "signrawtransaction", 1, "prevtxs" },
 97     { "signrawtransaction", 2, "privkeys" },
 98+    { "signrawtransactionwithwallet", 1, "prevtxs" },
 99+    { "signrawtransactionwithkey", 2, "prevtxs" },
100+    { "signrawtransactionwithkey", 1, "privkeys" },

jnewbery commented at 6:49 pm on August 29, 2017:

supernit: 1 usually comes before 2 :)

promag commented at 8:44 pm on August 29, 2017:

@jnewbery for me it’s more “Sort please” :trollface:

in src/Makefile.am:135 in 744c9a6a14 outdated

131@@ -132,6 +132,7 @@ BITCOIN_CORE_H = \
132   rpc/protocol.h \
133   rpc/server.h \
134   rpc/register.h \
135+  rpc/rawtransaction.h \

jnewbery commented at 6:59 pm on August 29, 2017:

This should be in the first commit (6fded798a77e8a754fa9455afd3328aee0842274)

promag commented at 8:45 pm on August 29, 2017:

… and sorted.

instagibbs commented at 9:21 pm on February 8, 2018:

following lines aren’t sorted either, not sure if it matters to clean this up

in test/functional/txn_clone.py:81 in 744c9a6a14 outdated

73@@ -74,7 +74,7 @@ def run_test(self):
74 
75         # Use a different signature hash type to sign.  This creates an equivalent but malleated clone.
76         # Don't send the clone anywhere yet
77-        tx1_clone = self.nodes[0].signrawtransactionwithwallet(clone_raw, None, None, "ALL|ANYONECANPAY")
78+        tx1_clone = self.nodes[0].signrawtransactionwithwallet(clone_raw, None, "ALL|ANYONECANPAY")

jnewbery commented at 7:00 pm on August 29, 2017:

Can you make this change before the scripted diff commit (to not break git bisect). You could change the args to be named args while you’re doing that.

in test/functional/signrawtransactions.py:41 in 744c9a6a14 outdated

33@@ -34,7 +34,17 @@ def successful_signing_test(self):
34         outputs = {'mpLQjfK79b7CCV4VMJWEWAj5Mpx8Up5zxB': 0.1}
35 
36         rawTx = self.nodes[0].createrawtransaction(inputs, outputs)
37-        rawTxSigned = self.nodes[0].signrawtransactionwithkey(rawTx, inputs, privKeys)
38+        rawTxSigned = self.nodes[0].signrawtransactionwithkey(rawTx, privKeys, inputs)
39+
40+        # 1) The transaction has a complete set of signatures
41+        assert 'complete' in rawTxSigned
42+        assert_equal(rawTxSigned['complete'], True)

jnewbery commented at 7:01 pm on August 29, 2017:

no need for assert_equal(thing, True). Just use assert thing

in test/functional/signrawtransactions.py:128 in 744c9a6a14 outdated

119@@ -110,6 +120,32 @@ def script_verification_error_test(self):
120         assert_equal(rawTxSigned['errors'][1]['vout'], inputs[2]['vout'])
121         assert not rawTxSigned['errors'][0]['witness']
122 
123+        # Perform same test with signrawtransaction
124+        rawTxSigned = self.nodes[0].signrawtransaction(rawTx, scripts, privKeys)
125+
126+        # 3) The transaction has no complete set of signatures
127+        assert 'complete' in rawTxSigned
128+        assert_equal(rawTxSigned['complete'], False)

jnewbery commented at 7:01 pm on August 29, 2017:

just use assert not thing

in src/test/rpc_tests.cpp:72 in 744c9a6a14 outdated

66@@ -67,14 +67,6 @@ BOOST_AUTO_TEST_CASE(rpc_rawparams)
67     BOOST_CHECK_EQUAL(find_value(r.get_obj(), "locktime").get_int(), 0);
68     BOOST_CHECK_THROW(r = CallRPC(std::string("decoderawtransaction ")+rawtx+" extra"), std::runtime_error);
69 
70-    BOOST_CHECK_THROW(CallRPC("signrawtransaction"), std::runtime_error);

jnewbery commented at 7:03 pm on August 29, 2017:

Remove these tests before the first commit.

in src/wallet/rpcwallet.h:30 in 744c9a6a14 outdated

22@@ -22,4 +23,6 @@ std::string HelpRequiringPassphrase(CWallet *);
23 void EnsureWalletIsUnlocked(CWallet *);
24 bool EnsureWalletIsAvailable(CWallet *, bool avoidException);
25 
26+UniValue signrawtransactionwithwallet(const JSONRPCRequest& request);

jnewbery commented at 7:08 pm on August 29, 2017:

I have a slight personal preference for walletsignrawtransaction. 4 keystrokes saved :)

instagibbs commented at 9:42 pm on February 8, 2018:

Although it’s slightly more verbose, parallel structure with other version is nice.

keysignrawtransaction doesn’t quite roll off the tongue

in src/wallet/rpcwallet.cpp:3219 in 744c9a6a14 outdated

3215@@ -3147,6 +3216,7 @@ static const CRPCCommand commands[] =
3216 { //  category              name                        actor (function)           okSafeMode
3217     //  --------------------- ------------------------    -----------------------    ----------
3218     { "rawtransactions",    "fundrawtransaction",       &fundrawtransaction,       false,  {"hexstring","options"} },
3219+    { "rawtransactions",    "signrawtransactionwithwallet",       &signrawtransactionwithwallet,       false, {"hexstring","prevtxs","sighashtype"} },

jnewbery commented at 7:13 pm on August 29, 2017:

nit: alignment

jnewbery commented at 7:17 pm on August 29, 2017:

I think this should be “wallet” category.

in src/wallet/rpcwallet.cpp:3012 in 744c9a6a14 outdated

3007+
3008+    LOCK2(cs_main, pwallet->cs_wallet);
3009+    RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VARR, UniValue::VSTR}, true);
3010+
3011+    CMutableTransaction mtx;
3012+    if (!DecodeHexTx(mtx, request.params[0].get_str(), true))

jnewbery commented at 7:20 pm on August 29, 2017:

style nit: braces for if statements please

in src/rpc/rawtransaction.cpp:1051 in 744c9a6a14 outdated

1046@@ -970,7 +1047,8 @@ static const CRPCCommand commands[] =
1047     { "rawtransactions",    "decodescript",           &decodescript,           true,  {"hexstring"} },
1048     { "rawtransactions",    "sendrawtransaction",     &sendrawtransaction,     false, {"hexstring","allowhighfees"} },
1049     { "rawtransactions",    "combinerawtransaction",  &combinerawtransaction,  true,  {"txs"} },
1050-    { "rawtransactions",    "signrawtransaction",     &signrawtransaction,     false, {"hexstring","prevtxs","privkeys","sighashtype"} }, /* uses wallet if enabled */
1051+    { "rawtransactions",    "signrawtransaction",     &signrawtransaction,     true, {"hexstring","prevtxs","privkeys","sighashtype"} },
1052+    { "rawtransactions",    "signrawtransactionwithkey", &signrawtransactionwithkey, true, {"hexstring","privkeys","prevtxs","sighashtype"} },

jnewbery commented at 7:21 pm on August 29, 2017:

nit: alignment

in src/rpc/rawtransaction.cpp:650 in 744c9a6a14 outdated

720-
721     // Fetch previous transactions (inputs):
722     CCoinsView viewDummy;
723     CCoinsViewCache view(&viewDummy);
724     {
725+        LOCK(cs_main);

jnewbery commented at 7:24 pm on August 29, 2017:

This function is called by signrawtransactionwithwallet which calls LOCK2(cs_main, pwallet->cs_wallet);. Is it ok to call LOCK(main) on top of that?

If not, you’ll need to move this LOCK(main) into the calling signrawtransactionwithkey() function.

If it’s ok to call locks this way, then you can change this for LOCK2(cs_main, mempool.cs)

achow101 commented at 10:57 pm on August 29, 2017:

since cs_main isn’t necessary in signrawtransactionwithwallet, I’ve removed it.

achow101 commented at 11:27 pm on August 29, 2017:

Actually it is needed, but locking twice in the same thread is fine since it is a recursive_mutex.

jnewbery commented at 2:53 pm on August 31, 2017:

looking at this some more, I don’t think cs_main is required here. You could turn this back into LOCK(mempool.cs) and change the lock in signrawtransactionwithwallet to LOCK(pwallet->cs_wallet)

achow101 commented at 2:56 pm on August 31, 2017:

@jnewbery that’s what I originally thought. Then when I removed it I got a bunch of AssertLockHeld problems that went away after I added it back in.

jnewbery commented at 4:10 pm on August 31, 2017:

You’re right. Suhas points out to me that pcoinsTip requires cs_main. This should be left as LOCK2(cs_main, mempool.cs)

in src/rpc/rawtransaction.cpp:944 in 744c9a6a14 outdated

939+    JSONRPCRequest newRequest;
940+    newRequest.id = request.id;
941+    newRequest.params.setArray();
942+
943+    // For signing with private keys
944+    if (request.params.size() > 2 && !request.params[2].isNull()) {

jnewbery commented at 7:29 pm on August 29, 2017:

I don’t think request.params.size() > 2 is required. Univalue does the bounds checking for you.

in src/rpc/rawtransaction.cpp:953 in 744c9a6a14 outdated

948+        newRequest.params.push_back(request.params[3]);
949+        return signrawtransactionwithkey(newRequest);
950+    }
951+    // Otherwise sign with the wallet
952+#ifdef ENABLE_WALLET
953+    else if (request.params[2].isNull()) {

jnewbery commented at 7:31 pm on August 29, 2017:

This should become an else, and remove the else below (if we drop down that far then we’ve failed and should throw the "No private keys available." error

in src/rpc/rawtransaction.cpp:946 in 744c9a6a14 outdated

941+    newRequest.params.setArray();
942+
943+    // For signing with private keys
944+    if (request.params.size() > 2 && !request.params[2].isNull()) {
945+        newRequest.params.push_back(request.params[0]);
946+        newRequest.params.push_back(request.params[2]);

jnewbery commented at 7:33 pm on August 29, 2017:

suggestion: add comment here to say that order of prevtxs and privkeys is reversed in signrawtranactionwithkey

in src/rpc/rawtransaction.cpp:955 in 744c9a6a14 outdated

950+    }
951+    // Otherwise sign with the wallet
952+#ifdef ENABLE_WALLET
953+    else if (request.params[2].isNull()) {
954+        newRequest.params.push_back(request.params[0]);
955+        newRequest.params.push_back(request.params[1]);

jnewbery commented at 7:33 pm on August 29, 2017:

suggestion: add a comment here to say that signrawtransactionwithwallet doesn’t take a privkeys parameter.

in src/rpc/rawtransaction.cpp:913 in 744c9a6a14 outdated

866+    }
867+
868+    return signtransaction(mtx, request.params[2], &keystore, true, request.params[3]);
869+}
870+
871+UniValue signrawtransaction(const JSONRPCRequest& request)

jnewbery commented at 7:43 pm on August 29, 2017:

Perhaps hide this RPC behind a deprecated command line argument (similar to #11031)

in src/rpc/rawtransaction.cpp:851 in 744c9a6a14 outdated

846+        );
847+
848+    RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VARR, UniValue::VARR, UniValue::VSTR}, true);
849+
850+    CMutableTransaction mtx;
851+    if (!DecodeHexTx(mtx, request.params[0].get_str(), true))

jnewbery commented at 7:45 pm on August 29, 2017:

nit: I know these are moves, but there’s so few of them that you might as well add braces to the ifs.

in src/rpc/rawtransaction.cpp:939 in 744c9a6a14 outdated

934+        );
935+
936+    RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VARR, UniValue::VARR, UniValue::VSTR}, true);
937+
938+    // Make a JSONRPCRequest to pass on to the right signrawtransaction* command
939+    JSONRPCRequest newRequest;

jnewbery commented at 7:46 pm on August 29, 2017:

nit: use snake_case for variables in new functions.

in src/rpc/rawtransaction.cpp:644 in 744c9a6a14 outdated

640@@ -641,86 +641,13 @@ UniValue combinerawtransaction(const JSONRPCRequest& request)
641     return EncodeHexTx(mergedTx);
642 }
643 
644-UniValue signrawtransaction(const JSONRPCRequest& request)
645+UniValue signtransaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool tempKeystore, const UniValue& hashType)

jnewbery commented at 8:00 pm on August 29, 2017:

nit: new function parameters should be snake_case

jnewbery changes_requested

jnewbery commented at 8:40 pm on August 29, 2017: member

Some review comments inline. It’s a shame that this doesn’t remove the server->wallet dependency, but it’s a good first step in that direction.

I felt bad about asking you to rebase and then not actually reviewing before it got stale again, so I rebased it myself here: https://github.com/jnewbery/bitcoin/tree/pr10579 . Feel free to grab that branch if it helps.

in test/functional/signrawtransactions.py:40 in 744c9a6a14 outdated

33@@ -34,6 +34,16 @@ def successful_signing_test(self):
34         outputs = {'mpLQjfK79b7CCV4VMJWEWAj5Mpx8Up5zxB': 0.1}
35 
36         rawTx = self.nodes[0].createrawtransaction(inputs, outputs)
37+        rawTxSigned = self.nodes[0].signrawtransactionwithkey(rawTx, privKeys, inputs)
38+
39+        # 1) The transaction has a complete set of signatures
40+        assert 'complete' in rawTxSigned

promag commented at 8:53 pm on August 29, 2017:

Remove key asserts since below you assert the value, which fail in case the key is not defined? cc @jnewbery.

MarcoFalke commented at 6:18 am on August 30, 2017:

Right. The line below should raise KeyError in that case.

in src/rpc/rawtransaction.h:13 in 744c9a6a14 outdated

 8+class CBasicKeyStore;
 9+
10+/** Sign a transaction with the given keystore and previous transactions */
11+UniValue signtransaction(CMutableTransaction& mtx, const UniValue& prevTxs, CBasicKeyStore *keystore, bool tempKeystore, const UniValue& hashType);
12+
13+#endif

promag commented at 9:02 pm on August 29, 2017:

Missing comment.

achow101 commented at 11:06 pm on August 29, 2017:

Huh?

promag commented at 11:13 pm on August 29, 2017:

0#endif // BITCOIN_RPC_RAWTRANSACTION_H

in src/rpc/rawtransaction.h:8 in 744c9a6a14 outdated

0@@ -0,0 +1,14 @@
1+// Copyright (c) 2017 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_RPC_RAWTRANSACTION_H
6+#define BITCOIN_RPC_RAWTRANSACTION_H
7+
8+class CBasicKeyStore;

promag commented at 9:03 pm on August 29, 2017:

Unknown types below: CMutableTransaction and UniValue.

jnewbery commented at 4:17 pm on August 31, 2017:

Still not addressed in f5a3e0d4a3b4b359714b7d380d7784b7ca0524c0

achow101 commented at 5:46 pm on August 31, 2017:

Done

promag commented at 8:52 am on September 28, 2017:

Still missing?

achow101 commented at 4:34 pm on September 28, 2017:

Hmm. It must have gotten lost somewhere.

in src/wallet/rpcwallet.cpp:20 in 744c9a6a14 outdated

17@@ -18,6 +18,7 @@
18 #include "policy/rbf.h"
19 #include "rpc/mining.h"
20 #include "rpc/server.h"
21+#include "rpc/rawtransaction.h"

promag commented at 9:03 pm on August 29, 2017:

Sort.

in src/rpc/rawtransaction.cpp:791 in 744c9a6a14 outdated

785@@ -888,6 +786,185 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
786     return result;
787 }
788 
789+UniValue signrawtransactionwithkey(const JSONRPCRequest& request)
790+{
791+

promag commented at 9:05 pm on August 29, 2017:

Remove newline.

in src/rpc/rawtransaction.cpp:856 in 744c9a6a14 outdated

851+    if (!DecodeHexTx(mtx, request.params[0].get_str(), true))
852+        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
853+
854+    CBasicKeyStore keystore;
855+    UniValue keys = request.params[1].get_array();
856+    for (unsigned int idx = 0; idx < keys.size(); idx++) {

promag commented at 9:06 pm on August 29, 2017:

++idx.

in src/rpc/rawtransaction.cpp:863 in 744c9a6a14 outdated

858+        CBitcoinSecret vchSecret;
859+        bool fGood = vchSecret.SetString(k.get_str());
860+        if (!fGood)
861+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key");
862+        CKey key = vchSecret.GetKey();
863+        if (!key.IsValid())

promag commented at 9:08 pm on August 29, 2017:

Braces.

in src/rpc/rawtransaction.cpp:860 in 744c9a6a14 outdated

855+    UniValue keys = request.params[1].get_array();
856+    for (unsigned int idx = 0; idx < keys.size(); idx++) {
857+        UniValue k = keys[idx];
858+        CBitcoinSecret vchSecret;
859+        bool fGood = vchSecret.SetString(k.get_str());
860+        if (!fGood)

promag commented at 9:08 pm on August 29, 2017:

Take the opportunity to kill fGood:

0if (!vchSecret.SetString(k.get_str())) {

in src/rpc/rawtransaction.cpp:960 in 744c9a6a14 outdated

955+        newRequest.params.push_back(request.params[1]);
956+        newRequest.params.push_back(request.params[3]);
957+        return signrawtransactionwithwallet(newRequest);
958+    }
959+#endif
960+    else

promag commented at 9:11 pm on August 29, 2017:

0else {

in src/rpc/rawtransaction.cpp:1050 in 744c9a6a14 outdated

1046@@ -970,7 +1047,8 @@ static const CRPCCommand commands[] =
1047     { "rawtransactions",    "decodescript",           &decodescript,           true,  {"hexstring"} },
1048     { "rawtransactions",    "sendrawtransaction",     &sendrawtransaction,     false, {"hexstring","allowhighfees"} },
1049     { "rawtransactions",    "combinerawtransaction",  &combinerawtransaction,  true,  {"txs"} },
1050-    { "rawtransactions",    "signrawtransaction",     &signrawtransaction,     false, {"hexstring","prevtxs","privkeys","sighashtype"} }, /* uses wallet if enabled */
1051+    { "rawtransactions",    "signrawtransaction",     &signrawtransaction,     true, {"hexstring","prevtxs","privkeys","sighashtype"} },

promag commented at 9:12 pm on August 29, 2017:

Ops, align this and above lines? 😞

achow101 force-pushed on Aug 29, 2017

achow101 commented at 11:52 pm on August 29, 2017: member

Rebased and addressed many comments

achow101 force-pushed on Aug 29, 2017

achow101 force-pushed on Aug 30, 2017

in test/functional/txn_clone.py:157 in 4f64ef9064 outdated

153@@ -154,5 +154,4 @@ def run_test(self):
154         assert_equal(self.nodes[1].getbalance("from0", 0), -(tx1["amount"] + tx2["amount"]))
155 
156 if __name__ == '__main__':
157-    TxnMallTest().main()
158-
159+    TxnMallTest().main()

MarcoFalke commented at 6:17 am on August 30, 2017:

Whitespace issue causing the scripted diff to fail

in test/functional/signrawtransactions.py:101 in 4f64ef9064 outdated

104+        # 4) Two script verification errors occurred
105+        assert 'errors' in rawTxSigned
106+        assert_equal(len(rawTxSigned['errors']), 2)
107+
108+        # 5) Script verification errors have certain properties
109+        assert 'txid' in rawTxSigned['errors'][0]

jnewbery commented at 1:56 pm on August 30, 2017:

As above, you don’t need to test presence of these keys if you’re going to test the values below. Recommend you flip # 5 and # 6 sections, and only test presence of keys where you haven’t tested the values.

achow101 commented at 7:17 pm on August 30, 2017:

These were copied from a previously existing test.

jnewbery commented at 4:17 pm on August 31, 2017:

ok, I don’t think there’s any need to copy bad patterns, but I’m not too concerned either way.

promag commented at 4:25 pm on August 31, 2017:

Agree with @jnewbery.

in src/rpc/rawtransaction.h:13 in 4f64ef9064 outdated

 8+class CBasicKeyStore;
 9+
10+/** Sign a transaction with the given keystore and previous transactions */
11+UniValue sign_transaction(CMutableTransaction& mtx, const UniValue& prevTxs, CBasicKeyStore *keystore, bool tempKeystore, const UniValue& hashType);
12+
13+#endif

promag commented at 2:02 pm on August 30, 2017:

Missing comment

0#endif // BITCOIN_RPC_RAWTRANSACTION_H

achow101 force-pushed on Aug 30, 2017

in src/wallet/rpcwallet.cpp:3296 in f5a3e0d4a3 outdated

3018+            "\nExamples:\n"
3019+            + HelpExampleCli("signrawtransactionwithwallet", "\"myhex\"")
3020+            + HelpExampleRpc("signrawtransactionwithwallet", "\"myhex\"")
3021+        );
3022+
3023+    LOCK2(cs_main, pwallet->cs_wallet);

jnewbery commented at 4:19 pm on August 31, 2017:

suggest you move this lock down to immediately before the call to sign_transaction(). No need to hold the lock if the parameters are invalid and we’re just going to throw.

achow101 commented at 5:46 pm on August 31, 2017:

Done

instagibbs commented at 9:41 pm on February 8, 2018:

I don’t see the changes suggested?

achow101 commented at 7:53 pm on February 16, 2018:

It probably got lost in one of the billion rebases. I added it back in.

in src/wallet/rpcwallet.cpp:3235 in f5a3e0d4a3 outdated

3231@@ -3162,6 +3232,7 @@ static const CRPCCommand commands[] =
3232 { //  category              name                        actor (function)           okSafeMode
3233     //  --------------------- ------------------------    -----------------------    ----------
3234     { "rawtransactions",    "fundrawtransaction",       &fundrawtransaction,       false,  {"hexstring","options"} },
3235+    { "wallet",             "signrawtransactionwithwallet",       &signrawtransactionwithwallet,       false, {"hexstring","prevtxs","sighashtype"} },

jnewbery commented at 4:19 pm on August 31, 2017:

alignment

achow101 commented at 5:06 pm on August 31, 2017:

Aligning this is annoying to do

jnewbery commented at 5:20 pm on August 31, 2017:

why? Can be done in a whitespace only commit at the end if you’re worried about the diff

achow101 commented at 5:46 pm on August 31, 2017:

Done

in src/rpc/rawtransaction.cpp:977 in f5a3e0d4a3 outdated

960+    }
961+#endif
962+    // If we have made it this far, then wallet is disabled and no private keys were given, so fail here.
963+    throw JSONRPCError(RPC_INVALID_PARAMETER, "No private keys available.");
964+
965+    return NullUniValue;

jnewbery commented at 4:21 pm on August 31, 2017:

I don’t think this is required (we can never get this far since we throw two lines above.

achow101 commented at 5:46 pm on August 31, 2017:

Done

jnewbery commented at 2:07 pm on September 27, 2017:

still not removed in ac4f385

achow101 commented at 3:23 pm on September 27, 2017:

I thought I did that already.. Should be done now.

jnewbery commented at 4:43 pm on August 31, 2017: member

A few more nits inline. I’m still not entirely convinced that this is the best approach. Would it be better to have a signrawtransaction RPC in the wallet and a different signrawtransaction RPC in the node? If signrawtransaction is called on the node without a prevkeys argument, then an error is returned telling the user to call signrawtransaction on the wallet endpoint.

That would be a breaking API change, but we need to break the API at some point (to disallow calling signrawtransaction without keys). Having different signrawtransaction RPCs for wallet and node would mean that clients who call signrawtransaction with privkeys wouldn’t need to change at all, and clients who call signrawtransaction for the wallet would simply need to change the endpoint that they send the RPC to. This change will eventually require all users to change the method they call (and the order/number of parameters)

The downside is that having two different signrawtransaction RPCs could be confusing.

achow101 force-pushed on Aug 31, 2017

achow101 force-pushed on Sep 5, 2017

in test/functional/txn_clone.py:55 in f36d682822 outdated

44@@ -45,11 +45,11 @@ def run_test(self):
45         # Coins are sent to node1_address
46         node1_address = self.nodes[1].getnewaddress("from0")
47 
48-        # Send tx1, and another transaction tx2 that won't be cloned

sipa commented at 10:05 pm on September 5, 2017:

This change line and a few others aren’t supposed to happen in a scripted diff.

achow101 commented at 11:53 pm on September 5, 2017:

Oops

achow101 force-pushed on Sep 5, 2017

achow101 force-pushed on Sep 26, 2017

achow101 commented at 8:36 pm on September 26, 2017: member

Rebased onto #11031 and put signrawtransaction behind IsDeprecatedRPCEnabled.

achow101 force-pushed on Sep 26, 2017

achow101 force-pushed on Sep 27, 2017

achow101 commented at 2:45 am on September 27, 2017: member

I don’t know why travis is failing here

in src/rpc/rawtransaction.cpp:830 in f786efcc73 outdated

792@@ -893,6 +793,190 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
793     return result;
794 }
795 
796+UniValue signrawtransactionwithkey(const JSONRPCRequest& request)

fanquake commented at 3:29 am on September 27, 2017:

Whitespace above this line seems to be causing the travis failure.

fanquake commented at 3:29 am on September 27, 2017: member

@achow101 Looks like the whitespace linter. Commented inline.

0diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
1@@ -895,0 +796,184 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
2+
3^---- failure generated from contrib/devtools/lint-whitespace.sh

achow101 force-pushed on Sep 27, 2017

achow101 commented at 3:40 am on September 27, 2017: member

@fanquake I figured that much. I think I found the line with the error and it is definitely not the one which the linter points to. That error message is super unhelpful.

fanquake commented at 6:49 am on September 27, 2017: member

@achow101 is that the first time you’ve seen the linter masking/hiding another error? We definitely don’t want a situation where the linter ends up being ignored because it’s triggering miscellaneously (the whitespace here wasn’t touched or introduced), when in actual fact it’s hiding errors.

in src/qt/rpcconsole.cpp:75 in ac4f385744 outdated

71@@ -72,6 +72,8 @@ const QStringList historyFilter = QStringList()
72     << "importmulti"
73     << "signmessagewithprivkey"
74     << "signrawtransaction"
75+    << "signrawtransactionwithwallet"

jnewbery commented at 2:04 pm on September 27, 2017:

signrawtransactionwithwallet doesn’t handle private keys, so I don’t think it needs to be included here.

achow101 commented at 3:23 pm on September 27, 2017:

Done.

achow101 commented at 2:17 pm on September 27, 2017: member

@fanquake Yes.

in test/functional/signrawtransactions.py:5 in ac4f385744 outdated

1@@ -2,7 +2,7 @@
2 # Copyright (c) 2015-2016 The Bitcoin Core developers
3 # Distributed under the MIT software license, see the accompanying
4 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
5-"""Test transaction signing using the signrawtransaction RPC."""
6+"""Test transaction signing using the signrawtransactionwithwallet RPC."""

jnewbery commented at 2:40 pm on September 27, 2017:

Test transaction signing using the signrawtransactionwithkey and signrawtransactionwithwallet RPCs.

achow101 commented at 3:23 pm on September 27, 2017:

Done.

in test/functional/signrawtransactions.py:37 in ac4f385744 outdated

33@@ -33,11 +34,19 @@ def successful_signing_test(self):
34         outputs = {'mpLQjfK79b7CCV4VMJWEWAj5Mpx8Up5zxB': 0.1}
35 
36         rawTx = self.nodes[0].createrawtransaction(inputs, outputs)
37+        rawTxSigned = self.nodes[0].signrawtransactionwithkey(rawTx, privKeys, inputs)

jnewbery commented at 2:49 pm on September 27, 2017:

I don’t think you need to repeat all these tests twice. Just add something like the following to the end of each section:

0rawTxSignedWithKey = self.nodes[0].signrawtransactionwithkey(rawTx, privKeys, inputs)
1assert_equal(rawTxSigned, rawTxSignedWithKey)

achow101 commented at 3:23 pm on September 27, 2017:

Done

achow101 force-pushed on Sep 27, 2017

achow101 commented at 3:24 pm on September 27, 2017: member

Rebased to master. Addressed @jnewbery’s comments.

achow101 force-pushed on Sep 27, 2017

jnewbery commented at 8:42 pm on September 27, 2017: member

scripted-diff failure

achow101 commented at 11:11 pm on September 27, 2017: member

scripted-diff failure

Doesn’t look like it…

jnewbery commented at 11:31 pm on September 27, 2017: member

Doesn’t look like it…

Sorry, I must’ve been seeing a cached result.

in src/rpc/rawtransaction.cpp:852 in a7c824076b outdated

813+            "    ]\n"
814+            "3. \"prevtxs\"       (string, optional) An json array of previous dependent transaction outputs\n"
815+            "     [               (json array of json objects, or 'null' if none provided)\n"
816+            "       {\n"
817+            "         \"txid\":\"id\",             (string, required) The transaction id\n"
818+            "         \"vout\":n,                  (numeric, required) The output number\n"

promag commented at 9:06 am on September 28, 2017:

Nit, fix alignment.

achow101 commented at 4:37 pm on September 28, 2017:

Alignment fixed

in src/rpc/rawtransaction.cpp:855 in a7c824076b outdated

816+            "       {\n"
817+            "         \"txid\":\"id\",             (string, required) The transaction id\n"
818+            "         \"vout\":n,                  (numeric, required) The output number\n"
819+            "         \"scriptPubKey\": \"hex\",   (string, required) script key\n"
820+            "         \"redeemScript\": \"hex\",   (string, required for P2SH or P2WSH) redeem script\n"
821+            "         \"amount\": value            (numeric, required) The amount spent\n"

promag commented at 9:06 am on September 28, 2017:

Nit, fix alignment.

in src/rpc/rawtransaction.cpp:862 in a7c824076b outdated

857+    if (!DecodeHexTx(mtx, request.params[0].get_str(), true)) {
858+        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
859+    }
860+
861+    CBasicKeyStore keystore;
862+    UniValue keys = request.params[1].get_array();

promag commented at 9:08 am on September 28, 2017:

0const UniValue& keys = ...

achow101 commented at 4:37 pm on September 28, 2017:

Done

in src/rpc/rawtransaction.cpp:936 in a7c824076b outdated

897+            "1. \"hexstring\"     (string, required) The transaction hex string\n"
898+            "2. \"prevtxs\"       (string, optional) An json array of previous dependent transaction outputs\n"
899+            "     [               (json array of json objects, or 'null' if none provided)\n"
900+            "       {\n"
901+            "         \"txid\":\"id\",             (string, required) The transaction id\n"
902+            "         \"vout\":n,                  (numeric, required) The output number\n"

promag commented at 9:09 am on September 28, 2017:

Fix alignment.

in src/rpc/rawtransaction.cpp:939 in a7c824076b outdated

900+            "       {\n"
901+            "         \"txid\":\"id\",             (string, required) The transaction id\n"
902+            "         \"vout\":n,                  (numeric, required) The output number\n"
903+            "         \"scriptPubKey\": \"hex\",   (string, required) script key\n"
904+            "         \"redeemScript\": \"hex\",   (string, required for P2SH or P2WSH) redeem script\n"
905+            "         \"amount\": value            (numeric, required) The amount spent\n"

promag commented at 9:09 am on September 28, 2017:

Fix alignment.

promag commented at 9:39 am on September 28, 2017: member

Some comments, I’ll test later today.

achow101 force-pushed on Sep 28, 2017

jnewbery commented at 6:01 pm on September 28, 2017: member

ACK 03ca3abb6963bbf621337f2de59407b6dfb6c8f0

achow101 force-pushed on Sep 29, 2017

achow101 commented at 6:36 pm on September 29, 2017: member

Rebased.

achow101 force-pushed on Sep 30, 2017

achow101 force-pushed on Oct 19, 2017

achow101 commented at 3:33 am on October 19, 2017: member

n’th rebase

achow101 force-pushed on Nov 30, 2017

achow101 commented at 5:19 pm on November 30, 2017: member

Rebased.

in src/wallet/rpcwallet.cpp:3600 in 3f4351dbd4 outdated

3562-    { "wallet",             "walletpassphrase",         &walletpassphrase,         {"passphrase","timeout"} },
3563-    { "wallet",             "removeprunedfunds",        &removeprunedfunds,        {"txid"} },
3564-    { "wallet",             "rescanblockchain",         &rescanblockchain,         {"start_height", "stop_height"} },
3565-
3566-    { "generating",         "generate",                 &generate,                 {"nblocks","maxtries"} },
3567+ { //  category              name                                actor (function)                argNames

jnewbery commented at 8:08 pm on December 1, 2017:

bad rebase: leading space character introduced.

achow101 commented at 7:58 pm on February 16, 2018:

Fixed.

jnewbery commented at 8:10 pm on December 1, 2017: member

utACK 3f4351dbd43a86c9197c30f13ce8b409c4a1548e

Confirm rebase is good except for a stray space character introduced.

in src/rpc/client.cpp:98 in 3f4351dbd4 outdated

94@@ -95,6 +95,9 @@ static const CRPCConvertParam vRPCConvertParams[] =
95     { "createrawtransaction", 3, "replaceable" },
96     { "signrawtransaction", 1, "prevtxs" },
97     { "signrawtransaction", 2, "privkeys" },
98+    { "signrawtransactionwithwallet", 1, "prevtxs" },

jnewbery commented at 6:20 pm on December 7, 2017:

nit: alphabetical sort please

achow101 commented at 7:57 pm on December 7, 2017:

Done.

in src/wallet/rpcwallet.cpp:3515 in 3f4351dbd4 outdated

3565-
3566-    { "generating",         "generate",                 &generate,                 {"nblocks","maxtries"} },
3567+ { //  category              name                                actor (function)                argNames
3568+    //  --------------------- ------------------------          -----------------------         ----------
3569+    { "rawtransactions",    "fundrawtransaction",               &fundrawtransaction,            {"hexstring","options"} },
3570+    { "wallet",             "signrawtransactionwithwallet",     &signrawtransactionwithwallet,  {"hexstring","prevtxs","sighashtype"} },

jnewbery commented at 6:21 pm on December 7, 2017:

nit: alphabetical sort please

achow101 commented at 7:57 pm on December 7, 2017:

Done.

achow101 force-pushed on Dec 7, 2017

achow101 commented at 7:57 pm on December 7, 2017: member

Addressed @jnewbery’s nits and rebased.

achow101 force-pushed on Dec 11, 2017

achow101 commented at 6:42 pm on December 11, 2017: member

Fixed that test failure.

achow101 force-pushed on Jan 10, 2018

achow101 commented at 7:25 pm on January 10, 2018: member

Rebased

laanwj removed this from the milestone 0.16.0 on Jan 11, 2018

laanwj added this to the milestone 0.17.0 on Jan 11, 2018

achow101 force-pushed on Jan 11, 2018

achow101 commented at 7:26 pm on January 11, 2018: member

Rebased

achow101 force-pushed on Feb 1, 2018

achow101 commented at 9:08 pm on February 1, 2018: member

Rebased

achow101 force-pushed on Feb 1, 2018

achow101 force-pushed on Feb 8, 2018

achow101 commented at 8:43 pm on February 8, 2018: member

Rebased yet again

jnewbery commented at 7:39 pm on February 16, 2018: member

Needed rebase again. Rebased here: https://github.com/jnewbery/bitcoin/tree/pr10579_rebased_again_so_achow_doesnt_want_to_kill_himself . You’re welcome :)

achow101 force-pushed on Feb 16, 2018

achow101 commented at 7:53 pm on February 16, 2018: member

Thanks @jnewbery for the rebase!

achow101 force-pushed on Feb 16, 2018

in src/rpc/rawtransaction.cpp:675 in 8c95521494 outdated

671@@ -672,88 +672,13 @@ UniValue combinerawtransaction(const JSONRPCRequest& request)
672     return EncodeHexTx(mergedTx);
673 }
674 
675-UniValue signrawtransaction(const JSONRPCRequest& request)
676+UniValue sign_transaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool tempKeystore, const UniValue& hashType)

instagibbs commented at 9:08 pm on February 16, 2018:

non blocking nit: if this stuff is being renamed we should follow snake case

achow101 commented at 9:59 pm on February 16, 2018:

I believe you meant CamelCase, not snake_case. I changed it to CanelCase.

instagibbs commented at 10:05 pm on February 16, 2018:

referring to “variable and namespace names are all lowercase, and may use _ to separate words (snake_case).” in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

achow101 commented at 10:20 pm on February 16, 2018:

Ah. Most of these aren’t renamed (I think).

jnewbery commented at 10:24 pm on February 16, 2018:

Please no more renames! :)

in src/rpc/rawtransaction.cpp:742 in 8c95521494 outdated

737@@ -834,8 +738,8 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
738             }
739 
740             // if redeemScript given and not using the local wallet (private keys
741-            // given), add redeemScript to the tempKeystore so it can be signed:
742-            if (fGivenKeys && (scriptPubKey.IsPayToScriptHash() || scriptPubKey.IsPayToWitnessScriptHash())) {
743+            // given), add redeemScript to the keystore so it can be signed:
744+            if (tempKeystore && (scriptPubKey.IsPayToScriptHash() || scriptPubKey.IsPayToWitnessScriptHash())) {

instagibbs commented at 9:11 pm on February 16, 2018:

this is a bit confusing since previously tempKeystore was an actual keystore, now it’s a bool. is_temp_keystore or something please

achow101 commented at 9:59 pm on February 16, 2018:

Done

instagibbs commented at 9:26 pm on February 16, 2018: member

looking pretty good, will do some testing

achow101 force-pushed on Feb 16, 2018

jnewbery commented at 10:09 pm on February 16, 2018: member

reACK 3808129837f12482fcadd030d4e720ff2498b452. Only changes are:

renaming suggested by instagibbs
removing stray space character
moving LOCK2() down a few lines in signrawtransactionwithwallet()

instagibbs commented at 10:47 pm on February 16, 2018: member

Not pushing! :)

On Fri, Feb 16, 2018, 5:25 PM John Newbery notifications@github.com wrote:

@jnewbery commented on this pull request.

In src/rpc/rawtransaction.cpp https://github.com/bitcoin/bitcoin/pull/10579#discussion_r168885756:

@@ -672,88 +672,13 @@ UniValue combinerawtransaction(const JSONRPCRequest& request) return EncodeHexTx(mergedTx); }

-UniValue signrawtransaction(const JSONRPCRequest& request) +UniValue sign_transaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool tempKeystore, const UniValue& hashType)

Please no more renames! :)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10579#discussion_r168885756, or mute the thread https://github.com/notifications/unsubscribe-auth/AFgC0xdQOxI6frA4Q39cT1VlA7ZPKX9vks5tVgBlgaJpZM4N3w_N .

Split signrawtransaction into wallet and non-wallet

Splits signrwatransaction into a wallet version (signrawtransactionwithwallet) and
non-wallet version (signrawtransactionwithkey). signrawtransaction is marked as DEPRECATED
and will call the right signrawtransaction* command as per the parameters in order to
maintain compatibility.

Updated signrawtransactions test to use new RPCs

1e79c055cd

scripted-diff: change signrawtransaction to signrawtransactionwithwallet in tests

-BEGIN VERIFY SCRIPT-
sed -i 's/\<signrawtransaction\>/signrawtransactionwithwallet/g' test/functional/*.py
sed -i 's/\<signrawtransaction\>/signrawtransactionwithwallet/g' test/functional/test_framework/*.py
-END VERIFY SCRIPT-

eefff65a4b

Add test for signrawtransaction

Add a brief test for signrawtransaction to ensure that compatibility is maintained.

d60234885b

achow101 force-pushed on Feb 17, 2018

achow101 commented at 5:06 pm on February 17, 2018: member

Rebased again.

instagibbs commented at 11:34 pm on February 17, 2018: member

re-utACK https://github.com/bitcoin/bitcoin/pull/10579/commits/d60234885bcc07b1a7f85ded7731549ec2fcfefa

in src/rpc/rawtransaction.h:9 in d60234885b

0@@ -0,0 +1,15 @@
1+// Copyright (c) 2017 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_RPC_RAWTRANSACTION_H
6+#define BITCOIN_RPC_RAWTRANSACTION_H
7+
8+class CBasicKeyStore;
9+class CMutableTransaction;

promag commented at 0:40 am on February 18, 2018:

Nit, gives compiler warning:

 0In file included from rpc/rawtransaction.cpp:20:
 1./rpc/rawtransaction.h:9:1: warning: class 'CMutableTransaction' was previously declared as a struct [-Wmismatched-tags]
 2class CMutableTransaction;
 3^
 4./primitives/transaction.h:362:8: note: previous use is here
 5struct CMutableTransaction
 6       ^
 7./rpc/rawtransaction.h:9:1: note: did you mean struct here?
 8class CMutableTransaction;
 9^~~~~
10struct

achow101 commented at 2:27 am on February 20, 2018:

I don’t see this on my machine..

promag commented at 0:42 am on February 18, 2018: member

utACK d602348.

jnewbery commented at 9:37 pm on February 19, 2018: member

reACK d60234885bcc07b1a7f85ded7731549ec2fcfefa

sipa commented at 2:25 am on February 20, 2018: member

utACK d60234885bcc07b1a7f85ded7731549ec2fcfefa

sipa merged this on Feb 20, 2018

sipa closed this on Feb 20, 2018

sipa referenced this in commit ffc6e48b29 on Feb 20, 2018

MarcoFalke referenced this in commit e4ffcacc21 on Feb 21, 2018

moodmosaic referenced this in commit 0064a138d2 on Nov 28, 2018

fanquake removed the label Needs release note on Mar 20, 2019

PastaPastaPasta referenced this in commit 3f66e7da20 on Dec 11, 2020

PastaPastaPasta referenced this in commit a320bf975e on Dec 11, 2020

PastaPastaPasta referenced this in commit fc96339839 on Dec 12, 2020

PastaPastaPasta referenced this in commit 0d87d008d8 on Dec 12, 2020

PastaPastaPasta referenced this in commit 38ca3c214e on Dec 12, 2020

PastaPastaPasta referenced this in commit 71d65d0a34 on Dec 12, 2020

PastaPastaPasta referenced this in commit af917fedae on Dec 12, 2020

PastaPastaPasta referenced this in commit b4f67a5e9e on Dec 15, 2020

PastaPastaPasta referenced this in commit f51d1683b6 on Dec 15, 2020

PastaPastaPasta referenced this in commit 5574b51570 on Dec 15, 2020

PastaPastaPasta referenced this in commit a2f107424b on Dec 15, 2020

PastaPastaPasta referenced this in commit 2bda82bdbe on Dec 15, 2020

PastaPastaPasta referenced this in commit d72f7c86f9 on Dec 15, 2020

PastaPastaPasta referenced this in commit d977daaff0 on Dec 18, 2020

PastaPastaPasta referenced this in commit 9436fa23d5 on Dec 18, 2020

PastaPastaPasta referenced this in commit 1d90698b6c on Dec 18, 2020

DrahtBot locked this on Dec 16, 2021

gades referenced this in commit 78b6718141 on Mar 22, 2022

[RPC] Split signrawtransaction into wallet and non-wallet RPC command #10579

@jnewbery commented on this pull request.