[RPC] Split signrawtransaction into wallet and non-wallet RPC command by achow101 · Pull Request #10579 · bitcoin/bitcoin

achow101 · 2017-06-12T22:28:00Z

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.

sipa · 2017-06-13T01:15:52Z

Incorrect scripted diff.

jonasschnelli · 2017-06-13T08:50:02Z

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 · 2017-06-13T18:23:35Z

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

laanwj · 2017-07-18T15:21:26Z

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

jnewbery · 2017-08-15T14:33:07Z

needs rebase

achow101 · 2017-08-15T21:33:43Z

rebased

jnewbery

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.

jnewbery · 2017-08-29T18:49:14Z

supernit: 1 usually comes before 2 :)

@jnewbery for me it's more "Sort please"

jnewbery · 2017-08-29T18:59:22Z

This should be in the first commit (6fded798a77e8a754fa9455afd3328aee0842274)

... and sorted.

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

jnewbery · 2017-08-29T19:00:29Z

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.

jnewbery · 2017-08-29T19:01:26Z

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

jnewbery · 2017-08-29T19:01:59Z

just use assert not thing

jnewbery · 2017-08-29T19:33:59Z

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

jnewbery · 2017-08-29T19:43:13Z

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

jnewbery · 2017-08-29T19:45:32Z

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

jnewbery · 2017-08-29T19:46:22Z

nit: use snake_case for variables in new functions.

jnewbery · 2017-08-29T20:00:25Z

nit: new function parameters should be snake_case

promag · 2017-08-29T20:53:03Z

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

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

promag · 2017-08-29T21:02:27Z

Missing comment.

#endif // BITCOIN_RPC_RAWTRANSACTION_H

promag · 2017-08-29T21:03:13Z

Unknown types below: CMutableTransaction and UniValue.

Still not addressed in f5a3e0d4a3b4b359714b7d380d7784b7ca0524c0

Still missing?

Hmm. It must have gotten lost somewhere.

promag · 2017-08-29T21:03:37Z

promag · 2017-08-29T21:05:52Z

Remove newline.

promag · 2017-08-29T21:06:29Z

promag · 2017-08-29T21:08:18Z

promag · 2017-08-29T21:08:19Z

Take the opportunity to kill fGood:

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

promag · 2017-08-29T21:11:04Z

promag · 2017-08-29T21:12:05Z

Ops, align this and above lines? 😞

jnewbery · 2018-02-16T22:09:12Z

reACK 3808129837f12482fcadd030d4e720ff2498b452. Only changes are:

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

instagibbs · 2018-02-16T22:47:03Z

Not pushing! :)

…

On Fri, Feb 16, 2018, 5:25 PM John Newbery ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/rpc/rawtransaction.cpp <#10579 (comment)>: > @@ -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 <#10579 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgC0xdQOxI6frA4Q39cT1VlA7ZPKX9vks5tVgBlgaJpZM4N3w_N> .

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

…let 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-

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

achow101 · 2018-02-17T17:06:58Z

Rebased again.

instagibbs · 2018-02-17T23:34:18Z

re-utACK d602348

promag

utACK d602348.

promag · 2018-02-18T00:40:50Z

+#define BITCOIN_RPC_RAWTRANSACTION_H
+
+class CBasicKeyStore;
+class CMutableTransaction;


Nit, gives compiler warning:

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

I don't see this on my machine..

jnewbery · 2018-02-19T21:37:55Z

reACK d602348

sipa · 2018-02-20T02:25:22Z

utACK d602348

…et RPC command d602348 Add test for signrawtransaction (Andrew Chow) eefff65 scripted-diff: change signrawtransaction to signrawtransactionwithwallet in tests (Andrew Chow) 1e79c05 Split signrawtransaction into wallet and non-wallet (Andrew Chow) Pull request description: 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. Tree-SHA512: d0adf5b4cd7077639c504ec07bee262a3b94658d34db0a5c86a263b6393f7aa62f45129eafe29a7c861aa58440dd19348ee0c8b685e8a62d6f4adae8ec8f8cb3

eacc5b2 Declare CMutableTransaction a struct in rawtransaction.h (Ben Woosley) Pull request description: Because it's a struct. Fix for #10579 - this was called out in code review. #10579 (comment) Tree-SHA512: 10758a667218481de6f50b5ed874e92eb350c621f7a6355fba7da6ab42b09e1764f827e89491c8663e554fcfd23f124b299f968237c6ad1ff7819e211bd7e521

See bitcoin/bitcoin#10579 See also - https://bitcoincore.org/en/doc/0.17.0/rpc/rawtransactions/signrawtransaction/ - https://bitcoincore.org/en/doc/0.17.0/rpc/rawtransactions/signrawtransactionwithkey/ - https://bitcoincore.org/en/doc/0.17.0/rpc/wallet/signrawtransactionwithwallet/

…on-wallet RPC command d602348 Add test for signrawtransaction (Andrew Chow) eefff65 scripted-diff: change signrawtransaction to signrawtransactionwithwallet in tests (Andrew Chow) 1e79c05 Split signrawtransaction into wallet and non-wallet (Andrew Chow) Pull request description: This PR is part of bitcoin#10570. It also builds on top of bitcoin#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. Tree-SHA512: d0adf5b4cd7077639c504ec07bee262a3b94658d34db0a5c86a263b6393f7aa62f45129eafe29a7c861aa58440dd19348ee0c8b685e8a62d6f4adae8ec8f8cb3

Signed-off-by: pasta <pasta@dashboost.org>

…action.h eacc5b2 Declare CMutableTransaction a struct in rawtransaction.h (Ben Woosley) Pull request description: Because it's a struct. Fix for bitcoin#10579 - this was called out in code review. bitcoin#10579 (comment) Tree-SHA512: 10758a667218481de6f50b5ed874e92eb350c621f7a6355fba7da6ab42b09e1764f827e89491c8663e554fcfd23f124b299f968237c6ad1ff7819e211bd7e521

achow101 mentioned this pull request Jun 12, 2017

[RPC] Split signrawtransaction into multiple distinct RPCs #10570

Closed

4 tasks

achow101 force-pushed the split-signraw branch from 89da21f to db216f8 Compare June 12, 2017 23:00

This was referenced Jun 12, 2017

Remaining instances of ENABLE_WALLET in libbitcoin_server.a #7965

Closed

Deprecate wallet-less signrawtransaction? #4844

Closed

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

achow101 force-pushed the split-signraw branch 2 times, most recently from 0b7c0c2 to 1283d87 Compare June 13, 2017 02:21

achow101 force-pushed the split-signraw branch from 1283d87 to 2d46b01 Compare June 13, 2017 18:51

sipa added the Needs release note label Jun 17, 2017

achow101 force-pushed the split-signraw branch 2 times, most recently from a1d577b to a15695a Compare June 19, 2017 18:16

laanwj added this to the 0.15.0 milestone Jul 6, 2017

jnewbery mentioned this pull request Jul 7, 2017

[wallet] Remove Wallet dependencies from init.cpp #10762

Merged

achow101 force-pushed the split-signraw branch 3 times, most recently from 7725585 to 05c3cf5 Compare July 8, 2017 00:36

achow101 force-pushed the split-signraw branch from 05c3cf5 to c1f9d3e Compare July 17, 2017 18:51

laanwj modified the milestones: 0.16.0, 0.15.0 Jul 18, 2017

achow101 force-pushed the split-signraw branch from 8674057 to aea7072 Compare August 15, 2017 21:33

achow101 force-pushed the split-signraw branch from aea7072 to 744c9a6 Compare August 15, 2017 22:16

jnewbery suggested changes Aug 29, 2017

View reviewed changes

promag reviewed Aug 29, 2017

View reviewed changes

achow101 force-pushed the split-signraw branch from 181dee9 to 3808129 Compare February 16, 2018 21:58

achow101 added 3 commits February 17, 2018 11:42

Add test for signrawtransaction

d602348

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

achow101 force-pushed the split-signraw branch from 3808129 to d602348 Compare February 17, 2018 17:06

promag reviewed Feb 18, 2018

View reviewed changes

sipa merged commit d602348 into bitcoin:master Feb 20, 2018

Empact mentioned this pull request Feb 20, 2018

Declare CMutableTransaction a struct in rawtransaction.h #12494

Merged

maflcko mentioned this pull request Feb 22, 2018

TODO for release notes 0.17.0 #12391

Closed

lautarodragan mentioned this pull request Oct 10, 2018

Deprecated signrawtransaction poetapp/node#535

Closed

fanquake removed the Needs release note label Mar 20, 2019

PastaPastaPasta mentioned this pull request Apr 14, 2020

Backports 0.16 pr17 dashpay/dash#3407

Merged

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Dec 12, 2020

Fix bitcoin#10579

71d65d0

Signed-off-by: pasta <pasta@dashboost.org>

Conversation

achow101 commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jun 13, 2017

Uh oh!

jonasschnelli commented Jun 13, 2017

Uh oh!

achow101 commented Jun 13, 2017

Uh oh!

laanwj commented Jul 18, 2017

Uh oh!

jnewbery commented Aug 15, 2017

Uh oh!

achow101 commented Aug 15, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Feb 16, 2018

achow101 commented Jun 12, 2017 •

edited

Loading

sipa commented Feb 20, 2018 •

edited

Loading