Page MenuHomePhabricator

Move transaction combining from signrawtransaction to new RPC
ClosedPublic

Authored by Fabien on Oct 24 2018, 13:46.

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Oct 24 2018, 13:46
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
Fabien updated this revision to Diff 5588.Oct 30 2018, 17:13

Fix nits as per review

Fabien updated this revision to Diff 5589.Oct 30 2018, 17:23

Rebase

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.

Fabien updated this revision to Diff 5622.Nov 2 2018, 16:41

Rebase

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.

deadalnix requested changes to this revision.Nov 4 2018, 18:31

This would need a release note.

This revision now requires changes to proceed.Nov 4 2018, 18:31
deadalnix added inline comments.Nov 4 2018, 18:34
src/rpc/rawtransaction.cpp
798 ↗(On Diff #5622)

size_t

Fabien updated this revision to Diff 5642.Nov 4 2018, 20:23

Rebase, fix size_t, add an entry to the release note

deadalnix accepted this revision.Nov 5 2018, 23:21
This revision is now accepted and ready to land.Nov 5 2018, 23:21
Closed by commit rABCed803dc3198f: Move transaction combining from signrawtransaction to new RPC (authored by Andrew Chow <achow101-github@achow101.com>, committed by Fabien). · Explain WhyNov 5 2018, 23:30
This revision was automatically updated to reflect the committed changes.