Details

Reviewers

jasonbcox
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGINGed803dc3198f: Move transaction combining from signrawtransaction to new RPC
rABCed803dc3198f: Move transaction combining from signrawtransaction to new RPC

Summary

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.

Backport of core PR10571
Required for T436

Test Plan

./test/functional/test_runner.py rawtransactions signrawtransactions

Diff Detail

Repository

rABC Bitcoin ABC

Branch

split_signrawtransaction

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 3762
Build 5599: Bitcoin ABC Buildbot (legacy)
Build 5598: arc lint + arc unit

Event Timeline

Fabien created this revision.Oct 24 2018, 13:46

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3673: Diff 5522.Oct 24 2018, 13:56

schancel edited the summary of this revision. (Show Details)Oct 24 2018, 19:26

deadalnix requested changes to this revision.Oct 24 2018, 23:18

deadalnix added inline comments.

src/rpc/rawtransaction.cpp
730 ↗	(On Diff #5522)	braces
782 ↗	(On Diff #5522)	Move comment on previous line
786 ↗	(On Diff #5522)	Move comment on previous line
790 ↗	(On Diff #5522)	Move comment on previous line
935 ↗	(On Diff #5522)	braces

This revision now requires changes to proceed.Oct 24 2018, 23:18

Fix nits as per review

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3722: Diff 5588.Oct 30 2018, 17:25

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3723: Diff 5589.Oct 30 2018, 17:32

schancel added a subscriber: schancel.Oct 30 2018, 23:16

schancel added inline comments.

src/rpc/rawtransaction.cpp
733 ↗	(On Diff #5589)	What is going on with this API? Can you point me to the docs? I doesn't look like this actually combines transactions, but only signatures?

Fabien marked an inline comment as done.Oct 31 2018, 13:51

Fabien added inline comments.

src/rpc/rawtransaction.cpp
733 ↗	(On Diff #5589)	My understanding is that it targets partially signed multisig txs, and effectively combines the signatures in order to make the tx fully signed, hence spendable. I could not find any documentation apart from the help lines, but fund the functional test case being quiet self-explanatory.

schancel added inline comments.Oct 31 2018, 15:46

src/rpc/rawtransaction.cpp
815 ↗	(On Diff #5589)	So this assumes that the prevouts are all the same?

Fabien marked an inline comment as done.Nov 2 2018, 15:00

Fabien added inline comments.

src/rpc/rawtransaction.cpp
815 ↗	(On Diff #5589)	My understanding is that the pk script should be the same.

Rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3748: Diff 5622.Nov 2 2018, 16:51

jasonbcox accepted this revision.Nov 2 2018, 19:35

jasonbcox added inline comments.

src/rpc/rawtransaction.cpp
1139 ↗	(On Diff #5622)	Just a note for other reviews, because I almost missed this: This line corresponds to `if (!fHashSingle \|\| (i < mtx.vout.size())) ProduceSignature(MutableTransactionSignatureCreator(&keystore, &mtx, i, amount, nHashType), prevPubKey, sigdata);` in the original PR: https://github.com/bitcoin/bitcoin/pull/10571/files It threw me off because we had some refactors on this line that made it easy to miss while scanning.