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 Passed
Unit
No Test Coverage
Build Status
Buildable 3762
Build 5599: Bitcoin ABC Buildbot (legacy)
Build 5598: arc lint + arc unit

Event Timeline

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

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.

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
src/rpc/rawtransaction.cpp
798 ↗(On Diff #5622)

size_t

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

This revision is now accepted and ready to land.Nov 5 2018, 23:21
This revision was automatically updated to reflect the committed changes.