/bitcoin

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

Merged
merged 1 commit into from Jul 20, 2017

Conversation

Reviewers

@laanwj laanwj

@morcos morcos

@gmaxwell gmaxwell

Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
  0.15.0
7 participants
@achow101 @sipa @jnewbery @morcos @laanwj @gmaxwell @fanquake
@achow101
Contributor

achow101 commented Jun 10, 2017

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

achow101 referenced this pull request Jun 10, 2017

Open

[RPC] Split signrawtransaction into multiple distinct RPCs #10570

3 of 4 tasks complete

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

@sipa
Owner

sipa commented Jun 12, 2017

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

achow101 referenced this pull request Jun 12, 2017

Open

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

@jnewbery
Member

jnewbery commented Jun 28, 2017

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
Owner

sipa commented Jun 28, 2017

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

laanwj added this to the 0.15.0 milestone Jul 6, 2017

jnewbery referenced this pull request Jul 7, 2017

Open

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

@sipa
Owner

sipa commented Jul 15, 2017

utACK c710eb9
@gmaxwell

gmaxwell approved these changes Jul 17, 2017
View changes

ACK

@morcos

morcos approved these changes Jul 17, 2017
View changes

utACK c710eb9

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

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);
@morcos

morcos Jul 17, 2017

Contributor

nit: braces

src/rpc/rawtransaction.cpp
+ }
+
+ if (txVariants.empty())
+ throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Missing transactions");
@morcos

morcos Jul 17, 2017

Contributor

nit: braces

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

morcos Jul 17, 2017

Contributor

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
Contributor

achow101 commented Jul 17, 2017

Nits addressed and rebased.
@morcos
Contributor

morcos commented Jul 17, 2017

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

morcos Jul 17, 2017

Contributor

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

@achow101
Contributor

achow101 commented Jul 17, 2017

@morcos sorry, I rebased out of habit.
@achow101
Contributor

achow101 commented Jul 17, 2017

Now the comment should be right.
@morcos
Contributor

morcos commented Jul 17, 2017

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
Contributor

morcos commented Jul 17, 2017

utACK 3cd7a5b
@sipa
Owner

sipa commented Jul 17, 2017

utACK 3cd7a5b
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);
@laanwj

laanwj Jul 18, 2017 • edited

Owner

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

@achow101

achow101 Jul 18, 2017

Contributor

Fixed

@achow101 achow101 Move transaction combining from signrawtransaction to new RPC 
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.
6b4f231

@laanwj laanwj merged commit 6b4f231 into bitcoin:master Jul 20, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laanwj laanwj added a commit that referenced this pull request Jul 20, 2017

@laanwj laanwj Merge #10571: [RPC]Move transaction combining from signrawtransaction… 
… 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
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
adf170d

TheBlueMatt referenced this pull request Jul 23, 2017

Open

TODO for release notes 0.15.0 #9889

0 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment