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 3673
Build 5421: Bitcoin ABC Buildbot (legacy)
Build 5420: 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	braces
782	Move comment on previous line
786	Move comment on previous line
790	Move comment on previous line
935	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.