[RPC]Move transaction combining from signrawtransaction to new RPC #10571

achow101 · 2017-06-10T05:40:44Z

Create a combinerawtransaction RPC which accepts a json array of hex raw transactions to combine them into one transaction. Signrawtransaction is changed to no longer combine transactions and only accept one transaction at a time.

The tests have been updated to test this. Tests for the signrawtransaction merge have also been removed.

This is part of #10570

sipa · 2017-06-12T22:20:54Z

Concept ACK. However, this does break backward compatibility with an undocumented but apparently intentional and tested feature. Does this require deprecation first?

jnewbery · 2017-06-28T14:55:20Z

Concept feedback: combining transactions seems like a utility. Is it possible to add it to bitcoin-tx rather than add a utility-only RPC?

sipa · 2017-06-28T14:56:26Z

It is not a utility-only RPC. It needs access to the UTXO set.

sipa · 2017-07-15T02:10:13Z

utACK c710eb9eaffa50daddfedb50a93036085b3296c7

gmaxwell

ACK

morcos

utACK c710eb9

although at least the documentation in the RPC test should probably be corrected

morcos · 2017-07-17T16:01:44Z

src/rpc/rawtransaction.cpp

+
+    for (unsigned int idx = 0; idx < txs.size(); idx++) {
+        if (!DecodeHexTx(txVariants[idx], txs[idx].get_str(), true))
+            throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed for tx " + idx);


nit: braces

morcos · 2017-07-17T16:01:58Z

src/rpc/rawtransaction.cpp

+    }
+
+    if (txVariants.empty())
+        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Missing transactions");


nit: braces

morcos · 2017-07-17T18:16:42Z

test/functional/rawtransactions.py

+        self.sync_all()
+
+        #THIS IS A INCOMPLETE FEATURE
+        #NODE2 HAS TWO OF THREE KEY AND THE FUNDS SHOULD BE SPENDABLE AND COUNT AT BALANCE CALCULATION


This is copy-paste error leading to incorrect documentation of the test. This test is a 2of2 multisig for which node 2 has only 1 key.

achow101 · 2017-07-17T18:47:08Z

Nits addressed and rebased.

morcos · 2017-07-17T19:08:33Z

@achow101 It's better if you address the nits separately from rebasing (was rebasing needed?) otherwise its hard to review just the differential and reACK.

morcos · 2017-07-17T19:09:28Z

test/functional/rawtransactions.py

+        self.sync_all()
+
+        #THIS IS A INCOMPLETE FEATURE
+        #NODE2 HAS ONE OF TWO KEY AND THE FUNDS SHOULD BE SPENDABLE AND COUNT AT BALANCE CALCULATION


This comment is still incorrect. The funds should not be spendable there is nothing incomplete here.

achow101 · 2017-07-17T19:11:26Z

@morcos sorry, I rebased out of habit.

achow101 · 2017-07-17T19:16:39Z

Now the comment should be right.

morcos · 2017-07-17T19:31:03Z

heh thanks...
and i think its ok to squash, especially if the old commit can still be found, b/c then a diff btwn old and new can show the differences. it's the rebase that makes it difficult

utACK f293282

morcos · 2017-07-17T19:57:07Z

utACK 3cd7a5b

sipa · 2017-07-17T22:20:12Z

utACK 3cd7a5bc31e1a3cdd1cc740a884f69d9c6689e2f

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

src/rpc/rawtransaction.cpp

+
+    for (unsigned int idx = 0; idx < txs.size(); idx++) {
+        if (!DecodeHexTx(txVariants[idx], txs[idx].get_str(), true)) {
+            throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed for tx " + idx);


This won't work, you're adding a string and an integer. Use strprintf?

Create a combinerawtransaction RPC which accepts a json array of hex raw transactions to combine them into one transaction. Signrawtransaction is changed to no longer combine transactions and only accept one transaction at a time.

… to new RPC 6b4f231 Move transaction combining from signrawtransaction to new RPC (Andrew Chow) Pull request description: Create a combinerawtransaction RPC which accepts a json array of hex raw transactions to combine them into one transaction. Signrawtransaction is changed to no longer combine transactions and only accept one transaction at a time. The tests have been updated to test this. Tests for the signrawtransaction merge have also been removed. This is part of #10570 Tree-SHA512: 035aebbd6537c1c017d5c8e06d309228b4c23fe52d5b31ffde19741c81a11a6346ddbbdc582b77b02a47f4c22b1952b69d3c2ee1109c29b3f0f1b612d8de53ed

…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

…saction to new RPC 6b4f231 Move transaction combining from signrawtransaction to new RPC (Andrew Chow) Pull request description: Create a combinerawtransaction RPC which accepts a json array of hex raw transactions to combine them into one transaction. Signrawtransaction is changed to no longer combine transactions and only accept one transaction at a time. The tests have been updated to test this. Tests for the signrawtransaction merge have also been removed. This is part of bitcoin#10570 Tree-SHA512: 035aebbd6537c1c017d5c8e06d309228b4c23fe52d5b31ffde19741c81a11a6346ddbbdc582b77b02a47f4c22b1952b69d3c2ee1109c29b3f0f1b612d8de53ed Conflicts: src/rpc/client.cpp src/rpc/rawtransaction.cpp

…saction to new RPC 6b4f231 Move transaction combining from signrawtransaction to new RPC (Andrew Chow) Pull request description: Create a combinerawtransaction RPC which accepts a json array of hex raw transactions to combine them into one transaction. Signrawtransaction is changed to no longer combine transactions and only accept one transaction at a time. The tests have been updated to test this. Tests for the signrawtransaction merge have also been removed. This is part of bitcoin#10570 Tree-SHA512: 035aebbd6537c1c017d5c8e06d309228b4c23fe52d5b31ffde19741c81a11a6346ddbbdc582b77b02a47f4c22b1952b69d3c2ee1109c29b3f0f1b612d8de53ed

…nrawtransaction to new RPC" This reverts commit 6478562.

…saction to new RPC 6b4f231 Move transaction combining from signrawtransaction to new RPC (Andrew Chow) Pull request description: Create a combinerawtransaction RPC which accepts a json array of hex raw transactions to combine them into one transaction. Signrawtransaction is changed to no longer combine transactions and only accept one transaction at a time. The tests have been updated to test this. Tests for the signrawtransaction merge have also been removed. This is part of bitcoin#10570 Tree-SHA512: 035aebbd6537c1c017d5c8e06d309228b4c23fe52d5b31ffde19741c81a11a6346ddbbdc582b77b02a47f4c22b1952b69d3c2ee1109c29b3f0f1b612d8de53ed

…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

achow101 mentioned this pull request Jun 10, 2017

[RPC] Split signrawtransaction into multiple distinct RPCs #10570

Closed

4 tasks

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

achow101 force-pushed the combineraw-rpc branch 2 times, most recently from d342280 to 44fa920 Compare June 12, 2017 17:43

achow101 mentioned this pull request Jun 12, 2017

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

Merged

achow101 force-pushed the combineraw-rpc branch 2 times, most recently from f821255 to 5a4a819 Compare June 19, 2017 18:37

achow101 force-pushed the combineraw-rpc branch from 5a4a819 to 41806ba Compare June 30, 2017 17:58

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 combineraw-rpc branch 2 times, most recently from aeebcd0 to c710eb9 Compare July 7, 2017 23:37

gmaxwell approved these changes Jul 17, 2017

View reviewed changes

morcos approved these changes Jul 17, 2017

View reviewed changes

achow101 force-pushed the combineraw-rpc branch from c710eb9 to 1023869 Compare July 17, 2017 18:46

morcos reviewed Jul 17, 2017

View reviewed changes

achow101 force-pushed the combineraw-rpc branch from f293282 to 3cd7a5b Compare July 17, 2017 19:41

laanwj reviewed Jul 18, 2017

View reviewed changes

achow101 force-pushed the combineraw-rpc branch from 3cd7a5b to 6b4f231 Compare July 18, 2017 17:59

laanwj merged commit 6b4f231 into bitcoin:master Jul 20, 2017

TheBlueMatt mentioned this pull request Jul 23, 2017

TODO for release notes 0.15.0 #9889

Closed

12 tasks

achow101 deleted the combineraw-rpc branch August 29, 2017 15:28

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Sep 20, 2019

Revert "Merge bitcoin#10571: [RPC]Move transaction combining from sig…

ed07315

…nrawtransaction to new RPC" This reverts commit 6478562.

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Sep 20, 2019

Revert "Merge bitcoin#10571: [RPC]Move transaction combining from sig…

7b99bc5

…nrawtransaction to new RPC" This reverts commit 6478562.

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[RPC]Move transaction combining from signrawtransaction to new RPC #10571

[RPC]Move transaction combining from signrawtransaction to new RPC #10571

Uh oh!

Conversation

achow101 commented Jun 10, 2017

Uh oh!

sipa commented Jun 12, 2017

Uh oh!

jnewbery commented Jun 28, 2017

Uh oh!

sipa commented Jun 28, 2017

Uh oh!

sipa commented Jul 15, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

morcos left a comment

Choose a reason for hiding this comment

Uh oh!

morcos Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

morcos Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

morcos Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 17, 2017

Uh oh!

morcos commented Jul 17, 2017

Uh oh!

morcos Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 17, 2017

Uh oh!

achow101 commented Jul 17, 2017

Uh oh!

morcos commented Jul 17, 2017

Uh oh!

morcos commented Jul 17, 2017

Uh oh!

sipa commented Jul 17, 2017

Uh oh!

laanwj Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 18, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj Jul 18, 2017 •

edited

Loading