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
Branch
split_signrawtransaction
Lint
Lint OK
Unit
No Unit 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
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
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
This revision was automatically updated to reflect the committed changes.